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

More precise type for LoggingTransaction.execute #15432

Merged
merged 8 commits into from
Apr 14, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
6 changes: 3 additions & 3 deletions synapse/storage/database.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
from synapse.metrics.background_process_metrics import run_as_background_process
from synapse.storage.background_updates import BackgroundUpdater
from synapse.storage.engines import BaseDatabaseEngine, PostgresEngine, Sqlite3Engine
from synapse.storage.types import Connection, Cursor
from synapse.storage.types import Connection, Cursor, _Parameters
Copy link
Member

Choose a reason for hiding this comment

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

If we're now importing _Parameters somewhere else can we rename it to TransactionParameters or something? It could also use a comment above it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. Will probably call it ExecuteParameters or QueryParameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went for SQLQueryParamters in the end (as opposed to URL query parameters).

from synapse.util.async_helpers import delay_cancellation
from synapse.util.iterutils import batch_iter

Expand Down Expand Up @@ -394,8 +394,8 @@ def execute_values(
sql,
)

def execute(self, sql: str, *args: Any) -> None:
self._do_execute(self.txn.execute, sql, *args)
def execute(self, sql: str, parameters: _Parameters = ()) -> None:
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
self._do_execute(self.txn.execute, sql, parameters)

def executemany(self, sql: str, *args: Any) -> None:
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
self._do_execute(self.txn.executemany, sql, *args)
Expand Down
19 changes: 11 additions & 8 deletions synapse/storage/databases/main/event_federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ def __init__(self, room_id: str):


class EventFederationWorkerStore(SignatureWorkerStore, EventsWorkerStore, SQLBaseStore):
# TODO: this attribute comes from EventPushActionWorkerStore. Should we inherit from
# that store so that mypy can deduce this for itself?
stream_ordering_month_ago: Optional[int]
Comment on lines +117 to +119
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't see why this should live on EventPushActionsWorkerStore (and I can't see anywhere else that uses it). I think it'd make more sense to move this onto EventsWorkerStore if we anticipate other users, and here if not.

Copy link
Member

Choose a reason for hiding this comment

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

EventPushActionsWorkerStore used to use it -- see #13118.

Copy link
Member

Choose a reason for hiding this comment

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

(This change seems unrelated, did the above changes shake it out somehow?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't make this change then mypy doesn't know what self.stream_ordering_month_ago is elsewhere in the file. That means it's treated as Any, which means the fix to the signature of execute doesn't spot the missing comma typo.

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd make more sense to move this onto EventsWorkerStore if we anticipate other users, and here if not.

I might have overly aggressively resolved this -- did we want to do this?

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 wasn't in any particular hurry tbh!


def __init__(
self,
database: DatabasePool,
Expand Down Expand Up @@ -1182,8 +1186,8 @@ async def have_room_forward_extremities_changed_since(
Throws a StoreError if we have since purged the index for
stream_orderings from that point.
"""

if stream_ordering <= self.stream_ordering_month_ago: # type: ignore[attr-defined]
assert self.stream_ordering_month_ago is not None
if stream_ordering <= self.stream_ordering_month_ago:
raise StoreError(400, f"stream_ordering too old {stream_ordering}")

sql = """
Expand Down Expand Up @@ -1231,7 +1235,8 @@ async def get_forward_extremities_for_room_at_stream_ordering(

# provided the last_change is recent enough, we now clamp the requested
# stream_ordering to it.
if last_change > self.stream_ordering_month_ago: # type: ignore[attr-defined]
assert self.stream_ordering_month_ago is not None
if last_change > self.stream_ordering_month_ago:
stream_ordering = min(last_change, stream_ordering)

return await self._get_forward_extremeties_for_room(room_id, stream_ordering)
Expand All @@ -1246,8 +1251,8 @@ async def _get_forward_extremeties_for_room(
Throws a StoreError if we have since purged the index for
stream_orderings from that point.
"""

if stream_ordering <= self.stream_ordering_month_ago: # type: ignore[attr-defined]
assert self.stream_ordering_month_ago is not None
if stream_ordering <= self.stream_ordering_month_ago:
raise StoreError(400, "stream_ordering too old %s" % (stream_ordering,))

sql = """
Expand Down Expand Up @@ -1707,9 +1712,7 @@ def _delete_old_forward_extrem_cache_txn(txn: LoggingTransaction) -> None:
DELETE FROM stream_ordering_to_exterm
WHERE stream_ordering < ?
"""
txn.execute(
sql, (self.stream_ordering_month_ago,) # type: ignore[attr-defined]
)
txn.execute(sql, (self.stream_ordering_month_ago,))

await self.db_pool.runInteraction(
"_delete_old_forward_extrem_cache",
Expand Down