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

erlend-aasland · 2021-02-02T21:03:35Z

https://bugs.python.org/issue40956

erlend-aasland · 2021-02-21T09:09:03Z

FYI, rebased onto master.

berkerpeksag · 2021-02-21T09:59:29Z

FYI, this is on my TODO list, but I want to take my time to review it since AC PRs tend to introduce regressions because of our lack of test coverage in this area.

erlend-aasland · 2021-02-21T10:02:26Z

FYI, this is on my TODO list, but I want to take my time to review it since AC PRs tend to introduce regressions because of our lack of test coverage in this area.

I can take a look at the code coverage later today. If it's possible to increase it by extending the unit test suite, we should do so first.

berkerpeksag · 2021-02-21T11:17:53Z

I don't think we have a lot of tests for optional or keyword arguments of public APIs. #24503 is a recent example of a regression introduced due to lack of tests.

erlend-aasland · 2021-02-21T12:15:11Z

I don't think we have a lot of tests for optional or keyword arguments of public APIs. #24503 is a recent example of a regression introduced due to lack of tests.

Exactly. As far as I can see, all arguments are covered except check_same_thread. Let's add a test for that before continuing.

EDIT: database keyword is also not tested.

erlend-aasland · 2021-02-21T15:54:26Z

@berkerpeksag This will do the trick for check_same_thread:

diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py
index 39c9bf5b61..050094f895 100644
--- a/Lib/sqlite3/test/dbapi.py
+++ b/Lib/sqlite3/test/dbapi.py
@@ -533,6 +533,20 @@ def tearDown(self):
         self.cur.close()
         self.con.close()
 
+    def test_dont_check_same_thread(self):
+        def run(con, err):
+            try:
+                cur = 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))
+
     def test_con_cursor(self):
         def run(con, errors):
             try:

BTW, ThreadTests could need a clean-up...

erlend-aasland · 2021-03-08T12:24:48Z

@berkerpeksag Can I add tests for the database and check_same_thread keywords, or do you want that in a separate PR?

UPDATE: Tests complementing keyword coverage has been added.

github-actions · 2021-04-08T00:12:40Z

This PR is stale because it has been open for 30 days with no activity.

…ment Clinic

erlend-aasland · 2021-05-07T20:32:14Z

Rebased onto main to resolve conflicts.

…3.connect

Modules/_sqlite/module.c

Lib/sqlite3/test/dbapi.py

pablogsal · 2021-06-15T22:09:34Z

Modules/_sqlite/connection.c

+    detect_types: int = 0
+    isolation_level: object = NULL
+    check_same_thread: bool(accept={int}) = True
+    factory: object(c_default='(PyObject*)clinic_state()->ConnectionType') = ConnectionType


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?

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.

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

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?

Lib/sqlite3/test/dbapi.py

…r.reap_threads

Modules/_sqlite/module.c

erlend-aasland · 2021-06-17T21:36:02Z

Not sure what's wrong with the "check generated files" CI; the last commit did not touch the clinic code in any way. I did a make patchcheck, and it does not complain. Maybe a spurious CI error?

pablogsal · 2021-06-17T21:40:03Z

You are changing the AC and either there are some changes you did and you didn't regenerate or as subset of the files in the main branch has moved since then and therefore your files are not up to date.

erlend-aasland · 2021-06-17T21:43:54Z

You are changing the AC and either there are some changes you did and you didn't regenerate or as subset of the files in the main branch has moved since then and therefore your files are not up to date.

Commit af0fc7e passed the CI perfectly. All 98f1a9d did was adding two C comments (out of clinic scope). make clinic produces nothing. I'll merge in main and see if that helps. UPDATE: Either that did the trick, or it was a spurious CI error.

erlend-aasland · 2021-06-18T09:29:05Z

Let me know if you want further changes, @pablogsal.

pablogsal · 2021-06-20T19:23:51Z

Great job @erlend-aasland! and thanks for the patience! 🚀

erlend-aasland · 2021-06-20T20:23:24Z

Great job @erlend-aasland! and thanks for the patience! 🚀

Many thanks, Pablo!

…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 <[email protected]>

…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 <[email protected]>. (cherry picked from commit a3d4d15) Co-authored-by: Erlend Egeberg Aasland <[email protected]>

…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 <[email protected]>. (cherry picked from commit a3d4d15) Co-authored-by: Erlend Egeberg Aasland <[email protected]>

erlend-aasland requested a review from berkerpeksag as a code owner February 2, 2021 21:03

the-knights-who-say-ni added the CLA signed label Feb 2, 2021

bedevere-bot added the awaiting review label Feb 2, 2021

erlend-aasland mentioned this pull request Feb 10, 2021

bpo-40956: [regression] first argument in sqlite3.Connection.backup is mandatory #24503

Merged

erlend-aasland force-pushed the bpo-40956/finale branch from bd403c2 to f70acce Compare February 21, 2021 09:08

github-actions bot added the stale Stale PR or inactive for long period of time. label Apr 8, 2021

Erlend E. Aasland added 4 commits May 7, 2021 22:05

Convert _sqlite3.connect() and _sqlite3.Connection.__init__() to Argu…

8192606

…ment Clinic

Add NEWS

b72a29c

Add test for 'check_same_thread' keyword

d361aeb

Add test for 'database' keyword

