| msg281066 - (view) |
Author: Peter Inglesby (inglesp) * |
Date: 2016-11-17 23:25 |
| The following code raises `sqlite3.InterfaceError: Error binding parameter 0 - probably unsupported type.` when I would expect it to raise `AssertionError: Problem in adapter`. import sqlite3 class Point: def __init__(self, x, y): self.x, self.y = x, y def adapt_point(point): assert False, 'Problem in adapter' sqlite3.register_adapter(Point, adapt_point) con = sqlite3.connect(":memory:") cur = con.cursor() p = Point(4.0, -3.2) cur.execute("select ?", (p,)) |
|
|
| msg281068 - (view) |
Author: Julien Palard (mdk) *  |
Date: 2016-11-18 00:02 |
| Problems looks from `Modules/_sqlite/statement.c`: ``` if (!_need_adapt(current_param)) { adapted = current_param; } else { adapted = pysqlite_microprotocols_adapt(current_param, (PyObject*)&pysqlite_PrepareProtocolType, NULL); if (adapted) { Py_DECREF(current_param); } else { PyErr_Clear(); adapted = current_param; } } ``` It has not changed since 2006, since e200ab5b3e (backport from pysqlite2 SVN repository). I read it as "If an parameter needs an adapter, and the adapter fails, that's OK, continue with the original (unadapted) parameter.", which looks wrong to me, but I may miss something obvious? I tried to set an error instead of restoring the `current_param` and the 261 tests went well, but I'm not yet aware of the coverage of adapters in tests. |
|
|
| msg281069 - (view) |
Author: Julien Palard (mdk) * |
Date: 2016-11-18 00:10 |
| I missed an occurrence of this "if/else" block, and by changing it, a lot of tests are failing, typically: ```====================================================================== ERROR: CheckBlob (sqlite3.test.types.SqliteTypeTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/mdk/cpython/Lib/sqlite3/test/types.py", line 72, in CheckBlob self.cur.execute("insert into test(b) values (?)", (val,)) sqlite3.InterfaceError: Problem in adapter ``` So this "if/else" giving back the unadapted parameter even when need_adapt is true is necessary, we'll have to understand why and how we can properly detect an adapter failure. |
|
|
| msg281176 - (view) |
Author: Julien Palard (mdk) * |
Date: 2016-11-18 22:40 |
| By moving: ``` /* else set the right exception and return NULL */ PyErr_SetString(pysqlite_ProgrammingError, "can't adapt"); ``` from `pysqlite_microprotocols_adapt` to `pysqlite_adapt` (to avoid changing the semantics from the outside) make the `pysqlite_microprotocols_adapt`protocol clearer: - if it went well return something - If it went well but had nothing to do: return NULL - If it broke, set an exception and return NULL It does not break any test. Then in `Modules/_sqlite/statement.c`, we can stop blindly muting exceptions with the `PyErr_Clear`s, replacing them with ``if (PyErr_Occurred()) return;``. Again it does not break any test. I added a test to check that if an adapter raises an exception it bubbles outside the execute method, and it passes. Sample code given by the issue now gives:: $ ./python .py Traceback (most recent call last): File ".py", line 18, in cur.execute("select ?", (p,)) File ".py", line 10, in adapt_point assert False, 'Problem in adapter' AssertionError: Problem in adapter I did not found precise documentation about pysqlite_microprotocols_adapt or pysqlite_adapt, so my changes may break a "well known protocol", but it looks safe as I do not change the _sqlite protocol as far as I can tell, (only when the exception comes directly from the adaptor, obviously), and there were no other uses of pysqlite_microprotocols_adapt, which is not exposed in the module. |
|
|
| msg341568 - (view) |
Author: Julien Palard (mdk) * |
Date: 2019-05-06 17:26 |
| This has been fixed in fc662ac332443a316a120fa5287c235dc4f8739b. |
|
|