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

[erlend-aasland]

https://bugs.python.org/issue40956

[erlend-aasland]

FYI, rebased onto master.

[berkerpeksag]

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]

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]

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]

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]

@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]

@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]

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

[erlend-aasland]

Rebased onto main to resolve conflicts.

[pablogsal]

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?

[erlend-aasland]

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.

[pablogsal]

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

[erlend-aasland]

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?

[erlend-aasland]

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]

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]

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]

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

[pablogsal]

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

[erlend-aasland]

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

Many thanks, Pablo!