I'm also +1 on those ideas, and I'd also like to add a recommendation to switch to a "standard" return signature for client methods, say Requested Object, Alerts, Summary (?), ReqInf, Error ? We have many endpoint methods that silently drop returned Alerts and I think there are a few that don't return ReqInf.
On Fri, May 15, 2020, 14:46 Zach Hoffman <[email protected]> wrote: > >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. > > Does a calling function have the responsibility of closing Response.Body, > since > Session.ErrUnlessOK() already defers doing so? > > >Returning a closed Body seems to violate the Principle of Least Surprise. > > The official documentation (https://golang.org/pkg/net/http/#Client.Do) > states: > > >On error, any Response can be ignored. A non-nil Response with a non-nil > error > >only occurs when CheckRedirect fails, and even then the returned > Response.Body > >is already closed. > > So, returning the full http.Response struct to callers in case of an error > does > not incur additional responsibility. > > >> 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. > > That sounds fine to me, as well. Then we should be able to get rid of all > of the > Session.RawRequest() usage that the Role, StaticDNSEntry, and > DeliveryServiceRequests client functions currently have. > > -Zach > > > On Fri, May 15, 2020 at 2:30 PM Rawlin Peters <[email protected]> wrote: > > > > 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 > > > > > > >
