SQLite error handling problem when using prepared queries.

heegaiximephoomeeghahyaiseekh commented 7 years ago

When certain errors occur while executing an SQLite prepared query, no condition is signalled until the next time the statement is used. As a result, I have to add special SQLite-specific code to my project to get correct results.

Reproduction:

(defvar *conn* (connect :sqlite3 :database-name #P"/tmp/test.sqlite"))
(dbi:execute (dbi:prepare *conn* "CREATE TABLE TEST (FOO INTEGER PRIMARY KEY);"))
(let ((prepared (dbi:prepare *conn* "INSERT INTO TEST (FOO) VALUES (?);")))
  (dbi:execute prepared 1)
  (dbi:execute prepared 1)
  (format t "That should have resulted in an error, since we inserted two 1s, but the error~%")
  (format t "will be deferred until SQLITE:RESET-STATEMENT is called next.~%")
  (format t "Press ENTER to trigger the improperly deferred error: ")
  (read-line)
  (dbi:execute prepared 2))

Solution: Call sqlite:reset-statement after using the statement, and before dbi:execute returns.

Workaround: End users can add the following method to their project to make the errors throw as soon as the failing query returns.

(defmethod dbi:execute :after ((query dbd.sqlite3::<dbd-sqlite3-query>)
                               &rest parameters)
  (declare (ignore parameters))
  (sqlite:reset-statement (slot-value query 'dbi.driver::prepared)))

TruePikachu commented 7 years ago

I'm able to confirm this bug, and have located the source, at https://github.com/fukamachi/cl-dbi/blob/master/src/dbd/sqlite3.lisp#L74:

(when (handler-case (step-statement prepared)
        (sqlite-error (e)
          @ignore e
          nil))

SQLITE:STEP-STATEMENT can result in one of the following three situations when passed a query to execute:

Returning T indicates that the query executed successfully and returned one or more rows
Returning NIL indicates that the query executed successfully, but didn't return any rows
Signalling a condition of type SQLITE-ERROR indicates that sqlite3_step returned something other than SQLITE_DONE or SQLITE_ROW.

Then, the HANDLER-CASE linked will, in the case of an error, consume the condition and make it appear as if the query executed successfully but didn't pass any rows.

I don't see any logical reason for the inclusion of the HANDLER-CASE.

EDIT: I do not suggest the workaround proposed by @heegaiximephoomeeghahyaiseekh; it results in odd behaviour relating to DBI:FETCH (including the query being executed a second time).

TruePikachu commented 6 years ago

I can confirm this bug still exists as of 20180430.

TruePikachu commented 6 years ago

I investigated things a bit further, since removing the HANDLER-CASE still causes sqlite:reset-statement to signal the error. Additionally, this bug prevents things such as DBI:WITH-TRANSACTION from operating as intended wrt error handling.

`sqlite3_step`:

SQLite3's API documentation says that sqlite3_step, under the v2 interface (which is what's being used), will return SQLITE_DONE, SQLITE_ROW, SQLITE_BUSY, SQLITE_MISUSE, or the error code resulting from executing the statement. SQLITE:STEP-STATEMENT signals a SQLITE-ERROR with the relevant error code when sqlite3_step returns a value other than SQLITE_DONE or SQLITE_ROW. However, DBI.DRIVER:FETCH-USING-CONNECTION in the relevant method currently uses HANDLER-CASE to ignore the condition signalled, and behaves as though sqlite3_step had returned SQLITE_DONE. This is a bug that needs correcting, as the call that resulted in the error should signal it.

`sqlite3_reset`:

SQLite3's API documentation says that sqlite3_reset returns either SQLITE_OK (if sqlite3_step returned SQLITE_DONE or SQLITE_ROW on the previous call, or it had not been called in the first place), or an error code. SQLITE:RESET-STATEMENT signals the SQLITE-ERROR in this latter situation. However, DBI.DRIVER:EXECUTE-USING-CONNECTION in the relevant method doesn't do any sort of error-checking for this scenario; while it is technically not in error, it is the cause of the SQLITE-ERRORs cropping up later than intended.

Suggestions:

Right now, my best idea for fixing this is to remove the HANDLER-CASE from around STEP-STATEMENT (so that the call that results in the error results in the error being signalled), and moving it to instead wrap around RESET-STATEMENT (which, as far as I can determine, will only suppress errors that resulted from resetting statements that resulted in an error). I will make this change in my local instance, and if things seem to work well enough, I'll submit a PR.

TruePikachu commented 6 years ago

Slight correction to the above: it is probably valid to still have STEP-STATEMENT's HANDLER-CASE capture SQLITE_MISUSE -- this allows FETCH-USING-CONNECTION to return NIL even after returning NIL already, keeping the existing behaviour.

fukamachi / cl-dbi