Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC3664: Pushrules for relations #3664

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

deepbluev7
Copy link
Contributor

@deepbluev7 deepbluev7 commented Jan 21, 2022

Rendered

Partial implementation was done by Beeper, but without the appropriate namespacing and it is missing the filtering by rel_type: https://gitlab.com/beeper/synapse/-/commit/44a1728b6b021f97900c89e0c00f7d1a23ce0d43

Implementations:

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

Preview: https://pr3664--matrix-org-previews.netlify.app

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@deepbluev7 deepbluev7 changed the title Pushrules for relations MSC3664: Pushrules for relations Jan 21, 2022
@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal proposal-in-review push labels Jan 21, 2022
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@turt2live turt2live self-requested a review January 21, 2022 01:54
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
proposals/3664-notifications-for-relations.md Outdated Show resolved Hide resolved
proposals/3664-notifications-for-relations.md Show resolved Hide resolved
proposals/3664-notifications-for-relations.md Outdated Show resolved Hide resolved
"kind": "related_event_match"
"rel_type": "m.in_reply_to",
"key": "sender",
"pattern": "[the user's Matrix ID]"
Copy link
Member

Choose a reason for hiding this comment

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

for reference: .m.rule.invite_for_me currently implements such a pattern.

proposals/3664-notifications-for-relations.md Outdated Show resolved Hide resolved
proposals/3664-notifications-for-relations.md Show resolved Hide resolved
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Co-authored-by: Stuart Mumford <stuart@cadair.com>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
proposals/3664-notifications-for-relations.md Outdated Show resolved Hide resolved
proposals/3664-notifications-for-relations.md Show resolved Hide resolved
proposals/3664-notifications-for-relations.md Outdated Show resolved Hide resolved
proposals/3664-notifications-for-relations.md Outdated Show resolved Hide resolved
deepbluev7 and others added 2 commits January 24, 2022 13:34
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
deepbluev7 and others added 3 commits January 25, 2022 14:58
Co-authored-by: Matthew Hodgson <matthew@arasphere.net>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Comment on lines 140 to 149
If the related to event is not present on the homeserver, evaluating the push
rule may be delayed or fail completely. For most rules this should not be an
issue. You can assume the event was not sent by a user on your server if the
event is not present on your server. In general clients and servers should do
their best to evaluate the condition. If they fail to do so (possibly because
they can't look up the event asynchronously) in a timely manner, the condition
may be ignored/evaluated to false. This should affect only a subset of events,
because in general relations happen to events in close proximity. There is a
risk of missing notifications for replies to very old messages and similar
relations.
Copy link
Member

Choose a reason for hiding this comment

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

stuff like this makes me feel even more strongly that I want to see a client implementation before this can proceed. Is this tractable for a client? does skipping the condition if we don't have the target event give an acceptable UX?

@turt2live
Copy link
Member

before I dive into this, I'd like to see what Threads has in mind for notifications. Implicitly, this means an approving review from people working on that feature.

@mscbot concern Need to consider the threads team's opinions (whatever they are)

@turt2live
Copy link
Member

This does not appear to be ready for FCP: Implementation was declared insufficient, which alone is enough to back this out of its current FCP state.

@mscbot fcp cancel

@mscbot mscbot added proposal-in-review and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Jul 4, 2022
@deepbluev7
Copy link
Contributor Author

Well, the implementation is blocked on #3825 since that would break notifications for threads instead of solving them.

tulir added a commit to mautrix/go that referenced this pull request Sep 27, 2022
@turt2live
Copy link
Member

#3825 appears to be stuck and #2781 depends on this MSC - what are the next steps to make this go forward?

@deepbluev7
Copy link
Contributor Author

Well, you can kinda ignore #3825 when implementing this MSC. It just makes the implementation more bothersome, but my client SDK already transforms the events using special rules for threads. Since we can get the SCT to accept #3825, we can just move ahead without it and add a "Note" box in the spec to explain how to do it properly.

Comment on lines +66 to +78
```json5
{
"kind": "related_event_match",
"rel_type": "m.replace"
}
```

```json5
{
"kind": "related_event_match",
"rel_type": "m.thread"
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these identical to:

{
  "kind": "event_match",
  "key": "content.m\\.relates_to.\\.rel_type",
  "pattern": "m.replace"
}

(And a corresponding one for m.thread.)

I'm not sure this example really shows the need for this MSC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, in the case of replies you need to also match, if it isn't a fallback. Also, it interacts better with #3051 or any other reorganization of the relation format. Matching dotted keys is also #3873. Yes, you can solve this differently, but that solution also has downsides. This is just a convenient extension of this MSC, the actual meat is matching the content of the related event.

Comment on lines +140 to +149
If the related to event is not present on the homeserver, evaluating the push
rule may be delayed or fail completely. For most rules this should not be an
issue. You can assume the event was not sent by a user on your server if the
event is not present on your server. In general clients and servers should do
their best to evaluate the condition. If they fail to do so (possibly because
they can't look up the event asynchronously) in a timely manner, the rule
may be ignored/the condition evaluated to false. This should affect only a
subset of events, because in general relations happen to events in close
proximity. There is a risk of missing notifications for replies to very old
messages and similar relations.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really define the homeserver behavior here -- are they to attempt to backfill if they can't find the event? Just skip it?

Similarly for clients, should they attempt to ask the homeservers for the missing event if they don't have it locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a quality of implementation issue and I don't think that needs to be defined. For example in the case of matching replies you usually already have the events sent from your server and don't need to backfill them. In threads this is similar, usually you have the participating thread already. If you don't have an event, it is a tradeoff. Do you want accurate notifications or low latency? Alternatively you can always update the notification counter asynchronously. I don't see a need to require any specific behaviour here.

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
@bradtgmurray
Copy link

Just wanted to jump in and thank @deepbluev7 for continuing to push this forward. We're using this functionality to enable notifications when someone reacts to one of the user's messages but ignore reactions to other user's messages, and I'm excited to flip from our custom implementation to the one that's been included in Synapse 1.70.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 14, 2022
Synapse 1.71.0 (2022-11-08)
===========================

Please note that, as announced in the release notes for Synapse
1.69.0, legacy Prometheus metric names are now disabled by default.
They will be removed altogether in Synapse 1.73.0.  If not already
done, server administrators should update their dashboards and
alerting rules to avoid using the deprecated metric names.  See the
[upgrade
notes](https://matrix-org.github.io/synapse/v1.71/upgrade.html#upgrading-to-v1710)
for more details.

**Note:** in line with our [deprecation
  policy](https://matrix-org.github.io/synapse/latest/deprecation_policy.html)
  for platform dependencies, this will be the last release to support
  PostgreSQL 10, which reaches upstream end-of-life on November 10th,
  2022. Future releases of Synapse will require PostgreSQL 11+.

Features
--------

- Support back-channel logouts from OpenID Connect
  providers. ([\#11414](matrix-org/synapse#11414))

- Allow use of Postgres and SQLlite full-text search operators in
  search
  queries. ([\#11635](matrix-org/synapse#11635),
  [\#14310](matrix-org/synapse#14310),
  [\#14311](matrix-org/synapse#14311))

- Implement
  [MSC3664](matrix-org/matrix-spec-proposals#3664),
  Pushrules for relations. Contributed by
  Nico. ([\#11804](matrix-org/synapse#11804))

- Improve aesthetics of HTML templates. Note that these changes do not
  retroactively apply to templates which have been
  [customised](https://matrix-org.github.io/synapse/latest/templates.html#templates)
  by server
  admins. ([\#13652](matrix-org/synapse#13652))

- Enable write-ahead logging for SQLite installations. Contributed by
  [@asymmetric](https://github.com/asymmetric). ([\#13897](matrix-org/synapse#13897))

- Show erasure status when [listing
  users](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#query-user-account)
  in the Admin
  API. ([\#14205](matrix-org/synapse#14205))

- Provide a specific error code when a `/sync` request provides a
  filter which doesn't represent a JSON
  object. ([\#14262](matrix-org/synapse#14262))
bradtgmurray added a commit to beeper/synapse-legacy-fork that referenced this pull request Nov 15, 2022
Synapse 1.71.0 (2022-11-08)
===========================

Please note that, as announced in the release notes for Synapse 1.69.0, legacy Prometheus metric names are now disabled by default.
They will be removed altogether in Synapse 1.73.0.
If not already done, server administrators should update their dashboards and alerting rules to avoid using the deprecated metric names.
See the [upgrade notes](https://matrix-org.github.io/synapse/v1.71/upgrade.html#upgrading-to-v1710) for more details.

**Note:** in line with our [deprecation policy](https://matrix-org.github.io/synapse/latest/deprecation_policy.html) for platform dependencies, this will be the last release to support PostgreSQL 10, which reaches upstream end-of-life on November 10th, 2022. Future releases of Synapse will require PostgreSQL 11+.

No significant changes since 1.71.0rc2.

Synapse 1.71.0rc2 (2022-11-04)
==============================

Improved Documentation
----------------------

- Document the changes to monthly active user metrics due to deprecation of legacy Prometheus metric names. ([\matrix-org#14358](matrix-org#14358), [\matrix-org#14360](matrix-org#14360))

Deprecations and Removals
-------------------------

- Disable legacy Prometheus metric names by default. They can still be re-enabled for now, but they will be removed altogether in Synapse 1.73.0. ([\matrix-org#14353](matrix-org#14353))

Internal Changes
----------------

- Run unit tests against Python 3.11. ([\matrix-org#13812](matrix-org#13812))

Synapse 1.71.0rc1 (2022-11-01)
==============================

Features
--------

- Support back-channel logouts from OpenID Connect providers. ([\matrix-org#11414](matrix-org#11414))
- Allow use of Postgres and SQLlite full-text search operators in search queries. ([\matrix-org#11635](matrix-org#11635), [\matrix-org#14310](matrix-org#14310), [\matrix-org#14311](matrix-org#14311))
- Implement [MSC3664](matrix-org/matrix-spec-proposals#3664), Pushrules for relations. Contributed by Nico. ([\matrix-org#11804](matrix-org#11804))
- Improve aesthetics of HTML templates. Note that these changes do not retroactively apply to templates which have been [customised](https://matrix-org.github.io/synapse/latest/templates.html#templates) by server admins. ([\matrix-org#13652](matrix-org#13652))
- Enable write-ahead logging for SQLite installations. Contributed by [@asymmetric](https://github.com/asymmetric). ([\matrix-org#13897](matrix-org#13897))
- Show erasure status when [listing users](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#query-user-account) in the Admin API. ([\matrix-org#14205](matrix-org#14205))
- Provide a specific error code when a `/sync` request provides a filter which doesn't represent a JSON object. ([\matrix-org#14262](matrix-org#14262))

Bugfixes
--------

- Fix a long-standing bug where the `update_synapse_database` script could not be run with multiple databases. Contributed by @thefinn93 @ Beeper. ([\matrix-org#13422](matrix-org#13422))
- Fix a bug which prevented setting an avatar on homeservers which have an explicit port in their `server_name` and have `max_avatar_size` and/or `allowed_avatar_mimetypes` configuration. Contributed by @ashfame. ([\matrix-org#13927](matrix-org#13927))
- Check appservice user interest against the local users instead of all users in the room to align with [MSC3905](matrix-org/matrix-spec-proposals#3905). ([\matrix-org#13958](matrix-org#13958))
- Fix a long-standing bug where Synapse would accidentally include extra information in the response to [`PUT /_matrix/federation/v2/invite/{roomId}/{eventId}`](https://spec.matrix.org/v1.4/server-server-api/#put_matrixfederationv2inviteroomideventid). ([\matrix-org#14064](matrix-org#14064))
- Fix a bug introduced in Synapse 1.64.0 where presence updates could be missing from `/sync` responses. ([\matrix-org#14243](matrix-org#14243))
- Fix a bug introduced in Synapse 1.60.0 which caused an error to be logged when Synapse received a SIGHUP signal if debug logging was enabled. ([\matrix-org#14258](matrix-org#14258))
- Prevent history insertion ([MSC2716](matrix-org/matrix-spec-proposals#2716)) during an partial join ([MSC3706](matrix-org/matrix-spec-proposals#3706)). ([\matrix-org#14291](matrix-org#14291))
- Fix a bug introduced in Synapse 1.34.0 where device names would be returned via a federation user key query request when `allow_device_name_lookup_over_federation` was set to `false`. ([\matrix-org#14304](matrix-org#14304))
- Fix a bug introduced in Synapse 0.34.0 where logs could include error spam when background processes are measured as taking a negative amount of time. ([\matrix-org#14323](matrix-org#14323))
- Fix a bug introduced in Synapse 1.70.0 where clients were unable to PUT new [dehydrated devices](matrix-org/matrix-spec-proposals#2697). ([\matrix-org#14336](matrix-org#14336))

Improved Documentation
----------------------

- Explain how to disable the use of [`trusted_key_servers`](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#trusted_key_servers). ([\matrix-org#13999](matrix-org#13999))
- Add workers settings to [configuration manual](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#individual-worker-configuration). ([\matrix-org#14086](matrix-org#14086))
- Correct the name of the config option [`encryption_enabled_by_default_for_room_type`](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#encryption_enabled_by_default_for_room_type). ([\matrix-org#14110](matrix-org#14110))
- Update docstrings of `SynapseError` and `FederationError` to bettter describe what they are used for and the effects of using them are. ([\matrix-org#14191](matrix-org#14191))

Internal Changes
----------------

- Remove unused `@lru_cache` decorator. ([\matrix-org#13595](matrix-org#13595))
- Save login tokens in database and prevent login token reuse. ([\matrix-org#13844](matrix-org#13844))
- Refactor OIDC tests to better mimic an actual OIDC provider. ([\matrix-org#13910](matrix-org#13910))
- Fix type annotation causing import time error in the Complement forking launcher. ([\matrix-org#14084](matrix-org#14084))
- Refactor [MSC3030](matrix-org/matrix-spec-proposals#3030) `/timestamp_to_event` endpoint to loop over federation destinations with standard pattern and error handling. ([\matrix-org#14096](matrix-org#14096))
- Add initial power level event to batch of bulk persisted events when creating a new room. ([\matrix-org#14228](matrix-org#14228))
- Refactor `/key/` endpoints to use `RestServlet` classes. ([\matrix-org#14229](matrix-org#14229))
- Switch to using the `matrix-org/backend-meta` version of `triage-incoming` for new issues in CI. ([\matrix-org#14230](matrix-org#14230))
- Build wheels on macos 11, not 10.15. ([\matrix-org#14249](matrix-org#14249))
- Add debugging to help diagnose lost device list updates. ([\matrix-org#14268](matrix-org#14268))
- Add Rust cache to CI for `trial` runs. ([\matrix-org#14287](matrix-org#14287))
- Improve type hinting of `RawHeaders`. ([\matrix-org#14303](matrix-org#14303))
- Use Poetry 1.2.0 in the Twisted Trunk CI job. ([\matrix-org#14305](matrix-org#14305))

<details>
<summary>Dependency updates</summary>

Runtime:

- Bump anyhow from 1.0.65 to 1.0.66. ([\matrix-org#14278](matrix-org#14278))
- Bump jinja2 from 3.0.3 to 3.1.2. ([\matrix-org#14271](matrix-org#14271))
- Bump prometheus-client from 0.14.0 to 0.15.0. ([\matrix-org#14274](matrix-org#14274))
- Bump psycopg2 from 2.9.4 to 2.9.5. ([\matrix-org#14331](matrix-org#14331))
- Bump pysaml2 from 7.1.2 to 7.2.1. ([\matrix-org#14270](matrix-org#14270))
- Bump sentry-sdk from 1.5.11 to 1.10.1. ([\matrix-org#14330](matrix-org#14330))
- Bump serde from 1.0.145 to 1.0.147. ([\matrix-org#14277](matrix-org#14277))
- Bump serde_json from 1.0.86 to 1.0.87. ([\matrix-org#14279](matrix-org#14279))

Tooling and CI:

- Bump black from 22.3.0 to 22.10.0. ([\matrix-org#14328](matrix-org#14328))
- Bump flake8-bugbear from 21.3.2 to 22.9.23. ([\matrix-org#14042](matrix-org#14042))
- Bump peaceiris/actions-gh-pages from 3.8.0 to 3.9.0. ([\matrix-org#14276](matrix-org#14276))
- Bump peaceiris/actions-mdbook from 1.1.14 to 1.2.0. ([\matrix-org#14275](matrix-org#14275))
- Bump setuptools-rust from 1.5.1 to 1.5.2. ([\matrix-org#14273](matrix-org#14273))
- Bump twine from 3.8.0 to 4.0.1. ([\matrix-org#14332](matrix-org#14332))
- Bump types-opentracing from 2.4.7 to 2.4.10. ([\matrix-org#14133](matrix-org#14133))
- Bump types-requests from 2.28.11 to 2.28.11.2. ([\matrix-org#14272](matrix-org#14272))
</details>
@bradtgmurray
Copy link

bradtgmurray commented Nov 21, 2022

We're now running this in production, here's an example base push rule for our users where users get notifications for reactions, but only to their messages, and only in rooms that are "small" (less than 20 members).

https://github.com/beeper/synapse/blob/ad6b10b70df5befa0d39726bd3481a6fbbdc41ce/rust/src/push/base_rules.rs#L420-L446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal push unresolved-concerns This proposal has at least one outstanding concern
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants