Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement MSC3051 to support multiple relations per event #16111

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

chayleaf
Copy link

@chayleaf chayleaf commented Aug 14, 2023

It seems there's considerable interest in MSC3051 - scalable relations (multiple relations per event in an array) as the limits of the current model become more and more apparent. This is a WIP attempt to implement it (there's no tests yet).

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)

@clokep
Copy link
Member

clokep commented Aug 14, 2023

Don't forget to sign-off on your changes. Shout if you want us to run CI, but seems like this is very much WIP so far.

@chayleaf
Copy link
Author

Yes, I'll rebase it later anyway. Is there a simple way to debug internal server errors in tests?

@clokep
Copy link
Member

clokep commented Aug 14, 2023

Yes, I'll rebase it later anyway. Is there a simple way to debug internal server errors in tests?

Look in the _trial_temp/test.log file. Feel free to ask questions in #synapse-dev:matrix.org.

@chayleaf
Copy link
Author

This passes tests now, but I'm not sure which new tests I should add. More importantly, there are a bunch of "XXX" comments where I'm not confident whether the existing checks should be relaxed with the introduction of multiple relations per an event. Overall I suppose this isn't "too big" of a change in terms of backwards compatibility.

@chayleaf chayleaf marked this pull request as ready for review August 14, 2023 14:54
@chayleaf chayleaf requested a review from a team as a code owner August 14, 2023 14:54
This allows using m.relations in place of m.relates_to for listing
multiple relations for a single event.

Signed-off-by: chayleaf <[email protected]>
@chayleaf chayleaf changed the title WIP: MSC3051 Implement MSC3051 Aug 14, 2023
@chayleaf chayleaf marked this pull request as draft August 14, 2023 15:09
Signed-off-by: chayleaf <[email protected]>
@clokep
Copy link
Member

clokep commented Aug 22, 2023

@chayleaf Are you looking for a review on this PR? (I'm guessing no since it was marked as a draft.)

@clokep clokep removed the request for review from a team August 22, 2023 14:01
@chayleaf
Copy link
Author

yes, sorry, I thought it's fine but it still has some bugs which I found by adding new tests

@clokep clokep changed the title Implement MSC3051 Implement MSC3051 to support multiple relations per event Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants