[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5747: Client consistency

[GitBox <gi...@apache.org>]

rawlinp commented on a change in pull request #5747:
URL: https://github.com/apache/trafficcontrol/pull/5747#discussion_r628312121



##########
File path: traffic_ops/toclientlib/toclientlib.go
##########
@@ -241,7 +240,7 @@ func (to *TOClient) login() (net.Addr, error) {
 func (to *TOClient) loginWithToken(token []byte) (net.Addr, error) {
 	path := to.APIBase() + "/user/login/token"
 	resp, remoteAddr, err := to.RawRequestWithHdr(http.MethodPost, path, token, nil)
-	resp, remoteAddr, err = to.errUnlessOKOrNotModified(resp, remoteAddr, err, path)
+	err = to.errorFromStatusCode(resp, err, path)

Review comment:
       Since you removed the `ioutil.Readall(resp.Body)` from this method, there is a possibility we are returning here without fully reading the body and closing it.

##########
File path: traffic_ops/toclientlib/toclientlib.go
##########
@@ -438,28 +437,28 @@ func ErrIsNotImplemented(err error) bool {
 // Users should check ErrIsNotImplemented rather than comparing directly, in case context was added.
 var ErrNotImplemented = errors.New("Traffic Ops Server returned 'Not Implemented', this client is probably newer than Traffic Ops, and you probably need to either upgrade Traffic Ops, or use a client whose version matches your Traffic Ops version.")
 
-// errUnlessOKOrNotModified returns the response, the remote address, and an error if the given Response's status code is anything
-// but 200 OK/ 304 Not Modified. This includes reading the Response.Body and Closing it. Otherwise, the given response, the remote
-// address, and a nil error are returned.
-func (to *TOClient) errUnlessOKOrNotModified(resp *http.Response, remoteAddr net.Addr, err error, path string) (*http.Response, net.Addr, error) {
+// errorFromStatusCode returns an error if and when the response status code of
+// `resp` warrants it. Specifically, it checks that the response code is either
+// in the < 300 range, but with a special exception for Not Modified.
+// The error given is any network-level error that might have occurred, which is
+// used in lieu of an HTTP-based error being unavailable. Path is the request
+// path, used for informational purposes in the error message text.
+func (to *TOClient) errorFromStatusCode(resp *http.Response, err error, path string) error {
 	if err != nil {
-		return resp, remoteAddr, err
+		return err
 	}
-	if resp.StatusCode < 300 || resp.StatusCode == 304 {
-		return resp, remoteAddr, err
+	if resp == nil {
+		return errors.New("error requesting Traffic Ops: empty/invalid response")
+	}
+	if resp.StatusCode < 300 || resp.StatusCode == http.StatusNotModified {
+		return nil
 	}
-
-	defer resp.Body.Close()
 
 	if resp.StatusCode == http.StatusNotImplemented {
-		return resp, remoteAddr, ErrNotImplemented
+		return ErrNotImplemented
 	}
 
-	body, readErr := ioutil.ReadAll(resp.Body)

Review comment:
       We need to be careful about removing this bit, because callers of this method may have relied on that to fully read the response body and close it:
   > // If the returned error is nil, the Response will contain a non-nil
   // Body which the user is expected to close. If the Body is not both
   // read to EOF and closed, the Client's underlying RoundTripper
   // (typically Transport) may not be able to re-use a persistent TCP
   // connection to the server for a subsequent "keep-alive" request.
   
   I _think_ the only callers that are affected are `logout` and `loginWithToken`.

##########
File path: traffic_ops/v4-client/servercapability.go
##########
@@ -13,69 +15,49 @@
    limitations under the License.
 */
 
-package client
-
 import (
-	"fmt"
-	"net/http"
 	"net/url"
 
 	"github.com/apache/trafficcontrol/lib/go-tc"
 	"github.com/apache/trafficcontrol/traffic_ops/toclientlib"
 )
 
-const (
-	// APIServerCapabilities is the full path to the /server_capabilities API
-	// endpoint.
-	APIServerCapabilities = "/server_capabilities"
-)
+// apiServerCapabilities is the full path to the /server_capabilities API
+// endpoint.
+const apiServerCapabilities = "/server_capabilities"
 
 // CreateServerCapability creates the given Server Capability.
-func (to *Session) CreateServerCapability(sc tc.ServerCapability) (*tc.ServerCapabilityDetailResponse, toclientlib.ReqInf, error) {
+func (to *Session) CreateServerCapability(sc tc.ServerCapability, opts RequestOptions) (tc.ServerCapabilityDetailResponse, toclientlib.ReqInf, error) {
 	var scResp tc.ServerCapabilityDetailResponse
-	reqInf, err := to.post(APIServerCapabilities, sc, nil, &scResp)
-	if err != nil {
-		return nil, reqInf, err
-	}
-	return &scResp, reqInf, nil
+	reqInf, err := to.post(apiServerCapabilities, opts, sc, &scResp)
+	return scResp, reqInf, err
 }
 
 // GetServerCapabilities returns all the Server Capabilities in Traffic Ops.
-func (to *Session) GetServerCapabilities(header http.Header) ([]tc.ServerCapability, toclientlib.ReqInf, error) {
+func (to *Session) GetServerCapabilities(opts RequestOptions) (tc.ServerCapabilitiesResponse, toclientlib.ReqInf, error) {
 	var data tc.ServerCapabilitiesResponse
-	reqInf, err := to.get(APIServerCapabilities, header, &data)
-	return data.Response, reqInf, err
-}
-
-// GetServerCapability retrieves the Server Capability with the given Name.
-func (to *Session) GetServerCapability(name string, header http.Header) (*tc.ServerCapability, toclientlib.ReqInf, error) {
-	reqURL := fmt.Sprintf("%s?name=%s", APIServerCapabilities, url.QueryEscape(name))
-	var data tc.ServerCapabilitiesResponse
-	reqInf, err := to.get(reqURL, header, &data)
-	if err != nil {
-		return nil, reqInf, err
-	}
-	if len(data.Response) == 1 {
-		return &data.Response[0], reqInf, nil
-	}
-	return nil, reqInf, fmt.Errorf("expected one server capability in response, instead got: %+v", data.Response)
+	reqInf, err := to.get(apiServerCapabilities, opts, &data)
+	return data, reqInf, err
 }
 
 // UpdateServerCapability updates a Server Capability by name.
-func (to *Session) UpdateServerCapability(name string, sc *tc.ServerCapability, header http.Header) (*tc.ServerCapability, toclientlib.ReqInf, error) {
-	route := fmt.Sprintf("%s?name=%s", APIServerCapabilities, url.QueryEscape(name))
-	var data tc.ServerCapability
-	reqInf, err := to.put(route, sc, header, &data)
-	if err != nil {
-		return nil, reqInf, err
+func (to *Session) UpdateServerCapability(name string, sc tc.ServerCapability, opts RequestOptions) (tc.ServerCapabilityDetailResponse, toclientlib.ReqInf, error) {
+	if opts.QueryParameters == nil {
+		opts.QueryParameters = url.Values{}
 	}
-	return &data, reqInf, nil
+	opts.QueryParameters.Set("name", name)
+	var data tc.ServerCapabilityDetailResponse
+	reqInf, err := to.put(apiServerCapabilities, opts, sc, &data)
+	return data, reqInf, err
 }
 
 // DeleteServerCapability deletes the given server capability by name.
-func (to *Session) DeleteServerCapability(name string) (tc.Alerts, toclientlib.ReqInf, error) {
-	reqURL := fmt.Sprintf("%s?name=%s", APIServerCapabilities, url.QueryEscape(name))
+func (to *Session) DeleteServerCapability(name string, opts RequestOptions) (tc.Alerts, toclientlib.ReqInf, error) {
+	if opts.QueryParameters == nil {
+		opts.QueryParameters = url.Values{}
+	}
+	opts.QueryParameters.Set("name", name)

Review comment:
       Since this seems to be such a common pattern, it would probably make sense to add a `SetQueryParams` method to the `RequestOptions` struct, but I would target that change for a future PR.

##########
File path: traffic_ops/toclientlib/toclientlib.go
##########
@@ -270,7 +269,7 @@ func (to *TOClient) logout() (net.Addr, error) {
 
 	path := to.APIBase() + "/user/logout"
 	resp, remoteAddr, err := to.RawRequestWithHdr("POST", path, credentials, nil)
-	resp, remoteAddr, err = to.errUnlessOKOrNotModified(resp, remoteAddr, err, path)
+	err = to.errorFromStatusCode(resp, err, path)

Review comment:
       Since you removed the `ioutil.Readall(resp.Body)` from this method, there is a possibility we are returning here without fully reading the body and closing it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org