Fixes content negotiation to respect q-values #245
Conversation
Uses werkzeug's Accept.best_match to decide on the best representation based on client's Accept header, including qvalues. Prior to this change, the flask-restful implementation ignored qvalues and used the order of the list of mime types in the Accept header to decide which representation to use. If default_mediatype is set in the API (which it is by default), it will be used in case none of the representations are acceptable to the client. If default_mediatype is set to None, a NotAcceptable exception will be raised, resulting in an HTTP 406 status code.
|
Flask has a snippet on this, in case that is helpful: http://flask.pocoo.org/snippets/45/ |
Changes content negotiation to use qvalues
Wraps long lines in make_response
Merge pull request #2 from wtsi-hgi/master
When default_mimetype is set to None, it is now overridden by the first representation that we support, such that the NotAcceptable (406) error content itself can be returned in that format. This fixes a bug wherein that only occurred on the first request, because it was setting default_mimetype on the whole API rather than just overriding it for one request.
Fixes handling of NotAcceptable exceptions
| data, | ||
| code, | ||
| override_default_mediatype = self.representations.keys()[0] | ||
| ) |
There was a problem hiding this comment.
Some style: this closing parenthesis should match the indentation of resp
|
I left a few comments. Overall the content of this pull looks good. I wish Thanks so much working on this, @jrandall! |
|
Lastly, this pull should have some tests to assert we're doing correct Accept header parsing on the corner cases. |
|
I'll see if I can put together some tests for this. |
|
@jrandall actually, in the interest of time (trying to get 1.0 out soon), I'll go ahead and take responsibility for adding tests for this so don't worry about it! |
|
Anything else I can help with to help get this merged? |
| ) | ||
| if mediatype is None: | ||
| raise NotAcceptable() | ||
| if mediatype in self.representations: |
There was a problem hiding this comment.
Would this be better as:
if mediatype in self.representations:
# Do the things...
else:
raise NotAcceptable()What would happen if mediatype was not None, and also not contained in self.representations? Should that be a NotAcceptable exception as well? Otherwise, make_response would return None.
There was a problem hiding this comment.
With the current PR code, I think the only way to trigger the situation you describe would be by having self.default_mediatype set to something for which there is no representation in self.representations - the best_match function will otherwise ensure that only media types that have representations are included. The only reason for keeping the original guard code if mediatype in self.representations: is in case default_mediatype is explicitly set to a media type for which it has no representation.
I believe what would happen in that case is that instead of raising a NotAcceptable exception triggering the 406 http status code, make_response would return None instead of a response object as you suggest - the end result of which would be a generic InternalServerError (500).
Given that the underlying issue in that case is one of an internal misconfiguration (the Flask-restful API defines a default mediatype for which it has no representation), I think the 500 status is probably more appropriate than the 406 status, but I don't feel strongly about that point, so if you'd like me to change it, I can do that. Another option might be to raise an InternalServerError explicitly and include the more specific problem in the description text. Along those lines, I also considered including the list of valid representations in the NotAcceptable description text - do you think that would be useful?
There was a problem hiding this comment.
I think you are correct, returning None from make_response would cause a 500 error (I think the last test case you wrote shows that since raising a non HTTPException would cause a 500 error). I think I agree with you that 500 is more appropriate in that case.
I don't think adding the valid representations to the response would be particularly useful at this point since handle_errors swallows custom exception messages right now. You could use abort, but theres issues with that too (see #205).
|
@jrandall sorry for the delay, I have one question (see diff). Tests would be appreciated as well. |
Note that one test is an expected fail until werkzeug releases a version with the q=0 fix (pallets/werkzeug@c9bb97c) I think this is not expected until werkzeug >= 1.0
|
Your tests look amazing, but I have some failures running them locally: I don't know why Travis isn't building this branch. Can you merge from diff --git a/flask_restful/__init__.py b/flask_restful/__init__.py
index 5517239..e408262 100644
--- a/flask_restful/__init__.py
+++ b/flask_restful/__init__.py
@@ -325,10 +325,11 @@ class Api(object):
resp = self.make_response(
data,
code,
+ headers,
fallback_mediatype = supported_mediatypes[0] if supported_mediatypes else None
)
else:
- resp = self.make_response(data, code)
+ resp = self.make_response(data, code, headers)
if code == 401:
resp = self.unauthorized(resp) |
… into feature/accept-best-match Conflicts: flask_restful/__init__.py
Adds a hardcoded text/plain handler to make_response for the purpose of outputting errors when there are no representations
Previous versions were raising exceptions on errors, but now it appears that http exceptions are able to be tested normally in the test client.
|
Tests are passing on 2.6 and 2.7 but not on 3.3 or 3.4 - I've never done python3 before, but I can take a look... |
1 similar comment
|
Not sure how you want to handle the test that depends on unreleased bugfix in werkzeug? I currently have it marked as an expected failure such that the test should start failing once it works properly in werkzeug (and the expected failure can then be removed). The werkzeug bugfix (pallets/werkzeug@c9bb97c) was merged in September, but there hasn't been a release since then... I guess they are preparing a 1.0 release? |
|
I think the |
Fixes content negotiation to respect q-values
|
Thanks! 🎆 |
Merge pull request flask-restful#245 from wtsi-hgi/feature/accept-best-match
Uses werkzeug's
Accept.best_matchto decide on the best representationbased on client's Accept header, including q-values.
Prior to this change, the flask-restful implementation ignored q-values
and used the order of the list of mime types in the Accept header to
decide which representation to use.
If
default_mediatypeis set in the API (which it is by default), itwill be used in case none of the representations are acceptable to the
client. If
default_mediatypeis set toNone, aNotAcceptableexceptionwill be raised, resulting in an HTTP 406 status code.
This also updates the
mediatypes()method to return the list ofmediatypes sorted in descending quality order, although possibly
it could be removed instead, as an ordered list without quality values
is not enough to properly implement matching.