GeezerVison

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

GeezerVison is showing github.com — bpo-32788: Better error handling in sqlite3. by serhiy-storchaka · Pull Request #3723 · python/cpython

Conversation

serhiy-storchaka commented Sep 24, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

https://bugs.python.org/issue32788

Sorry, something went wrong.

Uh oh!

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

All reactions


          bpo-31572: Get rid of PyDict_GetItem() and PyObject_HasAttrString() i…

e2a3c02

…n sqlite3.

serhiy-storchaka added skip news type-bug An unexpected behavior, bug, or error labels

Sep 24, 2017

the-knights-who-say-ni added the CLA signed label

Sep 24, 2017

bedevere-bot added the awaiting merge label

Sep 24, 2017

serhiy-storchaka changed the title ~~bpo-31572: Get rid of PyDict_GetItem() and PyObject_HasAttrString() in sqlite3.~~ bpo-31572: Get rid of PyDict_GetItem() and PyObject_HasAttrString() in sqlite3.

Sep 24, 2017

serhiy-storchaka added 5 commits

September 24, 2017 22:34


          Fix a refcount error.

423748a


          Fixed more suppressed errors.

4305fb7


          Merge branch 'master' into sqlite-no-hasattr

f102d23


          Merge branch 'master' into sqlite-no-hasattr

c989ed9


          Use _PyObject_LookupAttrId().

dafc64d

serhiy-storchaka changed the title ~~bpo-31572: Get rid of PyDict_GetItem() and PyObject_HasAttrString() in sqlite3.~~ bpo-32788: Better error handling in sqlite3.

Feb 7, 2018

serhiy-storchaka added 4 commits

February 7, 2018 14:04


          Merge branch 'master' into sqlite-no-hasattr

929d5b5


          Merge branch 'master' into sqlite-no-hasattr

1c094a4


          Minor refactoring.

4585ae8


          Merge branch 'master' into sqlite-no-hasattr

87f5945

serhiy-storchaka requested a review from berkerpeksag

July 22, 2018 20:39

berkerpeksag reviewed

Jul 23, 2018

View reviewed changes

berkerpeksag left a comment

Copy link

Copy Markdown

Member

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 old computer with no SQLite installed at the moment, but I think most of the patch looks good and safe to me. I have a question and a couple of comments:

Did you notice these by reading the code or noticed while using the module? For example, pysqlite_cache_get() (although it's exposed the cache implementation is an implementation detail and basically useless for third-party uses: https://bugs.python.org/issue30262) is only used by _pysqlite_query_execute() and it only accepts a string object. The only exception that may be raised here is OOM.
I'm fine with making old code PEP 7 compliant if you've already change the signature of the function, but in cases like below, I think it's better to keep the old code as-is:
```
-static int check_cursor(pysqlite_Cursor* cur)
+static int
+check_cursor(pysqlite_Cursor* cur)
```
The rest is fine because you've added static to them.
I haven't look at the code in microprotocols.c recently, so I still need a bit more time to review it and the code relevant to it (e.g. statement.c)

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka commented Jul 23, 2018

Copy link

Copy Markdown

Member Author

Did you notice these by reading the code or noticed while using the module?

By reading the code. Initially I wanted to only fix cases with PyDict_GetItem() and PyObject_HasAttrString(). Then noticed that exceptions are silenced in other cases, and couldn't stop until analyzed all code that silences or replaces exceptions, and also other suspicious code in the _sqlite3 module.

I think it's better to keep the old code as-is:

Do you mean just this function or all cases in which the only change is inserting a newline before the function name?

I haven't look at the code in microprotocols.c recently, so I still need a bit more time to review it and the code relevant to it (e.g. statement.c)

PEP 246 was rejected as a general protocol, but I think it is worth to fix its concrete implementation in sqlite3.

This PR is purposed for the master branch only, because of the chance of breaking user code. But some changes can be backported. I am wondering if it is worth to document and test some user-visible behavior changes. I can split this PR on several parts if you prefer.

All reactions

Sorry, something went wrong.

Uh oh!

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

berkerpeksag commented Jul 23, 2018

Copy link

Copy Markdown

Member

Do you mean just this function or all cases in which the only change is inserting a newline before the function name?

I missed that pysqlite_cursor_init() and pysqlite_cursor_dealloc() were already have static keyword. So, let's revert back to the old style in the following functions:

check_cursor()
pysqlite_cursor_init()
pysqlite_cursor_dealloc()

I am wondering if it is worth to document and test some user-visible behavior changes.

I think that would be great if you have time to do so :)

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka added 2 commits

July 23, 2018 10:37


          Keep the old formatting.

369164a


          Add tests and a news entry.

c5bc2a6

serhiy-storchaka force-pushed the sqlite-no-hasattr branch from dd78e66 to c5bc2a6 Compare

July 23, 2018 09:38


          Merge branch 'master' into sqlite-no-hasattr

e7e4112

serhiy-storchaka commented Dec 4, 2018

Copy link

Copy Markdown

Member Author

@berkerpeksag, have I addressed all your comments? Could you please take a look at this PR again?

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka commented Dec 10, 2018

Copy link

Copy Markdown

Member Author

I am going to merge this PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

berkerpeksag approved these changes

Dec 10, 2018

View reviewed changes

berkerpeksag left a comment

Copy link

Copy Markdown

Member

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, thanks! I'd like to deprecate and eventually get rid of this pre-PEP 246 protocol.

BTW, I'm moving to a new country in a month, so I don't have time to closely check my GitHub notifications. Feel free to send me an email if I'm blocking a pull request.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka commented Dec 10, 2018

Copy link

Copy Markdown

Member Author

Good luck with your move! 🚋 ✈️ 🚗 🏠 👍

berkerpeksag reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka merged commit fc662ac into python:master

Dec 10, 2018

bedevere-bot removed the awaiting merge label

Dec 10, 2018

serhiy-storchaka deleted the sqlite-no-hasattr branch

December 10, 2018 14:06

serhiy-storchaka mentioned this pull request

Apr 10, 2022

Avoid suppressing all exceptions in PyObject_HasAttr() #75753

Closed

Reviewers

berkerpeksag

berkerpeksag approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news 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

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.

GeezerVison

Conversation

serhiy-storchaka commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Jul 23, 2018

Uh oh!

berkerpeksag commented Jul 23, 2018

Uh oh!

serhiy-storchaka commented Dec 4, 2018

Uh oh!

serhiy-storchaka commented Dec 10, 2018

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Dec 10, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

serhiy-storchaka commented Sep 24, 2017 •

edited

Loading

Uh oh!

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