More flexible NewRequest parameters

- additionalQueryParams should be used for all request methods
- reqBody should be encoded as url query parameters for GET requests

tfe.go

@@ -246,11 +246,22 @@ func (c *Client) doForeignPUTRequest(ctx context.Context, foreignURL string, dat
 	return request.DoJSON(ctx, nil)
 }
 
-func (c *Client) NewRequest(method, path string, reqAttr any) (*ClientRequest, error) {
-	return c.NewRequestWithAdditionalQueryParams(method, path, reqAttr, nil)
+// NewRequest performs some basic API request preparation based on the method
+// specified. For GET requests, the reqBody is encoded as query parameters.
+// For DELETE, PATCH, and POST requests, the request body is serialized as JSONAPI.
+// For PUT requests, the request body is sent as a stream of bytes.
+func (c *Client) NewRequest(method, path string, reqBody any) (*ClientRequest, error) {
+	return c.NewRequestWithAdditionalQueryParams(method, path, reqBody, nil)
 }
 
-func (c *Client) NewRequestWithAdditionalQueryParams(method, path string, reqAttr any, additionalQueryParams map[string][]string) (*ClientRequest, error) {
+// NewRequestWithAdditionalQueryParams performs some basic API request
+// preparation based on the method specified. For GET requests, the reqBody is
+// encoded as query parameters. For DELETE, PATCH, and POST requests, the
+// request body is serialized as JSONAPI. For PUT requests, the request body is
+// sent as a stream of bytes. Additional query parameters can be added to the
+// request as a string map. Note that if a key exists in both the reqBody and
+// additionalQueryParams, the value in additionalQueryParams will be used.
+func (c *Client) NewRequestWithAdditionalQueryParams(method, path string, reqBody any, additionalQueryParams map[string][]string) (*ClientRequest, error) {
 	var u *url.URL
 	var err error
 	if strings.Contains(path, "/api/registry/") {
@@ -265,6 +276,8 @@ func (c *Client) NewRequestWithAdditionalQueryParams(method, path string, reqAtt
 		}
 	}
 
+	q := make(url.Values)
+
 	// Create a request specific headers map.
 	reqHeaders := make(http.Header)
 	reqHeaders.Set("Authorization", "Bearer "+c.token)
@@ -274,31 +287,34 @@ func (c *Client) NewRequestWithAdditionalQueryParams(method, path string, reqAtt
 	case "GET":
 		reqHeaders.Set("Accept", ContentTypeJSONAPI)
 
-		if reqAttr != nil {
-			q, err := query.Values(reqAttr)
+		// Encode the reqBody as query parameters
+		if reqBody != nil {
+			q, err = query.Values(reqBody)
 			if err != nil {
 				return nil, err
 			}
-			for k, v := range additionalQueryParams {
-				q[k] = v
-			}
-			u.RawQuery = encodeQueryParams(q)
 		}
 	case "DELETE", "PATCH", "POST":
 		reqHeaders.Set("Accept", ContentTypeJSONAPI)
 		reqHeaders.Set("Content-Type", ContentTypeJSONAPI)
 
-		if reqAttr != nil {
-			if body, err = serializeRequestBody(reqAttr); err != nil {
+		if reqBody != nil {
+			if body, err = serializeRequestBody(reqBody); err != nil {
 				return nil, err
 			}
 		}
 	case "PUT":
 		reqHeaders.Set("Accept", "application/json")
 		reqHeaders.Set("Content-Type", "application/octet-stream")
-		body = reqAttr
+		body = reqBody
 	}
 
+	for k, v := range additionalQueryParams {
+		q[k] = v
+	}
+
+	u.RawQuery = encodeQueryParams(q)
+
 	req, err := retryablehttp.NewRequest(method, u.String(), body)
 	if err != nil {
 		return nil, err
@@ -721,7 +737,9 @@ func (c *Client) configureLimiter(rawLimit string) {
 }
 
 // encodeQueryParams encodes the values into "URL encoded" form
-// ("bar=baz&foo=quux") sorted by key.
+// ("bar=baz&foo=quux") sorted by key. This version behaves as url.Values
+// Encode, except that it encodes certain keys as comma-separated values instead
+// of using multiple keys.
 func encodeQueryParams(v url.Values) string {
 	if v == nil {
 		return ""

tfe_test.go

@@ -5,9 +5,13 @@ package tfe
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -198,3 +202,105 @@ func Test_RegistryBasePath(t *testing.T) {
 		assert.Equal(t, req.retryableRequest.URL.String(), expected)
 	})
 }
+
+func Test_NewRequestWithAdditionalQueryParams(t *testing.T) {
+	testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if strings.HasSuffix(r.URL.Path, "/get_request_include") {
+			val := r.URL.Query().Get("include")
+			if val != "workspace,cost_estimate" {
+				t.Fatalf("unexpected include value: %q", val)
+			}
+			w.Write([]byte(`{}`))
+			return
+		} else if strings.HasSuffix(r.URL.Path, "/get_request_include_extra") {
+			val := r.URL.Query().Get("include")
+			if val != "workspace,cost_estimate" {
+				t.Fatalf("unexpected include value: expected %q, got %q", "extra,workspace,cost_estimate", val)
+			}
+			extra := r.URL.Query().Get("extra")
+			if extra != "value" {
+				t.Fatalf("unexpected extra value: expected %q, got %q", "value", extra)
+			}
+			w.Write([]byte(`{}`))
+			return
+		} else if strings.HasSuffix(r.URL.Path, "/get_request_include_raw") {
+			extra := r.URL.Query().Get("Name")
+			if extra != "yes" {
+				t.Fatalf("unexpected query: %s", r.URL.RawQuery)
+			}
+			w.Write([]byte(`{}`))
+			return
+		} else if strings.HasSuffix(r.URL.Path, "/delete_with_query") {
+			extra := r.URL.Query().Get("extra")
+			if extra != "value" {
+				t.Fatalf("unexpected query: expected %q, got %q", "value", extra)
+			}
+			w.Write([]byte(`{}`))
+			return
+		} else if strings.HasSuffix(r.URL.Path, "/api/v2/ping") {
+			w.Write([]byte(`{}`))
+			return
+		}
+
+		t.Fatalf("unexpected request: %s", r.URL.String())
+	}))
+	t.Cleanup(func() {
+		testServer.Close()
+	})
+
+	client, err := NewClient(&Config{
+		Address: testServer.URL,
+	})
+	require.NoError(t, err)
+
+	t.Run("with additional query parameters", func(t *testing.T) {
+		request, err := client.NewRequestWithAdditionalQueryParams("GET", "/get_request_include", nil, map[string][]string{
+			"include": {"workspace", "cost_estimate"},
+		})
+		require.NoError(t, err)
+
+		ctx := context.Background()
+		err = request.DoJSON(ctx, nil)
+		require.NoError(t, err)
+	})
+
+	type extra struct {
+		Extra string `url:"extra"`
+	}
+
+	// json-encoded structs use the field name as the query parameter name
+	type raw struct {
+		Name string `json:"extra"`
+	}
+
+	t.Run("GET request with req attr and additional request attributes", func(t *testing.T) {
+		request, err := client.NewRequestWithAdditionalQueryParams("GET", "/get_request_include_extra", &extra{Extra: "value"}, map[string][]string{
+			"include": {"workspace", "cost_estimate"},
+		})
+		require.NoError(t, err)
+
+		ctx := context.Background()
+		err = request.DoJSON(ctx, nil)
+		require.NoError(t, err)
+	})
+
+	t.Run("DELETE request with additional request attributes", func(t *testing.T) {
+		request, err := client.NewRequestWithAdditionalQueryParams("DELETE", "/delete_with_query", nil, map[string][]string{
+			"extra": {"value"},
+		})
+		require.NoError(t, err)
+
+		ctx := context.Background()
+		err = request.DoJSON(ctx, nil)
+		require.NoError(t, err)
+	})
+
+	t.Run("GET request with other kinds of annotations", func(t *testing.T) {
+		request, err := client.NewRequestWithAdditionalQueryParams("GET", "/get_request_include_raw", &raw{Name: "yes"}, nil)
+		require.NoError(t, err)
+
+		ctx := context.Background()
+		err = request.DoJSON(ctx, nil)
+		require.NoError(t, err)
+	})
+}