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

JSON body is not decoded when Content-Type: application/json is not present #14

Closed
wants to merge 1 commit into from

Conversation

derlaft
Copy link

@derlaft derlaft commented May 28, 2016

Thus, I think this is logical to decode JSON whether header is set or not, because user must explicitly pass successV/failureV.

Broken test defines current behaviour, If you agree I will fix it too.

@dghubble
Copy link
Owner

dghubble commented May 28, 2016

Receive(successV, failureV interface) and Receive(successV interface{}) decode JSON responses into the given structs. The status code (200-299 vs other) determines which struct to decode into. JSON responses should have a ContentType: "application/json", this is an intentional check.

You may look into why the API does not set the ContentType correctly. Some Facebook API endpoints do this incorrectly (using text/javascript instead), so setting the accept header is required to get proper responses. https://github.com/dghubble/gologin/blob/master/facebook/verify.go#L38 Perhaps a similar option is available to you.

@derlaft
Copy link
Author

derlaft commented May 30, 2016

@dghubble My server app ignores Accept header and always answers with 'Content-Type: text/plain'

JSON responses should have a ContentType: application/json, this is an intentional check.

Why should they? It's clients that are developed for use with servers, not the opposite. I wish I lived in the universe where every server cares about Content-Type header.

@dghubble
Copy link
Owner

dghubble commented Jun 2, 2016

Why should they?

https://tools.ietf.org/html/rfc7159

Is your server app one you can correct or is it a well known service? I'm curious what is returning that. I can see a case for allowing text/javascript since its such a common mistake. Perhaps for accommodating all bad actors, though its a few lines to write the decoder you need.

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.

2 participants