Fork me on GitHub
#sql
<
2021-07-16
>
indy18:07:25

Triggered by a gnarly issue (yet unresolved), I am trying to migrate to next.jdbc starting with a macro that is used to wrap forms that need to run in a transaction. For various practical reasons, the migration cannot be done in one shot. I did go through the migration bits in the documentation and about the with-transaction in https://cljdoc.org/d/com.github.seancorfield/next.jdbc/1.2.674/doc/migration-from-clojure-java-jdbc#further-minor-differences and also looked at the source for both for the old and new libraries, but I am still not able figure this out. The issue is that the new with-transaction does not rollback if there is an exception in the body (the block tagged as :new in the comment). But the :old and the :almost-ideal work as expected. Am I missing something obvious? Should I not be doing this? It would be nice if someone can shed some light on what marks the fact that a connection has an open transaction in the realm of JDBC. The current version of clojure.java.jdbc is 0.3.5. P.S. A lot of the things in the new macro are there for backward compatibility with the existing code, example the binding of the dynamic var. I understand that next.jdbc does this internally(?).

(def ^:dynamic *db-conn* nil)

(defn db-spec []
  (or *db-conn*
      (db-spec*)))

(defmacro with-transaction-old
  [[name & args :as binding] & body]
  `(clojure.java.jdbc/with-db-transaction ~binding
     (binding [*db-conn* ~name]
       ~@body)))

(defmacro with-transaction
  [[name db] & body]
  `(let [db#           ~db
         transactable# (or (:connection db#)
(:datasource db#))]
     (next.jdbc/with-transaction [txn# transactable#]
       (let [~name {:connection txn#}]
         (binding [*db-conn* ~name]
           ~@body)))))
           
(comment
 :almost-ideal
 (do
   (try
     (with-transaction [txn (db-spec)]
       (next.jdbc/execute! txn ["update member set first_name = 'Foo 3' where id = 1"])
       (throw (Exception.)))
     (catch Exception e
       nil))
   (clojure.java.jdbc/query (db-spec) ["select first_name from member where id = 1"]))
   
 :new
 (do
   (try
     (with-transaction [txn (db-spec)]
       (clojure.java.jdbc/execute! txn ["update member set first_name = 'Foo 1' where id = 1"])
       (throw (Exception.)))
     (catch Exception e
       nil))
   (clojure.java.jdbc/query (db-spec) ["select first_name from member where id = 1"]))

 :old
 (do
   (try
     (with-transaction-old [txn (db-spec)]
       (clojure.java.jdbc/execute! txn ["update member set first_name = 'Foo 2' where id = 1"])
       (throw (Exception.)))
     (catch Exception e
       nil))
   (clojure.java.jdbc/query (db-spec) ["select first_name from member where id = 1"])))

indy18:07:11

(db-spec*) returns the db-spec hash-map

seancorfield18:07:25

You absolutely cannot mix'n'match transactions between c.j.j and next.jdbc.

seancorfield18:07:58

There's a lot to unpack here but your problems are self-inflicted due to over-complicating things and/or misunderstanding the migration guide. I'll explain what we do at work, since we have a lot of code that mixes c.j.j and next.jdbc.

seancorfield18:07:46

We pretty much always traffic in connection pooled datasources (we use c3p0 but hikaricp is also a good choice). The "db" thing that we pass around is {:datasource c3p0-pool}. That's directly compatible with c.j.j as a "db-spec" and you can use it with next.jdbc via (:datasource db).

seancorfield18:07:34

That allows us to swap any section of code that currently uses c.j.j over to next.jdbc without touching any of the surrounding code -- as long as transactions are not in play higher up the call chain (we only ever use transactions for small blocks of code where it actually matters -- partly so we can be sure we never try to accidentally "nest" transaction calls... because transactions do not nest).

seancorfield19:07:09

When you do with-db-transaction in c.j.j, what it binds is still a "db-spec" but it has a connection inside it, that you can call c.j.j/`db-find-connection` on as I recall to get an actual Connection that you could use as-is with next.jdbc calls inside the c.j.j transaction.

seancorfield19:07:54

With you do with-transaction in next.jdbc, what it binds is an actual Connection object and you could pass that to a c.j.j call if you wrap it in {:connection con}.

seancorfield19:07:41

Except for transaction-based c.j.j code. That's like "mixing the streams". First off, the two libraries have very different behavior by default: c.j.j actually ignores any nested transaction calls, by keeping track of the "transaction level" in the "db-spec" hash map; next.jdbc just assumes you know what you're doing and will (try to) setup a new transaction on the existing Connection -- which means the nested call will commit/rollback to the beginning of the outermost transaction call (and this is why you should use save points instead). next.jdbc has a mode where it will prohibit nested transaction calls -- i.e., if it thinks there's already a tx in progress when you call with-transaction, it will throw an exception -- and it has a mode where it will ignore nested transaction calls (to mimic the c.j.j behavior). But the two mechanisms are not compatible with each other.

indy19:07:36

Thanks Sean, except for the last paragraph, I have understood it the same way as described. And I also read the next.jdbc.transaction/*nested-tx* bit which is used to toggle the modes. If it was the binding + dynamic-var and the overall use of a macro that wraps the libraries' with-transaction that make it seem complicated, then that is because any calls to (db-spec) inside the body of the transaction were also intended to return the same transaction hashmap (with the connection, level, and the rollback atom). This is code we inherited and there are a lot of places that use them now. If I set next.jdbc.transaction/*nested-tx* to :ignore , will I not be able to mimic the c.j.j behaviour?

seancorfield19:07:40

next.jdbc will not know about the context of tx started using with-db-transaction from c.j.j -- that's what I mean about not mixing the two types of TX.

seancorfield19:07:07

next.jdbc functions expect either a Connection, a Datasource, or something they can call get-datasource on to get a Datasource (and then they call get-connection on that). So what with-transaction binds and what with-db-transaction bind are very different.

indy19:07:54

Understood and that is why I'm doing (or (:connection db#) (:datasource db#)) in the new macro.

indy19:07:58

I will never be starting a transaction with c.j.j's with-db-transaction going forward. Will only be starting them with the new one.

(defmacro with-transaction
  [[name db] & body]
  `(let [db#           ~db
         transactable# (or (:connection db#)
(:datasource db#))]
     (next.jdbc/with-transaction [txn# transactable#]
       (let [~name {:connection txn#}]
         (binding [*db-conn* ~name]
           ~@body)))))
And all the downstream existing code will not break because I'm doing a (let [~name {:connection txn#}...) . Or at least that's what I expect. I mean the new macro reads and updates the DB, the issue is that it doesn't rollback when the body throws. This is all a temporary measure so that we can migrate gradually to next.jdbc while checking if changing this one possible culprit function to use the latest library solves a burning issue we are currently tackling.

indy19:07:59

The old macro was posted for comparison sake and will be replaced by the new one.

seancorfield20:07:30

You're binding ~name to a hash map containing :connection so I would not expect next.jdbc/execute! to be able to use that -- it'll think it's a db-spec hash map and it'll try to call get-datasource on it and then call get-connection on that... which won't work...

seancorfield20:07:45

You'd need (next.jdbc/execute! (:connection txn) ...) in those nested calls I think?

indy20:07:09

Oops sorry that was a copy paste mistake, I did mean (:connection txn) in the almost-ideal case. I'm calling it almost ideal because in the ideal case and our eventual goal is not have these macros and just use next.jdbc through and through. The almost ideal case was also pasted for comparison sake, it is the new case where rollback doesn't happen.

indy20:07:33

But thanks for your help Sean, I'll scan through the sources. And dig around a little more.

indy20:07:53

At the end of the day, from what I understand at least, the way with-transaction and with-db-transaction work is by setting setAutoCommit to false temporarily, run the body and call commit (or rollback in case the body throws) on the connection. Since I'm working with the connection correctly, I'm not sure why the rollback doesn't happen.

seancorfield20:07:09

I suspect c.j.j functions check whether they're in a TX and if they think they're not, they wrap each operation in a TX.

seancorfield20:07:27

So you'll need to fake whatever context c.j.j checks for that case.

seancorfield20:07:33

(this is why I said mixing TX styles "doesn't work": you can't just put c.j.j code inside a next.jdbc TX and nor can you put next.jdbc code inside a c.j.j TX)

seancorfield20:07:37

Ah, it's worse than that... c.j.j operations can be passed a :transaction? false option (in the hash map after the sql-params vector) to prevent that happening, but otherwise try to run a c.j.j with-db-transaction internally so the {:connection txn#} hash map you build also needs to have a fake c.j.j TX context in it, so that any attempts to run with-db-transaction inside with-transaction will think there's a c.j.j TX already in progress and not actually create another one.

indy20:07:52

Wow I just found it too

indy20:07:54

(clojure.java.jdbc/execute! txn ["update member set first_name = 'Foo 5' where id = 5"] :transaction? false)

indy20:07:18

I guess it creates a new connection(?) if I pass :transaction? as true which is the default.

seancorfield20:07:08

So, short of horrible hacking (which is the path you're taking), you can't nest c.j.j operations inside a next.jdbc/with-transaction call at all.

seancorfield20:07:29

I will make a note of that in the migration docs.

seancorfield20:07:46

And, no, it won't create a new connection but it will build an (overlapping) TX on the current open connection -- and will commit it at the end of the c.j.j operation.

indy20:07:00

Oooh okay, got it

seancorfield00:07:52

@UMPJRJU9E I've updated the next.jdbc migration docs to talk about transactions: https://github.com/seancorfield/next-jdbc/blob/develop/doc/migration-from-clojure-java-jdbc.md#transactions -- LMK if that helps and if there's anything else you can think of that could be added.

indy04:07:47

Thanks Sean for taking the time. I understood the updated bits about transaction pretty well since I faced the issues first hand. But for someone new they might find,

However, if you have migrated that with-db-transaction call over to next.jdbc/with-transaction then any clojure.java.jdbc operations invoked inside the body of that migrated transaction will still try to create their own transactions and with-db-transaction won't know about the outer with-transaction call so you will effectively get the "overlapping" behavior of next.jdbc since the clojure.java.jdbc operation will cause the outermost transaction to be committed or rolled back.
a bit of a mouthful. Maybe pause and break at "overlapping" part? Also, thought it'll be worthwhile pointing again that c.j.j's with-db-transaction is implicit in this part, "...that migrated transaction will still try to create their own transactions, and this implicit with-db-transaction won't know about the outer with-transaction..."

indy04:07:49

Also, fwiw, I spent the weekend migrating from c.j.j to next.jdbc for one of our microservices, all the tests are passing 🙂 The docs and the source of next.jdbc (especially that a lot of it is based on protocols), along with the fact that we had wrapped all the jdbc library calls made it easier than expected to migrate.

indy04:07:41

The plan API is super neat compared to what was being done with db-query-with-resultset in our code base.

seancorfield05:07:03

Thanks. I'll have another run at that tomorrow!

seancorfield17:07:06

@UMPJRJU9E I've split that long sentence in two: > However, if you have migrated that with-db-transaction call over to next.jdbc/with-transaction then any clojure.java.jdbc operations invoked inside the body of that migrated transaction will still try to create their own transactions and with-db-transaction won't know about the outer with-transaction call. > That means you will effectively get the "overlapping" behavior of next.jdbc since the clojure.java.jdbc operation will cause the outermost transaction to be committed or rolled back. I hope that's a bit easier to read. Thanks for the feedback, and I'm glad your migration is going well!

🎉 2
seancorfield20:07:11

So it will be "as if" you tried to nest with-transaction calls in next.jdbc (default behavior).

indy20:07:33

Hahaha, I really would love to do this properly, if not for the burning issue we are facing lately and I thought it could be done because we are after all dealing with connections at the end of the day. But I guess this is depending on very internal implementation details of the libraries to make this work. I should just talk to the team and prio a proper bump of the library.

seancorfield20:07:34

c.j.j really just had a very poor design in the area of TX 😞 That's why next.jdbc does things so very differently.

seancorfield20:07:20

c.j.j made it look like TX nest and then it built castles on top of that sand 😕

indy20:07:35

But this exercise was quite fun for me, thanks a ton for the help!

seancorfield20:07:26

Sorry you're in such a terrible situation to even be attempting this sort of mix'n'match migration 😞