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

PushNotification: Make 'devices' field optional #1949

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vanyasem
Copy link

Fixes #1947

@vanyasem vanyasem requested a review from a team as a code owner October 28, 2024 20:32
@CLAassistant
Copy link

CLAassistant commented Oct 28, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@krille-chan krille-chan left a comment

Choose a reason for hiding this comment

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

According to https://spec.matrix.org/v1.12/push-gateway-api/#post_matrixpushv1notify this is wrong. The devices field is required

@vanyasem
Copy link
Author

vanyasem commented Nov 11, 2024

@krille-chan the documentation that you linked specifies the JSON model for the API endpoint /_matrix/push/v1/notify, for which the devices field is indeed mandatory

You misunderstood the meaning of the model linked above. It's a server-side model for the Push Gateway itself. It's not meant to be sent back to users devices

sygnal is a reference Push Gateway by the Matrix Foundation

The definition of "reference" is:

A software example of a specification, intended to help others implement their own version of the specification or find problems during the creation of a specification.

You can learn more about the meaning of a "reference implementation" here: https://en.wikipedia.org/wiki/Reference_implementation

Arguing that an official reference implementation is somehow wrong, and your specific interpretation of the documentation is right is really bold.

Just to be sure, the field in question has been discussed directly with the Matrix team, and they confirmed that devices is only mandatory on the server-side, and that it makes no sense to include it in the model when sending a notification back to the device. See matrix-org/sygnal#319 for more details

Your implementation is the only one that does that, yet you still choose to stand by your incorrect interpretation of the docs.

@vanyasem
Copy link
Author

Btw the correct documentation to link would be https://github.com/matrix-org/sygnal/blob/main/docs/applications.md#firebase-cloud-messaging, which specifies the client-side model for the push:

{
  "event_id": "$3957tyerfgewrf384",
  "type": "m.room.message",
  "sender": "@exampleuser:example.org",
  "room_name": "Mission Control",
  "room_alias": "#exampleroom:example.org",
  "sender_display_name": "Major Tom",
  "content_msgtype": "m.text",
  "content_body": "I'm floating in a most peculiar way.",
  "room_id": "!slw48wfj34rtnrf:example.org",
  "prio": "high",
  "unread": "2",
  "missed_calls": "1"
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PushNotification's devices field is not compatible with Sygnal
3 participants