FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-45089: raise exceptions on error in `sqlite3` trace callback by erlend-aasland · Pull Request #28133 · python/cpython · GitHub

/ cpython Public

bpo-45089: raise exceptions on error in sqlite3 trace callback - #28133

Closed
erlend-aasland wants to merge 3 commits into
python:mainfrom
erlend-aasland:sqlite-trace-exception
Closed

bpo-45089: raise exceptions on error in sqlite3 trace callback#28133
erlend-aasland wants to merge 3 commits into
python:mainfrom
erlend-aasland:sqlite-trace-exception

Conversation

erlend-aasland commented Sep 2, 2021
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Contributor Author

I see no way to test PyUnicode_DecodeUTF8 errors: UTF8 errors (including too long strings) are handled when we create the statement.

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka I would like your opinion on this PR, if you don't mind.

encukou commented Sep 7, 2021

Copy link
Copy Markdown
Member

I assumed exceptions in a trace function were ignored on purpose, since there's no way to inform SQLite of the error?
Is it possible to get multiple trace function failures from one sqlite3 call? If so, what happens?

It would be nice to chain the original exception. It's a bit complicated to combine it with the enable_callback_tracebacks mechanism, but you could do something like:

        PyObject *type, *value, *traceback;
        // Clear the error indicator; get references to type, value, traceback
        PyErr_Fetch(&type, &value, &traceback);
        pysqlite_state *state = pysqlite_get_state(NULL);
        if (state->enable_callback_tracebacks) {
            // Get extra references to type, value & traceback
            Py_INCREF(type);
            Py_INCREF(value);
            Py_INCREF(traceback);
            // Restore the error indicator (losing the extra references)
            PyErr_Restore(type, value, traceback);
            // Print the error (clearing the error indicator)
            PyErr_Print();
        }
        PyErr_SetString(state->OperationalError,
                        "trace callback raised an exception");
        // Chain the original exception as __cause__ (losing our references)
        _PyErr_ChainExceptions(type, value, traceback);

And since since the user will get the original exception, I wonder if the printing could be skipped.

erlend-aasland commented Sep 7, 2021
edited
Loading

Copy link
Copy Markdown
Contributor Author

I assumed exceptions in a trace function were ignored on purpose, since there's no way to inform SQLite of the error?

Maybe so, but how else should we inform the user about bugs in trace functions if not through exceptions?

It would be nice to chain the original exception

Yes, good point.

encukou commented Sep 7, 2021

Copy link
Copy Markdown
Member

how else should we inform the user about bugs in trace functions if not through exceptions?

AFAIK, enable_callback_tracebacks is there exactly for that. It's crude but it seems to work; doing this right won't be an easy change.

Is it possible to get multiple trace function failures from one sqlite3 call?

It is:

>>> conn.executemany('INSERT INTO foo (bar) VALUES (?)', [(1,), (2,), (3,)])
Traceback (most recent call last):
  File "<stdin>", line 1, in <lambda>
ZeroDivisionError: division by zero
Traceback (most recent call last):
  File "<stdin>", line 1, in <lambda>
ZeroDivisionError: division by zero
Traceback (most recent call last):
  File "<stdin>", line 1, in <lambda>
ZeroDivisionError: division by zero
<sqlite3.Cursor object at 0x7faea47f8f80>

Without the ability to tell SQLite to abort on trace failure, I doubt we can do much better than the current behavior. Getting an exception and having the data inserted seems quite irregular.

Copy link
Copy Markdown
Contributor Author

AFAIK, enable_callback_tracebacks is there exactly for that.

Well, it kinda works, but you won't get an exception. However, getting an exception and having the database modified is worse.

I doubt we can do much better than the current behavior.

I do agree.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL