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

Python3.11: sqlite3.connect passes more arguments to factory() #95132

Closed
Steap opened this issue Jul 22, 2022 · 6 comments · Fixed by #95146
Closed

Python3.11: sqlite3.connect passes more arguments to factory() #95132

Steap opened this issue Jul 22, 2022 · 6 comments · Fixed by #95146
Assignees
Labels
3.11 only security fixes 3.12 bugs and security fixes topic-sqlite3 triaged The issue has been accepted as valid by a triager. type-bug An unexpected behavior, bug, or error

Comments

@Steap
Copy link

Steap commented Jul 22, 2022

Hello,

Bug report

Consider the following code snippet:

$ cat /tmp/test.py

import sqlite3


class SqliteConnection(sqlite3.Connection):
    def __init__(self, *args, **kwargs):
        print(args)
        print(kwargs)
        super(SqliteConnection, self).__init__(*args, **kwargs)


db_path = '/tmp/foo.db'
sqlite3.connect(db_path, factory=SqliteConnection)

And the execution:

$ python3.10 /tmp/test.py
('/tmp/foo.db',)
{'factory': <class '__main__.SqliteConnection'>}

$ python3.11 /tmp/test.py
('/tmp/foo.db', 5.0, 0, '', 1, <class '__main__.SqliteConnection'>, 128, 0)
{}

The default values of sqlite3.connect's arguments are passed to the factory in
Python3.11, which was not the case in Python3.10. This means that code like
this will fail:

$ cat /tmp/failure.py

import sqlite3


class SqliteConnection(sqlite3.Connection):
    def __init__(self, *args, **kwargs):
        kwargs['timeout'] = 42
        super(SqliteConnection, self).__init__(*args, **kwargs)


db_path = '/tmp/foo.db'
sqlite3.connect(db_path, factory=SqliteConnection)
$ python3.10 /tmp/failure.py
$

$ python3.11 /tmp/failure.py
Traceback (most recent call last):
  File "/tmp/failure.py", line 11, in <module>
    sqlite3.connect(db_path, factory=SqliteConnection)
  File "/tmp/failure.py", line 7, in __init__
    super(SqliteConnection, self).__init__(*args, **kwargs)
TypeError: Connection() takes at most 8 arguments (9 given)

Your environment
This behaviour appeared in 185ecdc and affects
Python 3.11 and later.

It was mentioned in #93044 (see the
second bullet point in the reporter's message) but has not been fixed.

@Steap Steap added the type-bug An unexpected behavior, bug, or error label Jul 22, 2022
@kumaraditya303 kumaraditya303 added 3.11 only security fixes 3.12 bugs and security fixes labels Jul 22, 2022
@erlend-aasland erlend-aasland moved this to TODO: Bugs in sqlite3 issues Jul 22, 2022
@erlend-aasland erlend-aasland added topic-sqlite3 triaged The issue has been accepted as valid by a triager. labels Jul 22, 2022
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 22, 2022

Thanks for the report. This is unfortunate. Reverting the offending commit is going to be hairy; there has been a lot of changes since that point in time.

The only thing sqlite3.connect needs in order to relay the call to sqlite3.Connection.__init__ is: *args, **kwargs, and factory. Unfortunately, doing such a manoeuvre is impossible (AFAIK) with Argument Clinic. Trying to bend AC to our needs, could be an option. Another option is to just parse the factory argument manually (kinda like the old code), and pass the rest using vector call (or PyObject_Call). I've already tried to look for a way to do these things because of keyword-only argument needs in another PR (#93823); I might actually be able to adopt that approach.

@serhiy-storchaka, do you have an opinion here?

Thanks again for testing the 3.11 beta! Reports like this help us improve.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jul 22, 2022

I've already tried to look for a way to do these things because of keyword-only argument needs in another PR (#93823); I might actually be able to adopt that approach.

FTR, this won't work. Arguments will still be passed incorrectly.

@erlend-aasland
Copy link
Contributor

I've partially reverted 185ecdc in #95146.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jul 22, 2022
@erlend-aasland
Copy link
Contributor

I've updated #95146 to use METH_FASTCALL and PyObject_Vectorcall.

@erlend-aasland erlend-aasland moved this from TODO: Bugs to In Progress in sqlite3 issues Jul 22, 2022
Repository owner moved this from In Progress to Done in sqlite3 issues Jul 23, 2022
erlend-aasland pushed a commit that referenced this issue Jul 23, 2022
…ctory (#95146)

This PR partially reverts gh-24421 (PR) and fixes the remaining concerns
given in gh-93044 (issue):

- keyword arguments are passed as positional arguments to factory()
- if an argument is not passed to sqlite3.connect(), its default value
  is passed to factory()

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this issue Jul 23, 2022
…connect to factory (pythonGH-95146)

This PR partially reverts pythongh-24421 (PR) and fixes the remaining concerns
given in pythongh-93044 (issue):

- keyword arguments are passed as positional arguments to factory()
- if an argument is not passed to sqlite3.connect(), its default value
  is passed to factory()

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>.
(cherry picked from commit a3d4d15)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
erlend-aasland pushed a commit that referenced this issue Jul 23, 2022
…t to factory (GH-95146) (#95158)

This PR partially reverts gh-24421 (PR) and fixes the remaining concerns
given in gh-93044 (issue):

- keyword arguments are passed as positional arguments to factory()
- if an argument is not passed to sqlite3.connect(), its default value
  is passed to factory()

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>.
(cherry picked from commit a3d4d15)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@vstinner
Copy link
Member

vstinner commented Aug 1, 2022

Nice fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 bugs and security fixes topic-sqlite3 triaged The issue has been accepted as valid by a triager. type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants