Type annotations in synapse.databases.main.devices

[DMRobertson]

This is one of the remaining files in the synapse source tree which isn't checked by mypy. This change starts checking that file.

Most of these fixes were relatively straightforward. The stream id generator/tracker was painful to deal with, though. I've type-ignored that and left a TODO.

Recommend commit-by-commit review.

                                                     Name    Anys    Exprs   Coverage
  ------------------------------------------------------------------------------------
--- out/any-exprs.txt	2022-06-10 19:23:16.151256162 +0100
+++ out2/any-exprs.txt	2022-06-10 19:22:47.460110866 +0100
@@ -358,0 +359 @@
+                   synapse.storage.databases.main.devices     133     1914     93.05%
@@ -625 +626 @@
-                                                    Total   24391   285258     91.45%
+                                                    Total   24524   287172     91.46%

[DMRobertson]

I wasn't sure I liked this one. The original complaint was

synapse/storage/databases/main/devices.py:1860: error: Argument 2 to "runInteraction" of "DatabasePool" has incompatible type "Callable[[LoggingTransaction], List[Tuple[str, str, str, int, Optional[Dict[str, str]]]]]"; expected "Callable[..., List[Tuple[str, str, str, int, Dict[str, str]]]]" [arg-type]

Which corresponds to the context argument of this function:

synapse/synapse/storage/databases/main/devices.py

Lines 1709 to 1756 in 9dc3293

	def _add_device_outbound_poke_to_stream_txn(
	self,
	txn: LoggingTransaction,
	user_id: str,
	device_ids: Iterable[str],
	hosts: Collection[str],
	stream_ids: List[int],
	context: Dict[str, str],
	) -> None:
	for host in hosts:
	txn.call_after(
	self._device_list_federation_stream_cache.entity_has_changed,
	host,
	stream_ids[-1],
	)
	now = self._clock.time_msec()
	stream_id_iterator = iter(stream_ids)
	encoded_context = json_encoder.encode(context)
	self.db_pool.simple_insert_many_txn(
	txn,
	table="device_lists_outbound_pokes",
	keys=(
	"destination",
	"stream_id",
	"user_id",
	"device_id",
	"sent",
	"ts",
	"opentracing_context",
	),
	values=[
	(
	destination,
	next(stream_id_iterator),
	user_id,
	device_id,
	not self.hs.is_mine_id(
	user_id
	), # We only need to send out update for *our* users
	now,
	encoded_context if whitelisted_homeserver(destination) else "{}",
	)
	for destination in hosts
	for device_id in device_ids
	],
	)

If we pass context=None, we'll first transform it to a JSON null and store that blob of json as the four-codepoint string null. I don't think that's what we want!!

[DMRobertson]

Note that opentracing_context is a nullable text field in the DB:

matrix=> \d device_lists_changes_in_room
Did not find any relation named "device_lists_changes_in_room".
matrix=> \d matrix.device_lists_changes_in_room
             Table "matrix.device_lists_changes_in_room"
          Column           |  Type   | Collation | Nullable | Default 
---------------------------+---------+-----------+----------+---------
 user_id                   | text    |           | not null | 
 device_id                 | text    |           | not null | 
 room_id                   | text    |           | not null | 
 stream_id                 | bigint  |           | not null | 
 converted_to_destinations | boolean |           | not null | 
 opentracing_context       | text    |           |          | 
Indexes:
    "device_lists_changes_in_stream_id" UNIQUE, btree (stream_id, room_id)
    "device_lists_changes_in_stream_id_unconverted" btree (stream_id) WHERE NOT converted_to_destinations

[clokep]

Do you know if we have any "null" data already stored though? It is very unclear to me if it is safe to change this.

[DMRobertson]

Not on matrix.org:

matrix=> select * from matrix.device_lists_changes_in_room where opentracing_context='null' limit 1;
(0 rows)

Time: 11322.423 ms (00:11.322)
matrix=> select * from matrix.device_lists_changes_in_room where opentracing_context='"null"' limit 1;
(0 rows)

Time: 10651.809 ms (00:10.652)
matrix=> select * from matrix.device_lists_changes_in_room where opentracing_context is NULL limit 1;
(0 rows)

Time: 3886.793 ms (00:03.887)

I suppose a safer approach would be to write a migration which makes the column non-nullable.

Maybe this is better dropped though; ideally type hints wouldn't kick off invasive changes.

[DMRobertson]

1569b14 reverts this and 134d3da presents an alternative.

[clokep]

Thanks. I'd be curious what @erikjohnston thinks here since this was was just added in #12321 (and maybe #13045). Is there a long term plan here we're unsure about?

[erikjohnston]

I'm a bit confused as to why we can't let it be None if I'm honest?

[DMRobertson]

I was mainly just trying to keep the existing type hints happy. Some of them say Dict[] and some of them say Optional[Dict].

Maybe we just use Optional[Dict] everywhere?

[erikjohnston]

Yeah, I'd go with Optional[Dict] if the DB has a nullable column?

[DMRobertson]

8395995 and 60e2ce0 do this.

[clokep]

@erikjohnston can you see the one question above? I think this PR is ready to go besides that.