>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 > > > >
