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: Improve error handling in sqlite3 and change some errors #27654

Merged
merged 2 commits into from
Aug 8, 2021

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Aug 7, 2021

  • MemoryError is now raised instead of sqlite3.Warning when
    memory is not enough for encoding a statement to UTF-8
    in Connection.call() and Cursor.execute().
  • UnicodEncodeError is now raised instead of sqlite3.Warning when
    the statement contains surrogate characters
    in Connection.__call__() and Cursor.execute().
  • TypeError is now raised instead of ValueError for non-string
    script argument in Cursor.executescript().
  • ValueError is now raised for script containing the null
    character instead of truncating it in Cursor.executescript().
  • Correctly handle exceptions raised when getting boolean value
    of the result of the progress handler.
  • Add may tests covering different exceptional cases.

https://bugs.python.org/issue44859

* MemoryError is now raised instead of sqlite3.Warning when
  memory is not enough for encoding a statement to UTF-8
  in Connection.__call__() and Cursor.execute().
* UnicodEncodeError is now raised instead of sqlite3.Warning when
  the statement contains surrogate characters
  in Connection.__call__() and Cursor.execute().
* TypeError is now raised instead of ValueError for non-string
  script argument in Cursor.executescript().
* ValueError is now raised for script containing the null
  character instead of truncating it in Cursor.executescript().
* Correctly handle exceptions raised when getting boolean value
  of the result of the progress handler.
* Add may tests covering different exceptional cases.
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM, in general. Region coverage for util.c is now above 80%; sqlite3 coverage is getting better and better.

I left some minor comments.

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
}
} else {
PyErr_SetString(PyExc_ValueError, "script argument must be unicode.");
sql_len = strlen(sql_script);
Copy link
Contributor

@erlend-aasland erlend-aasland Aug 7, 2021

Choose a reason for hiding this comment

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

What about adding a custom AC converter that can extract the string length, so we don't have to iterate over the string again? Is it worth it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is not worth. The code is simpler now (some checks were delegated to Argument Clinic), and it I prefer simplicity at the cost of additional strlen(). Custom converter would make it more complex than the original code. strlen() should be called in any case to check for embedded null characters. Now it is called 2 times instead of 1 time. The overhead is not too large.

sql_len = strlen(sql_script);
int max_length = sqlite3_limit(self->connection->db,
SQLITE_LIMIT_LENGTH, -1);
if (sql_len >= (unsigned)max_length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (sql_len >= (unsigned)max_length) {
if (sql_len >= (size_t)max_length) {

Copy link
Member Author

Choose a reason for hiding this comment

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

How does it convert: int -> unsigned -> size_t or int -> ssize_t -> size_t? The standard specifies what the intermediate conversion the compiler should used, but without looking into it I cannot say that it will be the correct one.

If convert explicitly to unsigned, the conversion sequence will be unambiguous: int -> unsigned -> size_t.

@erlend-aasland
Copy link
Contributor

BTW, that's a neat enhancement of the with_traceback decorator!

@erlend-aasland
Copy link
Contributor

We should consider changing some more errors, as discussed in #27642 (comment), #27645 (comment), and #27645 (comment)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@serhiy-storchaka serhiy-storchaka merged commit 0eec627 into python:main Aug 8, 2021
@serhiy-storchaka serhiy-storchaka deleted the sqlite-errors branch August 8, 2021 05:49
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants