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
