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.

Sign up for GitHub

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

Jump to bottom

Clarify what fields are required when deleting a pusher #1321

Merged
merged 3 commits into from
Nov 22, 2022
Merged

Clarify what fields are required when deleting a pusher #1321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ee7f0f
Clarify what fields are required when deleting a pusher
zecakeh Nov 3, 2022
3da9a0b
Add newsfragment
zecakeh Nov 3, 2022
ebbb256
Set kind as nullable in definition
zecakeh Nov 17, 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
1 change: 1 addition & 0 deletions changelogs/client_server/newsfragments/1321.clarification
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Clarify what fields are required when deleting a pusher
31 changes: 19 additions & 12 deletions data/api/client-server/pusher.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@ paths:
This endpoint allows the creation, modification and deletion of [pushers](/client-server-api/#push-notifications)
for this user ID. The behaviour of this endpoint varies depending on the
values in the JSON body.

If `kind` is not `null`, the pusher with this `app_id` and `pushkey`
richvdh marked this conversation as resolved.
Show resolved Hide resolved
for this user is updated, or it is created if it doesn't exist. If
`kind` is `null`, the pusher with this `app_id` and `pushkey` for this
user is deleted.
operationId: postPusher
security:
- accessToken: []
Expand Down Expand Up @@ -177,7 +182,9 @@ paths:
If the `kind` is `"email"`, this is the email address to
send notifications to.
kind:
type: string
type:
- "string"
- "null"
description: |-
The kind of pusher to configure. `"http"` makes a pusher that
sends HTTP pokes. `"email"` makes a pusher that emails the
Expand All @@ -194,13 +201,13 @@ paths:
app_display_name:
type: string
description: |-
A string that will allow the user to identify what application
owns this pusher.
Required if `kind` is not `null`. A string that will allow the
user to identify what application owns this pusher.
device_display_name:
type: string
description: |-
A string that will allow the user to identify what device owns
this pusher.
Required if `kind` is not `null`. A string that will allow the
user to identify what device owns this pusher.
profile_tag:
type: string
description: |-
Expand All @@ -209,14 +216,15 @@ paths:
lang:
type: string
description: |-
The preferred language for receiving notifications (e.g. 'en'
or 'en-US').
Required if `kind` is not `null`. The preferred language for
receiving notifications (e.g. 'en' or 'en-US').
data:
type: object
description: |-
A dictionary of information for the pusher implementation
itself. If `kind` is `http`, this should contain `url`
which is the URL to use to send notifications to.
Required if `kind` is not `null`. A dictionary of information
for the pusher implementation itself. If `kind` is `http`,
this should contain `url` which is the URL to use to send
notifications to.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is necessarily required in principle, although in practice all (both) types of pusher need some information here.

Copy link
Contributor Author

@zecakeh zecakeh Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Synapse requires the field to be present: https://github.com/matrix-org/synapse/blob/develop/synapse/rest/client/pusher.py#L95-L106

Is there really data required for an email pusher? The spec doesn't say so.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, no, there's no reason any data would be required for email. I'm not super keen on adding synapse's behaviour to the spec here since that could cause other HS impls to also start requiring it which will make the situation worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that mean that it should also not be required on the GET /_matrix/client/v3/pushers endpoint?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm - probably: I'd expect most to include it anyway to guard against lazy clients, but that doesn't mean the spec should force them to. What do others think?

Copy link
Member

Choose a reason for hiding this comment

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

Mostly, I think that making data optional here is even more of a divergence from the established spec, and harder to justify without an MSC. We can make an exception if kind is null, because (a) it's clearly nonsense to require a data in that case and (b) that's what synapse has done forever, but going beyond that seems a stretch.

If we want to relax the requirements for data when kind is not null, let's make it a separate change.

title: PusherData
properties:
url:
Expand All @@ -243,8 +251,7 @@ paths:
different user IDs. Otherwise, the homeserver must remove any
other pushers with the same App ID and pushkey for different
users. The default is `false`.
required: ['kind', 'app_id', 'app_display_name',
'device_display_name', 'pushkey', 'lang', 'data']
required: ['kind', 'app_id', 'pushkey']
responses:
200:
description: The pusher was set.
Expand Down