Incorrect type annotation in get_stream_id_for_event_txn

[DMRobertson]

Originally from this comment thread.

StreamWorkerStore.get_stream_id_for_event_txn is declared to return an int. If you call it with allow_none=True, it returns None, which contravenes the type hints. I think this behaviour is intentional and the type hint is incorrect, judging by how it's used. We only call get_stream_id_for_event_txn in two places, both in this function:

synapse/synapse/storage/databases/main/event_push_actions.py

Lines 184 to 216 in 3e0536c

	def _get_unread_counts_by_receipt_txn(
	self,
	txn: LoggingTransaction,
	room_id: str,
	user_id: str,
	last_read_event_id: Optional[str],
	) -> NotifCounts:
	stream_ordering = None
	if last_read_event_id is not None:
	stream_ordering = self.get_stream_id_for_event_txn( # type: ignore[attr-defined]
	txn,
	last_read_event_id,
	allow_none=True,
	)
	if stream_ordering is None:
	# Either last_read_event_id is None, or it's an event we don't have (e.g.
	# because it's been purged), in which case retrieve the stream ordering for
	# the latest membership event from this user in this room (which we assume is
	# a join).
	event_id = self.db_pool.simple_select_one_onecol_txn(
	txn=txn,
	table="local_current_membership",
	keyvalues={"room_id": room_id, "user_id": user_id},
	retcol="event_id",
	)
	stream_ordering = self.get_stream_id_for_event_txn(txn, event_id) # type: ignore[attr-defined]
	return self._get_unread_counts_by_pos_txn(
	txn, room_id, user_id, stream_ordering
	)

In particular line 194 of the quoted source looks like it's expecting stream_ordering to be possibly None.

Given the above, I think the right thing to do is

1. change get_stream_id_for_event_txn to return Optional[int] instead of int.
2. Try changing EventPushActionsWorkerStore to inherit from StreamWorkerStore and removing the type-ignores where we call get_stream_id_for_event_txn. Doing so might fail due with DataStore MRO problems (related: Improve type hints for data store usage #11165 and more generally our storage classes are hella confusing #11733). If so, that's fine and we can leave things as they are.

[gautamgiri-dev]

Hello @DMRobertson I have made some changes as suggested by you followed by some changes based on linting errors thrown by mypy. Could you please review it.. and please assign this issue to me

[clokep]

This was fixed in #12716.