From 252a71e582a7e618f1a388f9b5647362e6ec681a Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 28 Mar 2022 14:19:14 +0100 Subject: [PATCH 1/6] Add note about how we could use ParamSpec in _do_execute --- synapse/storage/database.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 367709a1a710..12c843c41304 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -326,6 +326,16 @@ def _make_sql_one_line(self, sql: str) -> str: "Strip newlines out of SQL so that the loggers in the DB are on one line" return " ".join(line.strip() for line in sql.splitlines() if line.strip()) + # TODO(ParamSpec): + # Once Mypy supports Concatenate, this could be written as + # def _do_execute( + # self, + # func: Callable[Concatenate[str, P], R], + # sql: str, + # *args: P.args, + # **kwargs: P.kwargs, + # ) -> R: + # so long as we also pass kwargs. def _do_execute(self, func: Callable[..., R], sql: str, *args: Any) -> R: sql = self._make_sql_one_line(sql) From 31b36c93e3e2bf24266edce17f77ec016520840b Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 28 Mar 2022 14:20:40 +0100 Subject: [PATCH 2/6] Add a narrower type annotation for execute_values's `args` parameter --- synapse/storage/database.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 12c843c41304..f506aaf66801 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -294,7 +294,9 @@ def execute_batch(self, sql: str, args: Iterable[Iterable[Any]]) -> None: else: self.executemany(sql, args) - def execute_values(self, sql: str, *args: Any, fetch: bool = True) -> List[Tuple]: + def execute_values( + self, sql: str, args: Iterable[Iterable[Any]], fetch: bool = True + ) -> List[Tuple]: """Corresponds to psycopg2.extras.execute_values. Only available when using postgres. @@ -305,15 +307,11 @@ def execute_values(self, sql: str, *args: Any, fetch: bool = True) -> List[Tuple from psycopg2.extras import execute_values return self._do_execute( - # Type ignore: mypy is unhappy because if `x` is a 5-tuple, then there will - # be two values for `fetch`: one given positionally, and another given - # as a keyword argument. We might be able to fix this by - # - propagating the signature of psycopg2.extras.execute_values to this - # function, or - # - changing `*args: Any` to `values: T` for some appropriate T. - lambda *x: execute_values(self.txn, *x, fetch=fetch), # type: ignore[misc] + lambda the_sql, the_args: execute_values( + self.txn, the_sql, the_args, fetch=fetch + ), sql, - *args, + args, ) def execute(self, sql: str, *args: Any) -> None: From 0732a8683c0178fe2dba4e3c4cc95dcaf8fb306b Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 28 Mar 2022 14:25:33 +0100 Subject: [PATCH 3/6] `values` is a better name than `args` --- synapse/storage/database.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index f506aaf66801..e2b577fe5104 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -295,7 +295,7 @@ def execute_batch(self, sql: str, args: Iterable[Iterable[Any]]) -> None: self.executemany(sql, args) def execute_values( - self, sql: str, args: Iterable[Iterable[Any]], fetch: bool = True + self, sql: str, values: Iterable[Iterable[Any]], fetch: bool = True ) -> List[Tuple]: """Corresponds to psycopg2.extras.execute_values. Only available when using postgres. @@ -307,11 +307,11 @@ def execute_values( from psycopg2.extras import execute_values return self._do_execute( - lambda the_sql, the_args: execute_values( - self.txn, the_sql, the_args, fetch=fetch + lambda the_sql, the_values: execute_values( + self.txn, the_sql, the_values, fetch=fetch ), sql, - args, + values, ) def execute(self, sql: str, *args: Any) -> None: From e5785b15d6efb4f7fce9cdb35eb69af44406c126 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 28 Mar 2022 14:27:19 +0100 Subject: [PATCH 4/6] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/12311.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12311.misc diff --git a/changelog.d/12311.misc b/changelog.d/12311.misc new file mode 100644 index 000000000000..df0e824a7ecc --- /dev/null +++ b/changelog.d/12311.misc @@ -0,0 +1 @@ +Improve type annotations for `execute_values`. \ No newline at end of file From 470d30934b233d41c02b150719e017d3dc7f3fe5 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 28 Mar 2022 15:54:48 +0100 Subject: [PATCH 5/6] Revert "Add note about how we could use ParamSpec in _do_execute" This reverts commit 252a71e582a7e618f1a388f9b5647362e6ec681a. --- synapse/storage/database.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index e2b577fe5104..04ee33056a20 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -324,16 +324,6 @@ def _make_sql_one_line(self, sql: str) -> str: "Strip newlines out of SQL so that the loggers in the DB are on one line" return " ".join(line.strip() for line in sql.splitlines() if line.strip()) - # TODO(ParamSpec): - # Once Mypy supports Concatenate, this could be written as - # def _do_execute( - # self, - # func: Callable[Concatenate[str, P], R], - # sql: str, - # *args: P.args, - # **kwargs: P.kwargs, - # ) -> R: - # so long as we also pass kwargs. def _do_execute(self, func: Callable[..., R], sql: str, *args: Any) -> R: sql = self._make_sql_one_line(sql) From 8d58994d497ee32c7bf59348a00e13f6521ddd19 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 28 Mar 2022 16:04:37 +0100 Subject: [PATCH 6/6] Don't thread args through _do_execute needlessly --- synapse/storage/database.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 04ee33056a20..72fef1533faa 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -290,7 +290,9 @@ def execute_batch(self, sql: str, args: Iterable[Iterable[Any]]) -> None: if isinstance(self.database_engine, PostgresEngine): from psycopg2.extras import execute_batch - self._do_execute(lambda *x: execute_batch(self.txn, *x), sql, args) + self._do_execute( + lambda the_sql: execute_batch(self.txn, the_sql, args), sql + ) else: self.executemany(sql, args) @@ -307,11 +309,8 @@ def execute_values( from psycopg2.extras import execute_values return self._do_execute( - lambda the_sql, the_values: execute_values( - self.txn, the_sql, the_values, fetch=fetch - ), + lambda the_sql: execute_values(self.txn, the_sql, values, fetch=fetch), sql, - values, ) def execute(self, sql: str, *args: Any) -> None: