Update module API "update room membership" method to allow for remote joins

[buffless-matt]

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)

Description

The current implementation is a manifestation of the discussion that ended here.

Following that discussion and prior to submitting this PR, I thought about the situation a bit more and came up with the following solutions:

1. Add some lines before this line, which automatically adds in the hostname from room_id if is_host_in_room is False. Basically the equivalent of these knocking lines.
2. Similar to the previous solution, but do the appending in the module API method (i.e. before this line).
3. Add a parameter to the module API method to control whether remote joining is done.

The solutions are ordered (in my opinion) from most invasive (i.e. least backwards compatible) to least invasive (most backwards compatible). They are also unfortunately reverse ordered (in my opinion) from most intuitive to least intuitive (from an API design/consumer perspective).

I decided to go with the third option, for its desirable backwards compatibility trait. Took a different approach because of this.

[buffless-matt]

I would have liked to include some unit test coverage for the changes, but I couldn't find a pre-existing test harness to nicely handle the federation mocking and didn't think the cost-benefit-ratio was favourable (at least in this case) to create such a test harness. If the reviewer(s) think otherwise, I'm happy to add test coverage in, but would appreciate some guidance/direction on how to do so.

[babolivier]

I do think it'd be best to have a test for this. One way you can do that is:

• mock RoomMemberMasterHandler._remote_join to return a fake event ID and stream ID
• catch the AssertionError from ModuleApi.update_room_membership in the test (since the join event doesn't actually exist) using self.get_failure
• check that the mock for _remote_join was called once with the right arguments

Feel free to ping me in #synapse-dev:matrix.org if something's not clear :)

[buffless-matt]

One way you can do that is:...

Thank you for the guidance ❤️. Tests added here (hopefully that's what you had in mind): 4c0380b

[babolivier]

While you're around this area, it would be great if you could remove the assert at L1018 of synapse/module_api/__init__.py (and the comment above it), since it's not really useful (get_event without allow_none=True will fail before that if the event is None) and makes this code a bit confusing.

[buffless-matt]

While you're around this area, it would be great if you could remove the assert at L1018 of synapse/module_api/__init__.py (and the comment above it), since it's not really useful (get_event without allow_none=True will fail before that if the event is None) and makes this code a bit confusing.

Done: f178b45

[babolivier]

Looks good to me! Thanks for bearing with me on this :)

[buffless-matt]

Looks good to me! Thanks for bearing with me on this :)

Thanks for helping me get these module API PRs through.