-
Notifications
You must be signed in to change notification settings - Fork 38
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
Improve marshal_schema / response header handling #26
Conversation
twosigmajab
commented
Oct 29, 2018
- No longer ignore headers in tuple responses
- Allow passing headers to request_utils.response
- No longer jsonify responses that have a None schema
74d08d1
to
1edc7ff
Compare
try: | ||
schema = marshal_schema[status_code] | ||
except KeyError: | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I didn't see a point in catching a KeyError only to throw it again (without any additional logging or handling) other than to call it out, so I replaced the try/except with a comment below.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct - this is better. I bet there used to be behavior in the except block, but now its gone.
schema = marshal_schema[status_code] # May raise KeyError. | ||
# The schema may be declared as None to bypass marshaling (e.g. for 204 responses). | ||
if schema is None: | ||
return make_response((data or '', status_code, headers)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we don't want to use request_helpers.response
since it jsonifies the data, which results in a body of "null"
if the user passes None and '""'
if the user passes ''
rather than an empty body.
1edc7ff
to
91b1cc3
Compare
- No longer ignore headers in tuple responses - Allow passing headers to request_utils.response - No longer jsonify responses that have a None schema
91b1cc3
to
6219676
Compare
@@ -83,36 +81,30 @@ def wrapped(*args, **kwargs): | |||
|
|||
rv = f(*args, **kwargs) | |||
|
|||
if marshal_schema: | |||
if isinstance(rv, tuple): | |||
data, status_code = rv[0], rv[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing behavior ignores any headers the user may have passed in the third element of the tuple, and also doesn't support passing a 2-tuple like (body, headers)
, or a string status (see "tuple" under http://flask.pocoo.org/docs/1.0/api/#flask.Flask.make_response)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
I think a few new tests would be awesome here:
- Verify that
response
adds headers properly, similar to this: https://github.com/plangrid/flask-rebar/blob/master/tests/test_request_utils.py#L36 - Verify all the various new view function response combinations. I.e.
data
,data, status
,data, headers
,data, status, headers
. I think this is the right module for that: https://github.com/plangrid/flask-rebar/blob/master/tests/test_rebar.py - Seems like we're missing some tests around returning
None
. It'd be great if we can add those too.
test_rebar.py
has become a little difficult to navigate... let me know if its too much trouble to add to those tests. If so, we can get this PR through first, and I can clean up the tests.
Again - good work.
Meant to say, this PR was prompted by what I expect to be a fairly typical use case: wanting to return a 201 Created response with an empty body and a Location header containing the URL of the newly-created resource. Passing |
Thanks for the quick review, @barakalon! I'm actually about to knock off for the day and am not sure when I'll have time to get back to adding the tests -- I'd love to pass it back to you at this point if that works for you. Thanks again! |
schema=schema | ||
) | ||
data, status_code, headers = rv, 200, None | ||
if isinstance(rv, tuple): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just found Flask's own code for handling tuple responses:
https://github.com/pallets/flask/blob/7e714bd28b6e96d82b2848b48cf8ff48b517b09b/flask/app.py#L1932
Would probably be better to match that more closely here, or maybe better yet just call it and then modify the result as needed.
No problems - I've picked up the baton here: #28 |