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

MSC2782: Pushers with the full event content #2782

Open
wants to merge 2 commits into
base: old_master
Choose a base branch
from

Conversation

Sorunome
Copy link
Contributor

Rendered

Signed-off-by: Sorunome mail@sorunome.de

@Sorunome Sorunome changed the title MSC2782TP Pushers with the full event content MSC2782: Pushers with the full event content Sep 19, 2020
@uhoreg uhoreg added proposal A matrix spec change proposal proposal-in-review labels Sep 19, 2020
@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Sep 19, 2020
@@ -0,0 +1,58 @@
# MSC2782: HTTP Pushers with the full event content
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this already covered by the spec?

It says:

When pushing notifications for events, the homeserver is expected to include all of the event-related fields in the /notify request. When the homeserver is performing a push where the format is "event_id_only", only the event_id, room_id, counts, and devices are required to be populated.

(https://matrix.org/docs/spec/push_gateway/r0.1.1#homeserver-behaviour)

So it sounds like by just omitting data.format (which isn't a required field) when creating the pusher, push notifications are expected to include all of the keys listed in this proposal - and after a quick look it looks like Synapse implements that correctly as well.

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, synapse behaviour on this is even documented in the introduction section.

It doesn't seem clear to soru that the spec implies this behaviour, which has lead to the creation of this MSC.

Perhaps it should be re-worded then to introduce full_event format and make the format required?

Copy link
Contributor

@babolivier babolivier Mar 17, 2021

Choose a reason for hiding this comment

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

This part:

When pushing notifications for events, the homeserver is expected to include all of the event-related fields in the /notify request

sonds pretty clear to me in that, unless told otherwise, the HS is expected to send everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it should be re-worded then to introduce full_event format and make the format required?

meow?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel like this is necessary but happy for this question to gather more opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I'm thinking about it again now, and I've changed my mind and think this is a good idea. The spec as it is is a bit confusing on that point. However, I think the proposal should also make the format mandatory when registering an http pusher, to eliminate any confusion around what happens if you register a pusher with no format.

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants