Use attempt_to_set_autocommit everywhere.#16615
Conversation
| if isinstance(db_engine, PostgresEngine): | ||
| import psycopg2.extensions | ||
|
|
||
| if USE_POSTGRES_FOR_TESTS: |
There was a problem hiding this comment.
This change is weirdly so that mypy treats db_engine as a BaseDatabaseEngine and not a PostgresEngine or it dislikes that db_conn is a Connection (protocol) instead of psycopg2.extensions.connection.
There was a problem hiding this comment.
Probably related to
synapse/synapse/storage/engines/postgres.py
Lines 34 to 36 in 4f1840a
There was a problem hiding this comment.
(Although if we had to do the type-assert on line -1042 then it clearly wasn't working.)
There was a problem hiding this comment.
Right -- from those mypy is able to deduce that the PostgresEngine creates a psycopg2.extensions.connection; but the API expects a Connection protocol, and it doesn't seem to understand that psycopg2.extensions.connection matches Connection?
I wonder if we should make LoggingTransaction etc. generic and be passing these through so that the type checking knows the real types of things? I guess you still need a generic protocol type though for use with the BaseDatabaseEngine class.
There was a problem hiding this comment.
it doesn't seem to understand that
psycopg2.extensions.connectionmatchesConnection?
That's a surprise to me:
2023-11-09 21:04:54 ✔ $ cat temp.py
import psycopg2.extensions
import typing
import synapse.storage.types
pg_conn: psycopg2.extensions.connection
generic_conn: synapse.storage.types.Connection
generic_conn = pg_conn
dmr on titan in synapse on develop is 📦 v1.96.0rc1 via 🐍 v3.11.6 (matrix-synapse-py3.11) via 🦀 v1.68.0
2023-11-09 21:05:01 ✔ $ mypy temp.py
Success: no issues found in 1 source file
There was a problem hiding this comment.
tests/server.py:1085: error: Argument 1 to "attempt_to_set_autocommit" of "PostgresEngine" has incompatible type "Connection"; expected "connection" [arg-type]
More than happy to do another PR changing it back though?
There was a problem hiding this comment.
🤷 No strong opinions here!
This is pulled from my psycopg3 branch, it uses
attempt_to_set_autocommitinstead of anassert isinstance(conn, psycopg2.extensions.connection)followed by directly setting the autocommit.tl;dr we're just being a bit more generic.