Skip to content
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 for issue 222 #249

Closed
wants to merge 1 commit into from
Closed

Fix for issue 222 #249

wants to merge 1 commit into from

Conversation

helgridly
Copy link

Raise HTTPError without attempting to unmarshal the response if the status code isn't 2xx.

@helgridly helgridly changed the title Fix for #222 Fix for issue 222 Sep 1, 2016
@coveralls
Copy link

coveralls commented Sep 1, 2016

Coverage Status

Coverage remained the same at 97.449% when pulling bff6722 on helgridly:fix_222 into 3c73bd3 on Yelp:master.

@sjaensch
Copy link
Contributor

sjaensch commented Sep 2, 2016

Thank you for the contribution! I think this is going too far though. We have services in production that return properly formatted JSON responses for e.g. 403, 404 HTTP status codes. We should either limit this to >= 500 or maybe check if there is a response defined in the spec for the status code, and if we do then unmarshal.

@macisamuele
Copy link
Collaborator

I would prefer to not run the unmarshal_response method only if the operation object has no knowledge about the response format or if the content type is not application/json (#250).

The swagger specs allow you to define the response format also for not HTTP/200 cases, like in the Operation Object Example (where is defined the format of the HTTP/405 case).

I believe that checking that incoming_response.content_type is a JSON could be enough.

if self.operation is not None and incoming_response.content_type is 'application/json':

@helgridly
Copy link
Author

helgridly commented Sep 2, 2016

The "throw an HTTPError if there's no response format defined" idea sounds like a good one. My only concern is that users (i.e. me) now have to handle non-2xx codes differently, based on whether the server returns JSON or not - and that one case looks "normal" while the other raises an exception. Does this sound like a problem to you? Should we just return the incoming response for unmarshalable non-2xx codes too, and skip the exception entirely?

edit:\ Oh, I just found this.

@helgridly
Copy link
Author

I dug into what's happening in my case some more and discovered I've run into a discrepancy between what Swagger describes and what we actually return. I'm happy for this to be closed if it's unhelpful, since it's user error.

@sjaensch sjaensch closed this Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants