Make get_room_version use cached get_room_version_id.

[lukasdenk]

Fixes #11706.

Make method get_room_version use cached get_room_version_id.

Signed-off by: Lukas Denk, lukasdenk@web.de

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[reivilibre]

Seems sensible — though would like to check the tests agree with me :-).
Thank you!

(By the way: if you write 'Fixes #11706' instead of 'Solves #11706', GitHub will detect that and link the issue to this PR so that the issue will be closed automatically when the PR is merged. I've changed it for you above, but may be useful for your reference)

[lukasdenk]

(By the way: if you write 'Fixes #11706' instead of 'Solves #11706', GitHub will detect that and link the issue to this PR so that the issue will be closed automatically when the PR is merged. I've changed it for you above, but may be useful for your reference)

Thanks :)

[reivilibre]

Also note isort is complaining (possibly about the import mock line in the test that you seem to have introduced).

[reivilibre]

Sorry for the delay; thanks for bearing with us!

[reivilibre]

Why is this needed?

It's bad practice to add invalidations for the tests since it might cover up an invalidation problem in the real code (I ran into this too during one of my recent PRs).
It's not clear to my why it's needed in the first place, but ideally we should fix it so it's not needed, or failing that, add a comment about it?

[lukasdenk]

The test tests whether a room summary still succeeds, even if the room's version is invalid. To do so, it sets the room version id to unknown-room-version in the lines above. However the room version has already been requested before. Therefore, get_room_version will not return unknown-room-version as the room version for room_id. Hence, get_room_version's cache must be invalidated.

I actually included a comment now.

[lukasdenk]

What is weird though, is that removing these lines

synapse/tests/handlers/test_room_summary.py

Lines 722 to 729 in 3655585

	self.get_success(
	self.hs.get_datastores().main.db_pool.simple_update(
	"rooms",
	keyvalues={"room_id": self.room},
	updatevalues={"room_version": "unknown-room-version"},
	desc="updated-room-version",
	)
	)

(and not invalidating get_version_id) results in a different exception than including the above lines (and not invalidating get_version_id). However, in both cases, get_version_id returns the same, namely '6'. However, I would expect both scenarios would result in the same exception. @reivilibre Do you have any idea why this could be the case? Should I dig more deeply into the reasons?

The first approach fails here

synapse/tests/handlers/test_room_summary.py

Line 734 in 3655585

self._assert_rooms(result, expected)

with

Failure
Traceback (most recent call last):
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 478, in addCallbacks
    self._runCallbacks()
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 858, in _runCallbacks
    current.result = callback(  # type: ignore[misc]
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_asynctest.py", line 152, in deferTestMethod
    d = self._run(self._testMethodName, result)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_asynctest.py", line 122, in _run
    d = defer.maybeDeferred(
--- <exception caught here> ---
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 191, in maybeDeferred
    result = f(*args, **kwargs)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/utils.py", line 209, in runWithWarningsSuppressed
    raise exc_info[1].with_traceback(exc_info[2])
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/utils.py", line 205, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/lukas/PycharmProjects/synapse/tests/handlers/test_room_summary.py", line 737, in test_unknown_room_version
    self._assert_rooms(result, expected)
  File "/home/lukas/PycharmProjects/synapse/tests/handlers/test_room_summary.py", line 178, in _assert_rooms
    self.assertCountEqual(
  File "/usr/lib/python3.8/unittest/case.py", line 1272, in assertCountEqual
    self.fail(msg)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_synctest.py", line 361, in fail
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: Element counts were not equal:
First has 1, Second has 0:  '!XgELWsagGJJtucVyif:test'

while the latter fails here

synapse/tests/handlers/test_room_summary.py

Line 731 in 3655585

result = self.get_success(self.handler.get_space_summary(self.user, self.space))

with

Failure
Traceback (most recent call last):
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 478, in addCallbacks
    self._runCallbacks()
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 858, in _runCallbacks
    current.result = callback(  # type: ignore[misc]
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_asynctest.py", line 152, in deferTestMethod
    d = self._run(self._testMethodName, result)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_asynctest.py", line 122, in _run
    d = defer.maybeDeferred(
--- <exception caught here> ---
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 191, in maybeDeferred
    result = f(*args, **kwargs)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/utils.py", line 209, in runWithWarningsSuppressed
    raise exc_info[1].with_traceback(exc_info[2])
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/utils.py", line 205, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/lukas/PycharmProjects/synapse/tests/handlers/test_room_summary.py", line 734, in test_unknown_room_version
    result = self.get_success(self.handler.get_space_summary(self.user, self.space))
  File "/home/lukas/PycharmProjects/synapse/tests/unittest.py", line 522, in get_success
    return self.successResultOf(d)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_synctest.py", line 700, in successResultOf
    self.fail(
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_synctest.py", line 361, in fail
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: Success result expected on <Deferred at 0x7f6e46cdd460 current result: None>, found failure result instead:
Traceback (most recent call last):
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 662, in callback
    self._startRunCallbacks(result)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 764, in _startRunCallbacks
    self._runCallbacks()
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 858, in _runCallbacks
    current.result = callback(  # type: ignore[misc]
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 1751, in gotResult
    current_context.run(_inlineCallbacks, r, gen, status)
--- <exception caught here> ---
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 1661, in _inlineCallbacks
    result = current_context.run(gen.send, result)
  File "/home/lukas/PycharmProjects/synapse/synapse/handlers/room_summary.py", line 176, in get_space_summary
    room_entry = await self._summarize_local_room(
  File "/home/lukas/PycharmProjects/synapse/synapse/handlers/room_summary.py", line 651, in _summarize_local_room
    if not await self._is_local_room_accessible(room_id, requester, origin):
  File "/home/lukas/PycharmProjects/synapse/synapse/handlers/room_summary.py", line 860, in _is_local_room_accessible
    join_rules_event = await self._store.get_event(join_rules_event_id)
  File "/home/lukas/PycharmProjects/synapse/synapse/storage/databases/main/events_worker.py", line 385, in get_event
    raise NotFoundError("Could not find event %s" % (event_id,))
synapse.api.errors.NotFoundError: 404: Could not find event $7nE7ON6WDexQYs-xMFG5SMOfIe2wx00BQPwgItkyyIg

[reivilibre]

synapse.api.errors.NotFoundError: 404: Could not find event $7nE7ON6WDexQYs-xMFG5SMOfIe2wx00BQPwgItkyyIg

Looking at this one, it seems like it occurs because the event fetching code fetches the room version separately and then encounters a room version it doesn't understand: it then skips fetching that event. (This is a design choice: events in unknown room versions are treated as nonexistent events because we don't know how to decode them)

I think that's fine; in practice the version of a room doesn't change and so there's no way the cache would get out of sync with the database.

twisted.trial.unittest.FailTest: Element counts were not equal: First has 1, Second has 0: '!XgELWsagGJJtucVyif:test'

This is happening because if you don't set the room version to something unknown, the code will (correctly) generate a room summary entry for it, which the test doesn't expect for unknown room versions.

I think your code is therefore fine as-is!

[reivilibre]

Thanks and so sorry for this one falling behind!

[reivilibre]

synapse.api.errors.NotFoundError: 404: Could not find event $7nE7ON6WDexQYs-xMFG5SMOfIe2wx00BQPwgItkyyIg

Looking at this one, it seems like it occurs because the event fetching code fetches the room version separately and then encounters a room version it doesn't understand: it then skips fetching that event. (This is a design choice: events in unknown room versions are treated as nonexistent events because we don't know how to decode them)

I think that's fine; in practice the version of a room doesn't change and so there's no way the cache would get out of sync with the database.

twisted.trial.unittest.FailTest: Element counts were not equal: First has 1, Second has 0: '!XgELWsagGJJtucVyif:test'

This is happening because if you don't set the room version to something unknown, the code will (correctly) generate a room summary entry for it, which the test doesn't expect for unknown room versions.

I think your code is therefore fine as-is!