Skip to content

bpo-31843: sqlite3.connect() now accepts PathLike objects as database name #4299

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
vstinner merged 8 commits into from
Nov 7, 2017
Merged

bpo-31843: sqlite3.connect() now accepts PathLike objects as database name #4299

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8dd99cb
sqlite3.connect() now accepts PathLike objects as database name
Phaqui Nov 6, 2017
954615c
Fixed ref leak in previous commit
Phaqui Nov 6, 2017
62a7d00
Documented that sqlite3.connect() now accepts pathlike objects
Phaqui Nov 6, 2017
be022cf
Added version change information to the docs
Phaqui Nov 6, 2017
ceb6de5
Added newline between the version history lines in the doc
Phaqui Nov 6, 2017
72e4cf7
Docs: sqlite3.connect() returns Connection objects by default. Not al…
Phaqui Nov 6, 2017
4cef352
Testing for PathLike objects in sqlite3.connect() no longer imports p…
Phaqui Nov 6, 2017
8b7f340
Added news entry in Misc/NEWS.d/next/
Phaqui Nov 6, 2017
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
13 changes: 10 additions & 3 deletions Doc/library/sqlite3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,13 @@ Module functions and constants

.. function:: connect(database[, timeout, detect_types, isolation_level, check_same_thread, factory, cached_statements, uri])

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.
Opens a connection to the SQLite database file *database* and return a
:class:`Connection` object.

@vstinner vstinner Nov 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"return a Connection" is wrong, since it can be overridden by the factory parameter.

Either remove this addition, or rephrase it like: "Open a connection (...). By default, return a Connection object."


*database* is a :term:`path-like object` giving the pathname (absolute or
relative to the current working directory) of the database file to be opened.
You can use ``":memory:"`` to open a database connection to a database that
resides in RAM instead of on disk.

@vstinner vstinner Nov 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You have to document the change. Something like:

.. versionchanged:: 3.7
   *database* can now also be a :term:`path-like object`, not only a string.

@vstinner vstinner Nov 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Done

When a database is accessed by multiple connections, and one of the processes
modifies the database, the SQLite database is locked until that transaction is
Expand Down Expand Up @@ -223,6 +227,9 @@ Module functions and constants
.. versionchanged:: 3.4
Added the *uri* parameter.

.. versionchanged:: 3.7
*database* can now also be a :term:`path-like object`, not only a string.


.. function:: register_converter(typename, callable)

Expand Down
9 changes: 9 additions & 0 deletions Lib/sqlite3/test/dbapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,15 @@ def CheckInTransactionRO(self):
with self.assertRaises(AttributeError):
self.cx.in_transaction = True

def CheckOpenWithPathLikeObject(self):
""" Checks that we can succesfully connect to a database using an object that
is PathLike, i.e. has __fspath__(). """
self.addCleanup(unlink, TESTFN)
import pathlib
path = pathlib.Path(TESTFN)

@vstinner vstinner Nov 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure that it's ok to depend on pathlib when testing sqlite.

I propose to replace it with:

class Path(os.PathLike):
    def __fspath__(self):
        return TESTFN

with sqlite.connect(path) as cx:
cx.execute('create table test(id integer)')

def CheckOpenUri(self):
if sqlite.sqlite_version_info < (3, 7, 7):
with self.assertRaises(sqlite.NotSupportedError):
Expand Down
9 changes: 7 additions & 2 deletions Modules/_sqlite/connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ int pysqlite_connection_init(pysqlite_Connection* self, PyObject* args, PyObject
};

char* database;
PyObject* database_obj;
int detect_types = 0;
PyObject* isolation_level = NULL;
PyObject* factory = NULL;
Expand All @@ -85,14 +86,16 @@ int pysqlite_connection_init(pysqlite_Connection* self, PyObject* args, PyObject
double timeout = 5.0;
int rc;

if (!PyArg_ParseTupleAndKeywords(args, kwargs, "s|diOiOip", kwlist,
&database, &timeout, &detect_types,
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;
}

database = PyBytes_AsString(database_obj);

self->initialized = 1;

self->begin_statement = NULL;
Expand Down Expand Up @@ -124,6 +127,8 @@ int pysqlite_connection_init(pysqlite_Connection* self, PyObject* args, PyObject
#endif
Py_END_ALLOW_THREADS

Py_DECREF(database_obj);

if (rc != SQLITE_OK) {
_pysqlite_seterror(self->db, NULL);
return -1;
Expand Down
4 changes: 2 additions & 2 deletions Modules/_sqlite/module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ static PyObject* module_connect(PyObject* self, PyObject* args, PyObject*
"check_same_thread", "factory", "cached_statements", "uri",
NULL
};
char* database;
PyObject* database;
int detect_types = 0;
PyObject* isolation_level;
PyObject* factory = NULL;
Expand All @@ -66,7 +66,7 @@ static PyObject* module_connect(PyObject* self, PyObject* args, PyObject*

PyObject* result;

if (!PyArg_ParseTupleAndKeywords(args, kwargs, "s|diOiOip", kwlist,
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|diOiOip", kwlist,

@vstinner vstinner Nov 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand why the function checks arguments.

Why not only relying on Connection constructor to check arguments?

Either remove the whole code to parse arguments, or fix the reference leak: Py_DECREF(database); is needed.

@vstinner vstinner Nov 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fix the reference leak: Py_DECREF(database); is needed.

Oops, it's not needed, you don't use PyUnicode_FSConverter, sorry. Your code is correct.

@vstinner vstinner Nov 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Phaqui pointed me on IRC that the function contains a comment explaining that we have to parse all arguments to just extract the "factory" keyword argument... In that case, the code is ok.

&database, &timeout, &detect_types,
&isolation_level, &check_same_thread,
&factory, &cached_statements, &uri))
Expand Down