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

Pushrules with Dotted Value keys break #1454

Closed
schmop opened this issue Aug 31, 2020 · 7 comments · Fixed by #3134
Closed

Pushrules with Dotted Value keys break #1454

schmop opened this issue Aug 31, 2020 · 7 comments · Fixed by #3134

Comments

@schmop
Copy link
Contributor

schmop commented Aug 31, 2020

I wanted to configure a pushrule ignoring "m.replace" events.
Therefore i used this configuration:

{
    scope: 'global',
    kind: 'override',
    ruleId: 'rule.no_edits',
    body: {
        actions: [
            'dont_notify'
        ],
        conditions: [
            {
                'kind': 'event_match',
                'key': 'type',
                'pattern': 'm.room.message'
            },
            {
                'kind': 'event_match',
                'key': 'content.m.relates_to.rel_type',
                'pattern': 'm.replace'
            },
        ]
    },
}

But the behaviour of Synapse and the matrix-js-sdk didn't match.
The condition key content.m.relates_to.rel_type is interpreted in the valueDottedForKey method wrong:

Actual part splitting:
['content', 'm', 'relates_to', 'rel_type']
Needed splitting:
['content', 'm.relates_to', 'rel_type']

https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/pushprocessor.js#L292

        const parts = key.split('.');

Maybe the approach of just splitting by the dot is undesirable.
Synapse takes the apporach the other way around, converting the nested object to a flat object and afterwards just accessing by the key.

Pseudo:

function accessDottedKey(evt, key) {
   flatEvt = makeFlat(evt);
   return flatEvt[key];
}
@turt2live
Copy link
Member

This is a feature of the spec.

@schmop
Copy link
Contributor Author

schmop commented Aug 31, 2020

Under https://matrix.org/docs/spec/client_server/latest#conditions i only see that key is defined as a

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

How can the feature of the spec result in inconsistent behaviour over server (synapse) and client (matrix-js-sdk)?
This bug renders fields in events, that contain dots (like 'm.relates_to') non-referencable.
Or does it mean, that synapse has the bug here, because synapse IMO correctly supresses the push notification, but the matrix-js-sdk does not.

@turt2live
Copy link
Member

it would be a synapse bug, though would likely get redirected to the spec to be fixed via an MSC

@t3chguy
Copy link
Member

t3chguy commented Sep 1, 2020

This doesn't sound like a Synapse bug as per https://github.com/matrix-org/matrix-doc/issues/2641#issuecomment-645275547

It sounds like a js-sdk pushprocessor.js bug if it is relating to encrypted rooms.

@t3chguy t3chguy reopened this Sep 1, 2020
@turt2live
Copy link
Member

Synapse is off-spec though?

@t3chguy
Copy link
Member

t3chguy commented Sep 1, 2020

Based on https://github.com/matrix-org/matrix-doc/issues/2641 saying

It's unclear how these are handled.

I'd say neither are incorrect and the spec needs clarification/changing

@turt2live
Copy link
Member

The spec is fairly clear on what to do, and the issue is categorized as a wart rather than a bug. All this means is that it needs an MSC, but until then it's something we live with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants