classification
Title: [sqlite3] Improve backup error handling
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.10
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, erlendaasland
Priority: normal Keywords: patch

Created on 2021-02-19 09:10 by erlendaasland, last changed 2021-02-20 17:29 by berker.peksag.

Pull Requests
URL Status Linked Edit
PR 24586 open erlendaasland, 2021-02-19 19:23
Messages (8)
msg387297 - (view) Author: Erlend Egeberg Aasland (erlendaasland) * Date: 2021-02-19 09:10
There are some issues with the error handling in pysqlite_connection_backup_impl():

1. ValueError is returned if the target connection equals source connection. Should be OperationalError, IMHO.

2. The aforementioned check is already performed by sqlite3_backup_init(), so we should just let SQLite take care of it and let _pysqlite_seterror() set the error if sqlite3_backup_init() returns NULL. This will also take care of 1.

3. The following comment seems to be wrong; errors are set on the connection object, not on the backup handle:
  /* We cannot use _pysqlite_seterror() here because the backup APIs do
     not set the error status on the connection object, but rather on
     the backup handle. */

After sqlite3_backup_finish(), we can just check the return code, and call _pysqlite_seterror() on the connection and return NULL. The mentioned comment can be removed.


Resolving these issues will save 18 lines of code, and make the backup function easier to maintain.


Berker?
msg387329 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2021-02-19 15:09
I'm not sure about 1) because if target == source it means a user error. OperationalError is usually used for non-user errors.

3) sounds good to me if that part is already covered by tests and they passed.
msg387343 - (view) Author: Erlend Egeberg Aasland (erlendaasland) * Date: 2021-02-19 19:01
> I'm not sure about 1) because if target == source it means a user error. OperationalError is usually used for non-user errors.

Yes, my bad. ProgrammingError would be better.
target == source results in SQLITE_ERROR, BTW.

I'll throw up a PR for 3., then.
msg387357 - (view) Author: Erlend Egeberg Aasland (erlendaasland) * Date: 2021-02-19 21:17
The unit test suite shows one case of improved "exception text". I'd say it's an improvement.

$ ./python.exe  # with GH-24586 applied
>>> import sqlite3
>>> c1 = sqlite3.connect(":memory:")
>>> c2 = sqlite3.connect(":memory:")
>>> c1.backup(c2, name="non-existing")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
sqlite3.OperationalError: unknown database non-existing

$ python3.10  # latest alpha from python.org
>>> import sqlite3
>>> c1 = sqlite3.connect(":memory:")
>>> c2 = sqlite3.connect(":memory:")
>>> c1.backup(c2, name="non-existing")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
sqlite3.OperationalError: SQL logic error
msg387416 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2021-02-20 17:00
I'd even consider a cryptic error message a minor usability bug and backport it to at least 3.10.
msg387418 - (view) Author: Erlend Egeberg Aasland (erlendaasland) * Date: 2021-02-20 17:16
3.9, I presume? Yeah, I guess so.
msg387419 - (view) Author: Erlend Egeberg Aasland (erlendaasland) * Date: 2021-02-20 17:18
Regarding 1: After thinking about it, I see nothing wrong with raising ValueError error. No need to change current behaviour.
msg387421 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2021-02-20 17:28
> 3.9, I presume?

Yes, I was going too fast :)
History
Date User Action Args
2021-02-20 17:29:27berker.peksagsetfiles: - DOC-20200420-WA0000
2021-02-20 17:28:25berker.peksagsetmessages: + msg387421
2021-02-20 17:18:42erlendaaslandsetmessages: + msg387419
2021-02-20 17:16:52erlendaaslandsetmessages: + msg387418
2021-02-20 17:00:48berker.peksagsetmessages: + msg387416
2021-02-19 21:17:48erlendaaslandsetmessages: + msg387357
2021-02-19 20:13:38erlendaaslandsettitle: Improve sqlite3 backup error handling -> [sqlite3] Improve backup error handling
2021-02-19 19:23:43erlendaaslandsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23366
2021-02-19 19:01:49erlendaaslandsetmessages: + msg387343
2021-02-19 15:24:48avoropaev866setfiles: + DOC-20200420-WA0000
2021-02-19 15:09:10berker.peksagsetmessages: + msg387329
2021-02-19 09:10:02erlendaaslandcreate