Skip to content
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

bpo-44859: Raise more accurate exceptions in sqlite3 #27695

Merged
merged 16 commits into from
Mar 17, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 9, 2021

Improve exception compliancy with PEP 249

  • Raise InterfaceError instead of ProgrammingError for SQLITE_MISUSE.
    If SQLITE_MISUSE is raised, it is a sqlite3 module bug. Users of the
    sqlite3 module are not responsible of using the SQLite C API correctly.
  • Don't overwrite BufferError with ValueError when conversion to BLOB fails.
  • Raise ProgrammingError instead of Warning if user tries to execute() more
    than one SQL statement.
  • Raise ProgrammingError instead of ValueError if an SQL query contains null characters.
  • Make sure _pysqlite_set_result raises an exception if it returns -1.

Fixes #89022

Erlend E. Aasland added 4 commits August 9, 2021 23:15
@erlend-aasland
Copy link
Contributor Author

@serhiy-storchaka would you mind reviewing this?

@erlend-aasland
Copy link
Contributor Author

@serhiy-storchaka PTAL. I just fixed bug in _pysqlite_set_result that could result in a segfault.

@erlend-aasland
Copy link
Contributor Author

@malemburg would you mind taking a look at this PR? I'm slowly trying to align the exceptions raised with PEP 249.

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Show resolved Hide resolved
@JelleZijlstra
Copy link
Member

Looks like you didn't push the changes I previously asked for.

@erlend-aasland
Copy link
Contributor Author

Looks like you didn't push the changes I previously asked for.

Actually, it seems I forgot about this; I didn't make any changes yet :) I'll see if I can get to it any time soon. Thanks for the reminder!

@erlend-aasland
Copy link
Contributor Author

I'd like to run this through the Django test suite before eventually merging. Also, I'll have a talk with MAL regarding this change.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Mar 2, 2022

I'd like to run this through the Django test suite before eventually merging. Also, I'll have a talk with MAL regarding this change.

I finally got to run this through the Django test suite, and it works all fine; one down, one to go.

Observations while running the Django test suite

I got sidetracked with a long bisect operation because I was using SQLite 3.39.0 (development version), and because b899126 produces a ton of warnings in the Django test suite. I reverted b899126 and compiled strictly against SQLite 3.36.0 (macOS bundled), and all was ok.

UPDATE: I had a chat the other day with @malemburg, to get his comments regarding three specific DB-API related changes in this PR. I take the liberty to quote him here (hope that's ok, Marc-Andre):

  1. Raise InterfaceError iso. ProgrammingError for SQLITE_MISUSE. If SQLITE_MISUSE is raised, it is a sqlite3 module bug. Users of the sqlite3 module are not responsible of using the SQLite C API correctly.

    This sounds fine. InterfaceError is meant for module related errors.

  2. Raise ProgrammingError iso.Warning if user tries to execute() more than one statement

    Yep, sounds fine.

  3. Raise DataError iso. ValueError if query contains NULL characters

    Since queries are passed in by the programmer, I would use ProgrammingError here. DataError would be appropriate for errors related to e.g. wrong values passed in as bound parameters.

I will update the PR with the suggested change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve some sqlite3 errors
4 participants