Send 204 when WebAPI response contains no data #21349
Conversation
Piccirello
force-pushed
the
204
branch
2 times, most recently
from
78b6f7b to
6a90fab
Compare
|
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. |
|
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. |
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. |
I think we handle this as a standard breaking API change. We just bump the API minor version. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
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. |
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. |
Signed-off-by: Thomas Piccirello <thomas@piccirello.com>
|
I've updated this PR so that existing APIs that don't send back any content will continue to serve a 200. |
src/webui/api/searchcontroller.cpp
Outdated
| @@ -300,6 +314,8 @@ void SearchController::checkForUpdatesFinished(const QHash<QString, PluginVersio | |||
| LogMsg(tr("Updating plugin %1").arg(pluginName), Log::INFO); | |||
| pluginManager->updatePlugin(pluginName); | |||
| } | |||
|
|
|||
| setResult(u""_s); | |||
There was a problem hiding this comment.
This addition results in two changes to maintain backwards compatibility:
- API responds with 200 status code
- Response contains
Content-Type: text/plain; charset=UTF-8header, as before
Without this line, the API would respond with a 204 and omit the Content-Type header (due to a lack of content).
This is to avoid introducing breaking changes. Without this change, these APIs would start returning a 204 due to their lack of response content.
|
@qbittorrent/lead-developers can someone review this? |
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