Enhance reliability of how we make requests and parse errors #108
Conversation
Codecov Report
@@ Coverage Diff @@
## main #108 +/- ##
==========================================
+ Coverage 94.75% 95.16% +0.40%
==========================================
Files 37 37
Lines 5799 5792 -7
==========================================
+ Hits 5495 5512 +17
+ Misses 244 226 -18
+ Partials 60 54 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| return false, nil | ||
| } | ||
|
|
||
| return false, newError(res.Body) | ||
| return false, newError(response) |
There was a problem hiding this comment.
We've changed the signature of the newError func to accept the entire response so we can have access to the status code as well. Further clarifications as to why this is needed are provided in the comments below.
| req, err := m.NewRequest("PUT", m.URI("branding", "templates", "universal-login"), ul.Body, opts...) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| res, err := m.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if res.StatusCode >= http.StatusBadRequest { | ||
| return newError(res.Body) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
All of this part was just replicating what the m.Request() was doing with the difference that we were not correctly closing the response body in this func, causing memory leaks.
| request, err := m.NewRequest("POST", m.URI("guardian", "enrollments", "ticket"), t, opts...) | ||
| if err != nil { | ||
| return EnrollmentTicket{}, err | ||
| } | ||
|
|
||
| res, err := m.Do(req) | ||
| response, err := m.Do(request) | ||
| if err != nil { | ||
| return EnrollmentTicket{}, err | ||
| } | ||
| defer res.Body.Close() | ||
| defer response.Body.Close() | ||
|
|
||
| if res.StatusCode != http.StatusOK { | ||
| return EnrollmentTicket{}, newError(res.Body) | ||
| if response.StatusCode != http.StatusOK { | ||
| return EnrollmentTicket{}, newError(response) | ||
| } | ||
|
|
||
| var out EnrollmentTicket | ||
| err = json.NewDecoder(res.Body).Decode(&out) | ||
| err = json.NewDecoder(response.Body).Decode(&out) |
There was a problem hiding this comment.
Unfortunately we couldn't do the same thing as we did in the branding.go file and simply replace all the logic with a call to m.Request. This is because m.Request expects to change in place the same object that gets passed through with the response data. In this case we pass CreateEnrollmentTicket but return an EnrollmentTicket which are 2 different structs.
| req, err := http.NewRequest("POST", m.URI("jobs", "users-imports"), &payload) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| req.Header.Add("Content-Type", mp.FormDataContentType()) | ||
|
|
||
| for _, option := range opts { | ||
| option.apply(req) | ||
| } | ||
|
|
||
| res, err := m.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if res.StatusCode < http.StatusOK || res.StatusCode >= http.StatusBadRequest { | ||
| return newError(res.Body) | ||
| } | ||
|
|
||
| if res.StatusCode != http.StatusNoContent { | ||
| defer res.Body.Close() | ||
| return json.NewDecoder(res.Body).Decode(j) | ||
| } | ||
| opts = append(opts, Header("Content-Type", mp.FormDataContentType())) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Again, this is doing exactly what the m.Request is doing so we replace it after appending the opts correctly.
| m := &managementError{} | ||
| if err := json.NewDecoder(r).Decode(m); err != nil { | ||
| return err | ||
| func newError(response *http.Response) error { |
There was a problem hiding this comment.
Quite a few improvements to this func:
- Preserve status code from the response instead of relying purely that the body contains the status code as well
- Preserve status code when we fail to decode the response when this is not a valid JSON
| } | ||
|
|
||
| if response.StatusCode != http.StatusNoContent && response.StatusCode != http.StatusAccepted { |
There was a problem hiding this comment.
This check was replaced by if len(responseBody) > 0 && string(responseBody) != "{}"
sergiught
force-pushed
the
patch/refactor-requests-mechanism
branch
from
25cf36d to
6aec75e
Compare
Description
The push for these improvements originally starts from this issue: auth0/terraform-provider-auth0#36, where under certain rare occasions we could get non HTML body in the responses and then throw an obscure error message, not really understanding what's happening. We try to make things a bit more obvious in this PR and increase the reliability of how we make http requests, there were also a couple of occasions when we didn't correctly close the response body. Further comments are provided in the code.
References
Testing
Checklist
main.