-
Notifications
You must be signed in to change notification settings - Fork 37
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
Fix bug that prevents returning Flask.Response
s.
#153
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,7 +71,7 @@ def _unpack_view_func_return_value(rv): | |
|
||
This imitates Flask's own `Flask.make_response` method. | ||
|
||
:param Optional[tuple] rv: (body, status, headers), (body, status), or (body, headers) | ||
:param rv: (body, status, headers), (body, status), (body, headers), or body | ||
:return: (body, status, headers) | ||
:rtype: tuple | ||
""" | ||
|
@@ -146,11 +146,19 @@ def wrapped(*args, **kwargs): | |
if not response_body_schema: | ||
return rv | ||
|
||
data, status_code, headers = _unpack_view_func_return_value(rv) | ||
if isinstance(rv, current_app.response_class): | ||
schema = response_body_schema[rv.status_code] | ||
# The schema may be set to None to bypass marshaling (e.g. for 204 responses). | ||
if schema is None: | ||
return rv | ||
# Otherwise, ensure the response body conforms to the promised schema. | ||
schema.loads(rv.data) # May raise ValidationError. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before calling |
||
return rv | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit concerned that the branching return here is an indicator that this code is in need of some refactoring. There schema validation assumes a dump were this validation requires a load, but I think otherwise we should be able to share one path. As it stands I'm worried about accidental drift. See normalize note. |
||
|
||
data, status_code, headers = _unpack_view_func_return_value(rv) | ||
schema = response_body_schema[status_code] # May raise KeyError. | ||
|
||
# The schema may be declared as None to bypass marshaling (e.g. for 204 responses). | ||
# The schema may be set to None to bypass marshaling (e.g. for 204 responses). | ||
if schema is None: | ||
return response( | ||
data=data, status_code=status_code, headers=headers, mimetype=mimetype | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
import unittest | ||
|
||
import marshmallow as m | ||
from flask import Flask | ||
from flask import Flask, make_response | ||
|
||
from flask_rebar import messages | ||
from flask_rebar import HeaderApiKeyAuthenticator, SwaggerV3Generator | ||
|
@@ -248,6 +248,46 @@ def update_foo(foo_uid): | |
) | ||
self.assertEqual(resp.status_code, 400) | ||
|
||
def test_flask_response_instance_interop_body_matches_schema(self): | ||
rebar = Rebar() | ||
registry = rebar.create_handler_registry() | ||
schema = FooUpdateSchema() | ||
|
||
@registry.handles(rule="/foo", method="PUT", response_body_schema=schema) | ||
def foo(): | ||
return make_response((json.dumps({"name": "foo"}), {"foo": "bar"})) | ||
|
||
app = create_rebar_app(rebar) | ||
resp = app.test_client().put(path="/foo") | ||
self.assertEqual(resp.status_code, 200) | ||
self.assertEqual(resp.headers["foo"], "bar") | ||
|
||
def test_flask_response_instance_interop_body_does_not_match_schema(self): | ||
rebar = Rebar() | ||
registry = rebar.create_handler_registry() | ||
schema = FooUpdateSchema() | ||
|
||
@registry.handles(rule="/foo", method="PUT", response_body_schema=schema) | ||
def foo(): | ||
return make_response(json.dumps({"does not match": "foo schema"})) | ||
|
||
app = create_rebar_app(rebar) | ||
resp = app.test_client().put(path="/foo") | ||
self.assertEqual(resp.status_code, 500) | ||
|
||
def test_flask_response_instance_interop_no_schema(self): | ||
rebar = Rebar() | ||
registry = rebar.create_handler_registry() | ||
|
||
@registry.handles(rule="/foo", response_body_schema={302: None}) | ||
def foo(): | ||
return make_response(("Redirecting", 302, {"Location": "http://foo.com"})) | ||
|
||
app = create_rebar_app(rebar) | ||
resp = app.test_client().get(path="/foo") | ||
self.assertEqual(resp.status_code, 302) | ||
self.assertEqual(resp.headers["Location"], "http://foo.com") | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May want to add tests for response_body_schema = {status: Schema()} and response_body_schema = Schema cases. |
||
def test_validate_query_parameters(self): | ||
rebar = Rebar() | ||
registry = rebar.create_handler_registry() | ||
|
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 know this is just duplicating the logic on 159, but I think it's worth noting I see issues with this approach.
IMO, we should either throw an explicit "invalid status" error if the status is not one defined in the response body schema, or return a default; throwing a KeyError here has the potential to cause unnecessary confusion.