From 2c9e96f15791ade3e55b3fdde1568c443476317b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 22 Jul 2022 19:54:16 +0200 Subject: [PATCH 1/9] Add test --- Lib/test/test_sqlite3/test_factory.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Lib/test/test_sqlite3/test_factory.py b/Lib/test/test_sqlite3/test_factory.py index 71603faa02840f..78edd095e8f65c 100644 --- a/Lib/test/test_sqlite3/test_factory.py +++ b/Lib/test/test_sqlite3/test_factory.py @@ -50,6 +50,15 @@ def __init__(self, *args, **kwargs): con = sqlite.connect(":memory:", factory=factory) self.assertIsInstance(con, factory) + def test_connection_factory_relayed_call(self): + # gh-95132: keyword args must not be passed as positional args + class Factory(sqlite.Connection): + def __init__(self, *args, **kwargs): + kwargs['timeout'] = 42 + super(Factory, self).__init__(*args, **kwargs) + + sqlite.connect(":memory:", factory=Factory) + class CursorFactoryTests(unittest.TestCase): def setUp(self): From 479959fb81f1a7ad783d626dbb3f88694233bab8 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 22 Jul 2022 21:12:31 +0200 Subject: [PATCH 2/9] [alt 1] gh-95132: partially revert 185ecdc --- Modules/_sqlite/clinic/module.c.h | 112 +----------------------------- Modules/_sqlite/module.c | 77 ++++++++++---------- 2 files changed, 42 insertions(+), 147 deletions(-) diff --git a/Modules/_sqlite/clinic/module.c.h b/Modules/_sqlite/clinic/module.c.h index ef0dd4d1c103ea..3e932a6117eb29 100644 --- a/Modules/_sqlite/clinic/module.c.h +++ b/Modules/_sqlite/clinic/module.c.h @@ -2,116 +2,6 @@ preserve [clinic start generated code]*/ -PyDoc_STRVAR(pysqlite_connect__doc__, -"connect($module, /, database, timeout=5.0, detect_types=0,\n" -" isolation_level=, check_same_thread=True,\n" -" factory=ConnectionType, cached_statements=128, uri=False)\n" -"--\n" -"\n" -"Opens a connection to the SQLite database file database.\n" -"\n" -"You can use \":memory:\" to open a database connection to a database that resides\n" -"in RAM instead of on disk."); - -#define PYSQLITE_CONNECT_METHODDEF \ - {"connect", _PyCFunction_CAST(pysqlite_connect), METH_FASTCALL|METH_KEYWORDS, pysqlite_connect__doc__}, - -static PyObject * -pysqlite_connect_impl(PyObject *module, PyObject *database, double timeout, - int detect_types, PyObject *isolation_level, - int check_same_thread, PyObject *factory, - int cached_statements, int uri); - -static PyObject * -pysqlite_connect(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) -{ - PyObject *return_value = NULL; - static const char * const _keywords[] = {"database", "timeout", "detect_types", "isolation_level", "check_same_thread", "factory", "cached_statements", "uri", NULL}; - static _PyArg_Parser _parser = {NULL, _keywords, "connect", 0}; - PyObject *argsbuf[8]; - Py_ssize_t noptargs = nargs + (kwnames ? PyTuple_GET_SIZE(kwnames) : 0) - 1; - PyObject *database; - double timeout = 5.0; - int detect_types = 0; - PyObject *isolation_level = NULL; - int check_same_thread = 1; - PyObject *factory = (PyObject*)clinic_state()->ConnectionType; - int cached_statements = 128; - int uri = 0; - - args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 1, 8, 0, argsbuf); - if (!args) { - goto exit; - } - database = args[0]; - if (!noptargs) { - goto skip_optional_pos; - } - if (args[1]) { - if (PyFloat_CheckExact(args[1])) { - timeout = PyFloat_AS_DOUBLE(args[1]); - } - else - { - timeout = PyFloat_AsDouble(args[1]); - if (timeout == -1.0 && PyErr_Occurred()) { - goto exit; - } - } - if (!--noptargs) { - goto skip_optional_pos; - } - } - if (args[2]) { - detect_types = _PyLong_AsInt(args[2]); - if (detect_types == -1 && PyErr_Occurred()) { - goto exit; - } - if (!--noptargs) { - goto skip_optional_pos; - } - } - if (args[3]) { - isolation_level = args[3]; - if (!--noptargs) { - goto skip_optional_pos; - } - } - if (args[4]) { - check_same_thread = _PyLong_AsInt(args[4]); - if (check_same_thread == -1 && PyErr_Occurred()) { - goto exit; - } - if (!--noptargs) { - goto skip_optional_pos; - } - } - if (args[5]) { - factory = args[5]; - if (!--noptargs) { - goto skip_optional_pos; - } - } - if (args[6]) { - cached_statements = _PyLong_AsInt(args[6]); - if (cached_statements == -1 && PyErr_Occurred()) { - goto exit; - } - if (!--noptargs) { - goto skip_optional_pos; - } - } - uri = PyObject_IsTrue(args[7]); - if (uri < 0) { - goto exit; - } -skip_optional_pos: - return_value = pysqlite_connect_impl(module, database, timeout, detect_types, isolation_level, check_same_thread, factory, cached_statements, uri); - -exit: - return return_value; -} - PyDoc_STRVAR(pysqlite_complete_statement__doc__, "complete_statement($module, /, statement)\n" "--\n" @@ -292,4 +182,4 @@ pysqlite_adapt(PyObject *module, PyObject *const *args, Py_ssize_t nargs) exit: return return_value; } -/*[clinic end generated code: output=9ac18606b0eaec03 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=d7f142e9a7a80468 input=a9049054013a1b77]*/ diff --git a/Modules/_sqlite/module.c b/Modules/_sqlite/module.c index eca25b94a4817d..92d83283dc407d 100644 --- a/Modules/_sqlite/module.c +++ b/Modules/_sqlite/module.c @@ -42,47 +42,52 @@ module _sqlite3 [clinic start generated code]*/ /*[clinic end generated code: output=da39a3ee5e6b4b0d input=81e330492d57488e]*/ -// NOTE: This must equal sqlite3.Connection.__init__ argument spec! -/*[clinic input] -_sqlite3.connect as pysqlite_connect - - database: object - timeout: double = 5.0 - detect_types: int = 0 - isolation_level: object = NULL - check_same_thread: bool(accept={int}) = True - factory: object(c_default='(PyObject*)clinic_state()->ConnectionType') = ConnectionType - cached_statements: int = 128 - uri: bool = False - -Opens a connection to the SQLite database file database. - -You can use ":memory:" to open a database connection to a database that resides -in RAM instead of on disk. -[clinic start generated code]*/ +PyDoc_STRVAR(pysqlite_connect__doc__, +"connect($module, /, database, timeout=5.0, detect_types=0,\n" +" isolation_level=, check_same_thread=True,\n" +" factory=ConnectionType, cached_statements=128, uri=False)\n" +"--\n" +"\n" +"Opens a connection to the SQLite database file database.\n" +"\n" +"You can use \":memory:\" to open a database connection to a database that resides\n" +"in RAM instead of on disk."); + +#define PYSQLITE_CONNECT_METHODDEF \ + {"connect", _PyCFunction_CAST(module_connect), METH_VARARGS|METH_KEYWORDS, pysqlite_connect__doc__}, static PyObject * -pysqlite_connect_impl(PyObject *module, PyObject *database, double timeout, - int detect_types, PyObject *isolation_level, - int check_same_thread, PyObject *factory, - int cached_statements, int uri) -/*[clinic end generated code: output=450ac9078b4868bb input=e16914663ddf93ce]*/ +module_connect(PyObject *self, PyObject *args, PyObject *kwargs) { - if (isolation_level == NULL) { - isolation_level = PyUnicode_FromString(""); - if (isolation_level == NULL) { - return NULL; - } + /* Python seems to have no way of extracting a single keyword-arg at + * C-level, so this code is redundant with the one in connection_init in + * connection.c and must always be copied from there ... */ + static char *kwlist[] = { + "database", "timeout", "detect_types", "isolation_level", + "check_same_thread", "factory", "cached_statements", "uri", + NULL + }; + PyObject *database; + int detect_types = 0; + PyObject *isolation_level; + PyObject *factory = NULL; + int check_same_thread = 1; + int cached_statements; + int uri = 0; + double timeout = 5.0; + + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|diOiOip", kwlist, + &database, &timeout, &detect_types, + &isolation_level, &check_same_thread, + &factory, &cached_statements, &uri)) + { + return NULL; } - else { - Py_INCREF(isolation_level); + if (factory == NULL) { + pysqlite_state *state = pysqlite_get_state(self); + factory = (PyObject *)state->ConnectionType; } - PyObject *res = PyObject_CallFunction(factory, "OdiOiOii", database, - timeout, detect_types, - isolation_level, check_same_thread, - factory, cached_statements, uri); - Py_DECREF(isolation_level); - return res; + return PyObject_Call(factory, args, kwargs); } /*[clinic input] From 507e5cabf2b3597cf7c6695c8c851dadfd357f00 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 22 Jul 2022 21:18:23 +0200 Subject: [PATCH 3/9] Add NEWS --- .../Library/2022-07-22-21-18-17.gh-issue-95132.n9anlw.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-07-22-21-18-17.gh-issue-95132.n9anlw.rst diff --git a/Misc/NEWS.d/next/Library/2022-07-22-21-18-17.gh-issue-95132.n9anlw.rst b/Misc/NEWS.d/next/Library/2022-07-22-21-18-17.gh-issue-95132.n9anlw.rst new file mode 100644 index 00000000000000..64666ad84fb4a6 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-07-22-21-18-17.gh-issue-95132.n9anlw.rst @@ -0,0 +1,4 @@ +Fix a :mod:`sqlite3` regression where ``*args`` and ``**kwds`` were +incorrectly relayed from :py:func:`~sqlite3.connect` to the +:class:`~sqlite3.Connection` factory. The regression was introduced in 3.11a1 +with PR 24421 (:gh:`85128`). Patch by Erlend E. Aasland.` From 95296cb66c758db921b78642d91ed9d51092c603 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 22 Jul 2022 21:23:45 +0200 Subject: [PATCH 4/9] Improve test --- Lib/test/test_sqlite3/test_factory.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_sqlite3/test_factory.py b/Lib/test/test_sqlite3/test_factory.py index 78edd095e8f65c..1a475ba57e0944 100644 --- a/Lib/test/test_sqlite3/test_factory.py +++ b/Lib/test/test_sqlite3/test_factory.py @@ -54,10 +54,11 @@ def test_connection_factory_relayed_call(self): # gh-95132: keyword args must not be passed as positional args class Factory(sqlite.Connection): def __init__(self, *args, **kwargs): - kwargs['timeout'] = 42 + kwargs["isolation_level"] = None super(Factory, self).__init__(*args, **kwargs) - sqlite.connect(":memory:", factory=Factory) + con = sqlite.connect(":memory:", factory=Factory) + self.assertIsNone(con.isolation_level) class CursorFactoryTests(unittest.TestCase): From b8ef03dacc194306fc6e2873620157e996da2695 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 22 Jul 2022 21:55:59 +0200 Subject: [PATCH 5/9] [alt 2] gh-95132: use vectorcall --- Modules/_sqlite/module.c | 51 +++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/Modules/_sqlite/module.c b/Modules/_sqlite/module.c index 92d83283dc407d..f930b691857d23 100644 --- a/Modules/_sqlite/module.c +++ b/Modules/_sqlite/module.c @@ -42,7 +42,7 @@ module _sqlite3 [clinic start generated code]*/ /*[clinic end generated code: output=da39a3ee5e6b4b0d input=81e330492d57488e]*/ -PyDoc_STRVAR(pysqlite_connect__doc__, +PyDoc_STRVAR(module_connect_doc, "connect($module, /, database, timeout=5.0, detect_types=0,\n" " isolation_level=, check_same_thread=True,\n" " factory=ConnectionType, cached_statements=128, uri=False)\n" @@ -54,40 +54,31 @@ PyDoc_STRVAR(pysqlite_connect__doc__, "in RAM instead of on disk."); #define PYSQLITE_CONNECT_METHODDEF \ - {"connect", _PyCFunction_CAST(module_connect), METH_VARARGS|METH_KEYWORDS, pysqlite_connect__doc__}, + {"connect", _PyCFunction_CAST(module_connect), METH_FASTCALL|METH_KEYWORDS, module_connect_doc}, static PyObject * -module_connect(PyObject *self, PyObject *args, PyObject *kwargs) +module_connect(PyObject *module, PyObject *const *args, Py_ssize_t nargsf, + PyObject *kwnames) { - /* Python seems to have no way of extracting a single keyword-arg at - * C-level, so this code is redundant with the one in connection_init in - * connection.c and must always be copied from there ... */ - static char *kwlist[] = { - "database", "timeout", "detect_types", "isolation_level", - "check_same_thread", "factory", "cached_statements", "uri", - NULL - }; - PyObject *database; - int detect_types = 0; - PyObject *isolation_level; - PyObject *factory = NULL; - int check_same_thread = 1; - int cached_statements; - int uri = 0; - double timeout = 5.0; - - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|diOiOip", kwlist, - &database, &timeout, &detect_types, - &isolation_level, &check_same_thread, - &factory, &cached_statements, &uri)) - { - return NULL; + pysqlite_state *state = pysqlite_get_state(module); + PyObject *factory = (PyObject *)state->ConnectionType; + + static const int FACTORY_POS = 5; + Py_ssize_t nargs = PyVectorcall_NARGS(nargsf); + if (nargs > FACTORY_POS) { + factory = args[FACTORY_POS]; } - if (factory == NULL) { - pysqlite_state *state = pysqlite_get_state(self); - factory = (PyObject *)state->ConnectionType; + else if (kwnames != NULL) { + for (Py_ssize_t i = 0; i < PyTuple_GET_SIZE(kwnames); i++) { + PyObject *item = PyTuple_GET_ITEM(kwnames, i); // borrowed ref. + if (PyUnicode_CompareWithASCIIString(item, "factory") == 0) { + factory = args[nargs + i]; + break; + } + } } - return PyObject_Call(factory, args, kwargs); + + return PyObject_Vectorcall(factory, args, nargsf, kwnames); } /*[clinic input] From b144689720685137f303287ae3dfaedb92ddbe42 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 22 Jul 2022 21:57:43 +0200 Subject: [PATCH 6/9] Add notes re docstrings --- Modules/_sqlite/connection.c | 1 + Modules/_sqlite/module.c | 1 + 2 files changed, 2 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index b6b7417fc0a6c1..ceb77bbf8420ff 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -140,6 +140,7 @@ class IsolationLevel_converter(CConverter): [python start generated code]*/ /*[python end generated code: output=da39a3ee5e6b4b0d input=cbcfe85b253061c2]*/ +// NB: This needs to be in sync with the sqlite3.connect docstring /*[clinic input] _sqlite3.Connection.__init__ as pysqlite_connection_init diff --git a/Modules/_sqlite/module.c b/Modules/_sqlite/module.c index f930b691857d23..8e44b1393292fa 100644 --- a/Modules/_sqlite/module.c +++ b/Modules/_sqlite/module.c @@ -42,6 +42,7 @@ module _sqlite3 [clinic start generated code]*/ /*[clinic end generated code: output=da39a3ee5e6b4b0d input=81e330492d57488e]*/ +// NB: This needs to be in sync with the Connection.__init__ docstring. PyDoc_STRVAR(module_connect_doc, "connect($module, /, database, timeout=5.0, detect_types=0,\n" " isolation_level=, check_same_thread=True,\n" From bef0572253d59a2106eb42c545808a9a7387ddce Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 22 Jul 2022 22:37:50 +0200 Subject: [PATCH 7/9] Further improve tests --- Lib/test/test_sqlite3/test_factory.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Lib/test/test_sqlite3/test_factory.py b/Lib/test/test_sqlite3/test_factory.py index 1a475ba57e0944..7fdc45ab69243d 100644 --- a/Lib/test/test_sqlite3/test_factory.py +++ b/Lib/test/test_sqlite3/test_factory.py @@ -59,6 +59,16 @@ def __init__(self, *args, **kwargs): con = sqlite.connect(":memory:", factory=Factory) self.assertIsNone(con.isolation_level) + self.assertIsInstance(con, Factory) + + def test_connection_factory_as_positional_arg(self): + class Factory(sqlite.Connection): + def __init__(self, *args, **kwargs): + super(Factory, self).__init__(*args, **kwargs) + + con = sqlite.connect(":memory:", 5.0, 0, None, True, Factory) + self.assertIsNone(con.isolation_level) + self.assertIsInstance(con, Factory) class CursorFactoryTests(unittest.TestCase): From cf5da8efe813e449a8356bbe0327bcd25278e03d Mon Sep 17 00:00:00 2001 From: Erlend Egeberg Aasland Date: Sat, 23 Jul 2022 08:34:04 +0200 Subject: [PATCH 8/9] Fix isolation_level default value --- Modules/_sqlite/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/module.c b/Modules/_sqlite/module.c index 8e44b1393292fa..431e51b0ada7fc 100644 --- a/Modules/_sqlite/module.c +++ b/Modules/_sqlite/module.c @@ -45,7 +45,7 @@ module _sqlite3 // NB: This needs to be in sync with the Connection.__init__ docstring. PyDoc_STRVAR(module_connect_doc, "connect($module, /, database, timeout=5.0, detect_types=0,\n" -" isolation_level=, check_same_thread=True,\n" +" isolation_level="", check_same_thread=True,\n" " factory=ConnectionType, cached_statements=128, uri=False)\n" "--\n" "\n" From ca41429baa49d3381fbbe441e8093a3a2c7003f2 Mon Sep 17 00:00:00 2001 From: Erlend Egeberg Aasland Date: Sat, 23 Jul 2022 08:59:04 +0200 Subject: [PATCH 9/9] Update Modules/_sqlite/module.c Co-authored-by: Serhiy Storchaka --- Modules/_sqlite/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/module.c b/Modules/_sqlite/module.c index 431e51b0ada7fc..707aae160ab4f3 100644 --- a/Modules/_sqlite/module.c +++ b/Modules/_sqlite/module.c @@ -45,7 +45,7 @@ module _sqlite3 // NB: This needs to be in sync with the Connection.__init__ docstring. PyDoc_STRVAR(module_connect_doc, "connect($module, /, database, timeout=5.0, detect_types=0,\n" -" isolation_level="", check_same_thread=True,\n" +" isolation_level='', check_same_thread=True,\n" " factory=ConnectionType, cached_statements=128, uri=False)\n" "--\n" "\n"