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

MSC3873: Escape keys when flattening dicts. #15004

Merged
merged 3 commits into from Feb 8, 2023
Merged

MSC3873: Escape keys when flattening dicts. #15004

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b69fe7
Escape keys when flattening dicts.
clokep Feb 6, 2023
81cfab0
Add an experimental config flag.
clokep Feb 8, 2023
d2e1f0a
Merge remote-tracking branch 'origin/develop' into clokep/escape-dict…
clokep Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15004.feature
View file
Open in desktop
@@ -0,0 +1 @@
Implement [MSC3873](https://github.com/matrix-org/matrix-spec-proposals/pull/3873) to unambiguate push rule keys with dots in them.
2 changes: 1 addition & 1 deletion rust/src/push/base_rules.rs
View file
Open in desktop
Expand Up @@ -71,7 +71,7 @@ pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
priority_class: 5,
conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch(
EventMatchCondition {
key: Cow::Borrowed("content.m.relates_to.rel_type"),
key: Cow::Borrowed("content.m\\.relates_to.rel_type"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only reasonable thing to do here -- make it abide by the new behavior?

Copy link
Contributor Author

@clokep clokep Feb 7, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does make it hard to have this behind an experimental configuration flag though? I'm not really sure how to handle this.

Thoughts are welcome!

Copy link
Contributor

@squahtx squahtx Feb 7, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change visible to users? I suppose we could represent keys unambiguously inside Synapse and degrade them to ambiguous form when matching / presenting them to users and the experimental flag is disabled.

Then that raises the question of how custom push rules would work when a server admin toggles the experimental flag. We could also record whether a key is in msc3873 format, then decide what to do based on the experimental flag, but that would add a bunch of complexity to a fairly simple PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change visible to users?

No, this rule is disabled by default. Mostly Synapse needs to be internally consistent here?

I suppose we could represent keys unambiguously inside Synapse and degrade them to ambiguous form when matching / presenting them to users and the experimental flag is disabled.

The MSC originally had this and we removed it.

pattern: Some(Cow::Borrowed("m.replace")),
pattern_type: None,
},
Expand Down
5 changes: 5 additions & 0 deletions synapse/push/bulk_push_rule_evaluator.py
View file
Open in desktop
Expand Up @@ -521,6 +521,11 @@ def _flatten_dict(
if result is None:
result = {}
for key, value in d.items():
# Escape periods in the key with a backslash (and backslashes with an
# extra backslash). This is since a period is used as a separator between
# nested fields.
key = key.replace("\\", "\\\\").replace(".", "\\.")

if isinstance(value, str):
result[".".join(prefix + [key])] = value.lower()
elif isinstance(value, Mapping):
Expand Down
4 changes: 4 additions & 0 deletions tests/push/test_push_rule_evaluator.py
View file
Open in desktop
Expand Up @@ -48,6 +48,10 @@ def test_nested(self) -> None:
input = {"foo": {"bar": "abc"}}
self.assertEqual({"foo.bar": "abc"}, _flatten_dict(input))

# If a field has a dot in it, escape it.
input = {"m.foo": {"b\\ar": "abc"}}
self.assertEqual({"m\\.foo.b\\\\ar": "abc"}, _flatten_dict(input))

def test_non_string(self) -> None:
"""Non-string items are dropped."""
input: Dict[str, Any] = {
Expand Down