The Wayback Machine - https://web.archive.org/web/20240112011513/https://github.com/python/cpython/pull/113762
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-98893: Improved error messages for sqlite3 UDFs #113762

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 6, 2024

Comment on lines 1000 to 1008
if (msg == NULL) {
// An exception happened while we tried to build a more detailed
// error message; write this as an unraisable exception and pass
// the preamble as the sqlite3 error message.
void *data = sqlite3_user_data(context);
callback_context *ctx = (callback_context *)data;
PyErr_WriteUnraisable(ctx->callable);
sqlite3_result_error(context, preamble, -1);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to test this, for example like this:

diff --git a/Lib/test/test_sqlite3/test_userfunctions.py b/Lib/test/test_sqlite3/test_userfunctions.py
index 67a13ad38b..d8b8c0a40e 100644
--- a/Lib/test/test_sqlite3/test_userfunctions.py
+++ b/Lib/test/test_sqlite3/test_userfunctions.py
@@ -24,6 +24,7 @@
 import sys
 import unittest
 import sqlite3 as sqlite
+import test.support
 
 from unittest.mock import Mock, patch
 from test.support import bigmemtest, gc_collect
@@ -35,6 +36,10 @@
 class CustomException(Exception):
     pass
 
+class EvilException(Exception):
+    def __str__(self):
+        return -1
+
 def func_returntext():
     return "foo"
 def func_returntextwithnull():
@@ -53,6 +58,8 @@ def func_returnlonglong():
     return 1<<31
 def func_raiseexception():
     raise CustomException("func except")
+def func_raiseevil():
+    raise EvilException("my __str__() does not return a string")
 def func_memoryerror():
     raise MemoryError
 def func_overflowerror():
@@ -162,6 +169,7 @@ def setUp(self):
         self.con.create_function("return_noncont_blob", 0,
                                  lambda: memoryview(b"blob")[::2])
         self.con.create_function("raiseexception", 0, func_raiseexception)
+        self.con.create_function("raiseevil", 0, func_raiseevil)
         self.con.create_function("memoryerror", 0, func_memoryerror)
         self.con.create_function("overflowerror", 0, func_overflowerror)
 
@@ -265,6 +273,18 @@ def test_func_exception(self):
             cur.execute("select raiseexception()")
             cur.fetchone()
 
+    @with_tracebacks(EvilException, name="func_raiseevil")
+    def test_func_evilexception(self):
+        cur = self.con.cursor()
+        # Since it is impossible to fetch a string with the exception message
+        # from EvilException, we will only receive the generic preamble
+        # exception message, hence the detailed regex.
+        msg = "^user-defined function raised exception$"
+        with self.assertRaisesRegex(sqlite.OperationalError, msg):
+            with test.support.catch_unraisable_exception() as cm:
+                cur.execute("select raiseevil()")
+                cur.fetchone()
+
     @with_tracebacks(MemoryError, name="func_memoryerror")

Unfortunately, our @with_tracebacks decorator gets in the way. I'll think of a way to solve that (we can also handle it later in a follow-up PR).

@erlend-aasland
Copy link
Contributor Author

@serhiy-storchaka, would you like to take a look at this? I'm not fully satisfied with it yet, but I think it is ready for an initial round of review.

I believe the error messages can be tuned to be slightly nicer. Also, I'd like to add coverage for one of the special cases, as mentioned in #113762 (comment).

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
}

static char *
build_error_message(const char *preamble, const char *message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be simpler to use PyUnicode_Format().

Also, it is worth to include also the type name of the original error, not only its message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, those are good suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants