Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Use attempt_to_set_autocommit everywhere. #16615

Merged
merged 4 commits into from Nov 9, 2023
Merged

Conversation

clokep
Copy link
Contributor

@clokep clokep commented Nov 9, 2023

This is pulled from my psycopg3 branch, it uses attempt_to_set_autocommit instead of an assert isinstance(conn, psycopg2.extensions.connection) followed by directly setting the autocommit.

tl;dr we're just being a bit more generic.

@clokep clokep marked this pull request as ready for review November 9, 2023 15:42
@clokep clokep requested a review from a team as a code owner November 9, 2023 15:42
if isinstance(db_engine, PostgresEngine):
import psycopg2.extensions

if USE_POSTGRES_FOR_TESTS:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably related to

class PostgresEngine(
BaseDatabaseEngine[psycopg2.extensions.connection, psycopg2.extensions.cursor]
):
which was my way of trying to teach mypy which connection and cursor type to expect when we're using the PostgresEngine.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Although if we had to do the type-assert on line -1042 then it clearly wasn't working.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't seem to understand that psycopg2.extensions.connection matches Connection?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷 No strong opinions here!

@clokep clokep merged commit 2c6a7df into develop Nov 9, 2023
39 of 41 checks passed
@clokep clokep deleted the clokep/set-autocommit branch November 9, 2023 21:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants