> We currently have client methods that take integer IDs as strings instead of actual integers
*shudder* +1 to changing that > we should have the corresponding TO client method take ` url.Values` as input +1 On Fri, May 15, 2020 at 4:30 PM Rawlin Peters <[email protected]> wrote: > 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 > > > > > > > > > > > > > >
