update `unmarshal_response` to handle `use_models=False` better

[terencehonles]

I believe this is the correct behavior, but it looks like there might be more things that should probably respect use_models based on my testing of this with a package which has a bit incomplete schema.

[sjaensch]

First of all, @terencehonles thank you for your contribution! I agree this change makes sense. It caused two tests to fail however - do you think you'll be able to fix those test failures?

[terencehonles]

@sjaensch I'll look into the tests. I think I also might want to change what happens on an error. I was playing around and an exception is thrown because there is no schema defined for an error, and I think it would be appropriate if use_models is False that it would just return the error JSON as is. I believe this was here not in https://github.com/Yelp/bravado-core, but I'll try to spend a little time looking into that today.

[coveralls]

Coverage decreased (-0.07%) to 96.367% when pulling 99042d2fc216be9226b72028d97447b7b921854a on terencehonles:update-unmarshall_response-on-use_models=False into a5f6898f0a9ee296a61bf9e7f4a2a88be58d41d5 on Yelp:master.

[terencehonles]

@sjaensch I'll look into the tests. I think I also might want to change what happens on an error. I was playing around and an exception is thrown because there is no schema defined for an error, and I think it would be appropriate if use_models is False that it would just return the error JSON as is. I believe this was here not in https://github.com/Yelp/bravado-core, but I'll try to spend a little time looking into that today.

I fixed the tests, and realized the error I was seeing I could fix in the same method I had this PR for. I made the adjustment and the tests don't fail because of the change. I'm testing it out locally, but I'm not sure if you'd rather split out the error case from this PR since I'm just getting familiar with this library and I'm not sure how easy it would be to distinguish that this is still an error response (I kinda imagine this would fall in line with however the library behaved before, because an error schema just results in a different model being returned and unmarshalled)

[terencehonles]

I tested this PR with my additional change against our API and it does perform as expected: It raises an error on a non success status code, and the swagger_result is the JSON when using use_models=False

[terencehonles]

I added some tests to better cover the change I made.

One thing to call out is that an empty response (as tested in the no_content test) will now return '' rather than None. I believe that's actually the correct behavior since as tested that's going to be using the path for non JSON/MSGPACK which is either the raw bytes or text. The original test update I had was to update the header to be "application/json", but it looks like that's invalid for no content, and the reason it passed is the mock returned a child mock when .json() was called. I had thought you could use HTTP 204 (no content) when returning a JSON api and that requests wouldn't complain, but testing locally it seems like that would raise an error when .json() was called and I don't think any special handling should be done at this point since that would be a more substantial change in behavior

[terencehonles]

@sjaensch not sure if you might have a chance to look at the PR :slightly_smiling_face:

[sjaensch]

Hey @terencehonles, so sorry for not noticing this sooner! Thank you for pinging me again, and thanks for writing those additional tests! I'm going to merge this in right now.

[sjaensch]

This change has been included in bravado version 10.6.1.

[terencehonles]

Thanks for the heads up @sjaensch!

[kpfleming]

This change broke my application; I do not have use_models set to false, but when I invoke an operation which does not return a response (only a 200 OK result with no body), the application crashes with a parsing failure in simplejson. Reverting to 10.6.0 results in the application working properly again. If there's anything I can to do help test this, please let me know.

[sjaensch]

Thanks @kpfleming for the report! Upon further reflection, I now think this change is incorrect. While it's true that unmarshal_response_inner returned None if there was no swagger schema defined, that seems correct to me. The result of that function is assigned to response.swagger_result. That should only be set for responses that have a defined schema, and for which the raw response was unmarshalled and potentially validated. No such thing happens in this case. @terencehonles, for responses without a swagger schema you should access the raw response. Please refer to the documentation on how to do so.

Tomorrow I'm going to do another point release which reverts these changes. Sorry everybody for the confusion!

[sjaensch]

bravado 10.6.2 has been released which reverts this change.

[terencehonles]

Thanks @kpfleming for the report! Upon further reflection, I now think this change is incorrect. While it's true that unmarshal_response_inner returned None if there was no swagger schema defined, that seems correct to me. The result of that function is assigned to response.swagger_result. That should only be set for responses that have a defined schema, and for which the raw response was unmarshalled and potentially validated. No such thing happens in this case. @terencehonles, for responses without a swagger schema you should access the raw response. Please refer to the documentation on how to do so.

Tomorrow I'm going to do another point release which reverts these changes. Sorry everybody for the confusion!

@sjaensch I'll have to look at this code path a bit more, I don't remember if this fixed an issue with that or if I opened a separate PR, but at one point I was unable to do what you were suggesting.

A 200 with an empty response is not actually JSON which is what @kpfleming noticed (and it "correctly" crashed). I had adjusted the tests to return a {} in that case so it was valid JSON. I looked around what would be the right thing to do, and if it would be better to special case this possibly common "error" I can adjust this PR to do so.

I called out this change in the comment:

I added some tests to better cover the change I made.

One thing to call out is that an empty response (as tested in the no_content test) will now return '' rather than None. I believe that's actually the correct behavior since as tested that's going to be using the path for non JSON/MSGPACK which is either the raw bytes or text. The original test update I had was to update the header to be "application/json", but it looks like that's invalid for no content, and the reason it passed is the mock returned a child mock when .json() was called. I had thought you could use HTTP 204 (no content) when returning a JSON api and that requests wouldn't complain, but testing locally it seems like that would raise an error when .json() was called and I don't think any special handling should be done at this point since that would be a more substantial change in behavior

and while I think the "correct" thing to do, would be to have @kpfleming and others use the right response content or content type header, I think it may make sense to log/warn about the error instead of crashing as I left it here.

By doing so it would be accepting of an existing corner case, but encourage people to produce an API surface which won't cause issues from clients that don't handle this case as gracefully.

[kpfleming]

That's a good point; in my case I can certainly improve the spec document which describes the API to indicate that no response should be expected, although it may require changing the response code as well which is slightly more challenging (still doable though, it's an open source project and I've contributed a number of API cleanups along this line already). There are some places that the server already replies with 204 when it is intentionally sending an empty response; in the other cases if I can't change the response code I could change it to send an empty JSON document.

[terencehonles]

That's a good point; in my case I can certainly improve the spec document which describes the API to indicate that no response should be expected, although it may require changing the response code as well which is slightly more challenging (still doable though, it's an open source project and I've contributed a number of API cleanups along this line already). There are some places that the server already replies with 204 when it is intentionally sending an empty response; in the other cases if I can't change the response code I could change it to send an empty JSON document.

yeah @kpfleming that's actually something I was already doing in my API (HTTP 204) but then I realized that's not actually "standard"/well defined behavior (though I think it is possibly common behavior). In that case the content type shouldn't be application/json since an empty string (even if 204) isn't a valid JSON document. Using HTTP 204 and omitting the content type may actually be more standards compliant.

See: https://stackoverflow.com/questions/21029351/what-content-type-should-a-204-no-response-use

Here's an example of someone frustrated by ASP.NET returning a 204 and breaking an Angular client.