> add the `http.Response.Status` and `http.Response.Header` to `ReqInf`, > instead of the `http.Response` itself.
+1, I can get on board with that. It's also entirely possible that not all of our client methods currently return a `ReqInf`, but we should definitely rectify that. - Rawlin On Fri, May 15, 2020 at 2:09 PM Robert O Butts <[email protected]> wrote: > > >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 > >
