MSC3873: Escape keys when flattening dicts. #15004

clokep · 2023-02-06T20:15:07Z

This is a potential implementation for MSC3873, ~~it doesn't quite match the MSC right now due to implementing a simplification of it.~~ (The MSC was updated to match this.)

~~Based on #15002.~~

clokep · 2023-02-07T20:00:29Z

rust/src/push/base_rules.rs

@@ -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"),


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

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!

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.

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.

squahtx

The current changes look good. Gating it behind an experimental flag would be nice but sounds like a lot of faff. If you can see a nice way to go about it, I'd go for it.

clokep · 2023-02-08T17:26:32Z

Gating it behind an experimental flag would be nice but sounds like a lot of faff. If you can see a nice way to go about it, I'd go for it.

I did this, unfortunately if you enable both it + MSC3958 then that one won't work properly. I think this is "OK" for experimental stuff though?

…-keys

squahtx · 2023-02-08T17:38:59Z

I did this, unfortunately if you enable both it + MSC3958 then that one won't work properly. I think this is "OK" for experimental stuff though?

Sounds okay to have those experimental features be mutually exclusive? I'm mildly concerned that we'll forget to make them work together when stabilizing them.

clokep · 2023-02-08T17:59:54Z

Sounds okay to have those experimental features be mutually exclusive? I'm mildly concerned that we'll forget to make them work together when stabilizing them.

There's a test that will fail if we forget! 😄

SuperSandro2000 · 2023-02-21T21:34:32Z

synapse/config/experimental.py

+        self.msc3783_escape_event_match_key = experimental.get(
+            "msc3783_escape_event_match_key", False


Shouldn't this be this?

Suggested change

self.msc3783_escape_event_match_key = experimental.get(

"msc3783_escape_event_match_key", False

self.msc3873_escape_event_match_key = experimental.get(

"msc3873_escape_event_match_key", False

Sorry -- I don't see any change in your suggestion?

😅 did the same typo. Now corrected.

Oh the MSC # is typoed, I see. Umm, yeah that's not awesome, although not super important. 😢

@thezaidbintariq

Synapse 1.78.0 (2023-02-28) =========================== Bugfixes -------- - Fix a bug introduced in Synapse 1.76 where 5s delays would occasionally occur in deployments using workers. ([\#15150](matrix-org/synapse#15150)) Synapse 1.78.0rc1 (2023-02-21) ============================== Features -------- - Implement the experimental `exact_event_match` push rule condition from [MSC3758](matrix-org/matrix-spec-proposals#3758). ([\#14964](matrix-org/synapse#14964)) - Add account data to the command line [user data export tool](https://matrix-org.github.io/synapse/v1.78/usage/administration/admin_faq.html#how-can-i-export-user-data). ([\#14969](matrix-org/synapse#14969)) - Implement [MSC3873](matrix-org/matrix-spec-proposals#3873) to disambiguate push rule keys with dots in them. ([\#15004](matrix-org/synapse#15004)) - Allow Synapse to use a specific Redis [logical database](https://redis.io/commands/select/) in worker-mode deployments. ([\#15034](matrix-org/synapse#15034)) - Tag opentracing spans for federation requests with the name of the worker serving the request. ([\#15042](matrix-org/synapse#15042)) - Implement the experimental `exact_event_property_contains` push rule condition from [MSC3966](matrix-org/matrix-spec-proposals#3966). ([\#15045](matrix-org/synapse#15045)) - Remove spurious `dont_notify` action from the defaults for the `.m.rule.reaction` pushrule. ([\#15073](matrix-org/synapse#15073)) - Update the error code returned when user sends a duplicate annotation. ([\#15075](matrix-org/synapse#15075)) Bugfixes -------- - Prevent clients from reporting nonexistent events. ([\#13779](matrix-org/synapse#13779)) - Return spec-compliant JSON errors when unknown endpoints are requested. ([\#14605](matrix-org/synapse#14605)) - Fix a long-standing bug where the room aliases returned could be corrupted. ([\#15038](matrix-org/synapse#15038)) - Fix a bug introduced in Synapse 1.76.0 where partially-joined rooms could not be deleted using the [purge room API](https://matrix-org.github.io/synapse/latest/admin_api/rooms.html#delete-room-api). ([\#15068](matrix-org/synapse#15068)) - Fix a long-standing bug where federated joins would fail if the first server in the list of servers to try is not in the room. ([\#15074](matrix-org/synapse#15074)) - Fix a bug introduced in Synapse v1.74.0 where searching with colons when using ICU for search term tokenisation would fail with an error. ([\#15079](matrix-org/synapse#15079)) - Reduce the likelihood of a rare race condition where rejoining a restricted room over federation would fail. ([\#15080](matrix-org/synapse#15080)) - Fix a bug introduced in Synapse 1.76 where workers would fail to start if the `health` listener was configured. ([\#15096](matrix-org/synapse#15096)) - Fix a bug introduced in Synapse 1.75 where the [portdb script](https://matrix-org.github.io/synapse/release-v1.78/postgres.html#porting-from-sqlite) would fail to run after a room had been faster-joined. ([\#15108](matrix-org/synapse#15108)) Improved Documentation ---------------------- - Document how to start Synapse with Poetry. Contributed by @thezaidbintariq. ([\#14892](matrix-org/synapse#14892), [\#15022](matrix-org/synapse#15022)) - Update delegation documentation to clarify that SRV DNS delegation does not eliminate all needs to serve files from .well-known locations. Contributed by @williamkray. ([\#14959](matrix-org/synapse#14959)) - Fix a mistake in registration_shared_secret_path docs. ([\#15078](matrix-org/synapse#15078)) - Refer to a more recent blog post on the [Database Maintenance Tools](https://matrix-org.github.io/synapse/latest/usage/administration/database_maintenance_tools.html) page. Contributed by @jahway603. ([\#15083](matrix-org/synapse#15083)) Internal Changes ---------------- - Re-type hint some collections as read-only. ([\#13755](matrix-org/synapse#13755)) - Faster joins: don't stall when another user joins during a partial-state room resync. ([\#14606](matrix-org/synapse#14606)) - Add a class `UnpersistedEventContext` to allow for the batching up of storing state groups. ([\#14675](matrix-org/synapse#14675)) - Add a check to ensure that locked dependencies have source distributions available. ([\#14742](matrix-org/synapse#14742)) - Tweak comment on `_is_local_room_accessible` as part of room visibility in `/hierarchy` to clarify the condition for a room being visible. ([\#14834](matrix-org/synapse#14834)) - Prevent `WARNING: there is already a transaction in progress` lines appearing in PostgreSQL's logs on some occasions. ([\#14840](matrix-org/synapse#14840)) - Use `StrCollection` to avoid potential bugs with `Collection[str]`. ([\#14929](matrix-org/synapse#14929)) - Improve performance of `/sync` in a few situations. ([\#14973](matrix-org/synapse#14973)) - Limit concurrent event creation for a room to avoid state resolution when sending bursts of events to a local room. ([\#14977](matrix-org/synapse#14977)) - Skip calculating unread push actions in /sync when enable_push is false. ([\#14980](matrix-org/synapse#14980)) - Add a schema dump symlinks inside `contrib`, to make it easier for IDEs to interrogate Synapse's database schema. ([\#14982](matrix-org/synapse#14982)) - Improve type hints. ([\#15008](matrix-org/synapse#15008), [\#15026](matrix-org/synapse#15026), [\#15027](matrix-org/synapse#15027), [\#15028](matrix-org/synapse#15028), [\#15031](matrix-org/synapse#15031), [\#15035](matrix-org/synapse#15035), [\#15052](matrix-org/synapse#15052), [\#15072](matrix-org/synapse#15072), [\#15084](matrix-org/synapse#15084)) - Update [MSC3952](matrix-org/matrix-spec-proposals#3952) support based on changes to the MSC. ([\#15037](matrix-org/synapse#15037)) - Avoid mutating a cached value in `get_user_devices_from_cache`. ([\#15040](matrix-org/synapse#15040)) - Fix a rare exception in logs on start up. ([\#15041](matrix-org/synapse#15041)) - Update pyo3-log to v0.8.1. ([\#15043](matrix-org/synapse#15043)) - Avoid mutating cached values in `_generate_sync_entry_for_account_data`. ([\#15047](matrix-org/synapse#15047)) - Refactor arguments of `try_unbind_threepid` and `_try_unbind_threepid_with_id_server` to not use dictionaries. ([\#15053](matrix-org/synapse#15053)) - Merge debug logging from the hotfixes branch. ([\#15054](matrix-org/synapse#15054)) - Faster joins: omit device list updates originating from partial state rooms in /sync responses without lazy loading of members enabled. ([\#15069](matrix-org/synapse#15069)) - Fix clashing database transaction name. ([\#15070](matrix-org/synapse#15070)) - Upper-bound frozendict dependency. This works around us being unable to test installing our wheels against Python 3.11 in CI. ([\#15114](matrix-org/synapse#15114)) - Tweak logging for when a worker waits for its view of a replication stream to catch up. ([\#15120](matrix-org/synapse#15120)) <details><summary>Locked dependency updates</summary> - Bump bleach from 5.0.1 to 6.0.0. ([\#15059](matrix-org/synapse#15059)) - Bump cryptography from 38.0.4 to 39.0.1. ([\#15020](matrix-org/synapse#15020)) - Bump ruff version from 0.0.230 to 0.0.237. ([\#15033](matrix-org/synapse#15033)) - Bump dtolnay/rust-toolchain from 9cd00a88a73addc8617065438eff914dd08d0955 to 25dc93b901a87e864900a8aec6c12e9aa794c0c3. ([\#15060](matrix-org/synapse#15060)) - Bump systemd-python from 234 to 235. ([\#15061](matrix-org/synapse#15061)) - Bump serde_json from 1.0.92 to 1.0.93. ([\#15062](matrix-org/synapse#15062)) - Bump types-requests from 2.28.11.8 to 2.28.11.12. ([\#15063](matrix-org/synapse#15063)) - Bump types-pillow from 9.4.0.5 to 9.4.0.10. ([\#15064](matrix-org/synapse#15064)) - Bump sentry-sdk from 1.13.0 to 1.15.0. ([\#15065](matrix-org/synapse#15065)) - Bump types-jsonschema from 4.17.0.3 to 4.17.0.5. ([\#15099](matrix-org/synapse#15099)) - Bump types-bleach from 5.0.3.1 to 6.0.0.0. ([\#15100](matrix-org/synapse#15100)) - Bump dtolnay/rust-toolchain from 25dc93b901a87e864900a8aec6c12e9aa794c0c3 to e12eda571dc9a5ee5d58eecf4738ec291c66f295. ([\#15101](matrix-org/synapse#15101)) - Bump dawidd6/action-download-artifact from 2.24.3 to 2.25.0. ([\#15102](matrix-org/synapse#15102)) - Bump types-pillow from 9.4.0.10 to 9.4.0.13. ([\#15104](matrix-org/synapse#15104)) - Bump types-setuptools from 67.1.0.0 to 67.3.0.1. ([\#15105](matrix-org/synapse#15105)) </details>

clokep mentioned this pull request Feb 6, 2023

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

Merged

Base automatically changed from clokep/flatten-tests to develop February 7, 2023 11:56

Escape keys when flattening dicts.

2b69fe7

clokep force-pushed the clokep/escape-dict-keys branch from 822a9f1 to 2b69fe7 Compare February 7, 2023 14:55

clokep commented Feb 7, 2023

View reviewed changes

clokep marked this pull request as ready for review February 7, 2023 20:13

clokep requested a review from a team as a code owner February 7, 2023 20:13

squahtx approved these changes Feb 7, 2023

View reviewed changes

Add an experimental config flag.

81cfab0

Merge remote-tracking branch 'origin/develop' into clokep/escape-dict…

d2e1f0a

…-keys

clokep merged commit c951fbe into develop Feb 8, 2023

clokep deleted the clokep/escape-dict-keys branch February 8, 2023 18:09

SuperSandro2000 reviewed Feb 21, 2023

View reviewed changes

clokep mentioned this pull request Feb 22, 2023

Fix a typo in MSC3873 config option. #15138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSC3873: Escape keys when flattening dicts. #15004

MSC3873: Escape keys when flattening dicts. #15004

clokep commented Feb 6, 2023 •

edited

clokep Feb 7, 2023

clokep Feb 7, 2023 •

edited

squahtx Feb 7, 2023 •

edited

clokep Feb 8, 2023

squahtx left a comment

clokep commented Feb 8, 2023

squahtx commented Feb 8, 2023 •

edited

clokep commented Feb 8, 2023

SuperSandro2000 Feb 21, 2023 •

edited

clokep Feb 22, 2023

SuperSandro2000 Feb 22, 2023

clokep Feb 22, 2023

clokep Feb 22, 2023

		self.msc3783_escape_event_match_key = experimental.get(
		"msc3783_escape_event_match_key", False

MSC3873: Escape keys when flattening dicts. #15004

MSC3873: Escape keys when flattening dicts. #15004

Conversation

clokep commented Feb 6, 2023 • edited

clokep Feb 7, 2023

Choose a reason for hiding this comment

clokep Feb 7, 2023 • edited

Choose a reason for hiding this comment

squahtx Feb 7, 2023 • edited

Choose a reason for hiding this comment

clokep Feb 8, 2023

Choose a reason for hiding this comment

squahtx left a comment

Choose a reason for hiding this comment

clokep commented Feb 8, 2023

squahtx commented Feb 8, 2023 • edited

clokep commented Feb 8, 2023

SuperSandro2000 Feb 21, 2023 • edited

Choose a reason for hiding this comment

clokep Feb 22, 2023

Choose a reason for hiding this comment

SuperSandro2000 Feb 22, 2023

Choose a reason for hiding this comment

clokep Feb 22, 2023

Choose a reason for hiding this comment

clokep Feb 22, 2023

Choose a reason for hiding this comment

clokep commented Feb 6, 2023 •

edited

clokep Feb 7, 2023 •

edited

squahtx Feb 7, 2023 •

edited

squahtx commented Feb 8, 2023 •

edited

SuperSandro2000 Feb 21, 2023 •

edited