java.jdbc

db-do-prepared does not correctly check if transaction? was passed

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    [org.clojure/java.jdbc "0.3.7"]
    Ubuntu Linux 12.04
    Testing against a DB2 database
     

Description

The function db-do-prepared allows an optional second parameter "transaction?". Whether or not this was passed is done by checking it for being a string:

(if (string? transaction?)
      (apply db-do-prepared db true transaction? opts)

The assumption behind this seems to be, that a string indicates an SQL query parameter.

But, as the code a little further down shows, users can also pass in a prepared statement:

(if (instance? PreparedStatement sql)

So, to find out if transaction? was passed as a parameter one could either check for string and prepared statement or turn the logic around an check for boolean:

(if (or (string? transaction?)
            (instance? PreparedStatement sql))
      (apply db-do-prepared db true transaction? opts)
      (if-let [^java.sql.Connection con (db-find-connection db)]

Or

(if (not (instance? Boolean transaction?))
      (apply db-do-prepared db true transaction? opts)

The latter does not deal with nil which would probably be a natural choice for passing falsehood while the former would have to be extended when another thing may be passed as SQL, which seems unlikely.

  1. DBC-111-check-PS.patch
    26/Jul/15 4:11 PM
    1 kB
    Stefan Kamphausen
  2. DBC-111-with-docstring.patch
    26/Jul/15 4:11 PM
    2 kB
    Stefan Kamphausen

Activity

Hide
Sean Corfield added a comment - - edited

The assumption was that if you wanted to pass a PreparedStatement, you had to provide transaction? first (i.e., it could only be omitted when passing a sql string).

I can see value in allowing it to be omitted when a PreparedStatement is passed by expanding the condition to: (if (or (string? transaction?) (instance? PreparedStatement transaction?)) ...

Show
Sean Corfield added a comment - - edited The assumption was that if you wanted to pass a PreparedStatement, you had to provide transaction? first (i.e., it could only be omitted when passing a sql string). I can see value in allowing it to be omitted when a PreparedStatement is passed by expanding the condition to: (if (or (string? transaction?) (instance? PreparedStatement transaction?)) ...
Hide
Stefan Kamphausen added a comment -

Assuming that transaction? be passed anyway, if a prepared statement is passed, is fair enough, too. In that case, I humbly suggest adjusting the docstring a bit.

BTW, just replacing

(if (string? transaction?)

with

(if (or (string? transaction?)
          (instance? PreparedStatement sql))

Breaks the tests. Seems like it is not as easy as it seemed at first glance.

Show
Stefan Kamphausen added a comment - Assuming that transaction? be passed anyway, if a prepared statement is passed, is fair enough, too. In that case, I humbly suggest adjusting the docstring a bit. BTW, just replacing
(if (string? transaction?)
with
(if (or (string? transaction?)
          (instance? PreparedStatement sql))
Breaks the tests. Seems like it is not as easy as it seemed at first glance.
Hide
Stefan Kamphausen added a comment -
(if (or (string? transaction?)
          (instance? PreparedStatement transaction?))

works of course. Sorry for the noise.

Show
Stefan Kamphausen added a comment -
(if (or (string? transaction?)
          (instance? PreparedStatement transaction?))
works of course. Sorry for the noise.
Hide
Stefan Kamphausen added a comment -

1. Adjust check for transaction? to test for PreparedStatement, too

2. First patch plus addition to docstring.

Show
Stefan Kamphausen added a comment - 1. Adjust check for transaction? to test for PreparedStatement, too 2. First patch plus addition to docstring.
Hide
Sean Corfield added a comment -

Fixed. Thank you!

Show
Sean Corfield added a comment - Fixed. Thank you!

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: