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

Make event_match work on booleans and integers #13466

Closed
wants to merge 3 commits into from
Closed

Make event_match work on booleans and integers #13466

wants to merge 3 commits into from

Conversation

Johennes
Copy link

@Johennes Johennes commented Aug 5, 2022 •
edited
Loading

Currently event_match push rule conditions are evaluated against a flattened version of the event dict that drops all non-string values. This change converts booleans and integers into their string representation so that they can be matched as well.

Fixes: #13465
Signed-off-by: Johannes Marbach johannesm@element.io

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)

@Johennes
Make event_match work on booleans and integers
be1ab60 
Currently `event_match` push rule conditions are evaluated against
a flattened version of the event dict that drops all non-string
values. This change converts booleans and integers into their
string representation so that they can be matched as well.

Fixes: #13465
Signed-off-by: Johannes Marbach <johannesm@element.io>
@Johennes Johennes requested a review from a team as a code owner August 5, 2022 07:24
@Johennes Johennes marked this pull request as draft August 5, 2022 07:24
@Johennes Johennes marked this pull request as ready for review August 5, 2022 07:46
@clokep
Copy link
Member

clokep commented Aug 5, 2022

I think this is essentially a duplicate of #12181, which was rejected as needing an MSC, which was written as MSC3758.

@clokep clokep removed the request for review from a team August 5, 2022 11:15
@Johennes
Copy link
Author

Johennes commented Aug 5, 2022

Ugh, thanks @clokep, I didn't realize there was another PR for this already. 🤦‍♂️

@clokep
Copy link
Member

clokep commented Aug 5, 2022

Ugh, thanks @clokep, I didn't realize there was another PR for this already. 🤦‍♂️

No problem, it took me a while to find but I was 99% sure there was one! They're not fully overlapping (and yours has tests!) though so could be good to combine them.

@Johennes Johennes mentioned this pull request Aug 5, 2022
Closed
@Johennes
Copy link
Author

Johennes commented Aug 5, 2022

I wrote up MSC3862 describing the variant in this PR, too. This is not necessarily better than MSC3758 but I think having both options laid out side by side helps decide the way forward.

@anoadragon453 anoadragon453 mentioned this pull request Sep 7, 2022
Open
@Johennes
Copy link
Author

Johennes commented Feb 6, 2023

Closing this since matrix-org/matrix-spec-proposals#3862 has been abandoned.

@Johennes Johennes closed this Feb 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

event_match push rule condition doesn't work on booleans or integers
2 participants
@Johennes @clokep