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

Tidy up and type-hint the database engine modules #12734

Merged
merged 10 commits into from
May 16, 2022
Merged

Tidy up and type-hint the database engine modules #12734

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e36410c
BaseDatabaseEngine: use ConnectionType generic
May 13, 2022
e9dcff7
Let the DatabaseEngines provide their db module
May 13, 2022
c8f1458
Don't propgate DB config upwards
May 13, 2022
7cf1719
Stub out the DB2API module
May 13, 2022
467b2a0
Let's disallow untyped defs while I'm at it
May 13, 2022
44af3e4
Changelog
May 13, 2022
4d8b840
Revert "Don't propgate DB config upwards"
May 16, 2022
6dcbf99
Describe the DBAPI2 exception heirarchy
May 16, 2022
24d9189
Tweak writing (thanks Sean)
May 16, 2022
27e90f0
Fix weird indent??
May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions synapse/storage/engines/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.
import abc
from enum import IntEnum
from typing import Any, Generic, Mapping, Optional, TypeVar
from typing import Generic, Optional, TypeVar

from synapse.storage.types import Connection

Expand All @@ -32,7 +32,7 @@ class IncorrectDatabaseSetup(RuntimeError):


class BaseDatabaseEngine(Generic[ConnectionType], metaclass=abc.ABCMeta):
def __init__(self, module, database_config: Mapping[str, Any]):
def __init__(self, module):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see that one might want database config that applies independently of your choice of DB (or DB driver, for that matter). So I don't mind dropping this change if people aren't convinced.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that one might want database config that applies independently of your choice of DB (or DB driver, for that matter).

Given your reasoning, I'm in favour of leaving the config parameter in, even if we don't use it currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. 4d8b840 reverts this.

self.module = module

@property
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/engines/postgres.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class PostgresEngine(BaseDatabaseEngine["psycopg2.connection"]):
def __init__(self, database_config: Mapping[str, Any]):
import psycopg2.extensions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dislike importing this here rather than at the top level. But this was the least-bad/easiest way I could see to not require existing installations to install psycopg2.


super().__init__(psycopg2, database_config)
super().__init__(psycopg2)
psycopg2.extensions.register_type(psycopg2.extensions.UNICODE)

# Disables passing `bytes` to txn.execute, c.f. #6186. If you do
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/engines/sqlite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

class Sqlite3Engine(BaseDatabaseEngine[sqlite3.Connection]):
def __init__(self, database_config: Mapping[str, Any]):
super().__init__(sqlite3, database_config)
super().__init__(sqlite3)

database = database_config.get("args", {}).get("database")
self._is_in_memory = database in (
Expand Down