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.

Sign up for GitHub

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

Jump to bottom

Always try to parse response if SetResult has been called #240

Closed
pborzenkov opened this issue Apr 18, 2019 · 7 comments · Fixed by #323
Closed

Always try to parse response if SetResult has been called #240

pborzenkov opened this issue Apr 18, 2019 · 7 comments · Fixed by #323
Assignees
@jeevatkm
Labels
enhancement v2 For resty v2
Milestone
v2.3.0 Milestone

Comments

@pborzenkov
Copy link
Contributor

It's possible to get a 200 OK response with HTML contents from a JSON endpoint due to faulty backend configuration (e.g. a problem in API gateway, etc.).
Since resty only parses response if content type is JSON or XML the call won't return an error. Such errors could be hard to debug.

I suggest to fix the default behaviour and always parse response if SetResult has been called.
@jeevatkm
Copy link
Member

@pborzenkov Thank you for reporting an issue. I will have a look and get back to you.

@jeevatkm jeevatkm added this to the v2.0.0 Milestone milestone Jun 3, 2019
@jeevatkm jeevatkm added enhancement v2 For resty v2 labels Jun 3, 2019
@jeevatkm jeevatkm self-assigned this Jun 3, 2019
@jeevatkm
Copy link
Member

jeevatkm commented Jun 3, 2019

@pborzenkov This I will take it up for v2.

jeevatkm added a commit that referenced this issue Jun 18, 2019
@jeevatkm
#240 adding test case for analyzes
01e423c
@jeevatkm
Copy link
Member

jeevatkm commented Jun 18, 2019 •
edited
Loading

@pborzenkov I have written a small test case for the scenario you have described. I didn't see any errors (This is the upcoming v2 branch, notably this particular part should be the same as v1).

$ go test -v -run TestPostJSONForGH240
=== RUN   TestPostJSONForGH240
<nil> 200 <htm><body>Test JSON request with HTML response</body></html>
&{ }
--- PASS: TestPostJSONForGH240 (0.00s)
    resty_test.go:224: Method: POST
    resty_test.go:225: Path: /login-json-html
    resty_test.go:226: RawQuery:
    resty_test.go:227: Content-Type: application/json; charset=utf-8
    request_test.go:332: Result Success: &{"" ""}
    resty_test.go:627: Response Status: 200 OK
    resty_test.go:628: Response Time: 996.7µs
    resty_test.go:629: Response Headers: map[Content-Length:[61] Content-Type:[text/html] Date:[Tue, 18 Jun 2019 04:50:32 GMT]]
    resty_test.go:630: Response Cookies: []
    resty_test.go:631: Response Body: <htm><body>Test JSON request with HTML response</body></html>
PASS
ok      github.com/go-resty/resty       0.294s

Could you please have a look this 01e423c?

Let me know what you're trying to explain. If possible provide an example or test case to demonstrate.

@jeevatkm jeevatkm added analysis and removed enhancement v2 For resty v2 labels Jun 18, 2019
@jeevatkm jeevatkm removed this from the v2.0.0 Milestone milestone Jun 19, 2019
@pborzenkov
Copy link
Contributor Author

@jeevatkm

Let me know what you're trying to explain. If possible provide an example or test case to demonstrate.

That's exactly what I was talking about. I'd have expected the err to be non-nil, since the response doesn't have the expected structure (AuthSuccess). But since Content-Type isn't a JSON, resty doesn't even try to parse it. That's why I proposed to always try and parse the response if SetResult has been called, even if the response doesn't have proper Content-Type.

@jeevatkm jeevatkm mentioned this issue Oct 10, 2019
Closed
@jeevatkm
Copy link
Member

jeevatkm commented Mar 2, 2020

@pborzenkov Currently resty have ExpectContentType and I have added new method ForceContentType for #276 in the branch result-parsing-enhancement

Does it satisfy your use case, can you please check, let me know?

@pborzenkov
Copy link
Contributor Author

@jeevatkm Yes, it does satisfy the describe use case. Thanks!

@jeevatkm
Copy link
Member

jeevatkm commented Mar 2, 2020

@pborzenkov wow, quick response, appreciated. Sorry for the delay!

@jeevatkm jeevatkm added enhancement v2 For resty v2 and removed analysis labels Mar 6, 2020
@jeevatkm jeevatkm added this to the v2.3.0 Milestone milestone Mar 6, 2020
@jeevatkm jeevatkm mentioned this issue Mar 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jeevatkm jeevatkm

Labels
enhancement v2 For resty v2
Milestone
v2.3.0 Milestone
Development

Successfully merging a pull request may close this issue.

#276 added force content type option for automatic unmarshalling go-resty/resty
2 participants
@pborzenkov @jeevatkm