-
Notifications
You must be signed in to change notification settings - Fork 619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use extended error codes #119
Conversation
It looks like the TravisCI builds do not recompile NativeDB.c. Is that correct? If so, how can I get the PR builds passing? |
The TravisCI build fix in PR #120 should make this one pass. |
Looks like you'll need to rebase this PR on master in order for Travis to be happy. |
|
||
public final int code; | ||
public final int extended; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we need to maintain both code
and extended
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an incomplete attempt at maintaining backwards compatibility. I'll rework it, hopefully by Monday.
Just to make sure: we still want user code doing SQLiteErrorCode.getErrorCode(ex.getErrorCode())
on an exception caused by an extended code like SQLITE_BUSY_RECOVERY
get the non-extended instance (in this caseSQLITE_BUSY
) by default, right? Otherwise code that does that and compares the result with an instance of SQLiteErrorCode
to determine how to handle the exception would suddenly fail, so we should have some kind of setting for getting the extended code instances instaed, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. My understanding was incomplete. I think your solution is probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an updated version that adds an internal pragma for enabling/disabling use of extended result codes as SQLException vendor codes (but still uses the extended result code to build the message text for the exception).
Would you like it as a force-push that replaces the commit in this PR, or as a new commit on top of this broken one?
8aad3be
to
5219094
Compare
5219094
to
6afed0a
Compare
Go ahead and force-push to this PR so we can keep the conversation together. I'm not sure it needs to be as complicated as you've implemented with a new PRAGMA. Seems like we could always set the extended result code and have accessor methods to return the basic result code and extended result code. Or have I missed something important? |
We could, but changing what we use as vendor code could be seen as a breaking API change. If that's fine, then it's a lot simpler: just use extended result codes all the time. If not, then using extended result codes instead of legacy error codes as the vendor code must be opt-in. So: does sqlite-jdbc guarantee stability of vendor codes of SQLExceptions? |
I don't see that What if we implemented a Or maybe the answer is using proper |
6afed0a
to
7509994
Compare
Something like this then? I believe SQLState is a different thing. We should provide the exact sqlite result code to applications that are OK with database-specific code for figuring out exactly what went wrong. We should probably also provide SQLState codes for applications that are OK with general descriptions of what happened as long as they don't have to add database-specific code. |
Yes, that looks good to me. @xerial Do you have an opinion? |
Nice. Thanks! |
Enable extended error code reporting in sqlite, and new error enum values in SQLiteErrorCode.java. The extended error code is held in a new member of SQLiteErrorCode, so that the code member retains its old behaviour, so that existing clients which depend on that are not affected. The message returned by toString does however change for errors for which there now are more detailed descriptions.