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

Send content rules with pattern_type to clients #14356

Merged
merged 2 commits into from
Nov 15, 2022
Merged

Send content rules with pattern_type to clients #14356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7d2ad2
Send content rules with pattern_type to clients
deepbluev7 Nov 2, 2022
ab1d7c7
Add changelog
deepbluev7 Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14356.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug introduced in 1.66 which would not send certain pushrules to clients. Contributed by Nico.
16 changes: 13 additions & 3 deletions synapse/push/clientformat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ def format_push_rules_for_user(

rulearray.append(template_rule)

pattern_type = template_rule.pop("pattern_type", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you completely sure this works?

  1. This mutates template_rule in-place. We explicitly take a copy before mutating it below, which suggests that it's not safe to do this.
  2. AFAICS "pattern_type" lives in the same place as "pattern", i.e. it is a key of things in template_rules["conditions"]. So I don't think this snippet actually does anything??

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 do know that without that the rule didn't come down the pushrules/ endpoint. If that corrupts some template or so, that I don't know. Rules not only have a pattern in the conditions, content rules, like the username rule, have them in the rule themselves and no conditions.

Copy link
Contributor Author

@deepbluev7 deepbluev7 Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also, isn't it mutated 2 lines lower as well? I am a bit confused now.

(The patch is also live on my servers and those still work)

Copy link
Contributor

Choose a reason for hiding this comment

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

template_rule = _rule_to_template(r) above gives you a fresh template_rule dict, so not sure why it wouldn't be safe to mutate.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks valid to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

(to confirm that content rules don't have the pattern inside a conditions block, but instead have them in the top level: https://spec.matrix.org/v1.4/client-server-api/#default-content-rules. I find it weird. I see why people want to re-write push rules...)

Copy link
Contributor

Choose a reason for hiding this comment

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

template_rule = _rule_to_template(r) above gives you a fresh template_rule dict, so not sure why it wouldn't be safe to mutate.

True. I think it was line -58 that had me worried, but perhaps that deepcopy isn't necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, isn't it mutated 2 lines lower as well? I am a bit confused now.

Oh you're quite right. I have no idea what that deepcopy is for then.

if pattern_type == "user_id":
template_rule["pattern"] = user.to_string()
elif pattern_type == "user_localpart":
template_rule["pattern"] = user.localpart

template_rule["enabled"] = enabled

if "conditions" not in template_rule:
Expand Down Expand Up @@ -93,10 +99,14 @@ def _rule_to_template(rule: PushRule) -> Optional[Dict[str, Any]]:
if len(rule.conditions) != 1:
return None
thecond = rule.conditions[0]
if "pattern" not in thecond:
return None

templaterule = {"actions": rule.actions}
templaterule["pattern"] = thecond["pattern"]
if "pattern" in thecond:
templaterule["pattern"] = thecond["pattern"]
elif "pattern_type" in thecond:
templaterule["pattern_type"] = thecond["pattern_type"]
else:
return None
else:
# This should not be reached unless this function is not kept in sync
# with PRIORITY_CLASS_INVERSE_MAP.
Expand Down