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

Annotations for state_deltas.py #11316

Merged
merged 3 commits into from
Nov 12, 2021
Merged

Annotations for state_deltas.py #11316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b66539e
Annotations for state_deltas.py
Nov 11, 2021
90af664
Changelog
Nov 11, 2021
de9e7c8
Merge branch 'develop' into dmr/typing/storage/state_deltas
Nov 11, 2021
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
1 change: 1 addition & 0 deletions changelog.d/11316.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add type hints to storage classes.
4 changes: 3 additions & 1 deletion mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ exclude = (?x)
|synapse/storage/databases/main/roommember.py
|synapse/storage/databases/main/search.py
|synapse/storage/databases/main/state.py
|synapse/storage/databases/main/state_deltas.py
|synapse/storage/databases/main/stats.py
|synapse/storage/databases/main/transactions.py
|synapse/storage/databases/main/user_directory.py
Expand Down Expand Up @@ -183,6 +182,9 @@ disallow_untyped_defs = True
[mypy-synapse.storage.databases.main.room_batch]
disallow_untyped_defs = True

[mypy-synapse.storage.databases.main.state_deltas]
disallow_untyped_defs = True
Copy link
Member

Choose a reason for hiding this comment

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

See #11322 instead. 😄


[mypy-synapse.storage.databases.main.user_erasure_store]
disallow_untyped_defs = True

Expand Down
16 changes: 13 additions & 3 deletions synapse/storage/databases/main/state_deltas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,17 @@
from typing import Any, Dict, List, Tuple

from synapse.storage._base import SQLBaseStore
from synapse.storage.database import LoggingTransaction
from synapse.util.caches.stream_change_cache import StreamChangeCache

logger = logging.getLogger(__name__)


class StateDeltasStore(SQLBaseStore):
# This class must be mixed in with a child class which provides the following
# attribute. TODO: can we get static analysis to enforce this?
_curr_state_delta_stream_cache: StreamChangeCache
Comment on lines 25 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this an abstract class perhaps, with this being an abstract.. field... if that exists ...

It doesn't seem like it does, but I would be tempted to try an abstract property:

    @property
    @abstractmethod
    def _curr_state_delta_stream_cache(self) -> StreamChangeCache:
        ...

Not super clean, but curious if that'd do the trick (and if it's allowed to just overwrite it with a field later)

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 got

synapse/app/generic_worker.py:222: error: Definition of "_curr_state_delta_stream_cache" in base class "StateDeltasStore" is incompatible with definition in base class "SlavedEventStore"  [misc]

after

class StateDeltasStore(SQLBaseStore, abc.ABC):
    @property
    @abc.abstractmethod
    def _curr_state_delta_stream_cache(self) -> StreamChangeCache:
        """This class must be mixed in with a child class which provides the
        following attribute."""
        ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might be interested in the discussion here: python/mypy#4426


async def get_current_state_deltas(
self, prev_stream_id: int, max_stream_id: int
) -> Tuple[int, List[Dict[str, Any]]]:
Expand Down Expand Up @@ -60,7 +66,9 @@ async def get_current_state_deltas(
# max_stream_id.
return max_stream_id, []

def get_current_state_deltas_txn(txn):
def get_current_state_deltas_txn(
txn: LoggingTransaction,
) -> Tuple[int, List[Dict[str, Any]]]:
# First we calculate the max stream id that will give us less than
# N results.
# We arbitrarily limit to 100 stream_id entries to ensure we don't
Expand Down Expand Up @@ -106,15 +114,17 @@ def get_current_state_deltas_txn(txn):
"get_current_state_deltas", get_current_state_deltas_txn
)

def _get_max_stream_id_in_current_state_deltas_txn(self, txn):
def _get_max_stream_id_in_current_state_deltas_txn(
self, txn: LoggingTransaction
) -> int:
return self.db_pool.simple_select_one_onecol_txn(
txn,
table="current_state_delta_stream",
keyvalues={},
retcol="COALESCE(MAX(stream_id), -1)",
)

async def get_max_stream_id_in_current_state_deltas(self):
async def get_max_stream_id_in_current_state_deltas(self) -> int:
return await self.db_pool.runInteraction(
"get_max_stream_id_in_current_state_deltas",
self._get_max_stream_id_in_current_state_deltas_txn,
Expand Down