>I've never really been a fan of the `ErrUnlessOK` behavior in the client
+1 - In my experience, the pain exceeds the convenience. >If the HTTP request was successful but returned a bad status code, a non-nil error would be returned along with the non-nil http.Response. But if the HTTP request failed (e.g. due to a failed connection), it would return a non-nil error but a nil http.Response. +1 My only concern is the `Response.Body`. It's an additional burden and potential error for clients to have to close the Body, especially since we've already deserialized and returned what it contains. In fact, because `http.Reader.Body` isn't an `io.Seeker`, I think it's impossible for us to reset it. I think we should either thoroughly document that the returned `http.Response.Body` will always be closed, or else add the `http.Response.Status` and `http.Response.Header` to `ReqInf`, instead of the `http.Response` itself. I'm leaning toward the latter. Returning a closed Body seems to violate the Principle of Least Surprise. I can imagine scenarios where someone might want some of the other data in `http.Response`, but they seem pretty unlikely. With the headers and remote IP you can usually figure out the rest. I've used the client a lot, and I don't think I've ever wanted/needed anything but the code and headers. Thoughts? On Fri, May 15, 2020 at 1:51 PM Rawlin Peters <[email protected]> wrote: > Honestly, I've never really been a fan of the `ErrUnlessOK` behavior > in the client, but I also know it makes things more convenient (at > least for non-test purposes). I know it's kind of "idiomatic Go" to > not rely on any other return value being useful when the error is > non-nil, but I personally don't see a problem with returning some kind > of response object even when returning a non-nil error (the 3rd option > you've mentioned). Most use cases for testing probably care mainly > about the status code returned, but I could see us wanting to test > things like response headers too. So rather than just returning the > status code from the client, we should probably just return the full > `*http.Response`. If the HTTP request was successful but returned a > bad status code, a non-nil error would be returned along with the > non-nil http.Response. But if the HTTP request failed (e.g. due to a > failed connection), it would return a non-nil error but a nil > http.Response. > > If we added the `*http.Response` to the ReqInf struct that is > typically returned by all client methods, that would save us from > having to go update the usage throughout the codebase (as opposed to > changing the function signature by adding a new return value). I think > that would be alright. > > - Rawlin > > On Fri, May 15, 2020 at 12:12 PM Zach Hoffman <[email protected]> wrote: > > > > In Go, we often use an error's nilness to verify that a subroutine was > > successful. While this is fine, the converse, returning a nil struct and > a > > non-nil error in the case of failure, is not necessarily sufficient, > > particularly in the case of HTTP response status codes, where it is often > > important to distinguish between responses that have a 400-level or > > 500-level status code. > > > > When we test Traffic Ops validation with negative Traffic Ops API tests, > we > > should assert that the response status code is 400-level, as 500-level > > indicades a server error. However, asserting status codes is not > currently > > practical in the TO API tests, because: > > > > • The status code is a field in the http.Response struct > > • For all cases of successfully receiving a response, > > client.Session.request() will return the result of > > client.Session.ErrUnlessOK() unless the status code is 401, 403, or > > 200-level: > > > https://github.com/apache/trafficcontrol/blob/683ba8cf8c/traffic_ops/client/session.go#L342-L351 > > • Unless the response status code is less than 300, > > client.Session.ErrUnlessOK() will return a nil response: > > > https://github.com/apache/trafficcontrol/blob/683ba8cf8c/traffic_ops/client/session.go#L321-L329 > > > > To provide a practical example of this problem, the status code check > added > > in https://github.com/apache/trafficcontrol/pull/4694 will always pass > > because the Topologies API tests will not know if a response's status > code > > was 500-level. > > > > One possible answer to this is "Then don't use client.Session.request(), > > just use client.Session.RawRequest() directly. This answer neglects the > > fact that, although the goal mentioned is to verify HTTP status codes in > > the API tests, the preferred method of sending requests from the API > tests > > to the Traffic Ops instance is using the Golang Traffic Ops client > library, > > which benefits from the additional validation that > client.Session.request() > > provides over client.Session.RawRequest(). Performing this validation > > outside of client.Session.request() or client.Session.RawRequest() but > > still using it would be duplicated effort. > > > > Another possible answer is "Just derive the status code from the error > > string". Besides the fact that the error is not guaranteed to be non-nil, > > building program flow around the anticipated structure of an error > message > > would be an anti-pattern that would take increasing effort to maintain as > > the TO client library grows and is worth avoiding. > > > > A third option is to modify client.Session.ErrUnlessOK() to stil return > the > > response in cases where there was an error. This option does not remove > any > > information, as the error from the validation that ErrUnlessOK() performs > > is still returned. When that error is non-nil, callers still know to > abort > > what they were doing, but we now can benefit from the additional > > information that the non-nil response provides (including status code). > > > > A fourth option is to add an additional request method that has all of > > client.Session.request()'s validation but still always returns the > > response. Code fragmentation like this incurs an additional maintenance > > cost and would be nice to avoid. > > > > Does anyone prefer one of these four options (or a different option)? > > > > Thanks for reading. > > > > -- > > Zach >