a861358

erlend-aasland force-pushed the bpo-40956/finale branch from f70acce to a861358 Compare May 7, 2021 20:17

erlend-aasland removed the stale Stale PR or inactive for long period of time. label May 7, 2021

Erlend E. Aasland added 5 commits June 5, 2021 21:20

Merge branch 'main' into bpo-40956/finale

1ccf5e5

Harden

07164cb

Simplify ref handling

a1e88c9

Use PyObject_CallFunction iso. PyObject_CallFunctionObjArgs in sqlite…

4977d78

…3.connect

Adjust comment

1b2720f

erlend-aasland marked this pull request as draft June 6, 2021 19:52

Erlend E. Aasland added 2 commits June 6, 2021 22:16

Merge branch 'main' into bpo-40956/finale

c7cd9ee

Remove unused var from test_dont_check_same_thread

39ed36b

erlend-aasland marked this pull request as ready for review June 6, 2021 20:41

erlend-aasland commented Jun 6, 2021

View reviewed changes

Modules/_sqlite/module.c Show resolved Hide resolved

Erlend E. Aasland added 3 commits June 13, 2021 23:26

Resolve conflict: cached_statements now defaults to 128, not 100

754fdc6

Clinic

e59af3b

Merge branch 'main' into bpo-40956/finale

ad345e0

pablogsal reviewed Jun 15, 2021

View reviewed changes

Lib/sqlite3/test/dbapi.py Show resolved Hide resolved

pablogsal reviewed Jun 15, 2021

View reviewed changes

Lib/sqlite3/test/dbapi.py Show resolved Hide resolved

pablogsal reviewed Jun 15, 2021

View reviewed changes

Lib/sqlite3/test/dbapi.py Show resolved Hide resolved

Address review: wrap test_dont_check_same_thread with threading_helpe…

af0fc7e

…r.reap_threads

erlend-aasland commented Jun 15, 2021

View reviewed changes

Modules/_sqlite/module.c Outdated Show resolved Hide resolved

Explain Py_DECREF of converted arguments

98f1a9d

erlend-aasland requested a review from pablogsal June 17, 2021 21:24

Merge branch 'main' into bpo-40956/finale

89926af

pablogsal approved these changes Jun 20, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jun 20, 2021

pablogsal merged commit 185ecdc into python:main Jun 20, 2021

bedevere-bot removed the awaiting merge label Jun 20, 2021

erlend-aasland deleted the bpo-40956/finale branch June 20, 2021 20:20

erlend-aasland mentioned this pull request Jul 23, 2022

gh-95132: Correctly relay *args and **kwds from sqlite3.connect to factory #95146

Merged

erlend-aasland mentioned this pull request Jul 23, 2022

[3.11] gh-95132: Correctly relay *args and **kwds from sqlite3.connect to factory (GH-95146) #95158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

erlend-aasland commented Feb 2, 2021 •

edited by bedevere-bot

Loading

erlend-aasland commented Feb 21, 2021

berkerpeksag commented Feb 21, 2021

erlend-aasland commented Feb 21, 2021

berkerpeksag commented Feb 21, 2021

erlend-aasland commented Feb 21, 2021 •

edited

Loading

erlend-aasland commented Feb 21, 2021

erlend-aasland commented Mar 8, 2021 •

edited

Loading

github-actions bot commented Apr 8, 2021

erlend-aasland commented May 7, 2021

pablogsal Jun 15, 2021

erlend-aasland Jun 15, 2021

pablogsal Jun 15, 2021

erlend-aasland Jun 15, 2021

erlend-aasland commented Jun 17, 2021

pablogsal commented Jun 17, 2021

erlend-aasland commented Jun 17, 2021 •

edited

Loading

erlend-aasland commented Jun 18, 2021

pablogsal commented Jun 20, 2021

erlend-aasland commented Jun 20, 2021

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

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

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

Conversation

erlend-aasland commented Feb 2, 2021 • edited by bedevere-bot Loading

erlend-aasland commented Feb 21, 2021

berkerpeksag commented Feb 21, 2021

erlend-aasland commented Feb 21, 2021

berkerpeksag commented Feb 21, 2021

erlend-aasland commented Feb 21, 2021 • edited Loading

erlend-aasland commented Feb 21, 2021

erlend-aasland commented Mar 8, 2021 • edited Loading

github-actions bot commented Apr 8, 2021

erlend-aasland commented May 7, 2021

pablogsal Jun 15, 2021

Choose a reason for hiding this comment

erlend-aasland Jun 15, 2021

Choose a reason for hiding this comment

pablogsal Jun 15, 2021

Choose a reason for hiding this comment

erlend-aasland Jun 15, 2021

Choose a reason for hiding this comment

erlend-aasland commented Jun 17, 2021

pablogsal commented Jun 17, 2021

erlend-aasland commented Jun 17, 2021 • edited Loading

erlend-aasland commented Jun 18, 2021

pablogsal commented Jun 20, 2021

erlend-aasland commented Jun 20, 2021

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

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

erlend-aasland commented Feb 2, 2021 •

edited by bedevere-bot

Loading

erlend-aasland commented Feb 21, 2021 •

edited

Loading

erlend-aasland commented Mar 8, 2021 •

edited

Loading

erlend-aasland commented Jun 17, 2021 •

edited

Loading