Skip to content

bpo-40956: Convert sqlite3.connect and sqlite3.Connection.__init__ to AC#24421

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.

Sign up for GitHub

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

Merged
pablogsal merged 17 commits into from
Jun 20, 2021
Merged

bpo-40956: Convert sqlite3.connect and sqlite3.Connection.__init__ to AC #24421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8192606
Convert _sqlite3.connect() and _sqlite3.Connection.__init__() to Argu…
Jun 14, 2020
b72a29c
Add NEWS
Feb 2, 2021
d361aeb
Add test for 'check_same_thread' keyword
Mar 8, 2021
a861358
Add test for 'database' keyword
Mar 8, 2021
1ccf5e5
Merge branch 'main' into bpo-40956/finale
Jun 5, 2021
07164cb
Harden
Jun 5, 2021
a1e88c9
Simplify ref handling
Jun 5, 2021
4977d78
Use PyObject_CallFunction iso. PyObject_CallFunctionObjArgs in sqlite…
Jun 5, 2021
1b2720f
Adjust comment
Jun 5, 2021
c7cd9ee
Merge branch 'main' into bpo-40956/finale
Jun 6, 2021
39ed36b
Remove unused var from test_dont_check_same_thread
Jun 6, 2021
754fdc6
Resolve conflict: cached_statements now defaults to 128, not 100
Jun 13, 2021
e59af3b
Clinic
Jun 13, 2021
ad345e0
Merge branch 'main' into bpo-40956/finale
Jun 15, 2021
af0fc7e
Address review: wrap test_dont_check_same_thread with threading_helpe…
Jun 15, 2021
98f1a9d
Explain Py_DECREF of converted arguments
Jun 17, 2021
89926af
Merge branch 'main' into bpo-40956/finale
Jun 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions Lib/sqlite3/test/dbapi.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
import unittest

from test.support.os_helper import TESTFN, unlink
from test.support import threading_helper


# Helper for tests using TESTFN
Expand DownExpand Up@@ -224,6 +225,10 @@ def test_open_uri(self):
with managed_connect(f"file:{TESTFN}?mode=ro", uri=True) as cx:
cx.execute(self._sql)

def test_database_keyword(self):
with sqlite.connect(database=":memory:") as cx:
self.assertEqual(type(cx), sqlite.Connection)


class CursorTests(unittest.TestCase):
def setUp(self):
Expand DownExpand Up@@ -724,6 +729,22 @@ def run(cur, errors):
if len(errors) > 0:
self.fail("\n".join(errors))

@threading_helper.reap_threads
def test_dont_check_same_thread(self):
def run(con, err):
try:
con.execute("select 1")
except sqlite.Error:
err.append("multi-threading not allowed")

con = sqlite.connect(":memory:", check_same_thread=False)
err = []
t = threading.Thread(target=run, kwargs={"con": con, "err": err})
t.start()
t.join()
self.assertEqual(len(err), 0, "\n".join(err))


class ConstructorTests(unittest.TestCase):
def test_date(self):
d = sqlite.Date(2004, 10, 28)
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2020-10-01-21-46-34.bpo-40956._tvsZ7.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
Use Argument Clinic in :mod:`sqlite3`. Patches by Erlend E. Aasland.
103 changes: 102 additions & 1 deletion Modules/_sqlite/clinic/connection.c.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,6 +2,107 @@
preserve
[clinic start generated code]*/

static int
pysqlite_connection_init_impl(pysqlite_Connection *self,
PyObject *database_obj, double timeout,
int detect_types, PyObject *isolation_level,
int check_same_thread, PyObject *factory,
int cached_statements, int uri);

static int
pysqlite_connection_init(PyObject *self, PyObject *args, PyObject *kwargs)
{
int return_value = -1;
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, "Connection", 0};
PyObject *argsbuf[8];
PyObject * const *fastargs;
Py_ssize_t nargs = PyTuple_GET_SIZE(args);
Py_ssize_t noptargs = nargs + (kwargs ? PyDict_GET_SIZE(kwargs) : 0) - 1;
PyObject *database_obj;
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;

fastargs = _PyArg_UnpackKeywords(_PyTuple_CAST(args)->ob_item, nargs, kwargs, NULL, &_parser, 1, 8, 0, argsbuf);
if (!fastargs){
goto exit;
}
if (!PyUnicode_FSConverter(fastargs[0], &database_obj)){
goto exit;
}
if (!noptargs){
goto skip_optional_pos;
}
if (fastargs[1]){
if (PyFloat_CheckExact(fastargs[1])){
timeout = PyFloat_AS_DOUBLE(fastargs[1]);
}
else
{
timeout = PyFloat_AsDouble(fastargs[1]);
if (timeout == -1.0 && PyErr_Occurred()){
goto exit;
}
}
if (!--noptargs){
goto skip_optional_pos;
}
}
if (fastargs[2]){
detect_types = _PyLong_AsInt(fastargs[2]);
if (detect_types == -1 && PyErr_Occurred()){
goto exit;
}
if (!--noptargs){
goto skip_optional_pos;
}
}
if (fastargs[3]){
isolation_level = fastargs[3];
if (!--noptargs){
goto skip_optional_pos;
}
}
if (fastargs[4]){
check_same_thread = _PyLong_AsInt(fastargs[4]);
if (check_same_thread == -1 && PyErr_Occurred()){
goto exit;
}
if (!--noptargs){
goto skip_optional_pos;
}
}
if (fastargs[5]){
factory = fastargs[5];
if (!--noptargs){
goto skip_optional_pos;
}
}
if (fastargs[6]){
cached_statements = _PyLong_AsInt(fastargs[6]);
if (cached_statements == -1 && PyErr_Occurred()){
goto exit;
}
if (!--noptargs){
goto skip_optional_pos;
}
}
uri = PyObject_IsTrue(fastargs[7]);
if (uri < 0){
goto exit;
}
skip_optional_pos:
return_value = pysqlite_connection_init_impl((pysqlite_Connection *)self, database_obj, timeout, detect_types, isolation_level, check_same_thread, factory, cached_statements, uri);

exit:
return return_value;
}

PyDoc_STRVAR(pysqlite_connection_cursor__doc__,
"cursor($self, /, factory=<unrepresentable>)\n"
"--\n"
Expand DownExpand Up@@ -710,4 +811,4 @@ pysqlite_connection_exit(pysqlite_Connection *self, PyObject *const *args, Py_ss
#ifndef PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF
#define PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF
#endif /* !defined(PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF) */
/*[clinic end generated code: output=1ee2f6173f4acec3 input=a9049054013a1b77]*/
/*[clinic end generated code: output=c350732a2758c8c1 input=a9049054013a1b77]*/
114 changes: 113 additions & 1 deletion Modules/_sqlite/clinic/module.c.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,6 +2,118 @@
preserve
[clinic start generated code]*/

PyDoc_STRVAR(pysqlite_connect__doc__,
"connect($module, /, database, timeout=5.0, detect_types=0,\n"
" isolation_level=<unrepresentable>, 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)(void(*)(void))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;
}
if (!PyUnicode_FSConverter(args[0], &database)){
goto exit;
}
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"
Expand DownExpand Up@@ -219,4 +331,4 @@ pysqlite_adapt(PyObject *module, PyObject *const *args, Py_ssize_t nargs)
exit:
return return_value;
}
/*[clinic end generated code: output=e9c2442673289cab input=a9049054013a1b77]*/
/*[clinic end generated code: output=ef03fdbf018d3391 input=a9049054013a1b77]*/
48 changes: 21 additions & 27 deletions Modules/_sqlite/connection.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -79,40 +79,34 @@ new_statement_cache(pysqlite_Connection *self, int maxsize)
return res;
}

/*[clinic input]
_sqlite3.Connection.__init__ as pysqlite_connection_init

database as database_obj: object(converter='PyUnicode_FSConverter')
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
Copy link
Member

@pablogsalpablogsalJun 15, 2021

Choose a reason for hiding this comment

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

Hummmm, i don't fully get how this is preserving the state as before. Seems that the previous code didn't have a default for factory and used NULL as the c default. Could you briefly explain how this default is preserving previous behaviour?

Copy link
ContributorAuthor

@erlend-aaslanderlend-aaslandJun 15, 2021

Choose a reason for hiding this comment

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

Sure :) This is the current code:

cpython/Modules/_sqlite/module.c

Lines 92 to 95 in 1d10bf0

if (factory==NULL){
pysqlite_state*state=pysqlite_get_state(self);
factory= (PyObject*)state->ConnectionType;
}

In practice, the factory default is ConnectionType, not NULL, so I chose to let AC take care of this instead of writing it out explicitly in the function body. This also aligns well with the documentation, which says that factory defaults to the sqlite3.Connection type.

Copy link
Member

@pablogsalpablogsalJun 15, 2021

Choose a reason for hiding this comment

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

Oh, I see. Is other code using this branch in module.c:92? I winder if we can enforce that a factory is always provided after this PR

Copy link
ContributorAuthor

@erlend-aaslanderlend-aaslandJun 15, 2021

Choose a reason for hiding this comment

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

Is other code using this branch in module.c:92?

Not in the stdlib (except for some tests in the test suite), but I guess it's used in the wild. Personally, I would not have added this feature in the first place.

I winder if we can enforce that a factory is always provided after this PR

That's kind of what we're doing no with the default AC argument, right?

cached_statements: int = 128
uri: bool = False
[clinic start generated code]*/

static int
pysqlite_connection_init(pysqlite_Connection *self, PyObject *args,
PyObject *kwargs)
pysqlite_connection_init_impl(pysqlite_Connection *self,
PyObject *database_obj, double timeout,
int detect_types, PyObject *isolation_level,
int check_same_thread, PyObject *factory,
int cached_statements, int uri)
/*[clinic end generated code: output=dc19df1c0e2b7b77 input=aa1f21bf12fe907a]*/
{
static char *kwlist[] ={
"database", "timeout", "detect_types", "isolation_level",
"check_same_thread", "factory", "cached_statements", "uri",
NULL
};

const char* database;
PyObject* database_obj;
int detect_types = 0;
PyObject* isolation_level = NULL;
PyObject* factory = NULL;
int check_same_thread = 1;
int cached_statements = 128;
int uri = 0;
double timeout = 5.0;
int rc;

if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O&|diOiOip", kwlist,
PyUnicode_FSConverter, &database_obj, &timeout, &detect_types,
&isolation_level, &check_same_thread,
&factory, &cached_statements, &uri))
{
return -1;
}

if (PySys_Audit("sqlite3.connect", "O", database_obj) < 0){
return -1;
}

database = PyBytes_AsString(database_obj);
const char *database = PyBytes_AsString(database_obj);

self->initialized = 1;

Expand All@@ -134,7 +128,7 @@ pysqlite_connection_init(pysqlite_Connection *self, PyObject *args,
(uri ? SQLITE_OPEN_URI : 0), NULL);
Py_END_ALLOW_THREADS

Py_DECREF(database_obj);
Py_DECREF(database_obj); // needed bco. the AC FSConverter

if (rc != SQLITE_OK){
_pysqlite_seterror(self->db);
Expand Down
Loading