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

Optional body parameters #3237

Closed
lukaslihotzki opened this issue Jun 10, 2021 · 9 comments · Fixed by #3238
Closed

Optional body parameters #3237

lukaslihotzki opened this issue Jun 10, 2021 · 9 comments · Fixed by #3238
Labels
aesthetic A suggestion or issue relating to the representation of the spec

Comments

@lukaslihotzki
Copy link
Contributor

There are 5 operations in data/api/client-server that don't require their body parameter (missing required: true):

Is this intentional? If yes, how can requests be made without body (for example, send null as application/json body)? And what is the effect of omitting the body? Except for searchUserDirectory, there aren't required parameters, so an empty body can be sent instead without obvious differences. About searchUserDirectory: Is search without search term supported by omitting the body completely? If so, why is the search term a required parameter of the body? postReceipt adds an extra field in the body, which would not work if the body is null.

I think this isn't intentional. Would you accept a PR that sets required: true in these bodies?

@lukaslihotzki lukaslihotzki added the aesthetic A suggestion or issue relating to the representation of the spec label Jun 10, 2021
@richvdh
Copy link
Member

richvdh commented Jun 11, 2021

yeah I don't think it is intentional either.

@richvdh
Copy link
Member

richvdh commented Aug 4, 2021

It seems like element-android does not provide a body for postReceipt, so this is actually a breaking change. I don't know if that should really change anything here.

@ananace
Copy link
Contributor

ananace commented Aug 5, 2021

Other REST - or REST-like - APIs very often allow for completely omitting empty bodies, so this could easily become confusing for developers.

@reivilibre
Copy link
Contributor

Other REST - or REST-like - APIs very often allow for completely omitting empty bodies, so this could easily become confusing for developers.

On the other hand, this is something that's easily noticed and fixed whilst developing clients (if it gets rejected), but it's more annoying leaving the lack of clarity open to the point that different homeserver implementations might act differently.

@richvdh
Copy link
Member

richvdh commented Aug 5, 2021

Other REST - or REST-like - APIs very often allow for completely omitting empty bodies, so this could easily become confusing for developers.

examples would be useful, otherwise it's hard to evaluate the truth of this assertion.

Presumably you'd advocate opening up other endpoints to allow omitting bodies too, for consistency?

@richvdh
Copy link
Member

richvdh commented Aug 5, 2021

it's more annoying leaving the lack of clarity open to the point that different homeserver implementations might act differently.

yes, it's clear that the spec should be making a stand in one direction or the other.

@ananace
Copy link
Contributor

ananace commented Aug 5, 2021

examples would be useful, otherwise it's hard to evaluate the truth of this assertion.

I'll have a look through some other APIs too, unfortunately the three I have in mind right now aren't public so I don't know if I'm allowed to share them as examples.

yes, it's clear that the spec should be making a stand in one direction or the other.

This is basically the core of my concern, doesn't actually matter overly much if it's allowed or not, as long it is clear to the developers what is to be expected - and that the actual implementation doesn't differ between servers.

Presumably you'd advocate opening up other endpoints to allow omitting bodies too, for consistency?

And yeah, if the endpoints don't require arguments then - if empty bodies should be allowed - they should also allow omitting the empty object entirely.

@reivilibre
Copy link
Contributor

reivilibre commented Aug 5, 2021

I'd instead say that it's better to have the body required in all instances — it's my opinion that there should be only one way to do things.

If there's only one way to do things, clients will very quickly switch to doing it in only one way. If clients only do it in one way, then servers will definitely support doing it that way (otherwise they notice quickly that clients don't work!!).

If there are two ways to do things, then we have to test that you can do it both ways, and that all homeservers can do it both ways (even though common clients may actually only do it in one of those two ways).
This leaves the opportunity for a new homeserver developer to come along and only implement one way (the common case), perhaps not noticing there was another way clients could use (but perhaps only rare/exotic clients do).

You could say that we could make the spec mandate that empty bodies are ALWAYS omitted and then that would satisfy my opinion above. That's true, but it probably means having extra special-case code in clients and servers when, actually, emitting empty bodies was probably easier to do as it didn't need a special code path.

(naturally, this is just my opinion, nothing more or less :)

@richvdh
Copy link
Member

richvdh commented Aug 6, 2021

Another datapoint here: synapse currently requires a body for all of the endpoints mentioned above except POST /rooms/{roomId}/receipt/{receiptType}/{eventId} - and that is covered by matrix-org/synapse#10534.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aesthetic A suggestion or issue relating to the representation of the spec
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants