Implement MSC3873 to handle escaped dots in push rule keys by clokep · Pull Request #3134 · matrix-org/matrix-js-sdk

clokep · 2023-02-07T19:08:17Z

This implements MSC3873 which allows for disambiguating the key used in event_match between fields that have a . in them vs. using . as a field separator.

The MSC doesn't have any backwards compatibility clauses as none of the default push rules are affected. Some custom rules might behave differently after this change. This would fix #1454.

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
~~Sign-off given on the changes (see CONTRIBUTING.md)~~

Here's what your changelog entry will look like:

✨ Features

Implement MSC3873 to handle escaped dots in push rule keys (#3134). Fixes undefined/matrix-js-sdk#1454.

andybalaam

Some questions, and an ask for more unit tests.

spec/unit/pushprocessor.spec.ts

src/pushprocessor.ts

This is ~80% slower than the basic split(".").

This is ~50% slower than a simple split(".").

andybalaam

One typo, looks great!

spec/unit/pushprocessor.spec.ts

andybalaam · 2023-02-24T09:31:38Z

The downstream test failed https://github.com/matrix-org/matrix-js-sdk/actions/runs/4260935958/jobs/7414674597

(i.e. matrix-react-sdk tests failed with this change)

clokep · 2023-02-24T13:49:39Z

The downstream test failed matrix-org/matrix-js-sdk/actions/runs/4260935958/jobs/7414674597

(i.e. matrix-react-sdk tests failed with this change)

matrix-org/matrix-react-sdk#10237 should fix it (passes locally). I'm not sure if CI will pick-up on the "fix" though or not?

clokep · 2023-02-24T13:55:41Z

The downstream test failed matrix-org/matrix-js-sdk/actions/runs/4260935958/jobs/7414674597
(i.e. matrix-react-sdk tests failed with this change)

matrix-org/matrix-react-sdk#10237 should fix it (passes locally). I'm not sure if CI will pick-up on the "fix" though or not?

Although this now seems to be failing lint because we don't provide the keyParts when creating push rules. I think we really need to separate what we're using "internally" vs. what the protocol expects here. Everything kind of assumes they're one and the same though.

andybalaam · 2023-02-27T09:43:29Z

@clokep are you blocked on me/us for this? Give me a ping if so.

clokep · 2023-02-27T13:46:15Z

@clokep are you blocked on me/us for this? Give me a ping if so.

No, just trying to figure out how to refactor tests after the changes I made.

clokep · 2023-02-27T14:04:54Z

@andybalaam I think this is ready for another look!

clokep · 2023-02-27T14:07:55Z

src/pushprocessor.ts

        if (!newRules.global) newRules.global = {} as PushRuleSet;
        if (!newRules.global.override) newRules.global.override = [];
-        if (!newRules.global.override) newRules.global.underride = [];
+        if (!newRules.global.underride) newRules.global.underride = [];


This seems to have been a separate, long-standing bug?

Yeah, how exciting. What would the symptoms have been? Maybe we can find an issue to close....

It was added in #2873, but that also added (on line 191) a newRules.global.underride ?? [], likely to solve the same problem? 🤷 (I don't think this would have a user-visible bug.)

clokep · 2023-02-27T14:08:46Z

src/pushprocessor.ts

+        if (!newRules) newRules = {} as IPushRules;
+        if (!newRules.global) newRules.global = {} as PushRuleSet;
+        if (!newRules.global.override) newRules.global.override = [];
+        if (!newRules.global.room) newRules.global.room = [];
+        if (!newRules.global.sender) newRules.global.sender = [];
+        if (!newRules.global.underride) newRules.global.underride = [];


This was copied from rewriteDefaultRules, but it might make more sense to just skip any that are undefined instead of mutating the input value?

Seems fine like this, if it's more consistent.

andybalaam

Looks good to me.

@EnricoSchw

* Implement MSC3758: a push rule condition to match event properties exactly ([\matrix-org#3179](matrix-org#3179)). * Enable group calls without video and audio track by configuration of MatrixClient ([\matrix-org#3162](matrix-org#3162)). Contributed by @EnricoSchw. * Updates to protocol used for Sign in with QR code ([\matrix-org#3155](matrix-org#3155)). Contributed by @hughns. * Implement MSC3873 to handle escaped dots in push rule keys ([\matrix-org#3134](matrix-org#3134)). Fixes undefined/matrix-js-sdk#1454. * Fix spec compliance issue around encrypted `m.relates_to` ([\matrix-org#3178](matrix-org#3178)). * Fix reactions in threads sometimes causing stuck notifications ([\matrix-org#3146](matrix-org#3146)). Fixes element-hq/element-web#24000. Contributed by @justjanne.

clokep added the T-Enhancement label Feb 7, 2023

clokep mentioned this pull request Feb 7, 2023

MSC3873: event_match dotted keys matrix-org/matrix-spec-proposals#3873

Merged

clokep added 3 commits February 8, 2023 10:08

Add comments.

1facb89

Implment MSC3873 to handle escaped dots in keys.

0600523

Add some comments about tests.

0ac33c8

clokep force-pushed the dotted-keys branch from 7e90014 to 0ac33c8 Compare February 8, 2023 15:08

github-actions bot deployed to PR Documentation Preview February 8, 2023 15:11 View deployment

clokep marked this pull request as ready for review February 8, 2023 15:56

clokep requested a review from a team as a code owner February 8, 2023 15:56

clokep requested review from andybalaam and weeman1337 February 8, 2023 15:56

andybalaam requested changes Feb 14, 2023

View reviewed changes

clokep added 4 commits February 14, 2023 10:03

Merge remote-tracking branch 'upstream/develop' into dotted-keys

9da576c

Clarify spec behavior.

f680ad8

Fix typo.

a23ed41

Don't manually iterate string.

704ed20

github-actions bot deployed to PR Documentation Preview February 14, 2023 15:27 View deployment

clokep added 2 commits February 14, 2023 10:32

Clean-up tests.

0b9d896

Simplify tests.

08144fa

clokep removed the request for review from weeman1337 February 15, 2023 12:53

Add more tests & fix bug with empty parts.

b724cf1

github-actions bot deployed to PR Documentation Preview February 15, 2023 14:41 View deployment

clokep added 4 commits February 15, 2023 14:57

Add more edge cases.

7e11af8

Add a regular expression solution.

97fe8e9

This is ~80% slower than the basic split(".").

Split on a simpler regular expression.

322913e

This is ~50% slower than a simple split(".").

Merge remote-tracking branch 'upstream/develop' into dotted-keys

7016e10

github-actions bot deployed to PR Documentation Preview February 15, 2023 20:05 View deployment

clokep requested a review from andybalaam February 15, 2023 20:09

clokep added 2 commits February 15, 2023 15:24

Remove redundant case in regex.

d7fcd36

Enable sticky regex.

c8a8f09

github-actions bot deployed to PR Documentation Preview February 22, 2023 20:01 View deployment

andybalaam approved these changes Feb 23, 2023

View reviewed changes

spec/unit/pushprocessor.spec.ts Outdated Show resolved Hide resolved

Fix typo.

3c445f6

github-actions bot deployed to PR Documentation Preview February 23, 2023 18:03 View deployment

andybalaam added this pull request to the merge queue Feb 24, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2023

clokep mentioned this pull request Feb 24, 2023

Pass push rules through rewriteDefaultRules for compatibility with matrix-js-sdk#3134. matrix-org/matrix-react-sdk#10237

Closed

2 tasks

Switch back to external cache, but clean out obsolete cached values.

ab9f212

clokep requested a review from andybalaam February 27, 2023 14:04

clokep added 3 commits February 27, 2023 09:06

Remove obsolete property.

fc66cec

Merge remote-tracking branch 'upstream/develop' into dotted-keys

cdb8760

Remove more obsolete properties.

9e7c3c9

clokep commented Feb 27, 2023

View reviewed changes

github-actions bot deployed to PR Documentation Preview February 27, 2023 14:10 View deployment

This was referenced Feb 27, 2023

GYU: Implement MSC3952 in matrix-js-sdk element-hq/element-web#24376

Closed

Implement MSC3758: a push rule condition to match event properties exactly #3179

Merged

andybalaam approved these changes Mar 1, 2023

View reviewed changes

andybalaam added this pull request to the merge queue Mar 1, 2023

Merged via the queue into matrix-org:develop with commit c8a4d9b Mar 1, 2023

clokep deleted the dotted-keys branch March 1, 2023 13:33

clokep mentioned this pull request May 16, 2023

Fix-up event_fields in filters matrix-org/synapse#15607

Merged

Uh oh!

Conversation

clokep commented Feb 7, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

✨ Features

Uh oh!

andybalaam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andybalaam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andybalaam commented Feb 24, 2023

Uh oh!

clokep commented Feb 24, 2023

Uh oh!

clokep commented Feb 24, 2023

Uh oh!

andybalaam commented Feb 27, 2023

Uh oh!

clokep commented Feb 27, 2023

Uh oh!

clokep commented Feb 27, 2023

Uh oh!

clokep Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

andybalaam Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

clokep Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

clokep Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

andybalaam Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

andybalaam left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clokep commented Feb 7, 2023 •

edited by github-actions bot

Loading