Merge pull request moby#50432 from thaJeztah/cleanup_request

client: cleanup encoding body and add test-coverage

Author: thaJeztah

client/hijack.go

@@ -17,11 +17,11 @@ import (
 
 // postHijacked sends a POST request and hijacks the connection.
 func (cli *Client) postHijacked(ctx context.Context, path string, query url.Values, body interface{}, headers map[string][]string) (types.HijackedResponse, error) {
-	bodyEncoded, err := encodeData(body)
+	jsonBody, err := jsonEncode(body)
 	if err != nil {
 		return types.HijackedResponse{}, err
 	}
-	req, err := cli.buildRequest(ctx, http.MethodPost, cli.getAPIPath(ctx, path, query), bodyEncoded, headers)
+	req, err := cli.buildRequest(ctx, http.MethodPost, cli.getAPIPath(ctx, path, query), jsonBody, headers)
 	if err != nil {
 		return types.HijackedResponse{}, err
 	}

client/request.go

@@ -27,25 +27,25 @@ func (cli *Client) get(ctx context.Context, path string, query url.Values, heade
 	return cli.sendRequest(ctx, http.MethodGet, path, query, nil, headers)
 }
 
-// post sends an http request to the docker API using the method POST with a specific Go context.
-func (cli *Client) post(ctx context.Context, path string, query url.Values, obj interface{}, headers http.Header) (*http.Response, error) {
-	body, headers, err := encodeBody(obj, headers)
+// post sends an http POST request to the API.
+func (cli *Client) post(ctx context.Context, path string, query url.Values, body interface{}, headers http.Header) (*http.Response, error) {
+	jsonBody, headers, err := prepareJSONRequest(body, headers)
 	if err != nil {
 		return nil, err
 	}
-	return cli.sendRequest(ctx, http.MethodPost, path, query, body, headers)
+	return cli.sendRequest(ctx, http.MethodPost, path, query, jsonBody, headers)
 }
 
 func (cli *Client) postRaw(ctx context.Context, path string, query url.Values, body io.Reader, headers http.Header) (*http.Response, error) {
 	return cli.sendRequest(ctx, http.MethodPost, path, query, body, headers)
 }
 
-func (cli *Client) put(ctx context.Context, path string, query url.Values, obj interface{}, headers http.Header) (*http.Response, error) {
-	body, headers, err := encodeBody(obj, headers)
+func (cli *Client) put(ctx context.Context, path string, query url.Values, body interface{}, headers http.Header) (*http.Response, error) {
+	jsonBody, headers, err := prepareJSONRequest(body, headers)
 	if err != nil {
 		return nil, err
 	}
-	return cli.putRaw(ctx, path, query, body, headers)
+	return cli.putRaw(ctx, path, query, jsonBody, headers)
 }
 
 // putRaw sends an http request to the docker API using the method PUT.
@@ -64,26 +64,36 @@ func (cli *Client) delete(ctx context.Context, path string, query url.Values, he
 	return cli.sendRequest(ctx, http.MethodDelete, path, query, nil, headers)
 }
 
-func encodeBody(obj interface{}, headers http.Header) (io.Reader, http.Header, error) {
-	if obj == nil {
+// prepareJSONRequest encodes the given body to JSON and returns it as an [io.Reader], and sets the Content-Type
+// header. If body is nil, or a nil-interface, a "nil" body is returned without
+// error.
+//
+// TODO(thaJeztah): should this return an error if a different Content-Type is already set?
+// TODO(thaJeztah): is "nil" the appropriate approach for an empty body, or should we use [http.NoBody] (or similar)?
+func prepareJSONRequest(body interface{}, headers http.Header) (io.Reader, http.Header, error) {
+	if body == nil {
 		return nil, headers, nil
 	}
 	// encoding/json encodes a nil pointer as the JSON document `null`,
 	// irrespective of whether the type implements json.Marshaler or encoding.TextMarshaler.
 	// That is almost certainly not what the caller intended as the request body.
-	if reflect.TypeOf(obj).Kind() == reflect.Ptr && reflect.ValueOf(obj).IsNil() {
+	//
+	// TODO(thaJeztah): consider moving this to jsonEncode, which would also allow returning an (empty) reader instead of nil.
+	if reflect.TypeOf(body).Kind() == reflect.Ptr && reflect.ValueOf(body).IsNil() {
 		return nil, headers, nil
 	}
 
-	body, err := encodeData(obj)
+	jsonBody, err := jsonEncode(body)
 	if err != nil {
 		return nil, headers, err
 	}
-	if headers == nil {
-		headers = make(map[string][]string)
+	hdr := http.Header{}
+	if headers != nil {
+		hdr = headers.Clone()
 	}
-	headers["Content-Type"] = []string{"application/json"}
-	return body, headers, nil
+
+	hdr.Set("Content-Type", "application/json")
+	return jsonBody, hdr, nil
 }
 
 func (cli *Client) buildRequest(ctx context.Context, method, path string, body io.Reader, headers http.Header) (*http.Request, error) {
@@ -293,14 +303,14 @@ func (cli *Client) addHeaders(req *http.Request, headers http.Header) *http.Requ
 	return req
 }
 
-func encodeData(data interface{}) (*bytes.Buffer, error) {
-	params := bytes.NewBuffer(nil)
+func jsonEncode(data interface{}) (io.Reader, error) {
+	var params bytes.Buffer
 	if data != nil {
-		if err := json.NewEncoder(params).Encode(data); err != nil {
+		if err := json.NewEncoder(&params).Encode(data); err != nil {
 			return nil, err
 		}
 	}
-	return params, nil
+	return &params, nil
 }
 
 func ensureReaderClosed(response *http.Response) {

client/request_test.go

@@ -279,3 +279,83 @@ func TestDeadlineExceededContext(t *testing.T) {
 	_, err := client.sendRequest(ctx, http.MethodGet, testEndpoint, nil, nil, nil)
 	assert.Check(t, is.ErrorIs(err, context.DeadlineExceeded))
 }
+
+func TestPrepareJSONRequest(t *testing.T) {
+	tests := []struct {
+		doc        string
+		body       any
+		headers    http.Header
+		expBody    string
+		expNilBody bool
+		expHeaders http.Header
+	}{
+		{
+			doc:        "nil body",
+			body:       nil,
+			headers:    http.Header{"Something": []string{"something"}},
+			expNilBody: true,
+			expHeaders: http.Header{
+				// currently, no content-type is set on empty requests.
+				"Something": []string{"something"},
+			},
+		},
+		{
+			doc:        "nil interface body",
+			body:       (*struct{})(nil),
+			headers:    http.Header{"Something": []string{"something"}},
+			expNilBody: true,
+			expHeaders: http.Header{
+				// currently, no content-type is set on empty requests.
+				"Something": []string{"something"},
+			},
+		},
+		{
+			doc:     "empty struct body",
+			body:    &struct{}{},
+			headers: http.Header{"Something": []string{"something"}},
+			expBody: `{}`,
+			expHeaders: http.Header{
+				"Content-Type": []string{"application/json"},
+				"Something":    []string{"something"},
+			},
+		},
+		{
+			doc:     "json raw message",
+			body:    json.RawMessage("{}"),
+			expBody: `{}`,
+			expHeaders: http.Header{
+				"Content-Type": []string{"application/json"},
+			},
+		},
+		{
+			doc:     "empty body",
+			body:    http.NoBody,
+			expBody: `{}`,
+			expHeaders: http.Header{
+				"Content-Type": []string{"application/json"},
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			req, hdr, err := prepareJSONRequest(tc.body, tc.headers)
+			assert.NilError(t, err)
+
+			var body string
+			if tc.expNilBody {
+				assert.Check(t, is.Nil(req))
+			} else {
+				assert.Assert(t, req != nil)
+
+				resp, err := io.ReadAll(req)
+				assert.NilError(t, err)
+				body = strings.TrimSpace(string(resp))
+			}
+
+			assert.Check(t, is.Equal(body, tc.expBody))
+			assert.Check(t, is.DeepEqual(hdr, tc.expHeaders))
+			assert.Check(t, is.Equal(tc.headers.Get("Content-Type"), ""), "Should not have mutated original headers")
+		})
+	}
+}