Send content rules with pattern_type to clients #14356

deepbluev7 · 2022-11-02T20:13:16Z

Dirty fix for #14348

A proper fix would have proper tests and factor out the pattern_type cases as well as clean up the logic.

Signed-off-by: Nicolas Werner nicolas.werner@hotmail.de

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Pull request includes a sign off
Code style is correct
(run the linters)

Dirty fix for matrix-org#14348 A proper fix would have proper tests and factor out the pattern_type cases as well as clean up the logic. Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>

DMRobertson · 2022-11-02T21:11:49Z

synapse/push/clientformat.py

@@ -44,6 +44,12 @@ def format_push_rules_for_user(

        rulearray.append(template_rule)

+        pattern_type = template_rule.pop("pattern_type", None)


Are you completely sure this works?

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.

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??

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.

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)

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.

looks valid to me.

(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...)

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?

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.

reivilibre

I'm happy with this in principle; will leave time for someone else to have a look but otherwise would merge

reivilibre · 2022-11-15T14:01:59Z

synapse/push/clientformat.py

@@ -44,6 +44,12 @@ def format_push_rules_for_user(

        rulearray.append(template_rule)

+        pattern_type = template_rule.pop("pattern_type", None)


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.

reivilibre · 2022-11-15T14:05:20Z

synapse/push/clientformat.py

@@ -44,6 +44,12 @@ def format_push_rules_for_user(

        rulearray.append(template_rule)

+        pattern_type = template_rule.pop("pattern_type", None)


looks valid to me.

reivilibre · 2022-11-15T14:10:42Z

synapse/push/clientformat.py

@@ -44,6 +44,12 @@ def format_push_rules_for_user(

        rulearray.append(template_rule)

+        pattern_type = template_rule.pop("pattern_type", None)


(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...)

@ashfame

Synapse 1.72.0 (2022-11-22) =========================== Please note that Synapse now only supports PostgreSQL 11+, because PostgreSQL 10 has reached end-of-life, c.f. our [Deprecation Policy](https://github.com/matrix-org/synapse/blob/develop/docs/deprecation_policy.md). Bugfixes -------- - Update forgotten references to legacy metrics in the included Grafana dashboard. ([\matrix-org#14477](matrix-org#14477)) Synapse 1.72.0rc1 (2022-11-16) ============================== Features -------- - Add experimental support for [MSC3912](matrix-org/matrix-spec-proposals#3912): Relation-based redactions. ([\matrix-org#14260](matrix-org#14260)) - Build Debian packages for Ubuntu 22.10 (Kinetic Kudu). ([\matrix-org#14396](matrix-org#14396)) - Add an [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html) endpoint for user lookup based on third-party ID (3PID). Contributed by @ashfame. ([\matrix-org#14405](matrix-org#14405)) - Faster joins: include heroes' membership events in the partial join response, for rooms without a name or canonical alias. ([\matrix-org#14442](matrix-org#14442)) Bugfixes -------- - Faster joins: do not block creation of or queries for room aliases during the resync. ([\matrix-org#14292](matrix-org#14292)) - Fix a bug introduced in Synapse 1.64.0rc1 which could cause log spam when fetching events from other homeservers. ([\matrix-org#14347](matrix-org#14347)) - Fix a bug introduced in 1.66 which would not send certain pushrules to clients. Contributed by Nico. ([\matrix-org#14356](matrix-org#14356)) - Fix a bug introduced in v1.71.0rc1 where the power level event was incorrectly created during initial room creation. ([\matrix-org#14361](matrix-org#14361)) - Fix the refresh token endpoint to be under /r0 and /v3 instead of /v1. Contributed by Tulir @ Beeper. ([\matrix-org#14364](matrix-org#14364)) - Fix a long-standing bug where Synapse would raise an error when encountering an unrecognised field in a `/sync` filter, instead of ignoring it for forward compatibility. ([\matrix-org#14369](matrix-org#14369)) - Fix a background database update, introduced in Synapse 1.64.0, which could cause poor database performance. ([\matrix-org#14374](matrix-org#14374)) - Fix PostgreSQL sometimes using table scans for queries against the `event_search` table, taking a long time and a large amount of IO. ([\matrix-org#14409](matrix-org#14409)) - Fix rendering of some HTML templates (including emails). Introduced in v1.71.0. ([\matrix-org#14448](matrix-org#14448)) - Fix a bug introduced in Synapse 1.70.0 where the background updates to add non-thread unique indexes on receipts could fail when upgrading from 1.67.0 or earlier. ([\matrix-org#14453](matrix-org#14453)) Updates to the Docker image --------------------------- - Add all Stream Writer worker types to `configure_workers_and_start.py`. ([\matrix-org#14197](matrix-org#14197)) - Remove references to legacy worker types in the multi-worker Dockerfile. ([\matrix-org#14294](matrix-org#14294)) Improved Documentation ---------------------- - Upload documentation PRs to Netlify. ([\matrix-org#12947](matrix-org#12947), [\matrix-org#14370](matrix-org#14370)) - Add addtional TURN server configuration example based on [eturnal](https://github.com/processone/eturnal) and adjust general TURN server doc structure. ([\matrix-org#14293](matrix-org#14293)) - Add example on how to load balance /sync requests. Contributed by [aceArt](https://aceart.de). ([\matrix-org#14297](matrix-org#14297)) - Edit sample Nginx reverse proxy configuration to use HTTP/1.1. Contributed by Brad Jones. ([\matrix-org#14414](matrix-org#14414)) Deprecations and Removals ------------------------- - Remove support for PostgreSQL 10. ([\matrix-org#14392](matrix-org#14392), [\matrix-org#14397](matrix-org#14397)) Internal Changes ---------------- - Run unit tests against Python 3.11. ([\matrix-org#13812](matrix-org#13812)) - Add TLS support for generic worker endpoints. ([\matrix-org#14128](matrix-org#14128), [\matrix-org#14455](matrix-org#14455)) - Switch to a maintained action for installing Rust in CI. ([\matrix-org#14313](matrix-org#14313)) - Add override ability to `complement.sh` command line script to request certain types of workers. ([\matrix-org#14324](matrix-org#14324)) - Enabling testing of [MSC3874](matrix-org/matrix-spec-proposals#3874) (filtering of `/messages` by relation type) in complement. ([\matrix-org#14339](matrix-org#14339)) - Concisely log a failure to resolve state due to missing `prev_events`. ([\matrix-org#14346](matrix-org#14346)) - Use a maintained Github action to install Rust. ([\matrix-org#14351](matrix-org#14351)) - Cleanup old worker datastore classes. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#14375](matrix-org#14375)) - Test against PostgreSQL 15 in CI. ([\matrix-org#14394](matrix-org#14394)) - Remove unreachable code. ([\matrix-org#14410](matrix-org#14410)) - Clean-up event persistence code. ([\matrix-org#14411](matrix-org#14411)) - Update docstring to clarify that `get_partial_state_events_batch` does not just give you completely arbitrary partial-state events. ([\matrix-org#14417](matrix-org#14417)) - Fix mypy errors introduced by bumping the locked version of `attrs` and `gitpython`. ([\matrix-org#14433](matrix-org#14433)) - Make Dependabot only bump Rust deps in the lock file. ([\matrix-org#14434](matrix-org#14434)) - Fix an incorrect stub return type for `PushRuleEvaluator.run`. ([\matrix-org#14451](matrix-org#14451)) - Improve performance of `/context` in large rooms. ([\matrix-org#14461](matrix-org#14461))

Send content rules with pattern_type to clients

f7d2ad2

Dirty fix for matrix-org#14348 A proper fix would have proper tests and factor out the pattern_type cases as well as clean up the logic. Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>

deepbluev7 requested a review from a team as a code owner November 2, 2022 20:13

Add changelog

ab1d7c7

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>

deepbluev7 mentioned this pull request Nov 2, 2022

No .m.rule.contains_user_name rule sent to clients #14348

Closed

DMRobertson reviewed Nov 2, 2022

View reviewed changes

erikjohnston requested a review from a team November 15, 2022 13:50

reivilibre reviewed Nov 15, 2022

View reviewed changes

reivilibre requested a review from a team November 15, 2022 14:15

DMRobertson approved these changes Nov 15, 2022

View reviewed changes

DMRobertson merged commit 63cc56a into matrix-org:develop Nov 15, 2022

MatMaul mentioned this pull request Jun 14, 2023

push rules: fix internal conversion from _type to value #15781

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send content rules with pattern_type to clients #14356

Send content rules with pattern_type to clients #14356

deepbluev7 commented Nov 2, 2022 •

edited

Loading

DMRobertson Nov 2, 2022

deepbluev7 Nov 3, 2022

deepbluev7 Nov 3, 2022 •

edited

Loading

reivilibre Nov 15, 2022

reivilibre Nov 15, 2022

reivilibre Nov 15, 2022

DMRobertson Nov 15, 2022

DMRobertson Nov 15, 2022

reivilibre left a comment

reivilibre Nov 15, 2022

reivilibre Nov 15, 2022

reivilibre Nov 15, 2022

		@@ -44,6 +44,12 @@ def format_push_rules_for_user(

		rulearray.append(template_rule)

		pattern_type = template_rule.pop("pattern_type", None)

Send content rules with pattern_type to clients #14356

Send content rules with pattern_type to clients #14356

Conversation

deepbluev7 commented Nov 2, 2022 • edited Loading

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepbluev7 Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reivilibre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepbluev7 commented Nov 2, 2022 •

edited

Loading

deepbluev7 Nov 3, 2022 •

edited

Loading