GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44859: Improve error handling in sqlite3 and change some errors by serhiy-storchaka · Pull Request #27654 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Aug 7, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
bpo-44859: Improve error handling in sqlite3 and change some errors
75d79c2 
* 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.
@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error tests Tests in the Lib/test dir labels Aug 7, 2021
@serhiy-storchaka
serhiy-storchaka requested a review from erlend-aasland August 7, 2021 14:33
@serhiy-storchaka
serhiy-storchaka requested a review from berkerpeksag as a code owner August 7, 2021 14:33
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 7, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Aug 7, 2021
This was referenced Aug 7, 2021
bpo-43853: Expand test suite for SQLite UDF's #27642
Merged
gh-89018: Improve documentation of sqlite3 exceptions #27645
Merged
erlend-aasland
erlend-aasland reviewed Aug 7, 2021
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

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

I left some minor comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2021-08-07-17-28-56.bpo-44859.CCopjk.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Library/2021-08-07-17-28-56.bpo-44859.CCopjk.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/cursor.c
}
} else {
PyErr_SetString(PyExc_ValueError, "script argument must be unicode.");
sql_len = strlen(sql_script);

@erlend-aasland erlend-aasland Aug 7, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Aug 7, 2021

Copy link
Copy Markdown
Member Author

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/_sqlite/cursor.c
sql_len = strlen(sql_script);
int max_length = sqlite3_limit(self->connection->db,
SQLITE_LIMIT_LENGTH, -1);
if (sql_len >= (unsigned)max_length) {

@erlend-aasland erlend-aasland Aug 7, 2021

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Aug 7, 2021

Copy link
Copy Markdown
Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@erlend-aasland

erlend-aasland commented Aug 7, 2021

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Aug 7, 2021

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka Erlend Egeberg Aasland
Apply suggestions from code review
664b14e 
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
erlend-aasland
erlend-aasland approved these changes Aug 7, 2021
View reviewed changes
@serhiy-storchaka
serhiy-storchaka merged commit 0eec627 into python:main Aug 8, 2021
@bedevere-bot bedevere-bot removed the awaiting core review label 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
Erlend E. Aasland
Sync with 'main' bco. pythonGH-27654
476edeb
@serhiy-storchaka serhiy-storchaka mentioned this pull request May 16, 2022
Improve some sqlite3 errors #89022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@serhiy-storchaka @erlend-aasland @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.