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
Open
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d5f43bb
Pushrules for relations
deepbluev7 Jan 21, 2022
53a4607
Mention beeper implementation
deepbluev7 Jan 21, 2022
fa8fc97
Remove my misunderstanding of push rule naming in synapse.
deepbluev7 Jan 21, 2022
b90cdb6
Rule order, clarifications and thread discussion
deepbluev7 Jan 24, 2022
a6eebe7
fix typo
deepbluev7 Jan 24, 2022
0161daa
Update proposals/3664-notifications-for-relations.md
deepbluev7 Jan 24, 2022
242a65c
value -> pattern
deepbluev7 Jan 24, 2022
070babc
Define how a client can check for support
deepbluev7 Jan 24, 2022
06b5393
Clarify pattern and key more
deepbluev7 Jan 24, 2022
f2a5c57
Update proposals/3664-notifications-for-relations.md
deepbluev7 Jan 25, 2022
aebe3e9
Update proposals/3664-notifications-for-relations.md
deepbluev7 Jan 25, 2022
1ead1da
Remove partial solution for threads in favour of a different proposal
deepbluev7 Jan 26, 2022
6e1d1bb
Mention issues with unexposed notification settings
deepbluev7 Jan 26, 2022
3a288b6
Apply suggestions from code review
deepbluev7 Feb 1, 2022
df9aab5
Fix conflict with threading MSC
deepbluev7 May 24, 2022
00b0ef9
Apply suggestions from code review
deepbluev7 May 24, 2022
519b95d
Update proposals/3664-notifications-for-relations.md
deepbluev7 May 24, 2022
c9d55e2
Add capabilities flag
deepbluev7 Oct 27, 2022
22e67c4
Apply suggestions from code review
deepbluev7 Oct 27, 2022
7248826
Clarify rule ignored <-> condition evaluated to false
deepbluev7 Oct 27, 2022
69fe03e
Clarify how key and pattern work
deepbluev7 Oct 27, 2022
b1897fa
Update proposals/3664-notifications-for-relations.md
deepbluev7 Nov 1, 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
244 changes: 244 additions & 0 deletions proposals/3664-notifications-for-relations.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
# MSC3664: Notifications for relations

Relations are very powerful and are becoming a platform to build new features
like replies, edits, reactions, threads, polls and much more on.

On the other hand there is currently no way to control, what you are getting
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
notified for. Some people want to get notified, when someone replies to their
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
message. Some want to get notified for reactions to their message. Some people
explicitly do not want that. You might want to be able to mute a thread and you
may want to get notified for poll responeses or not. Some people like getting
notified for edits, others prefer to not get notified, when someone fixes typos
20 times in a row for a long message they sent a week ago.

We should extend push rules so that a server can provide sane defaults and users
can adjust them to their own wishes.

## Proposal

### New push rule condition: `related_event_match`

Notifications for relation based features need to distinguish, what type of
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
relation was used and potentially match on the content of the related-to event.

To do that we introduce a new type of condition: `related_event_match`. This is
largely similar to the existing `event_match`, but operates on the related-to
event. Such a condition could look like this:

```json5
{
"kind": "related_event_match",
"rel_type": "m.in_reply_to",
"key": "sender",
"pattern": "@me:my.server"
}
```

This condition can be used to notify me whenever someone sends a reply to my
messages.

