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

Send 204 when WebAPI response contains no data #21349

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

Conversation

Piccirello
Copy link
Member

This change is part of a broader push to have the WebAPI send back semantically correct status codes.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/204

@Piccirello Piccirello added the WebAPI WebAPI-related issues/changes label Sep 16, 2024
@Piccirello Piccirello requested a review from a team September 16, 2024 21:05
@glassez glassez added this to the 5.1 milestone Sep 17, 2024
@glassez glassez requested a review from a team September 17, 2024 05:47
Copy link
Member

@Chocobo1 Chocobo1 left a comment

Choose a reason for hiding this comment

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

I'm still unsure it is a good idea. I'll leave it for others to decide.
The coding style LGTM.

@glassez
Copy link
Member

glassez commented Sep 28, 2024

To be honest, I also doubt its usefulness. If we know for sure that some API method isn't supported to return any data, we can safely make do with the usual 200 status.

@Piccirello
Copy link
Member Author

If an API doesn't return any data, it SHOULD return a 204. This is not a matter of opinion - this is how it's supposed to work. Our API should be more closely following web standards.

@glassez
Copy link
Member

glassez commented Sep 28, 2024

Our API should be more closely following web standards.

Many parts of these web standards look confusing and do not contain clear (unambiguous) definitions. That's why I've had a bit of an aversion to this area for a long time.
Putting aside my subjective opinion, the following remark still remains.
Current clients may be explicitly dependent on a specific status code, so this change may be regarded as "highly breaking". Wouldn't it be better to leave everything as it is, at least until someone decides to do a deep redesign of the entire WebAPI? Otherwise, I could foresee a lot of extremely negative feedbacks.

@Piccirello
Copy link
Member Author

Wouldn't it be better to leave everything as it is, at least until someone decides to do a deep redesign of the entire WebAPI? Otherwise, I could foresee a lot of extremely negative feedbacks.

I think we handle this as a standard breaking API change. We just bump the API minor version.

@zze0s

This comment was marked as off-topic.

@glassez

This comment was marked as off-topic.

@KyleSanderson
Copy link

Wouldn't it be better to leave everything as it is, at least until someone decides to do a deep redesign of the entire WebAPI? Otherwise, I could foresee a lot of extremely negative feedbacks.

I think we handle this as a standard breaking API change. We just bump the API minor version.

So, this actually just happened with qb5.0. Almost every tool has some level of brokenness right now because the v2 wasn't bumped to v3 (and further) where the pause endpoints were renamed(?) along with others. It doesn't have to be ad infinium, but from 4.3~ to 4.6.7 there was a relatively stable API where features were added, not removed or renamed.

I don't know of a better way to handle this, besides keeping the old API endpoints around and formally deprecating them over-time, but this level of change was not expected to be preventing the uptake of 5.0 in the greater community.

The reason why I'm bringing this up here, is this also has the potential to break a number of tools, again.

@Piccirello
Copy link
Member Author

So, this actually just happened with qb5.0. Almost every tool has some level of brokenness right now because the v2 wasn't bumped to v3 (and further) where the pause endpoints were renamed(?) along with others. It doesn't have to be ad infinium, but from 4.3~ to 4.6.7 there was a relatively stable API where features were added, not removed or renamed.

I don't know of a better way to handle this, besides keeping the old API endpoints around and formally deprecating them over-time, but this level of change was not expected to be preventing the uptake of 5.0 in the greater community.

The reason why I'm bringing this up here, is this also has the potential to break a number of tools, again.

Thanks for raising this point. I think you're right that we've just broken a bunch of tools and don't want to do that again. I also do want to move forward with new APIs using 204 by default. I think a good compromise would be to ensure that all existing endpoints continue to respond with a 200. We can then update these individually at some later point.

@KyleSanderson
Copy link

Thanks for raising this point. I think you're right that we've just broken a bunch of tools and don't want to do that again. I also do want to move forward with new APIs using 204 by default. I think a good compromise would be to ensure that all existing endpoints continue to respond with a 200. We can then update these individually at some later point.

As an end-user - I appreciate that.

I don't think you have to be this polarized though, honestly for 5.0.1 I would add those old endpoints back (and revert whatever other changes there were), and move the new api to /v2.1/ or something and plan removal for qB 6.0, with deprecation immediately. That way you can phase these in, and once you hit critical mass in the new world, remove the old one once it's truly not able to continue forward.

There's some library in python, rust, golang, and a couple other bindings. They don't all receive the same amount of love, or back compatibility as one would expect. It would be very cool if qB maintained these bindings, but that's outside the scope of this change now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebAPI WebAPI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants