Implementation for MSC3664: Pushrules for relations

[deepbluev7]

An inefficient implementation for matrix-org/matrix-spec-proposals#3664 based on the work from @bradtgmurray: https://gitlab.com/beeper/synapse/-/commit/44a1728b6b021f97900c89e0c00f7d1a23ce0d43

This probably should also be hidden behind an experimental flag, but I have no idea how to do that :3

I have also tested it on my homeserver and there it seems to work properly.

Signed-off-by: Nicolas Werner nicolas.werner@hotmail.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)

[bradtgmurray]

Thanks for doing the upstreaming work here, @deepbluev7. Happy to provide whatever signoff is needed.

[deepbluev7]

@bradtgmurray It seems like adding a comment that has the signoff line on this PR should be enough. Thank you! <3

[bradtgmurray]

Signed-off-by: Brad Murray brad@beeper.com

[DMRobertson]

A unit test seems to be failing. Can be reproduced with trial tests.rest.client.test_relations.RelationsTestCase.test_deny_invalid_event.

Traceback (most recent call last):
  File "/home/runner/work/synapse/synapse/tests/rest/client/test_relations.py", line 125, in test_deny_invalid_event
    self.assertEquals(200, channel.code, channel.json_body)
  File "/home/runner/work/synapse/synapse/.tox/py-noextras/lib/python3.10/site-packages/twisted/trial/_synctest.py", line 424, in assertEqual
    super().assertEqual(first, second, msg)
  File "/opt/hostedtoolcache/Python/3.10.2/x64/lib/python3.10/unittest/case.py", line 845, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/opt/hostedtoolcache/Python/3.10.2/x64/lib/python3.10/unittest/case.py", line 838, in _baseAssertEqual
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: 200 != 404 : {'errcode': 'M_NOT_FOUND', 'error': 'Could not find event foo'}

tests.rest.client.test_relations.RelationsTestCase.test_deny_invalid_event

Any idea what's going on here?

[deepbluev7]

I don't see how that would in any way be related to my code. I was looking at it and then assumed it was because of something else, but if it only happens on my branch, it has to be my fault, I guess?

[deepbluev7]

Ah, it seems like the issue is that synapse throws an exception, if an event is not in the store, which causes a 404 when evaluating the pushrule

[deepbluev7]

That should fix it.

[deepbluev7]

Well, https://github.com/matrix-org/synapse/blob/develop/tests/handlers/test_deactivate_account.py was new :D

[deepbluev7]

Why would mypy error out now? I didn't touch that? D:

[DMRobertson]

Why would mypy error out now? I didn't touch that? D:

Should be fixed by #11832 and #11834. Can you merge in develop please?

[deepbluev7]

Sure, will do

[erikjohnston]

This looks plausible enough to me, in terms of validating that the MSC works.

[erikjohnston]

This probably should also be hidden behind an experimental flag, but I have no idea how to do that :3

You add the flag to

synapse/synapse/config/experimental.py

Lines 47 to 48 in 9f9d82a

	# MSC3266 (room summary api)
	self.msc3266_enabled: bool = experimental.get("msc3266_enabled", False)

and then you can access it via hs.config.experimental.*

[deepbluev7]

I might have something soon now, seems like the conflicts were much easier to resolve now and it was just a few lines to implement in Rust. It will take a while for me to compile rust analyzer and stuff so that I can actually test, if the code works though. So I might have something by Monday.

[deepbluev7]

Sup, updated, added capabilities and experimental config support, ported it to rust. No docstring, since the functions there have no docstrings anyway.

[erikjohnston]

Looks good, just some Qs and tidy ups

[erikjohnston]

@deepbluev7 as an aside I'd love to know how you found hacking on the Rust bit from a dev ex point of view?

[deepbluev7]

@deepbluev7 as an aside I'd love to know how you found hacking on the Rust bit from a dev ex point of view?

It's quite nice. Definitely makes some type errors easier to track down than Python. Biggest problems I had were, that poetry didn't tell me about compilation errors and figuring out this line:

let haystack = if let Some(haystack) = event.get(&**key) {

Also the separate install step is slightly annoying, but I was also just too lazy to setup the other tool that fixes that, so that is partially my own doing.

(Note, I have no prior rust experience, apart from reading what people write about it so far.)

[erikjohnston]

These changes look good, but I think there must have been a problem with merging in develop? I'm failing to see what has happened but the tests are really not happy

[erikjohnston]

Other than tests going wibbly, LGTM

[nico-famedly]

I think it was actually a typing error from switching Dict to dict. I should probably read up on what is the difference :D

[DMRobertson]

I think it was actually a typing error from switching Dict to dict. I should probably read up on what is the difference :D

dict with subscripts only works on Python 3.9+.

[nico-famedly]

Well, that seems to have fixed it at least!