Fix bug that prevents returning Flask.Responses.

[twosigmajab]

https://flask-rebar.readthedocs.io/en/latest/quickstart/basics.html#marshaling currently documents the following: "This means the if response_body_schema is None, the return value must be a return value that Flask supports, e.g. a string or a Flask.Response object.

However, Flask-Rebar currently fails to interoperate properly with a Flask.Response object, as demonstrated by the included test. Without this bugfix to rebar.py, the included test fails with KeyError: 200. This is because the code currently passes rv to _unpack_view_func_return_value, which always returns a 200 status when rv is not a tuple, not realizing that a Flask.Response instance can get passed through as rv which can have a non-200 .status_code (and also custom .headers as well).

[airstandley]

Thanks for the PR twosigmajab!

I think in this case, the functionality
is as intended, but the error could be clearer and the documentation is confusing. If you can propose a way we could make either clearer that would be fantastic!

The confusion seems to be over setting response_schema_body to None vs setting
response_schema_body to a dictionary with one of the key having a value of None.

The use case you supply in that test is technically not supported per the documentation. It supplies a dictionary to response_body_schema which would mean the follow documentation applies:

In the case of a dictionary mapping integers to marshmallow.Schemas, the integers are interpreted as status codes, and the handler function must return a tuple of (response_body, status_code):

@registry.handles(
rule='/todos',
method='POST',
response_body_schema={
201: TodoSchema()
}
)
def create_todo():
...
return {'id': id}, 201

The documentation does state that dictionary values should be Schema objects. Setting one to None is not a supported case.

The None case is if you want to completely opt out of rebar's response validation/swagger generation. It only applies if response_body_schema=None.

Hope that helps.

[jab]

I don’t think that’s accurate, @airstandley (though agreed the docs should be clearer about this, and need improvement in some other ways too (shout out to #153)).

None as a value in the response_body_schema dictionary to mean “bypass serialization for this response code” has been supported by Flask-Rebar for a very long time, and many users (myself included) have been relying on this. This not only allows a particular status code for an endpoint to be documented and testable from the Swagger UI, but also may affect how clients that are code-generated from the generated Swagger spec are able to call the service.

Flask-Rebar's unit tests demonstrate using response_body_schema={204: None, ...} all over the place. Here is one example:

flask-rebar/tests/test_rebar.py

Line 300 in 367bfa8

@registry.handles(rule="/me", method="DELETE", response_body_schema={204: None})

See also the following comment: #26 (comment) (whose changes were eventually incorporated by @barakalon as part of #28 to improve Flask-Rebar’s support for such use cases).

Assuming this is just a momentary mixup and Flask-Rebar does deliberately support dicts with None values here, I think allowing pass-through of flask.Response instances gives users finer-grained control than they have currently using the tuple hack.

Also looks like @mjbryant tried to get similar changes to this landed in #16 a while ago, which is more evidence of demand for this. That was before the changes from #28 were merged. Now that #28 has landed, it seems like it’s time to land these changes too.

[airstandley]

@jab True, I wasn't entirely accurate.
I'm aware that response_body_schema={<status>: None} is a supported use case, even if our documentation doesn't reflect that.

I was hoping to avoid the complications of intentions vs behaviour vs documentation since all of those are constantly changing by just focusing on what we currently promise in the documentation.
Which admittedly doesn't really add much to the conversation...

Sorry, and thanks for all the details, there's stuff in there I was not aware of.

@twosigmajab
I think that extending rebar to work with Flask.Response objects is a great idea.
The issue I have with the solution you are proposing here is that we lose the guarantee that a status defined in response_body_schema will always follow the defined schema.

As an example:

@registry.handles(
    "/test",
    response_body_schema={
        200: ObjectSchema,
})
def view():
   return Response(None, status=200)

Rather than just skipping marshaling/validation when a Response object is returned, why not look at the rv.status, and then validate the rv.data if a schema is defined for that status?

[airstandley]

Seems like we could potentially simplify the logic quite a bit if we first passed rv into make_reponse and then validated on the returned Response object.

[twosigmajab]

Thanks for taking another look, @airstandley! Hoping to get time to take another look at this later this week (but if at any point someone else is eager to submit a patch please don't hesitate). Sorry for the delay and will check back in asap!

[Sytten]

Hum that's weird because I have some code that return a 302 Response object without a tuple and it works :S
Something like:

from flask import redirect

@registry.handles(
    rule="/oauth/callback",
    method="GET",
    query_string_schema=OAuthCallbackSchema(),
)
def callback(service: OAuthService):
    ...
    return redirect(login_redirect_uri)

[jab]

@Sytten, you would have to add response_body_schema={302: None} to your example (so that this response gets included in your generated Swagger spec/documentation) in order to reproduce this.

[Sytten]

I see, then I think the test isn't really testing the new behaviour?

[jab]

What makes you think that? The test includes response_body_schema={302: None}, returns a Response instance, and fails without the included changes to rebar.py, as mentioned in the PR description.

[Sytten]

Ho right, but shouldn't we check for the status code of the response object before returning it? Otherwise we can basically specify anything in the response_schema_body and it might not be what we really return.

[twosigmajab]

@airstandley, @RookieRick, I added the additional validation of the response body as requested, along with corresponding tests. Another look?

Note that this requires an extra deserialization of the response body that the user has just serialized, since the pre-serialized object the user passed into make_response does not get stashed anywhere on the Response object before it's converted into a serialized body. It's unfortunate this validation costs a little more than it'd otherwise cost (and perhaps there's an argument that some users who choose to use make_response directly may want to avoid this validation), but on balance I think it's more important for Rebar to help users avoid accidentally returning responses that don't conform to the schemas they intend.

[twosigmajab]

Rather than deal with Python 3.5-only test failures, I submitted #221 to drop support for 3.5 (it's passed end-of-life) and rebased on top of that branch.

[airstandley]

@twosigmajab Thank you so much for finding the time to pick this back up!

Approving because at this point this is definitely a clear improvement over what we have, though there are some potential gotchas I see.

[airstandley]

Suggested change

	schema = response_body_schema[rv.status_code]
	schema = response_body_schema.get(rv.status_code, None)

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.

[airstandley]

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.

[airstandley]

Before calling schema.loads I think we may need to call normalize_schema or risk breaking when schema is a class and not an instance.

[airstandley]

May want to add tests for response_body_schema = {status: Schema()} and response_body_schema = Schema cases.

[RookieRick]

Thanks @airstandley and thanks @twosigmajab ! In the spirit of incremental improvement, I'll get this one merged this afternoon and will open a new Issue for the things noted 🎉