- `rel_type` is the relation type. For the sake of compatibility replies
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
should be matched as if they were sent in the relation format from
[MSC2674](https://github.com/matrix-org/matrix-doc/pull/2674) with a
`rel_type` of `m.in_reply_to`. If the event has any relation of this type,
the related event should be matched using `pattern` and `key`.
Copy link
Member

Choose a reason for hiding this comment

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

This seems to implicitly usurp a relation type of m.in_reply_to, which might not be desired? I think there's been some thought these should be replaced eventually with MSC3267 (m.reference relations).

Regardless, it means that m.in_reply_to could not be used for a relation in the future and would need to be reserved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the intention is to treat m.in_reply_to as a relation for now. #3440 actually makes it impossible to convert replies to normal relations easily, because it abuses the different format for fallback semantics. So we will probably have to live with this for a while (maybe #3051 can solve that), but I think this interface makes the most sense for now, while we dig ourselves deeper into this hole. We could also just name the reply rel_type m.in_reply_to, which is what I prefer (because reference is too generic) and then this rule would (hopefully) need no changes. :D

Copy link
Member

Choose a reason for hiding this comment

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

If I've undersrood correctly, you could still use a relation type of m.in_reply_to, you'd just also get old-style replies mixed in to the results too? It would effectively reserve it for replies, but this probably ought to be the case anyway?

Copy link
Member

Choose a reason for hiding this comment

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

It would effectively reserve it for replies, but this probably ought to be the case anyway?

This is the bulk of my concern. (I think calling that out explicitly would probably be ideal?)

If I've undersrood correctly, you could still use a relation type of m.in_reply_to, you'd just also get old-style replies mixed in to the results too?

I don't think homeservers doesn't treat m.in_reply_to as a relation right now (at least Synapse doesn't), so this is quite a change in behavior in my opinion. (E.g. you can't query /relations from MSC2675 using this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imo at least for the context of this MSC, they should be treated the same. It is a bit of a shame, that /relations doesn't tbh and maybe we should either allow that in the future or make replies proper relations, but I didn't want to affect any other behaviour with this MSC. I'll clarify.

Copy link
Member

@richvdh richvdh May 24, 2022

Choose a reason for hiding this comment

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

Honestly this feels like putting a cart before a horse. It seems to assume that at some point we will replace replies with "rel_type": "m.in_reply_to" - which, afaik, hasn't even been proposed as an MSC, and as you note MSC3440 actually makes it harder to do so.

It's kinda fine if we do make that change in the near term, but if we don't, then this is going to be a very strange thing for the spec to say.

This is all a horrible mess and I don't really know what to do about it :(.

Copy link
Member

Choose a reason for hiding this comment

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

I still find it a bit hidden in this MSC that this grabs the m.in_reply_to as if it is a releation type when it isn't. I think this will be confusing for implementers (and difficult to specify properly).

deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
- `key` (optional): The dot-separated field of the event to match, e.g. `content.body`
or `sender`. If it is not present, the condition should match all events,
that have a relation of type `rel_type`.
- `pattern` (optional): The glob-style pattern to match against.

`key` and `pattern` have exactly the same meaning as in `event_match`
conditions. See https://github.com/matrix-org/matrix-doc/issues/2637 for a
clarification of their behaviour.
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

`key` and `pattern` are optional to allow you to enable or suppress all
richvdh marked this conversation as resolved.
Show resolved Hide resolved
notifications for a specific relation type. For example one could suppress
notifications for all events with a relation from
[threads](https://github.com/matrix-org/matrix-doc/pull/3440) and all
richvdh marked this conversation as resolved.
Show resolved Hide resolved
[edits](https://github.com/matrix-org/matrix-doc/pull/2676) with the following
two conditions:
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

```json5
{
"kind": "related_event_match",
"rel_type": "m.replace"
}
```

```json5
{
"kind": "related_event_match",
"rel_type": "m.thread"
}
```
Comment on lines +66 to +78
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.


Without a `key` and `pattern` the push rule can be evaluated without fetching
the related to event. If one of those two fields is missing, a server should
prevent those rules from being added with the appropriate error code. (A client
wouldn't have a choice but to ignore those keys if the server failed to prevent
the rule from being added.)
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

A client can check for the `related_event_match` condition being supported by
testing for an existing `.m.rule.reply` in the default rules.

### A push rule for replies

To enable notifications for replies without relying on the reply fallback, but
with similar semantics we also define a new default push rule. The proposed
push rule differs slightly from the old behaviour, because it only notifies you
for replies to your events, but it does not notify you for replies to events,
which contained your display name or matrix ID. The rule should look like this:
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

```json5
{
"rule_id": ".m.rule.reply",
"default": true,
"enabled": true,
"conditions": [
{
"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.

}
],
"actions": [
"notify",
{
"set_tweak": "sound",
"value": "default"
},
{
"set_tweak": "highlight"
}
]
}
```

This should be an override rule, since it can't be a content rule and should
not be overridden when setting a room to mentions only. It should be placed just
before `.m.rule.contains_display_name` in the list. This ensures you get
notified for replies to all events you sent. The actions are the same as for
`.m.rule.contains_display_name` and `.m.rule.contains_user_name`.
richvdh marked this conversation as resolved.
Show resolved Hide resolved

No other rules are proposed as no other relations are in the specification as of
writing this MSC to decrease dependencies.

## Potential issues
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved

Most push rules for relations will need a lookup into a second event. This
Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful to number each separate issue.

causes additional implementation complexity and can potentially be expensive.
Looking up one event shouldn't be that heavy, but it is overhead, that wasn't
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
there before and it needs to be evaluated for every event, so it clearly is
somewhat performance sensitive.

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,
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
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?



[Threads](https://github.com/matrix-org/matrix-doc/pull/3440) use replies
[as a fallback](https://github.com/matrix-org/matrix-doc/pull/3440/files#diff-113727ce0257b4dc0ad6f1087b6402f2cfcb6ff93272757b947bf1ce444056aeR82).
deepbluev7 marked this conversation as resolved.
Show resolved Hide resolved
This would cause a notification with the new `.m.rule.reply` rule. To prevent
that the threads MSC could add rules like this to suppress notifications for
threads without the `render_in` attribute:

```json5
{
"rule_id": ".m.rule.suppress_reply_notify_in_threads",
"default": true,
"enabled": true,
"conditions": [
{
"kind": "related_event_match",
"rel_type": "m.in_reply_to"
},
{
"kind": "related_event_match",
"rel_type": "m.thread"
}
],
"actions": [
"notify"
]
},
{
"rule_id": ".m.rule.reply_in_thread",
"default": true,
"enabled": true,
"conditions": [
{
"kind": "related_event_match",
"rel_type": "m.in_reply_to",
"key": "sender",
"pattern": "[the user's Matrix ID]"
},
{
"kind": "related_event_match",
"rel_type": "m.thread"
},
{
"kind": "event_match",
"key": "m.relates_to.m.in_reply_to.render_in",
"pattern": "m.thread"
}
],
"actions": [
"notify",
{
"set_tweak": "sound",
"value": "default"
},
{
"set_tweak": "highlight"
}
]
}
```

This would be significantly easier if there were ways to match for fields NOT
being present and if a pattern can match a field in an array is not clearly
defined in the specification at the moment: https://github.com/matrix-org/matrix-doc/issues/3082

That will need a solution, but there are multiple ways to solve this and it
probably does not need to happen on this MSC? The easiest solution would be to
be able to just invert a pattern. Then you could just suppress notifications for
events without `render_in` or the threading MSC could inverts its event format
to make it easier to match with pushrules (i.e. `dont_render_in` or
`fallback_relation`).

## Alternatives

- One could add an optional `rel_type` key to all existing conditions. This
would allow you to also easily match by `contains_display_name`,
`sender_notification_permission` and `room_member_count`. Out of those
conditions only `contains_display_name` seems to be useful in a related
event context. Having a potentially expensive key like `rel_type` available
for all conditions would also increase implementation complexity. As such
this MSC proposes the minimum amount of conditions to support push rules for
most relations, although allowing `rel_type` on `contains_display_name` and
`event_match` could be a good alternative.
- Beeper has a
[similar feature in their synapse](https://gitlab.com/beeper/synapse/-/commit/44a1728b6b021f97900c89e0c00f7d1a23ce0d43),
but it does not allow you to filter by relation type.



## Security considerations

- These pushrules could be used to increase load on the homeserver. Apart from
that there shouldn't be any potential security issues.

## Unstable prefix

While this proposal is still in progress, implementations should use the
unstable prefix `im.nheko.msc3664` for the `related_event_match` condition. As
a result it should be called `im.nheko.msc3664.related_event_match`.