bpo-32788: Better error handling in sqlite3.

[serhiy-storchaka]

https://bugs.python.org/issue32788

[berkerpeksag]

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)

[serhiy-storchaka]

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.

[berkerpeksag]

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 :)

[serhiy-storchaka]

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

[serhiy-storchaka]

I am going to merge this PR.

[berkerpeksag]

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.

[serhiy-storchaka]

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