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
