Use maintained version of clojure.java.jdbc

[scttnlsn]

There are some updates to the maintained version of clojure.java.jdbc that would be nice to use with Korma (Korma is currently using clojure.java.jdbc.deprecated).

This addresses issue https://github.com/korma/Korma/issues/207

[sco11morgan]

This PR seems very close, but there is some oddness when we have entities from multiple DBs (with the database defined on the entity).

Creating a transaction needs have a current or default DB connection, but then if we try to query an entity that uses a different DB inside that transaction, those queries use current-conn instead of the entities conn. eg:

(with-db mem-db (transaction (select user (database other-db)) (delete-some-rows-from-mem-db)))

Maybe transaction could optionally take a db arg?

[scttnlsn]

@captmorgan I'm not sure I completely understand here. Is the case you described above specific to transactions? What about...

(with-db mem-db
  (select user (database other-db))
  (delete-some-rows-from-mem-db))

Won't that suffer the same issue?

The way I understand this (and correct me if I'm wrong) is that if we call do-query with a db value that does not correspond to *current-conn* then we need to open/use a new connection. At a high level we're saying that the database passed to (database ...) should take precedence over any (with-db ...) that we might be inside of. Am I understanding this correctly?

[sco11morgan]

Yes I think the database defined on the entity or in the query should take precedence over with-db

transaction can only use the db connection from the default or the *current-conn*. And there can be only one default db, so we must use with-db if we want to use transactions in a multi db setup (without going to clojure jdbc calls).

Hope that clears things up. For 95% of the Korma users I think this PR is fine, unfortunately I'm stuck in the 5% that have to use multi DBs. Maybe for people using multi DBs we should just mixin jdbc calls for transaction / rollback

[scttnlsn]

@captmorgan Added a new commit addressing the issue you mentioned. Please let me know if there are more changes that need to be made.

[sco11morgan]

Looks good

[immoh]

Apologies for taking so long before taking a look at this. It is great to see this moving forward, but I have couple of things I would like to get clarified before I can merge this:

1. Why query-params function is needed? Why the existing implementation (apply vector sql-str params) is not sufficient?
2. Why do you need to give :row-fn to jdbc/query? It shouldn’t use the fields function from naming options since that is meant for transforming field names from clojure to sql. As far as I can tell passing :identifiers option should be enough.
3. I don’t understand how this change (first commit) breaks the behavior with multiple DBs and hence why the second commit is needed. I ran the test that was added in the second commit without any other changes and it fails, which means the second commit is actually changing behavior of Korma. In order to better understand what it breaks, we need a test that passes with the old version and fails after first the first commit.

Thanks.

[scttnlsn]

Thanks for taking a look at this!

1. Removed
2. Removed
3. I think you're right that this changes the behavior of Korma (it was previously undefined?). Perhaps the problem that @captmorgan encountered is just a special case of the fix that I implemented. @captmorgan Could you provide a failing test for the regression you experienced?

If you'd like I can revert the second commit, squash the remaining commits and we can address the regression in a separate issue.

[immoh]

I would like to address regression (if it exists) before merging to master. Let's decide what to do about the second commit after we hear from @captmorgan.

[sco11morgan]

I think we can split out the last change. My issue is that there is no way using Korma to setup a transaction when there is not a default database configured. The only way I found was to use with-db, but if I did that then the per query/entity defined DBs weren't getting respected. Maybe I should just drop down to c.j.j transactions.

I wrote a test but am seeing some odd behavior. This test (test/korma/test/integration/transactions.clj) fails, but if I uncomment the test defined at the end of the file (transaction-rollback-for-other-db-works) then all tests pass?

https://github.com/captmorgan/Korma/commit/9c2d2c6e3a2fdbfe280d707f070070b81e5d3a45

[scttnlsn]

Okay, sounds like this is unrelated to using the new JDBC code. I reverted the unrelated commit and squashed everything else.

@captmorgan The transaction commit that I made is still available here if you need/want to reference it: https://github.com/scttnlsn/Korma/commit/c7cfb7451c8f7a9b69bb827c7365c17e6dfecf91

@immoh Anything else to do with regard to the JDBC updates?

[immoh]

I think this is good to go. Thanks!