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

Clarify what fields are required when deleting a pusher #1321

Merged
merged 3 commits into from
Nov 22, 2022

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Nov 3, 2022

The required fields match those used by Synapse, Dendrite and Conduit to delete existing pushers.

Related: #233

Signed-off-by: Kévin Commaille zecakeh@tedomum.fr

Preview: https://pr1321--matrix-spec-previews.netlify.app

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@zecakeh zecakeh requested a review from a team as a code owner November 3, 2022 12:47
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Copy link
Contributor

@MTRNord MTRNord left a comment

Choose a reason for hiding this comment

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

This is technically not concerning this msc but the topic: is there a reason why this weird way of unsetting/explicit null is user over a REST DELETE endpoint?

(sorry the android mobile app doesn't allow me to do threads apparently. I hoped this would cause a thread)

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

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.

@dbkr dbkr requested a review from a team November 8, 2022 21:14
data/api/client-server/pusher.yaml Show resolved Hide resolved
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.

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.

@richvdh
Copy link
Member

richvdh commented Nov 15, 2022

This is technically not concerning this msc

It's not an MSC.

but the topic: is there a reason why this weird way of unsetting/explicit null is user over a REST DELETE endpoint?

No. That is the topic of #233.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks!

@richvdh richvdh merged commit 8d4a3bb into matrix-org:main Nov 22, 2022
@zecakeh zecakeh deleted the delete-pusher branch November 28, 2022 11:39
clokep pushed a commit to clokep/matrix-spec that referenced this pull request May 3, 2023
)

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
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.

None yet

4 participants