From 69dcee5c9b5d8e7890b40c178cb62c881ca65fad Mon Sep 17 00:00:00 2001 From: Zane Rockenbaugh Date: Wed, 6 Aug 2025 13:25:20 -0500 Subject: [PATCH 1/2] add test to catch and fix for null body problem --- pkg/requests/nil_body_test.go | 93 +++++++++++++++++++++++++++++++++++ pkg/requests/requests.go | 8 ++- 2 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 pkg/requests/nil_body_test.go diff --git a/pkg/requests/nil_body_test.go b/pkg/requests/nil_body_test.go new file mode 100644 index 0000000..7e1879f --- /dev/null +++ b/pkg/requests/nil_body_test.go @@ -0,0 +1,93 @@ +package requests + +import ( + "bytes" + "encoding/json" + "io" + "testing" +) + +// TestDoMethodNilBodyHandling tests the actual body handling logic from Do method +func TestDoMethodNilBodyHandling(t *testing.T) { + // This simulates the exact logic from the Do method + testCases := []struct { + name string + body any + expectNil bool + expectContent string + }{ + { + name: "nil body should produce nil reader", + body: nil, + expectNil: true, + expectContent: "", + }, + { + name: "empty struct produces JSON", + body: struct{}{}, + expectNil: false, + expectContent: "{}", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // This is the exact code from Do method + var reqBody io.Reader + switch body := tc.body.(type) { + case nil: + // For nil body (common with GET requests), use nil reader + reqBody = nil + case []byte: + reqBody = bytes.NewReader(body) + case *bytes.Buffer: + reqBody = bytes.NewReader(body.Bytes()) + default: + serialized, err := json.Marshal(body) + if err != nil { + t.Fatalf("Failed to marshal: %v", err) + } + reqBody = bytes.NewReader(serialized) + } + + // Check the result + if tc.expectNil { + if reqBody != nil { + // Read to see what's in it + buf := new(bytes.Buffer) + buf.ReadFrom(reqBody) + content := buf.String() + + // This should fail when fix is commented out! + if content == "null" { + t.Errorf("BUG DETECTED: nil body was marshaled to 'null' (4 bytes) instead of remaining nil") + } else { + t.Errorf("Expected nil reader for nil body, got reader with content: %q", content) + } + } + } else { + if reqBody == nil { + t.Errorf("Expected non-nil reader, got nil") + } + } + }) + } +} + +// TestNilBodyMarshalBehavior verifies what happens when we marshal nil +// This documents the root cause of the bug: json.Marshal(nil) returns "null" +func TestNilBodyMarshalBehavior(t *testing.T) { + data, err := json.Marshal(nil) + if err != nil { + t.Fatalf("Failed to marshal nil: %v", err) + } + + // This is the bug - nil gets marshaled to "null" (4 bytes) + if string(data) != "null" { + t.Errorf("json.Marshal(nil) = %q, expected \"null\"", string(data)) + } + + if len(data) != 4 { + t.Errorf("json.Marshal(nil) length = %d, expected 4", len(data)) + } +} \ No newline at end of file diff --git a/pkg/requests/requests.go b/pkg/requests/requests.go index e3dc597..c3e2ceb 100644 --- a/pkg/requests/requests.go +++ b/pkg/requests/requests.go @@ -46,6 +46,9 @@ func (c HTTPClient) Do(method, uri, contentType string, body any) ([]byte, error var reqBody io.Reader switch body := body.(type) { + case nil: + // For nil body (common with GET requests), use nil reader + reqBody = nil case []byte: reqBody = bytes.NewReader(body) case *bytes.Buffer: @@ -65,7 +68,10 @@ func (c HTTPClient) Do(method, uri, contentType string, body any) ([]byte, error return nil, fmt.Errorf("error creating API request: %w", err) } - req.Header.Set("Content-Type", contentType) + // Only set Content-Type if there's a body + if reqBody != nil { + req.Header.Set("Content-Type", contentType) + } req.Header.Set("User-Agent", fmt.Sprintf("%s-%s-%s-%s", "shipyard-cli", version.Version, runtime.GOOS, runtime.GOARCH)) req.Header.Set("x-api-token", token) From a31db49a4028010490c0ebb2632634a19bbef938 Mon Sep 17 00:00:00 2001 From: Zane Rockenbaugh Date: Wed, 6 Aug 2025 18:30:58 -0500 Subject: [PATCH 2/2] rework test to avoid race condition and standardize all tests on same expected base URL for simplicity --- pkg/client/client_test.go | 6 ------ pkg/requests/uri/uri_test.go | 30 ++++++++++++++++++------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 37090cb..4f69431 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -14,8 +14,6 @@ import ( ) func TestEnvByID(t *testing.T) { - t.Parallel() - client, cleanup := setup() defer cleanup() @@ -31,8 +29,6 @@ func TestEnvByID(t *testing.T) { } func TestAllServices(t *testing.T) { - t.Parallel() - client, cleanup := setup() defer cleanup() @@ -48,8 +44,6 @@ func TestAllServices(t *testing.T) { } func TestFindService(t *testing.T) { - t.Parallel() - client, cleanup := setup() defer cleanup() diff --git a/pkg/requests/uri/uri_test.go b/pkg/requests/uri/uri_test.go index c3c7441..f26cd61 100644 --- a/pkg/requests/uri/uri_test.go +++ b/pkg/requests/uri/uri_test.go @@ -7,6 +7,13 @@ import ( "github.com/shipyard/shipyard-cli/pkg/requests/uri" ) +func TestMain(m *testing.M) { + // Set a consistent base URL for all tests + _ = os.Setenv("SHIPYARD_BUILD_URL", "http://localhost:8080/api/v1") + code := m.Run() + os.Exit(code) +} + func TestCreateResourceURI(t *testing.T) { testCases := []struct { action string @@ -21,48 +28,48 @@ func TestCreateResourceURI(t *testing.T) { { name: "Get all environments", action: "", resource: "environment", id: "", subresource: "", params: nil, - want: "https://shipyard.build/api/v1/environment", + want: "http://localhost:8080/api/v1/environment", }, { name: "Get all environments in a specific org", action: "", resource: "environment", id: "", subresource: "", params: map[string]string{"org": "myorg"}, - want: "https://shipyard.build/api/v1/environment?org=myorg", + want: "http://localhost:8080/api/v1/environment?org=myorg", }, { name: "Get all environments with filters applied", action: "", resource: "environment", id: "", subresource: "", params: map[string]string{"branch": "newfeature", "repo_name": "shipyard", "page_size": "9"}, - want: "https://shipyard.build/api/v1/environment?branch=newfeature&page_size=9&repo_name=shipyard", + want: "http://localhost:8080/api/v1/environment?branch=newfeature&page_size=9&repo_name=shipyard", }, { name: "Get a single environment", action: "", resource: "environment", id: "123abc", subresource: "", - want: "https://shipyard.build/api/v1/environment/123abc", + want: "http://localhost:8080/api/v1/environment/123abc", }, { name: "Get a single environment in a specific org", action: "", resource: "environment", id: "123abc", subresource: "", params: map[string]string{"org": "myorg"}, - want: "https://shipyard.build/api/v1/environment/123abc?org=myorg", + want: "http://localhost:8080/api/v1/environment/123abc?org=myorg", }, { name: "Get a kubeconfig for a single environment", action: "", resource: "environment", id: "123abc", subresource: "kubeconfig", params: nil, - want: "https://shipyard.build/api/v1/environment/123abc/kubeconfig", + want: "http://localhost:8080/api/v1/environment/123abc/kubeconfig", }, { name: "Get a kubeconfig for a single environment in a specific org", action: "", resource: "environment", id: "123abc", subresource: "kubeconfig", params: map[string]string{"org": "myorg"}, - want: "https://shipyard.build/api/v1/environment/123abc/kubeconfig?org=myorg", + want: "http://localhost:8080/api/v1/environment/123abc/kubeconfig?org=myorg", }, { name: "Stop a single environment", action: "stop", resource: "environment", id: "123abc", subresource: "", params: nil, - want: "https://shipyard.build/api/v1/environment/123abc/stop", + want: "http://localhost:8080/api/v1/environment/123abc/stop", }, { name: "Stop a single environment in a specific org", action: "stop", resource: "environment", id: "123abc", subresource: "", params: map[string]string{"org": "myorg"}, - want: "https://shipyard.build/api/v1/environment/123abc/stop?org=myorg", + want: "http://localhost:8080/api/v1/environment/123abc/stop?org=myorg", }, } @@ -77,9 +84,8 @@ func TestCreateResourceURI(t *testing.T) { } func TestCreateResourceURIWithCustomBase(t *testing.T) { - _ = os.Setenv("SHIPYARD_BUILD_URL", "localhost:8000") - - want := "localhost:8000/environment/123abc" + // Test with the base URL set in TestMain + want := "http://localhost:8080/api/v1/environment/123abc" got := uri.CreateResourceURI("", "environment", "123abc", "", nil) if got != want { t.Errorf("expected %s, but got %s", want, got)