Handle responses as JSON only if required to. #175
Conversation
A JSON response will have "application/json", and only if this is set should the response content be decoded as such. Otherwise (until something smarter comes along), on might as well return the raw response content (which happens to be the correct thing to do for "text/plain" content).
| content_value = response.json() | ||
| content_type = response.headers.get('content-type', '').lower() | ||
|
|
||
| if content_type.startswith(APP_JSON): |
There was a problem hiding this comment.
Any reason you went with startswith instead of equality?
There was a problem hiding this comment.
To opportunistically capture "application/json-rpc" should that ever occur? Admittedly this test may need to evolve over time since there are various other content types that could contain json. I don't feel strongly about this either way.
|
This might be a breaking change for users where the server isn't sending the correct content type header. I'm wondering if we should make this behavior configurable, i.e. add an option to make bravado-core behave the way it used to. The other option would be to ship this as-is and wait for user feedback. Thoughts? |
|
My approach here is to KISS. And yes in the future there will be some
breakage. But there's arguably more value in detecting the breakage,
understanding the variety of non-compliant systems and putting in explicit
fixes. The alternative is that you'd never be sure as to whether the
configuability you've added preemptively is ever used and therefore whether
you could ever change it - since you don't know how/if it is being used.
For that reason I don't think the existing code (which assumed JSON
content) was a bad idea - it was good enough until the time it proved to be
insufficient and is now being updated because of a real need.
…--
Michal Ostrowski
On Tue, Jun 6, 2017 at 6:45 AM Stephan Jaensch ***@***.***> wrote:
This might be a breaking change for users where the server isn't sending
the correct content type header. I'm wondering if we should make this
behavior configurable, i.e. add an option to make bravado-core behave the
way it used to. The other option would be to ship this as-is and wait for
user feedback. Thoughts?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#175 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALnLD1jVeWs8EGzZE9G5ky5BEkTq7jgpks5sBTvigaJpZM4NsC-G>
.
|
|
Yeah, that sounds reasonable. Let's make sure we release this as a major version. |
A JSON response will have "application/json", and only if this is set should
the response content be decoded as such. Otherwise (until something smarter
comes along), on might as well return the raw response content (which
happens to be the correct thing to do for "text/plain" content).