Skip to content

bpo-16379: expose SQLite error codes and error names in sqlite3#27786

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.

Sign up for GitHub

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

Jump to bottom
Merged
pablogsal merged 34 commits into from
Aug 30, 2021
Merged

bpo-16379: expose SQLite error codes and error names in sqlite3#27786

pablogsal merged 34 commits into from
Aug 30, 2021

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented Aug 16, 2021•
edited
Loading

Original PR: GH-1108 (contributed by @palaviv, based on patches by @danielshahaf)

This PR is a rebase and rework of Aviv & Daniel's work.

Co-authored-by: Aviv Pavlivoda
Co-authored-by: Daniel Shahaf
Co-authored-by: Erlend E. Aasland

https://bugs.python.org/issue16379

palavivand others added 29 commits May 9, 2019 12:54
@palaviv
sqlite3 module expose sqlite error code and name in exceptions
83ab267
@palaviv
Fix syntax error
eeb10f4
@palaviv
Add NEWS entry
42636f9
@palaviv
Update version
f224223
@palaviv
Fix whitespace
31467e3
Merge branch 'main' into sqlite-expose-error-code
679280d
Move exception documentation to the exceptions section
8aa0859
Use PyModule_AddObjectRef to add error codes
e4195c0
Refactor: add add_error_constants()
a4a0762
Sort error codes alphabetically
c18d227
Normalise style with the rest of the code base (PEP 7, etc.)
67e49e8
Improve "get error name" function
1893ced
- Naming: sqlite3ErrName => pysqlite_error_name - Error handling: * return NULL if no exception matched * receiver handles errors * don't use error table to store SQLITE_UNKNOWN string
Refactor _pysqlite_seterror changes
67ea5f2
- Use intermingled declarations - Simplify ref count handling
Refactor error code table
6ced38c
- Declutter add_error_constants() - No need to typedef struct - Simplify naming - Use PyModule_AddIntConstant
Use macro to create error table
467f7ba
To avoid mismatch between char *name and constant
Add missing SQLITE_NOTICE and SQLITE_WARNING codes
4abd093
Add comment
70507dd
Merge branch 'main' into sqlite-expose-error-code
b617192
Assert exception class is always set
db053b9
Use explicit API's iso. Py_BuildValue, etc.
98106dd
Remove duplicate return code entries
0567938
Group stuff
16889a1
Use 'unknown' when a proper SQLite exception cannot be found
a7a1cf8
IMO, we should not pollute the SQLITE_* namespace
Fix complete_statement.py example
772a29c
- Use correct attribute name - Normalise error variable naming - Remove 'Errno' from printed string to avoid confusion
Improve unit tests
fa5a1af
- Use assertRaisesRegex iso. two assert functions - Normalise quotes - Test that constants are added to the module
Adjust NEWS entry and update What's New
cf551b1
Use PyObject_SetAttrString iso. _PyObject_SetAttrId => slow path/read…
a3646c9
…ability
Refactor _pysqlite_seterror
8a49659
Link to sqlite.org/rescode.html iso. sqlite.org/c3ref/c_abort.html
8d11bc5
@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Aug 16, 2021

cc. @nedbat

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Aug 16, 2021

cc. @auvipy & @matrixise who reviewed the original PR.

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Aug 19, 2021•
edited
Loading

FYI, I pushed a small tweak to the unit test: use the temp_dir helper iso. a hard coded path.

Thanks for reviewing, @auvipy

pablogsal
pablogsal reviewed Aug 25, 2021
View reviewed changes
Modules/_sqlite/util.c
int
_pysqlite_seterror(pysqlite_state*state, sqlite3*db)
staticPyObject*
get_exception_class(pysqlite_state*state, interrorcode)
Copy link
Member

@pablogsalpablogsalAug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slighly confusing because normally returning NULL signifies an error. Could you please document this both on the call site and in this function? Otherwise the call site reads weirdly because

 PyObject *exc_class = get_exception_class(state, errorcode); if (exc_class == NULL){return errorcode}

seems that is handling an error

Copy link
ContributorAuthor

@erlend-aaslanderlend-aaslandAug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see now that it can be a bit confusing. I'll add a comment. Thanks!

Copy link
ContributorAuthor

@erlend-aaslanderlend-aaslandAug 25, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it clearer with 3589a6a?

(FYI, I need to rebase onto main again bco. GH-26202)

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Aug 25, 2021•
edited
Loading

I'll sync with main after #27942 is merged. => synced with a3c45b6

Spacetown
Spacetown reviewed Aug 26, 2021
View reviewed changes
Spacetown
Spacetown reviewed Aug 26, 2021
View reviewed changes
pablogsal
pablogsal approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@pablogsalpablogsal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Good work!

@pablogsalpablogsal merged commit 86d8b46 into python:mainAug 30, 2021
@erlend-aaslanderlend-aasland deleted the sqlite-expose-error-code branch August 30, 2021 19:04
@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Aug 30, 2021

Thanks Pablo, and you can thank @danielshahaf & @palaviv for the good work; I just "stole" it, rebased onto main and made some minor adjustments :)

@erlend-aaslanderlend-aasland mentioned this pull request Aug 30, 2021
Closed
@erlend-aaslanderlend-aasland mentioned this pull request Oct 7, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shihai1991shihai1991shihai1991 approved these changes

@pablogsalpablogsalpablogsal approved these changes

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksagberkerpeksag is a code owner

+2 more reviewers

@SpacetownSpacetownSpacetown left review comments

@auvipyauvipyauvipy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@erlend-aasland@auvipy@shihai1991@pablogsal@Spacetown@the-knights-who-say-ni@bedevere-bot@palaviv