LoggingTransaction.executemany should have a proper type hint

[DMRobertson]

executemany is also presumably vulnerable to the same kind of problem?

Originally posted by @DMRobertson in #15432 (comment)

The context here is that we committed a bug to develop that wasn't spotted by tests and wasn't spotted by linting, because the annotation for LoggingTransaction.execute wasn't strict enough. It looks like executemany has a similar problem.

[DMRobertson]

Starting from the checkout of #15432, if I try to apply a better type hint

diff --git a/synapse/storage/database.py b/synapse/storage/database.py
index 1f5f5eb6f..e0080f468 100644
--- a/synapse/storage/database.py
+++ b/synapse/storage/database.py
@@ -37,6 +37,7 @@
     Union,
     cast,
     overload,
+    Sequence,
 )
 
 import attr
@@ -407,12 +408,14 @@ def execute_values(
     def execute(self, sql: str, parameters: SQLQueryParameters = ()) -> None:
         self._do_execute(self.txn.execute, sql, parameters)
 
-    def executemany(self, sql: str, *args: Any) -> None:
+    def executemany(
+        self, sql: str, parameter_groups: Sequence[SQLQueryParameters]
+    ) -> None:
         # TODO: we should add a type for *args here. Looking at Cursor.executemany
         # and DBAPI2 it ought to be Sequence[_Parameter], but we pass in
         # Iterable[Iterable[Any]] in execute_batch and execute_values above, which mypy
         # complains about.
-        self._do_execute(self.txn.executemany, sql, *args)
+        self._do_execute(self.txn.executemany, sql, parameter_groups)
 
     def executescript(self, sql: str) -> None:
         if isinstance(self.database_engine, Sqlite3Engine):

I see

$ mypy
synapse/storage/database.py:387: error: Argument 2 to "executemany" of "LoggingTransaction" has incompatible type "Iterable[Iterable[Any]]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/room.py:1108: error: Argument 2 to "executemany" of "LoggingTransaction" has incompatible type "Generator[Tuple[Optional[str], str, str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
Found 2 errors in 2 files (checked 795 source files)

so it looks like we're always passing generators around throughout Synapse.

The DBAPI2 says that executemany takes a sequence (rather than an iterable) of parameters, so I'm not sure if this is legit.

Having said that, even *args: Any to args: Any would be an improvement.

[DMRobertson]

Trying to tighten the annotation on execute_batch

$ git --no-pager diff 
diff --git a/synapse/storage/database.py b/synapse/storage/database.py
index 1f5f5eb6f..7af51eb19 100644
--- a/synapse/storage/database.py
+++ b/synapse/storage/database.py
@@ -37,6 +37,7 @@
     Union,
     cast,
     overload,
+    Sequence,
 )
 
 import attr
@@ -361,7 +362,9 @@ def rowcount(self) -> int:
     def description(self) -> Any:
         return self.txn.description
 
-    def execute_batch(self, sql: str, args: Iterable[Iterable[Any]]) -> None:
+    def execute_batch(
+        self, sql: str, parameter_groups: Sequence[SQLQueryParameters]
+    ) -> None:
         """Similar to `executemany`, except `txn.rowcount` will not be correct
         afterwards.
 
@@ -375,7 +378,7 @@ def execute_batch(self, sql: str, args: Iterable[Iterable[Any]]) -> None:
             # https://www.psycopg.org/docs/extras.html?highlight=execute_batch#psycopg2.extras.execute_batch
             # suggests each arg in args should be a sequence or mapping
             self._do_execute(
-                lambda the_sql: execute_batch(self.txn, the_sql, args), sql
+                lambda the_sql: execute_batch(self.txn, the_sql, parameter_groups), sql
             )
         else:
             # TODO: is it safe for values to be Iterable[Iterable[Any]] here?
@@ -383,7 +386,7 @@ def execute_batch(self, sql: str, args: Iterable[Iterable[Any]]) -> None:
             # suggests that the outer collection may be iterable, but
             # https://docs.python.org/3/library/sqlite3.html?highlight=sqlite3#how-to-use-placeholders-to-bind-values-in-sql-queries
             # suggests that the inner collection should be a sequence or dict.
-            self.executemany(sql, args)
+            self.executemany(sql, parameter_groups)
 
     def execute_values(
         self, sql: str, values: Iterable[Iterable[Any]], fetch: bool = True
@@ -407,12 +410,14 @@ def execute_values(
     def execute(self, sql: str, parameters: SQLQueryParameters = ()) -> None:
         self._do_execute(self.txn.execute, sql, parameters)
 
-    def executemany(self, sql: str, *args: Any) -> None:
+    def executemany(
+        self, sql: str, parameter_groups: Sequence[SQLQueryParameters]
+    ) -> None:
         # TODO: we should add a type for *args here. Looking at Cursor.executemany
         # and DBAPI2 it ought to be Sequence[_Parameter], but we pass in
         # Iterable[Iterable[Any]] in execute_batch and execute_values above, which mypy
         # complains about.
-        self._do_execute(self.txn.executemany, sql, *args)
+        self._do_execute(self.txn.executemany, sql, parameter_groups)
 
     def executescript(self, sql: str) -> None:

throws more errors:

$ mypy
synapse/storage/database.py:1163: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Iterable[Iterable[Any]]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/media_repository.py:537: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[int, str, str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/media_repository.py:548: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[int, str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/state/store.py:768: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[int], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/state/store.py:772: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[int], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/search.py:101: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[str, str, str, str, Optional[int], int], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/room.py:1108: error: Argument 2 to "executemany" of "LoggingTransaction" has incompatible type "Generator[Tuple[Optional[str], str, str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/events.py:1099: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[int, str, str, str, str, Optional[str], str, str, str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/events.py:1119: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[str, str, str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/events.py:1151: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[str, str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/events.py:2200: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[str, Optional[int], int, str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/events.py:2215: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[str], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
synapse/storage/databases/main/devices.py:1328: error: Argument 2 to "execute_batch" of "LoggingTransaction" has incompatible type "Generator[Tuple[Any, Any], None, None]"; expected "Sequence[Union[Sequence[Any], Mapping[str, Any]]]"  [arg-type]
Found 13 errors in 7 files (checked 795 source files)