We also need to standardize the input parameters as well. We currently have client methods that take integer IDs as strings instead of actual integers for some reason, so most code that uses it has to `strconv.Itoa` to convert the integer ID into a string when calling the client method. That should not be a thing. Also, for API endpoints that support multiple query params, like most `GET` endpoints, I think we should have the corresponding TO client method take ` url.Values` as input -- e.g. `GetCacheGroups(qparams url.Values)` -- so that clients can set the filters/paging/etc that they want -- rather than having one method per qparam filter or one method that takes all qparams as either nil or a pointer to a string. That basically future-proofs client methods in terms of adding support for new qparams.
- Rawlin On Fri, May 15, 2020 at 3:38 PM ocket 8888 <[email protected]> wrote: > > 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 > > > > > > > > > >
