SqliteClient silently swallows almost all SQLite errors on ExecuteStatement

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create a table with a constraint, say an INTEGER PRIMARY KEY constraint.
2. Using the SqliteClient, violate the constraint.  For example, by INSERTing 
the same PK twice.

Expected behavior: Exception thrown, because SQLite detected a constraint 
violation.

Actual behavior: Error silently swallowed by SqliteClient.

Cause: SqliteCommand.ExecuteStatement only checks for result codes ERROR, BUSY, 
and MISUSE.  Any other result code is assumed to be ROW or DONE.

Affected code:

   329  err = (SqliteError)Sqlite3.sqlite3_step( pStmt );
   330
   331  if ( err == SqliteError.ERROR )
   ...
   347  if ( err == SqliteError.BUSY )
   348    throw new SqliteBusyException();
   349
   350  if ( err == SqliteError.MISUSE )
   351    throw new SqliteExecutionException();
   352
   353  // err is either ROW or DONE.
   354  return err == SqliteError.ROW;

Suggested change:

It is always dangerous to blacklist error codes, because more codes can be 
added later.

Instead, the code should be flipped around to whitelist the success codes.  You 
can then check for specific errors, with a catch-all for unknown errors.

For example:

  if (err == SqliteError.ROW)
    return true;
  else if (err == SqliteError.DONE)
    return false;
  else if (err == SqliteError.ERROR)
    throw new SqliteExecutionException (...);
  else if (err == ...)
    throw new ...
  ...
  else
    throw new SqliteExecutionException();

Original issue reported on code.google.com by tanza...@gmail.com on 5 Nov 2011 at 2:50

[GoogleCodeExporter]

Original comment by noah.hart@gmail.com on 11 Nov 2011 at 1:33

• Changed state: Accepted
• Added labels: Milestone-3.7.9

[GoogleCodeExporter]

What can also be done is to actually set the Exception's ErrorCode to the 
appropriate SqliteError enum value. Then a generic exception can be thrown as 
well and we could use this value instead of having to check all the Exception 
types

Original comment by mattleib...@gmail.com on 29 Mar 2012 at 1:28

[GoogleCodeExporter]

And just to say again, this is quite important as it causes unit test to fail 
and we do need to know if something fails.

    if ( err == SqliteError.BUSY )
        throw new SqliteBusyException();

    if ( err == SqliteError.MISUSE )
        throw new SqliteExecutionException();

    // ... 

    if (err != SqliteError.ROW && err != SqliteError.DONE)
        throw new SqliteExecutionException((int)err);

    // err is either ROW or DONE.
    return err == SqliteError.ROW;

Original comment by mattleib...@gmail.com on 30 Mar 2012 at 12:17

[GoogleCodeExporter]

I have crated a patch that may help. It now will be similar to 
System.Data.Sqlite and Mono.Data.Sqlite.

I know that I have duplicated the Error codes enum and messages array, but 
there are some reasons:

- Firstly, the duplicated enum has both the same names and casing as the other 
two libraries. This is important as I would like to use minimal code 
differences in my cross platform projects (Mono for Android and Windows Phone).

- The duplicated messages can allow for more user friendly messages. There will 
have to be updates to either the SqliteException or the SqliteCommand and 
SqliteDataReader classes as both sets add the error message to the exception 
message text: the two by passing the result of Sqlite3.GetError3() to the 
exceptionand then again the exception by looking at the message array. 
I included both options so that whoever commits this patch (hopefully it will), 
can choose.

Original comment by mattleib...@gmail.com on 8 Apr 2012 at 8:31

Attachments:

• Exceptions.patch

[GoogleCodeExporter]

Thank you, patch accepted into current release

Original comment by market.n...@gmail.com on 10 Apr 2012 at 1:41

• Added labels: Milestone-3.7.11
• Removed labels: Milestone-3.7.9