Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

event_match key in push rule conditions does not cater for field names with dots #648

Closed
richvdh opened this issue Jun 17, 2020 · 12 comments · Fixed by matrix-org/matrix-spec-proposals#3873
Labels
A-Client-Server Issues affecting the CS API A-Push wart A point where the protocol is inconsistent or inelegant

Comments

@richvdh
Copy link
Member

richvdh commented Jun 17, 2020

https://matrix.org/docs/spec/client_server/r0.6.1#conditions says:

key The dot-separated field of the event to match, e.g. content.body

Various standard event fields (notably those in matrix-org/matrix-spec-proposals#1849) have dots in their names. It's unclear how these are handled.

@richvdh
Copy link
Member Author

richvdh commented Jun 17, 2020

In practice, it looks like {"key": "m.foo", pattern: "bar"} will match both:

{
    "m": { "foo": "bar" }
}

and

{
    "m.foo": "bar"
}

@richvdh
Copy link
Member Author

richvdh commented Jun 17, 2020

related: matrix-org/matrix-spec-proposals#2637

@richvdh richvdh added A-Client-Server Issues affecting the CS API A-Push spec-bug Something which is in the spec, but is wrong labels Jun 17, 2020
@t3chguy
Copy link
Member

t3chguy commented Jun 17, 2020

What will it match in the case of ambiguity?

{
    "m": { "foo": "bar" },
    "m.foo": "baz"
}

@richvdh
Copy link
Member Author

richvdh commented Jun 17, 2020

IT DEPENDS 🎉

depends what order python decides to go through the dictionary, which is probably ultimately determined by the order they arrive over the wire from the client.

so that's fun.

@jplatte
Copy link
Contributor

jplatte commented Jun 17, 2020

Could we just deprecate the "dot-separated" in

key The dot-separated field of the event to match, e.g. content.body

?

@t3chguy
Copy link
Member

t3chguy commented Jun 17, 2020

How would you refer to match content.body then?

@jplatte
Copy link
Contributor

jplatte commented Jun 17, 2020

Well, like in your example,

{
    "content": { "body": "foo" }
}

@t3chguy
Copy link
Member

t3chguy commented Jun 17, 2020

My example is of an event body, how would you refer to it in the key string for the push rule which is what this issue is about?

@jplatte
Copy link
Contributor

jplatte commented Jun 17, 2020

Okay, I misunderstood then. In that case, and if we want to continue only matching on one key (which is probably a good idea, otherwise we'd open another can of worms), maybe we could adopt JSON pointers? That would make the implementation much more straight-forward, though we'd have to figure out the compatibility story for that.

@richvdh
Copy link
Member Author

richvdh commented Jun 17, 2020

yeah the problem is that we could do any number of better things, but it's going to be hard to maintain compatibility. Ultimately servers are probably going to have to support either of two formats. Meanwhile it's more of a wart than a practical problem.

@richvdh richvdh added wart A point where the protocol is inconsistent or inelegant and removed spec-bug Something which is in the spec, but is wrong labels Jun 17, 2020
@uhoreg
Copy link
Member

uhoreg commented Jun 17, 2020

FWIW, there's a similar issue with filters, and it's solved there by escaping "." in field names by a "\".
image

@schmop
Copy link

schmop commented Nov 8, 2021

Escaping would be fairly cheap to implement and it would fix the issues with dots in a prop key.
Would love to see this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Client-Server Issues affecting the CS API A-Push wart A point where the protocol is inconsistent or inelegant
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants