Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add check if JDBC driver supports generated keys #107

Closed
wants to merge 15 commits into from

2 participants

@andeee

JDBC API has DatabaseMetaData to check for features of the database. Since JDBC3 supportsGetGeneratedKeys is available.

See http://download.oracle.com/javase/6/docs/api/java/sql/DatabaseMetaData.html#supportsGetGeneratedKeys().

Attention: sqlite-jdbc reports false, but supports it - I'll file a bug on this for sqlite-jdbc

@andeee

sorry the amount of commits, just trying to get started with git

@andeee andeee closed this
@andeee andeee reopened this
@bendlas
Collaborator

Thanks for the pull request. I have to ask you to clarify and change a few things, before commiting this.

  • Splitting things up into multiple commits is a good thing. Especially good: first commiting the test cases, then the solution. Makes verifying that much easier. OTOH multiple commits are less than helpful, if you are introducing changes and reverting (some of) them a few commits later.

    Could you please clean up the commits? If nothing else, you can do that by creating a new branch with its HEAD at the latest master commit. Then add (possibly failing) test cases, commit. Then commit the fix. If you have an erroneous commit, roll it back with git reset. Don't git revert, unless the change is already released.

  • You can leave in the test cases against an embedded database, like SA. More databases to run the test cases against, w/o config overhead == good.

  • Can you clarify, which advantage your approach has, above the currently implemented heuristic?
    Is performance better? Does it handle some corner cases better?
    If so why and which, respectively?

@andeee

Thank you for your comments and advice.

I'll try to clarify my intent:

  • Not every JDBC driver does support getGeneratedKeys, in my example Sybase SQL Anywhere doesn't support it and throws an SQLException when e. g. trying to conj! onto a table
  • We can make clojureql work with these drivers when calling supportsGetGeneratedKeys onto DatabaseMetaData before we create the statement and append the generated keys to the result
  • I also check for the corner case when supportsGetGeneratedKeys isn't even implemented, catching the AbstractMethodError.
    Support for generated keys was introduced with JDBC Version 3, so here we could start to support even older drivers - tested with a SQL Anywhere JDBC Driver Version 9 which only supports JDBC Version 2

I'll now cleanup the commits and make another pull request.
If you have further questions, please ask.

@bendlas
Collaborator

Great! As far as I have understood, SQL Anywhere is embedded and thus can be used in the test suite w/o any external config. If so, please add it and add test cases, highlighting the wrong behavior. Please also double check, that there are test cases in place for everything that might be affected by your patch (to a reasonable extent, it will be audited anyway).

The latest problems we had in that area (of auto-generated keys), was with newer MySQL drivers. They would throw, if you didn't fetch the generated keys after a query.

Whereas the Postgres Driver would throw, when trying to get generated keys in batch mode.

I implemented a workaround, by only getting generated keys when not in batch mode, as per 257a5e0

You might want to check how your work interacts with that behavior.

Thanks for your help!

@andeee

Thanks for your further info!
SQL Anywhere is not as embedded as for example derby or hsqldb. Therefore one must download the Developer Editon from Sybase, create the database and deploy the jdbc driver to a local maven repo since it's not available on maven central.

Should I add it to the test cases anyway?

@bendlas
Collaborator

In this case, please not.
If you can easily reproduce it with another db, go ahead, otherwise just make sure it doesn't break anything.

Thanks!

@andeee

cleaned up commits in #108

@andeee andeee closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 25, 2011
  1. added old hsqldb to test against (jdbc2 driver)

    Andreas Wurzer authored
Commits on Aug 27, 2011
  1. initial setup for tests against Sybase SQL Anywhere

    Andreas Wurzer authored
Commits on Aug 29, 2011
  1. check if jdbc driver supports getGeneratedKeys

    Andreas Wurzer authored
  2. added doc strings, cleanup

    Andeee authored
  3. cleanup, remove references to sql anywhere

    Andeee authored
  4. removed ignored files

    Andeee authored
  5. added old hsqldb to test against (jdbc2 driver)

    Andreas Wurzer authored
  6. initial setup for tests against Sybase SQL Anywhere

    Andreas Wurzer authored
  7. check if jdbc driver supports getGeneratedKeys

    Andreas Wurzer authored
  8. added doc strings, cleanup

    Andeee authored
  9. cleanup, remove references to sql anywhere

    Andeee authored
  10. removed ignored files

    Andeee authored
  11. added missing new-line

    Andeee authored
  12. Merge branch 'generated-keys' of https://github.com/andeee/clojureql

    Andeee authored
    …into generated-keys
  13. added missing newline

    Andreas Wurzer authored
This page is out of date. Refresh to see the latest.
Showing with 30 additions and 12 deletions.
  1. +23 −7 src/clojureql/internal.clj
  2. +7 −5 test/clojureql/test/integration.clj
View
30 src/clojureql/internal.clj
@@ -360,22 +360,38 @@
(with-open [rset (.executeQuery stmt)]
(func (result-seq rset))))))
+(defn supports-generated-keys? [conn]
+ "Checks if the JDBC Driver supports generated keys"
+ (try (.supportsGetGeneratedKeys (.getMetaData conn))
+ (catch AbstractMethodError _ false)))
+
+(defn prepare-statement [conn sql]
+ "When supported by the JDBC Driver, creates a new prepared statement which will return the generated keys - else returns a 'normal' prepared statement"
+ (if (supports-generated-keys? conn)
+ (jdbc/prepare-statement conn sql :return-keys true)
+ (jdbc/prepare-statement conn sql)))
+
+(defn generated-keys [stmt]
+ "When supported by the JDBC driver, returns the generated keys of the latest executed statement"
+ (when (supports-generated-keys? (.getConnection stmt))
+ (let [ks (.getGeneratedKeys stmt)]
+ {:last-index
+ (and
+ (.next ks)
+ (.getInt ks 1))})))
+
(defn exec-prepared
"Executes an (optionally parameterized) SQL prepared statement on the
open database connection. Each param-group is a seq of values for all of
the parameters."
([sql param-group]
- (with-open [stmt (jdbc/prepare-statement (:connection jdbcint/*db*)
- sql :return-keys true)]
+ (with-open [stmt (prepare-statement (:connection jdbcint/*db*) sql)]
(doseq [[idx v] (map vector (iterate inc 1) param-group)]
(.setObject stmt idx v))
(jdbc/transaction
- (let [retr (.execute stmt)
- ks (.getGeneratedKeys stmt)]
+ (let [retr (.execute stmt)]
(with-meta [(.getUpdateCount stmt)]
- {:last-index (and
- (.next ks)
- (.getInt ks 1))})))))
+ (generated-keys stmt))))))
([sql param-group & param-groups]
(with-open [stmt (jdbc/prepare-statement (:connection jdbcint/*db*) sql)]
(doseq [param-group (cons param-group param-groups)]
View
12 test/clojureql/test/integration.clj
@@ -7,7 +7,8 @@
to the parameters you can find in test/clojureql/test.clj"
(:import java.sql.Timestamp)
- (:use clojure.test clojureql.core clojureql.test)
+ (:use clojure.test clojureql.core clojureql.test
+ [clojureql.internal :only [supports-generated-keys?]])
(:refer-clojure
:exclude [compile take sort drop distinct conj! disj! case]
:rename {take take-coll}))
@@ -25,11 +26,12 @@
{:wage 400, :id 8}))))
(database-test test-generated-keys
- (is (= 5 (-> (conj! salary {:wage 1337})
- meta :last-index)))
- (is (= 6 (-> (update-in! salary (where (= :id 512))
+ (when (supports-generated-keys? (:connection clojure.java.jdbc.internal/*db*))
+ (is (= 5 (-> (conj! salary {:wage 1337})
+ meta :last-index)))
+ (is (= 6 (-> (update-in! salary (where (= :id 512))
{:wage 1337})
- meta :last-index))))
+ meta :last-index)))))
(database-test test-join-explicitly
(is (= @(join users salary (where (= :users.id :salary.id)))
Something went wrong with that request. Please try again.