From bf064ed5790768f8aba20638946fe25773ac9482 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 25 Feb 2026 22:42:40 +0000 Subject: [PATCH 1/3] fix: connection upsert fails when updating rules on connection with destination auth Fixes three bugs reported in #209: 1. Auth type sent without credentials: The `destination-cli-path` flag defaulted to "/" in the upsert command, making `hasAnyDestinationFlag()` always return true. This caused `buildDestinationInputForUpdate` to copy the entire existing destination config (including auth_type) even when no destination flags were provided, sending auth_type without credentials. Fix: Change the default to "" for upsert and add a destination ID preservation fallback. 2. Source name validation issue: `validateSourceFlags()` required both --source-name and --source-type even during upsert updates. For updates, providing just --source-name should be valid since the existing type can be preserved. Fix: Relax validation for upsert and fill in missing type from existing connection. 3. Status code format problem: `buildConnectionRules` sent `response_status_codes` as a single comma-separated string instead of parsing it into a string array, causing API rejection. Fix: Split the comma-separated string into a []string array. Note: Bug 3 fix affects all commands using buildConnectionRules (create, update, upsert) since the function is shared. https://claude.ai/code/session_01D8k5gyzeqzjZUanVrwv5dL --- pkg/cmd/connection_common.go | 6 +- pkg/cmd/connection_upsert.go | 41 ++- pkg/cmd/connection_upsert_bugs_test.go | 191 ++++++++++++ .../acceptance/connection_upsert_bugs_test.go | 284 ++++++++++++++++++ 4 files changed, 510 insertions(+), 12 deletions(-) create mode 100644 pkg/cmd/connection_upsert_bugs_test.go create mode 100644 test/acceptance/connection_upsert_bugs_test.go diff --git a/pkg/cmd/connection_common.go b/pkg/cmd/connection_common.go index b71fbf9..009bb05 100644 --- a/pkg/cmd/connection_common.go +++ b/pkg/cmd/connection_common.go @@ -191,7 +191,11 @@ func buildConnectionRules(f *connectionRuleFlags) ([]hookdeck.Rule, error) { rule["interval"] = f.RuleRetryInterval } if f.RuleRetryResponseStatusCode != "" { - rule["response_status_codes"] = f.RuleRetryResponseStatusCode + codes := strings.Split(f.RuleRetryResponseStatusCode, ",") + for i := range codes { + codes[i] = strings.TrimSpace(codes[i]) + } + rule["response_status_codes"] = codes } rules = append(rules, rule) } diff --git a/pkg/cmd/connection_upsert.go b/pkg/cmd/connection_upsert.go index 95ad92e..840124d 100644 --- a/pkg/cmd/connection_upsert.go +++ b/pkg/cmd/connection_upsert.go @@ -94,7 +94,7 @@ func newConnectionUpsertCmd() *connectionUpsertCmd { cu.cmd.Flags().StringVar(&cu.destinationType, "destination-type", "", "Destination type (CLI, HTTP, MOCK)") cu.cmd.Flags().StringVar(&cu.destinationDescription, "destination-description", "", "Destination description") cu.cmd.Flags().StringVar(&cu.destinationURL, "destination-url", "", "URL for HTTP destinations") - cu.cmd.Flags().StringVar(&cu.destinationCliPath, "destination-cli-path", "/", "CLI path for CLI destinations (default: /)") + cu.cmd.Flags().StringVar(&cu.destinationCliPath, "destination-cli-path", "", "CLI path for CLI destinations (default: / for new connections)") // Use a string flag to allow explicit true/false values var pathForwardingDisabledStr string @@ -257,10 +257,10 @@ func (cu *connectionUpsertCmd) validateSourceFlags() error { return fmt.Errorf("cannot use --source-id with --source-name or --source-type") } - // If creating inline, require both name and type - if (cu.sourceName != "" || cu.sourceType != "") && (cu.sourceName == "" || cu.sourceType == "") { - return fmt.Errorf("both --source-name and --source-type are required for inline source creation") - } + // For upsert, we don't require both --source-name and --source-type. + // If the connection already exists, providing just --source-name is valid + // (the existing source type will be preserved). The API will reject + // incomplete data if this is actually a create. return nil } @@ -272,10 +272,10 @@ func (cu *connectionUpsertCmd) validateDestinationFlags() error { return fmt.Errorf("cannot use --destination-id with --destination-name or --destination-type") } - // If creating inline, require both name and type - if (cu.destinationName != "" || cu.destinationType != "") && (cu.destinationName == "" || cu.destinationType == "") { - return fmt.Errorf("both --destination-name and --destination-type are required for inline destination creation") - } + // For upsert, we don't require both --destination-name and --destination-type. + // If the connection already exists, providing just --destination-name is valid + // (the existing destination type will be preserved). The API will reject + // incomplete data if this is actually a create. return nil } @@ -304,7 +304,11 @@ func (cu *connectionUpsertCmd) runConnectionUpsertCmd(cmd *cobra.Command, args [ cu.DestinationRateLimit != 0 || cu.DestinationAuthMethod != "") && cu.destinationName == "" && cu.destinationType == "" && cu.destinationID == "" - needsExisting := cu.dryRun || (!cu.hasAnySourceFlag() && !cu.hasAnyDestinationFlag()) || hasSourceConfigOnly || hasDestinationConfigOnly + // Also need to fetch existing when name is provided without type (to fill in the type) + hasPartialSourceInline := (cu.sourceName != "" && cu.sourceType == "" && cu.sourceID == "") + hasPartialDestinationInline := (cu.destinationName != "" && cu.destinationType == "" && cu.destinationID == "") + + needsExisting := cu.dryRun || (!cu.hasAnySourceFlag() && !cu.hasAnyDestinationFlag()) || hasSourceConfigOnly || hasDestinationConfigOnly || hasPartialSourceInline || hasPartialDestinationInline var existing *hookdeck.Connection var isUpdate bool @@ -411,6 +415,10 @@ func (cu *connectionUpsertCmd) buildUpsertRequest(existing *hookdeck.Connection, if cu.sourceID != "" { req.SourceID = &cu.sourceID } else if cu.sourceName != "" || cu.sourceType != "" { + // For upsert updates, fill in missing source type from existing connection + if cu.sourceType == "" && isUpdate && existing != nil && existing.Source != nil { + cu.sourceType = existing.Source.Type + } sourceInput, err := cu.buildSourceInput() if err != nil { return nil, err @@ -442,6 +450,14 @@ func (cu *connectionUpsertCmd) buildUpsertRequest(existing *hookdeck.Connection, if cu.destinationID != "" { req.DestinationID = &cu.destinationID } else if cu.destinationName != "" || cu.destinationType != "" { + // For upsert updates, fill in missing destination type from existing connection + if cu.destinationType == "" && isUpdate && existing != nil && existing.Destination != nil { + cu.destinationType = existing.Destination.Type + } + // Default CLI path to "/" for new CLI destinations when not explicitly set + if strings.ToUpper(cu.destinationType) == "CLI" && cu.destinationCliPath == "" { + cu.destinationCliPath = "/" + } destinationInput, err := cu.buildDestinationInput() if err != nil { return nil, err @@ -469,10 +485,13 @@ func (cu *connectionUpsertCmd) buildUpsertRequest(existing *hookdeck.Connection, } } - // Also preserve source if not specified + // Preserve existing source/destination if not specified if req.SourceID == nil && req.Source == nil && isUpdate && existing != nil && existing.Source != nil { req.SourceID = &existing.Source.ID } + if req.DestinationID == nil && req.Destination == nil && isUpdate && existing != nil && existing.Destination != nil { + req.DestinationID = &existing.Destination.ID + } // Handle Rules rules, err := buildConnectionRules(&cu.connectionCreateCmd.connectionRuleFlags) diff --git a/pkg/cmd/connection_upsert_bugs_test.go b/pkg/cmd/connection_upsert_bugs_test.go new file mode 100644 index 0000000..f76ffe4 --- /dev/null +++ b/pkg/cmd/connection_upsert_bugs_test.go @@ -0,0 +1,191 @@ +package cmd + +import ( + "encoding/json" + "testing" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestBug3_BuildConnectionRulesStatusCodesArray tests that buildConnectionRules +// produces response_status_codes as a []string array, not a single string. +// +// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3 +func TestBug3_BuildConnectionRulesStatusCodesArray(t *testing.T) { + tests := []struct { + name string + flags connectionRuleFlags + wantCodes []string + wantCodeCount int + wantRuleCount int + }{ + { + name: "comma-separated status codes should produce array", + flags: connectionRuleFlags{ + RuleRetryStrategy: "linear", + RuleRetryCount: 3, + RuleRetryInterval: 5000, + RuleRetryResponseStatusCode: "500,502,503,504", + }, + wantCodes: []string{"500", "502", "503", "504"}, + wantCodeCount: 4, + wantRuleCount: 1, + }, + { + name: "single status code should produce single-element array", + flags: connectionRuleFlags{ + RuleRetryStrategy: "exponential", + RuleRetryResponseStatusCode: "500", + }, + wantCodes: []string{"500"}, + wantCodeCount: 1, + wantRuleCount: 1, + }, + { + name: "no status codes should not include response_status_codes", + flags: connectionRuleFlags{ + RuleRetryStrategy: "linear", + RuleRetryCount: 3, + }, + wantCodes: nil, + wantCodeCount: 0, + wantRuleCount: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rules, err := buildConnectionRules(&tt.flags) + require.NoError(t, err, "buildConnectionRules should not error") + require.Len(t, rules, tt.wantRuleCount, "Expected %d rule(s)", tt.wantRuleCount) + + if tt.wantRuleCount == 0 { + return + } + + retryRule := rules[len(rules)-1] // retry rule is always last + assert.Equal(t, "retry", retryRule["type"], "Last rule should be retry") + + if tt.wantCodes == nil { + _, exists := retryRule["response_status_codes"] + assert.False(t, exists, "response_status_codes should not be present when not specified") + return + } + + statusCodes, ok := retryRule["response_status_codes"] + require.True(t, ok, "response_status_codes should be present") + + // The value should be a []string, not a string + codesSlice, ok := statusCodes.([]string) + require.True(t, ok, "response_status_codes should be []string, got %T", statusCodes) + assert.Equal(t, tt.wantCodeCount, len(codesSlice), "Expected %d status codes", tt.wantCodeCount) + assert.Equal(t, tt.wantCodes, codesSlice, "Status codes should match") + + // Also verify it serializes to a JSON array + jsonBytes, err := json.Marshal(retryRule) + require.NoError(t, err, "JSON marshal should not error") + + var parsed map[string]interface{} + err = json.Unmarshal(jsonBytes, &parsed) + require.NoError(t, err, "JSON unmarshal should not error") + + jsonCodes, ok := parsed["response_status_codes"].([]interface{}) + require.True(t, ok, "JSON response_status_codes should be an array, got %T", parsed["response_status_codes"]) + assert.Len(t, jsonCodes, tt.wantCodeCount, "JSON array should have %d elements", tt.wantCodeCount) + }) + } +} + +// TestBug1_UpsertBuildRequestNoDestinationFlags tests that when upserting with +// only rule flags, the request uses destination_id (not a full destination object +// with potentially incomplete auth config). +// +// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1 +func TestBug1_UpsertBuildRequestNoDestinationFlags(t *testing.T) { + cu := &connectionUpsertCmd{ + connectionCreateCmd: &connectionCreateCmd{}, + } + cu.name = "test-conn" + // Only set rule flags, no source/destination flags + cu.RuleRetryStrategy = "linear" + cu.RuleRetryCount = 3 + + // Simulate existing connection with destination that has auth + existing := &hookdeck.Connection{ + ID: "conn_123", + Name: strPtr("test-conn"), + Source: &hookdeck.Source{ + ID: "src_123", + Name: "test-source", + Type: "WEBHOOK", + }, + Destination: &hookdeck.Destination{ + ID: "dst_123", + Name: "test-dest", + Type: "HTTP", + Config: map[string]interface{}{ + "url": "https://api.example.com", + "auth_type": "AWS_SIGNATURE", + "auth": map[string]interface{}{ + "access_key_id": "AKIA...", + "secret_access_key": "...", + "region": "us-east-1", + "service": "execute-api", + }, + }, + }, + } + + req, err := cu.buildUpsertRequest(existing, true) + require.NoError(t, err, "buildUpsertRequest should not error") + + // The request should use DestinationID to reference the existing destination, + // NOT Destination with a full config (which could include auth_type without credentials) + assert.NotNil(t, req.DestinationID, "Should use DestinationID to reference existing destination") + assert.Equal(t, "dst_123", *req.DestinationID, "DestinationID should match existing") + assert.Nil(t, req.Destination, "Should NOT send full Destination object when no destination flags provided") + + // Source should also be preserved by ID + assert.NotNil(t, req.SourceID, "Should use SourceID to reference existing source") + assert.Equal(t, "src_123", *req.SourceID, "SourceID should match existing") + assert.Nil(t, req.Source, "Should NOT send full Source object when no source flags provided") + + // Rules should be present + assert.NotEmpty(t, req.Rules, "Rules should be present") +} + +// TestBug1_HasAnyDestinationFlagDefaultCliPath tests that hasAnyDestinationFlag +// does not return true just because destination-cli-path has its default value. +// +// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1 root cause +func TestBug1_HasAnyDestinationFlagDefaultCliPath(t *testing.T) { + cu := &connectionUpsertCmd{ + connectionCreateCmd: &connectionCreateCmd{}, + } + // Don't set any destination flags - leave defaults + // The CLI path default is "/" in the flag definition, but for upsert it should be "" + + result := cu.hasAnyDestinationFlag() + assert.False(t, result, "hasAnyDestinationFlag should return false when no destination flags are explicitly set") +} + +// TestBug2_ValidateSourceFlagsNameOnly tests that validateSourceFlags does NOT +// require --source-type when only --source-name is provided during upsert. +// +// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 2 +func TestBug2_ValidateSourceFlagsNameOnly(t *testing.T) { + cu := &connectionUpsertCmd{ + connectionCreateCmd: &connectionCreateCmd{}, + } + cu.sourceName = "my-source" + // sourceType is intentionally empty + + err := cu.validateSourceFlags() + assert.NoError(t, err, "validateSourceFlags should NOT require --source-type when only --source-name is provided during upsert") +} + +func strPtr(s string) *string { + return &s +} diff --git a/test/acceptance/connection_upsert_bugs_test.go b/test/acceptance/connection_upsert_bugs_test.go new file mode 100644 index 0000000..7d28235 --- /dev/null +++ b/test/acceptance/connection_upsert_bugs_test.go @@ -0,0 +1,284 @@ +package acceptance + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestConnectionUpsertBug1_AuthTypeSentWithoutCredentials tests that upserting a +// connection that has destination auth (e.g. bearer token) with ONLY rule flags +// does NOT send the auth_type without credentials, which would cause an API error. +// +// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1 +func TestConnectionUpsertBug1_AuthTypeSentWithoutCredentials(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + timestamp := generateTimestamp() + + connName := "test-upsert-auth-bug1-" + timestamp + sourceName := "test-upsert-src-bug1-" + timestamp + destName := "test-upsert-dst-bug1-" + timestamp + + // Step 1: Create a connection WITH destination authentication (bearer token) + var createResp map[string]interface{} + err := cli.RunJSON(&createResp, + "gateway", "connection", "create", + "--name", connName, + "--source-type", "WEBHOOK", + "--source-name", sourceName, + "--destination-type", "HTTP", + "--destination-name", destName, + "--destination-url", "https://api.example.com/webhook", + "--destination-auth-method", "bearer", + "--destination-bearer-token", "test_secret_token_123", + ) + require.NoError(t, err, "Should create connection with bearer auth") + + connID, ok := createResp["id"].(string) + require.True(t, ok && connID != "", "Expected connection ID in creation response") + + t.Cleanup(func() { + deleteConnection(t, cli, connID) + }) + + // Verify auth was set + dest, ok := createResp["destination"].(map[string]interface{}) + require.True(t, ok, "Expected destination object") + destConfig, ok := dest["config"].(map[string]interface{}) + require.True(t, ok, "Expected destination config") + assert.Equal(t, "BEARER_TOKEN", destConfig["auth_type"], "Auth type should be BEARER_TOKEN after creation") + + t.Logf("Created connection %s with bearer auth", connID) + + // Step 2: Upsert with ONLY rule flags (no source/destination flags) + // This is the bug scenario: the CLI should preserve the existing destination + // by referencing its ID, NOT by copying its config (which includes auth_type + // but not the actual credentials). + var upsertResp map[string]interface{} + err = cli.RunJSON(&upsertResp, + "gateway", "connection", "upsert", connName, + "--rule-retry-strategy", "linear", + "--rule-retry-count", "3", + "--rule-retry-interval", "5000", + ) + require.NoError(t, err, "Should upsert connection with only rule flags (bug: auth_type sent without credentials)") + + // Verify the connection was updated with the rules + rules, ok := upsertResp["rules"].([]interface{}) + require.True(t, ok, "Expected rules array in response") + + foundRetry := false + for _, r := range rules { + rule, ok := r.(map[string]interface{}) + if ok && rule["type"] == "retry" { + foundRetry = true + assert.Equal(t, "linear", rule["strategy"], "Retry strategy should be linear") + break + } + } + assert.True(t, foundRetry, "Should have a retry rule after upsert") + + // Verify auth was preserved + upsertDest, ok := upsertResp["destination"].(map[string]interface{}) + require.True(t, ok, "Expected destination in upsert response") + upsertDestConfig, ok := upsertDest["config"].(map[string]interface{}) + require.True(t, ok, "Expected destination config in upsert response") + assert.Equal(t, "BEARER_TOKEN", upsertDestConfig["auth_type"], "Auth type should still be BEARER_TOKEN after upsert") + + t.Logf("Successfully upserted connection %s with only rule flags, auth preserved", connID) +} + +// TestConnectionUpsertBug2_SourceNameWithoutType tests that upserting an existing +// connection with --source-name alone (without --source-type) works during updates. +// +// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 2 +func TestConnectionUpsertBug2_SourceNameWithoutType(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + timestamp := generateTimestamp() + + connName := "test-upsert-src-bug2-" + timestamp + sourceName := "test-upsert-srcname-bug2-" + timestamp + destName := "test-upsert-dstname-bug2-" + timestamp + newSourceName := "test-upsert-newsrc-bug2-" + timestamp + + // Step 1: Create a connection + var createResp map[string]interface{} + err := cli.RunJSON(&createResp, + "gateway", "connection", "create", + "--name", connName, + "--source-type", "WEBHOOK", + "--source-name", sourceName, + "--destination-type", "HTTP", + "--destination-name", destName, + "--destination-url", "https://api.example.com/webhook", + ) + require.NoError(t, err, "Should create connection") + + connID, ok := createResp["id"].(string) + require.True(t, ok && connID != "", "Expected connection ID") + + t.Cleanup(func() { + deleteConnection(t, cli, connID) + }) + + t.Logf("Created connection %s", connID) + + // Step 2: Upsert with --source-name only (no --source-type) + // Bug: CLI requires both --source-name and --source-type, even for updates + var upsertResp map[string]interface{} + err = cli.RunJSON(&upsertResp, + "gateway", "connection", "upsert", connName, + "--source-name", newSourceName, + "--source-type", "WEBHOOK", + ) + require.NoError(t, err, "Should upsert connection with --source-name and --source-type") + + // Verify the source was updated + upsertSource, ok := upsertResp["source"].(map[string]interface{}) + require.True(t, ok, "Expected source in upsert response") + assert.Equal(t, newSourceName, upsertSource["name"], "Source name should be updated") + + t.Logf("Successfully upserted connection %s with new source name", connID) +} + +// TestConnectionUpsertBug3_RetryResponseStatusCodesAsArray tests that +// --rule-retry-response-status-codes is sent as an array (not a string) to the API. +// +// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3 +func TestConnectionUpsertBug3_RetryResponseStatusCodesAsArray(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + timestamp := generateTimestamp() + + connName := "test-upsert-status-bug3-" + timestamp + sourceName := "test-upsert-src-bug3-" + timestamp + destName := "test-upsert-dst-bug3-" + timestamp + + // Step 1: Create a connection + var createResp map[string]interface{} + err := cli.RunJSON(&createResp, + "gateway", "connection", "create", + "--name", connName, + "--source-type", "WEBHOOK", + "--source-name", sourceName, + "--destination-type", "HTTP", + "--destination-name", destName, + "--destination-url", "https://api.example.com/webhook", + ) + require.NoError(t, err, "Should create connection") + + connID, ok := createResp["id"].(string) + require.True(t, ok && connID != "", "Expected connection ID") + + t.Cleanup(func() { + deleteConnection(t, cli, connID) + }) + + t.Logf("Created connection %s", connID) + + // Step 2: Upsert with retry rule that includes response status codes + // Bug: CLI sends "500,502,503,504" as a string instead of ["500","502","503","504"] array + var upsertResp map[string]interface{} + err = cli.RunJSON(&upsertResp, + "gateway", "connection", "upsert", connName, + "--source-name", sourceName, + "--source-type", "WEBHOOK", + "--destination-name", destName, + "--destination-type", "HTTP", + "--destination-url", "https://api.example.com/webhook", + "--rule-retry-strategy", "linear", + "--rule-retry-count", "3", + "--rule-retry-interval", "5000", + "--rule-retry-response-status-codes", "500,502,503,504", + ) + require.NoError(t, err, "Should upsert connection with retry response status codes (bug: sent as string instead of array)") + + // Verify the rules + rules, ok := upsertResp["rules"].([]interface{}) + require.True(t, ok, "Expected rules array in response") + + foundRetry := false + for _, r := range rules { + rule, ok := r.(map[string]interface{}) + if ok && rule["type"] == "retry" { + foundRetry = true + + // Verify response_status_codes is an array + statusCodes, ok := rule["response_status_codes"].([]interface{}) + require.True(t, ok, "response_status_codes should be an array, got: %T (%v)", rule["response_status_codes"], rule["response_status_codes"]) + assert.Len(t, statusCodes, 4, "Should have 4 status codes") + + // Check the actual values (they could be strings or numbers depending on API) + codes := make([]string, len(statusCodes)) + for i, c := range statusCodes { + codes[i] = strings.TrimSpace(strings.Replace(strings.Replace(strings.Replace(c.(string), " ", "", -1), "\t", "", -1), "\n", "", -1)) + } + assert.Contains(t, codes, "500") + assert.Contains(t, codes, "502") + assert.Contains(t, codes, "503") + assert.Contains(t, codes, "504") + break + } + } + assert.True(t, foundRetry, "Should have a retry rule after upsert") + + t.Logf("Successfully upserted connection %s with retry status codes as array", connID) +} + +// TestConnectionUpsertBug3_RetryStatusCodesViaUpdate tests the same bug via the +// update command path, since buildConnectionRules is shared. +func TestConnectionUpsertBug3_RetryStatusCodesViaUpdate(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + + connID := createTestConnection(t, cli) + require.NotEmpty(t, connID, "Connection ID should not be empty") + + t.Cleanup(func() { + deleteConnection(t, cli, connID) + }) + + // Update with retry rule that includes response status codes + var updated Connection + err := cli.RunJSON(&updated, + "gateway", "connection", "update", connID, + "--rule-retry-strategy", "linear", + "--rule-retry-count", "3", + "--rule-retry-interval", "5000", + "--rule-retry-response-status-codes", "500,502,503", + ) + require.NoError(t, err, "Should update connection with retry response status codes") + require.NotEmpty(t, updated.Rules, "Connection should have rules") + + // Find the retry rule and verify status codes are an array + foundRetry := false + for _, rule := range updated.Rules { + if rule["type"] == "retry" { + foundRetry = true + + statusCodes, ok := rule["response_status_codes"].([]interface{}) + require.True(t, ok, "response_status_codes should be an array, got: %T (%v)", rule["response_status_codes"], rule["response_status_codes"]) + assert.Len(t, statusCodes, 3, "Should have 3 status codes") + break + } + } + assert.True(t, foundRetry, "Should have a retry rule") + + t.Logf("Successfully verified retry status codes are sent as array via update command") +} From 66b26358eb0b0445d75cea48163a0513b4ab90f5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 25 Feb 2026 22:50:13 +0000 Subject: [PATCH 2/3] refactor: merge bug tests into existing test files with descriptive names Move acceptance tests from separate connection_upsert_bugs_test.go into the existing connection_upsert_test.go (as subtests of TestConnectionUpsertPartialUpdates) and connection_update_test.go. Rename unit tests from connection_upsert_bugs_test.go to connection_upsert_test.go with descriptive test function names. https://claude.ai/code/session_01D8k5gyzeqzjZUanVrwv5dL --- pkg/cmd/connection_upsert_bugs_test.go | 191 ------------ pkg/cmd/connection_upsert_test.go | 239 +++++++++++++++ test/acceptance/connection_update_test.go | 46 +++ .../acceptance/connection_upsert_bugs_test.go | 284 ------------------ test/acceptance/connection_upsert_test.go | 200 ++++++++++++ 5 files changed, 485 insertions(+), 475 deletions(-) delete mode 100644 pkg/cmd/connection_upsert_bugs_test.go create mode 100644 pkg/cmd/connection_upsert_test.go delete mode 100644 test/acceptance/connection_upsert_bugs_test.go diff --git a/pkg/cmd/connection_upsert_bugs_test.go b/pkg/cmd/connection_upsert_bugs_test.go deleted file mode 100644 index f76ffe4..0000000 --- a/pkg/cmd/connection_upsert_bugs_test.go +++ /dev/null @@ -1,191 +0,0 @@ -package cmd - -import ( - "encoding/json" - "testing" - - "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestBug3_BuildConnectionRulesStatusCodesArray tests that buildConnectionRules -// produces response_status_codes as a []string array, not a single string. -// -// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3 -func TestBug3_BuildConnectionRulesStatusCodesArray(t *testing.T) { - tests := []struct { - name string - flags connectionRuleFlags - wantCodes []string - wantCodeCount int - wantRuleCount int - }{ - { - name: "comma-separated status codes should produce array", - flags: connectionRuleFlags{ - RuleRetryStrategy: "linear", - RuleRetryCount: 3, - RuleRetryInterval: 5000, - RuleRetryResponseStatusCode: "500,502,503,504", - }, - wantCodes: []string{"500", "502", "503", "504"}, - wantCodeCount: 4, - wantRuleCount: 1, - }, - { - name: "single status code should produce single-element array", - flags: connectionRuleFlags{ - RuleRetryStrategy: "exponential", - RuleRetryResponseStatusCode: "500", - }, - wantCodes: []string{"500"}, - wantCodeCount: 1, - wantRuleCount: 1, - }, - { - name: "no status codes should not include response_status_codes", - flags: connectionRuleFlags{ - RuleRetryStrategy: "linear", - RuleRetryCount: 3, - }, - wantCodes: nil, - wantCodeCount: 0, - wantRuleCount: 1, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - rules, err := buildConnectionRules(&tt.flags) - require.NoError(t, err, "buildConnectionRules should not error") - require.Len(t, rules, tt.wantRuleCount, "Expected %d rule(s)", tt.wantRuleCount) - - if tt.wantRuleCount == 0 { - return - } - - retryRule := rules[len(rules)-1] // retry rule is always last - assert.Equal(t, "retry", retryRule["type"], "Last rule should be retry") - - if tt.wantCodes == nil { - _, exists := retryRule["response_status_codes"] - assert.False(t, exists, "response_status_codes should not be present when not specified") - return - } - - statusCodes, ok := retryRule["response_status_codes"] - require.True(t, ok, "response_status_codes should be present") - - // The value should be a []string, not a string - codesSlice, ok := statusCodes.([]string) - require.True(t, ok, "response_status_codes should be []string, got %T", statusCodes) - assert.Equal(t, tt.wantCodeCount, len(codesSlice), "Expected %d status codes", tt.wantCodeCount) - assert.Equal(t, tt.wantCodes, codesSlice, "Status codes should match") - - // Also verify it serializes to a JSON array - jsonBytes, err := json.Marshal(retryRule) - require.NoError(t, err, "JSON marshal should not error") - - var parsed map[string]interface{} - err = json.Unmarshal(jsonBytes, &parsed) - require.NoError(t, err, "JSON unmarshal should not error") - - jsonCodes, ok := parsed["response_status_codes"].([]interface{}) - require.True(t, ok, "JSON response_status_codes should be an array, got %T", parsed["response_status_codes"]) - assert.Len(t, jsonCodes, tt.wantCodeCount, "JSON array should have %d elements", tt.wantCodeCount) - }) - } -} - -// TestBug1_UpsertBuildRequestNoDestinationFlags tests that when upserting with -// only rule flags, the request uses destination_id (not a full destination object -// with potentially incomplete auth config). -// -// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1 -func TestBug1_UpsertBuildRequestNoDestinationFlags(t *testing.T) { - cu := &connectionUpsertCmd{ - connectionCreateCmd: &connectionCreateCmd{}, - } - cu.name = "test-conn" - // Only set rule flags, no source/destination flags - cu.RuleRetryStrategy = "linear" - cu.RuleRetryCount = 3 - - // Simulate existing connection with destination that has auth - existing := &hookdeck.Connection{ - ID: "conn_123", - Name: strPtr("test-conn"), - Source: &hookdeck.Source{ - ID: "src_123", - Name: "test-source", - Type: "WEBHOOK", - }, - Destination: &hookdeck.Destination{ - ID: "dst_123", - Name: "test-dest", - Type: "HTTP", - Config: map[string]interface{}{ - "url": "https://api.example.com", - "auth_type": "AWS_SIGNATURE", - "auth": map[string]interface{}{ - "access_key_id": "AKIA...", - "secret_access_key": "...", - "region": "us-east-1", - "service": "execute-api", - }, - }, - }, - } - - req, err := cu.buildUpsertRequest(existing, true) - require.NoError(t, err, "buildUpsertRequest should not error") - - // The request should use DestinationID to reference the existing destination, - // NOT Destination with a full config (which could include auth_type without credentials) - assert.NotNil(t, req.DestinationID, "Should use DestinationID to reference existing destination") - assert.Equal(t, "dst_123", *req.DestinationID, "DestinationID should match existing") - assert.Nil(t, req.Destination, "Should NOT send full Destination object when no destination flags provided") - - // Source should also be preserved by ID - assert.NotNil(t, req.SourceID, "Should use SourceID to reference existing source") - assert.Equal(t, "src_123", *req.SourceID, "SourceID should match existing") - assert.Nil(t, req.Source, "Should NOT send full Source object when no source flags provided") - - // Rules should be present - assert.NotEmpty(t, req.Rules, "Rules should be present") -} - -// TestBug1_HasAnyDestinationFlagDefaultCliPath tests that hasAnyDestinationFlag -// does not return true just because destination-cli-path has its default value. -// -// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1 root cause -func TestBug1_HasAnyDestinationFlagDefaultCliPath(t *testing.T) { - cu := &connectionUpsertCmd{ - connectionCreateCmd: &connectionCreateCmd{}, - } - // Don't set any destination flags - leave defaults - // The CLI path default is "/" in the flag definition, but for upsert it should be "" - - result := cu.hasAnyDestinationFlag() - assert.False(t, result, "hasAnyDestinationFlag should return false when no destination flags are explicitly set") -} - -// TestBug2_ValidateSourceFlagsNameOnly tests that validateSourceFlags does NOT -// require --source-type when only --source-name is provided during upsert. -// -// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 2 -func TestBug2_ValidateSourceFlagsNameOnly(t *testing.T) { - cu := &connectionUpsertCmd{ - connectionCreateCmd: &connectionCreateCmd{}, - } - cu.sourceName = "my-source" - // sourceType is intentionally empty - - err := cu.validateSourceFlags() - assert.NoError(t, err, "validateSourceFlags should NOT require --source-type when only --source-name is provided during upsert") -} - -func strPtr(s string) *string { - return &s -} diff --git a/pkg/cmd/connection_upsert_test.go b/pkg/cmd/connection_upsert_test.go new file mode 100644 index 0000000..5a6490f --- /dev/null +++ b/pkg/cmd/connection_upsert_test.go @@ -0,0 +1,239 @@ +package cmd + +import ( + "encoding/json" + "testing" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func strPtr(s string) *string { + return &s +} + +// TestBuildConnectionRulesRetryStatusCodesArray verifies that buildConnectionRules +// produces response_status_codes as a []string array, not a single string. +// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3. +func TestBuildConnectionRulesRetryStatusCodesArray(t *testing.T) { + tests := []struct { + name string + flags connectionRuleFlags + wantCodes []string + wantCodeCount int + wantRuleCount int + }{ + { + name: "comma-separated status codes should produce array", + flags: connectionRuleFlags{ + RuleRetryStrategy: "linear", + RuleRetryCount: 3, + RuleRetryInterval: 5000, + RuleRetryResponseStatusCode: "500,502,503,504", + }, + wantCodes: []string{"500", "502", "503", "504"}, + wantCodeCount: 4, + wantRuleCount: 1, + }, + { + name: "single status code should produce single-element array", + flags: connectionRuleFlags{ + RuleRetryStrategy: "exponential", + RuleRetryResponseStatusCode: "500", + }, + wantCodes: []string{"500"}, + wantCodeCount: 1, + wantRuleCount: 1, + }, + { + name: "status codes with spaces should be trimmed", + flags: connectionRuleFlags{ + RuleRetryStrategy: "linear", + RuleRetryResponseStatusCode: "500, 502, 503", + }, + wantCodes: []string{"500", "502", "503"}, + wantCodeCount: 3, + wantRuleCount: 1, + }, + { + name: "no status codes should not include response_status_codes", + flags: connectionRuleFlags{ + RuleRetryStrategy: "linear", + RuleRetryCount: 3, + }, + wantCodes: nil, + wantCodeCount: 0, + wantRuleCount: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rules, err := buildConnectionRules(&tt.flags) + require.NoError(t, err, "buildConnectionRules should not error") + require.Len(t, rules, tt.wantRuleCount, "Expected %d rule(s)", tt.wantRuleCount) + + if tt.wantRuleCount == 0 { + return + } + + retryRule := rules[len(rules)-1] + assert.Equal(t, "retry", retryRule["type"], "Last rule should be retry") + + if tt.wantCodes == nil { + _, exists := retryRule["response_status_codes"] + assert.False(t, exists, "response_status_codes should not be present when not specified") + return + } + + statusCodes, ok := retryRule["response_status_codes"] + require.True(t, ok, "response_status_codes should be present") + + codesSlice, ok := statusCodes.([]string) + require.True(t, ok, "response_status_codes should be []string, got %T", statusCodes) + assert.Equal(t, tt.wantCodeCount, len(codesSlice)) + assert.Equal(t, tt.wantCodes, codesSlice) + + // Verify it serializes to a JSON array + jsonBytes, err := json.Marshal(retryRule) + require.NoError(t, err) + + var parsed map[string]interface{} + err = json.Unmarshal(jsonBytes, &parsed) + require.NoError(t, err) + + jsonCodes, ok := parsed["response_status_codes"].([]interface{}) + require.True(t, ok, "JSON response_status_codes should be an array, got %T", parsed["response_status_codes"]) + assert.Len(t, jsonCodes, tt.wantCodeCount) + }) + } +} + +// TestUpsertBuildRequestRulesOnlyPreservesDestinationByID verifies that when +// upserting with only rule flags, the request uses destination_id (not a full +// destination object that could include incomplete auth config). +// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1. +func TestUpsertBuildRequestRulesOnlyPreservesDestinationByID(t *testing.T) { + cu := &connectionUpsertCmd{ + connectionCreateCmd: &connectionCreateCmd{}, + } + cu.name = "test-conn" + // Only set rule flags, no source/destination flags + cu.RuleRetryStrategy = "linear" + cu.RuleRetryCount = 3 + + existing := &hookdeck.Connection{ + ID: "conn_123", + Name: strPtr("test-conn"), + Source: &hookdeck.Source{ + ID: "src_123", + Name: "test-source", + Type: "WEBHOOK", + }, + Destination: &hookdeck.Destination{ + ID: "dst_123", + Name: "test-dest", + Type: "HTTP", + Config: map[string]interface{}{ + "url": "https://api.example.com", + "auth_type": "AWS_SIGNATURE", + }, + }, + } + + req, err := cu.buildUpsertRequest(existing, true) + require.NoError(t, err) + + // Should reference existing destination by ID, not recreate it + assert.NotNil(t, req.DestinationID, "Should use DestinationID") + assert.Equal(t, "dst_123", *req.DestinationID) + assert.Nil(t, req.Destination, "Should NOT send full Destination object") + + // Source should also be preserved by ID + assert.NotNil(t, req.SourceID, "Should use SourceID") + assert.Equal(t, "src_123", *req.SourceID) + assert.Nil(t, req.Source, "Should NOT send full Source object") + + // Rules should be present + assert.NotEmpty(t, req.Rules) +} + +// TestUpsertHasAnyDestinationFlagIgnoresDefault verifies that hasAnyDestinationFlag +// returns false when no destination flags are explicitly set (the cli-path default +// of "/" was previously causing this to always return true). +// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1. +func TestUpsertHasAnyDestinationFlagIgnoresDefault(t *testing.T) { + cu := &connectionUpsertCmd{ + connectionCreateCmd: &connectionCreateCmd{}, + } + // No destination flags set at all (cli-path default is "" for upsert) + + result := cu.hasAnyDestinationFlag() + assert.False(t, result, "hasAnyDestinationFlag should be false with no destination flags set") +} + +// TestUpsertValidateSourceFlagsAllowsNameOnly verifies that validateSourceFlags +// allows --source-name without --source-type for the upsert command. +// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 2. +func TestUpsertValidateSourceFlagsAllowsNameOnly(t *testing.T) { + cu := &connectionUpsertCmd{ + connectionCreateCmd: &connectionCreateCmd{}, + } + cu.sourceName = "my-source" + // sourceType intentionally empty + + err := cu.validateSourceFlags() + assert.NoError(t, err, "validateSourceFlags should allow --source-name alone for upsert") +} + +// TestUpsertValidateDestinationFlagsAllowsNameOnly verifies the same relaxation +// for destination flags. +func TestUpsertValidateDestinationFlagsAllowsNameOnly(t *testing.T) { + cu := &connectionUpsertCmd{ + connectionCreateCmd: &connectionCreateCmd{}, + } + cu.destinationName = "my-dest" + // destinationType intentionally empty + + err := cu.validateDestinationFlags() + assert.NoError(t, err, "validateDestinationFlags should allow --destination-name alone for upsert") +} + +// TestUpsertBuildRequestFillsSourceTypeFromExisting verifies that when +// --source-name is provided without --source-type during an update, +// the existing source type is used. +func TestUpsertBuildRequestFillsSourceTypeFromExisting(t *testing.T) { + cu := &connectionUpsertCmd{ + connectionCreateCmd: &connectionCreateCmd{}, + } + cu.name = "test-conn" + cu.sourceName = "new-source-name" + // sourceType intentionally empty - should be filled from existing + + existing := &hookdeck.Connection{ + ID: "conn_123", + Name: strPtr("test-conn"), + Source: &hookdeck.Source{ + ID: "src_123", + Name: "old-source-name", + Type: "WEBHOOK", + }, + Destination: &hookdeck.Destination{ + ID: "dst_123", + Name: "test-dest", + Type: "HTTP", + Config: map[string]interface{}{ + "url": "https://api.example.com", + }, + }, + } + + req, err := cu.buildUpsertRequest(existing, true) + require.NoError(t, err) + + // Source should be an inline input (not just ID) since name was changed + require.NotNil(t, req.Source, "Should have Source input for name change") + assert.Equal(t, "new-source-name", req.Source.Name) + assert.Equal(t, "WEBHOOK", req.Source.Type, "Should fill type from existing source") +} diff --git a/test/acceptance/connection_update_test.go b/test/acceptance/connection_update_test.go index 9d97418..aebeb33 100644 --- a/test/acceptance/connection_update_test.go +++ b/test/acceptance/connection_update_test.go @@ -316,6 +316,52 @@ func TestConnectionUpdateDelayRule(t *testing.T) { t.Logf("Connection update with delay rule verified: %s", connID) } +// TestConnectionUpdateRetryResponseStatusCodes verifies that +// --rule-retry-response-status-codes is sent as an array to the API. +// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3. +func TestConnectionUpdateRetryResponseStatusCodes(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + + connID := createTestConnection(t, cli) + require.NotEmpty(t, connID, "Connection ID should not be empty") + + t.Cleanup(func() { + deleteConnection(t, cli, connID) + }) + + // Update with retry rule including response status codes + var updated Connection + err := cli.RunJSON(&updated, + "gateway", "connection", "update", connID, + "--rule-retry-strategy", "linear", + "--rule-retry-count", "3", + "--rule-retry-interval", "5000", + "--rule-retry-response-status-codes", "500,502,503", + ) + require.NoError(t, err, "Should update connection with retry response status codes") + require.NotEmpty(t, updated.Rules, "Connection should have rules") + + // Find the retry rule and verify status codes are an array + foundRetry := false + for _, rule := range updated.Rules { + if rule["type"] == "retry" { + foundRetry = true + + statusCodes, ok := rule["response_status_codes"].([]interface{}) + require.True(t, ok, "response_status_codes should be an array, got: %T (%v)", rule["response_status_codes"], rule["response_status_codes"]) + assert.Len(t, statusCodes, 3, "Should have 3 status codes") + break + } + } + assert.True(t, foundRetry, "Should have a retry rule") + + t.Logf("Successfully verified retry status codes are sent as array via update: %s", connID) +} + // TestConnectionUpdateWithRulesJSON verifies update with --rules JSON string func TestConnectionUpdateWithRulesJSON(t *testing.T) { if testing.Short() { diff --git a/test/acceptance/connection_upsert_bugs_test.go b/test/acceptance/connection_upsert_bugs_test.go deleted file mode 100644 index 7d28235..0000000 --- a/test/acceptance/connection_upsert_bugs_test.go +++ /dev/null @@ -1,284 +0,0 @@ -package acceptance - -import ( - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestConnectionUpsertBug1_AuthTypeSentWithoutCredentials tests that upserting a -// connection that has destination auth (e.g. bearer token) with ONLY rule flags -// does NOT send the auth_type without credentials, which would cause an API error. -// -// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1 -func TestConnectionUpsertBug1_AuthTypeSentWithoutCredentials(t *testing.T) { - if testing.Short() { - t.Skip("Skipping acceptance test in short mode") - } - - cli := NewCLIRunner(t) - timestamp := generateTimestamp() - - connName := "test-upsert-auth-bug1-" + timestamp - sourceName := "test-upsert-src-bug1-" + timestamp - destName := "test-upsert-dst-bug1-" + timestamp - - // Step 1: Create a connection WITH destination authentication (bearer token) - var createResp map[string]interface{} - err := cli.RunJSON(&createResp, - "gateway", "connection", "create", - "--name", connName, - "--source-type", "WEBHOOK", - "--source-name", sourceName, - "--destination-type", "HTTP", - "--destination-name", destName, - "--destination-url", "https://api.example.com/webhook", - "--destination-auth-method", "bearer", - "--destination-bearer-token", "test_secret_token_123", - ) - require.NoError(t, err, "Should create connection with bearer auth") - - connID, ok := createResp["id"].(string) - require.True(t, ok && connID != "", "Expected connection ID in creation response") - - t.Cleanup(func() { - deleteConnection(t, cli, connID) - }) - - // Verify auth was set - dest, ok := createResp["destination"].(map[string]interface{}) - require.True(t, ok, "Expected destination object") - destConfig, ok := dest["config"].(map[string]interface{}) - require.True(t, ok, "Expected destination config") - assert.Equal(t, "BEARER_TOKEN", destConfig["auth_type"], "Auth type should be BEARER_TOKEN after creation") - - t.Logf("Created connection %s with bearer auth", connID) - - // Step 2: Upsert with ONLY rule flags (no source/destination flags) - // This is the bug scenario: the CLI should preserve the existing destination - // by referencing its ID, NOT by copying its config (which includes auth_type - // but not the actual credentials). - var upsertResp map[string]interface{} - err = cli.RunJSON(&upsertResp, - "gateway", "connection", "upsert", connName, - "--rule-retry-strategy", "linear", - "--rule-retry-count", "3", - "--rule-retry-interval", "5000", - ) - require.NoError(t, err, "Should upsert connection with only rule flags (bug: auth_type sent without credentials)") - - // Verify the connection was updated with the rules - rules, ok := upsertResp["rules"].([]interface{}) - require.True(t, ok, "Expected rules array in response") - - foundRetry := false - for _, r := range rules { - rule, ok := r.(map[string]interface{}) - if ok && rule["type"] == "retry" { - foundRetry = true - assert.Equal(t, "linear", rule["strategy"], "Retry strategy should be linear") - break - } - } - assert.True(t, foundRetry, "Should have a retry rule after upsert") - - // Verify auth was preserved - upsertDest, ok := upsertResp["destination"].(map[string]interface{}) - require.True(t, ok, "Expected destination in upsert response") - upsertDestConfig, ok := upsertDest["config"].(map[string]interface{}) - require.True(t, ok, "Expected destination config in upsert response") - assert.Equal(t, "BEARER_TOKEN", upsertDestConfig["auth_type"], "Auth type should still be BEARER_TOKEN after upsert") - - t.Logf("Successfully upserted connection %s with only rule flags, auth preserved", connID) -} - -// TestConnectionUpsertBug2_SourceNameWithoutType tests that upserting an existing -// connection with --source-name alone (without --source-type) works during updates. -// -// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 2 -func TestConnectionUpsertBug2_SourceNameWithoutType(t *testing.T) { - if testing.Short() { - t.Skip("Skipping acceptance test in short mode") - } - - cli := NewCLIRunner(t) - timestamp := generateTimestamp() - - connName := "test-upsert-src-bug2-" + timestamp - sourceName := "test-upsert-srcname-bug2-" + timestamp - destName := "test-upsert-dstname-bug2-" + timestamp - newSourceName := "test-upsert-newsrc-bug2-" + timestamp - - // Step 1: Create a connection - var createResp map[string]interface{} - err := cli.RunJSON(&createResp, - "gateway", "connection", "create", - "--name", connName, - "--source-type", "WEBHOOK", - "--source-name", sourceName, - "--destination-type", "HTTP", - "--destination-name", destName, - "--destination-url", "https://api.example.com/webhook", - ) - require.NoError(t, err, "Should create connection") - - connID, ok := createResp["id"].(string) - require.True(t, ok && connID != "", "Expected connection ID") - - t.Cleanup(func() { - deleteConnection(t, cli, connID) - }) - - t.Logf("Created connection %s", connID) - - // Step 2: Upsert with --source-name only (no --source-type) - // Bug: CLI requires both --source-name and --source-type, even for updates - var upsertResp map[string]interface{} - err = cli.RunJSON(&upsertResp, - "gateway", "connection", "upsert", connName, - "--source-name", newSourceName, - "--source-type", "WEBHOOK", - ) - require.NoError(t, err, "Should upsert connection with --source-name and --source-type") - - // Verify the source was updated - upsertSource, ok := upsertResp["source"].(map[string]interface{}) - require.True(t, ok, "Expected source in upsert response") - assert.Equal(t, newSourceName, upsertSource["name"], "Source name should be updated") - - t.Logf("Successfully upserted connection %s with new source name", connID) -} - -// TestConnectionUpsertBug3_RetryResponseStatusCodesAsArray tests that -// --rule-retry-response-status-codes is sent as an array (not a string) to the API. -// -// Reproduces: https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3 -func TestConnectionUpsertBug3_RetryResponseStatusCodesAsArray(t *testing.T) { - if testing.Short() { - t.Skip("Skipping acceptance test in short mode") - } - - cli := NewCLIRunner(t) - timestamp := generateTimestamp() - - connName := "test-upsert-status-bug3-" + timestamp - sourceName := "test-upsert-src-bug3-" + timestamp - destName := "test-upsert-dst-bug3-" + timestamp - - // Step 1: Create a connection - var createResp map[string]interface{} - err := cli.RunJSON(&createResp, - "gateway", "connection", "create", - "--name", connName, - "--source-type", "WEBHOOK", - "--source-name", sourceName, - "--destination-type", "HTTP", - "--destination-name", destName, - "--destination-url", "https://api.example.com/webhook", - ) - require.NoError(t, err, "Should create connection") - - connID, ok := createResp["id"].(string) - require.True(t, ok && connID != "", "Expected connection ID") - - t.Cleanup(func() { - deleteConnection(t, cli, connID) - }) - - t.Logf("Created connection %s", connID) - - // Step 2: Upsert with retry rule that includes response status codes - // Bug: CLI sends "500,502,503,504" as a string instead of ["500","502","503","504"] array - var upsertResp map[string]interface{} - err = cli.RunJSON(&upsertResp, - "gateway", "connection", "upsert", connName, - "--source-name", sourceName, - "--source-type", "WEBHOOK", - "--destination-name", destName, - "--destination-type", "HTTP", - "--destination-url", "https://api.example.com/webhook", - "--rule-retry-strategy", "linear", - "--rule-retry-count", "3", - "--rule-retry-interval", "5000", - "--rule-retry-response-status-codes", "500,502,503,504", - ) - require.NoError(t, err, "Should upsert connection with retry response status codes (bug: sent as string instead of array)") - - // Verify the rules - rules, ok := upsertResp["rules"].([]interface{}) - require.True(t, ok, "Expected rules array in response") - - foundRetry := false - for _, r := range rules { - rule, ok := r.(map[string]interface{}) - if ok && rule["type"] == "retry" { - foundRetry = true - - // Verify response_status_codes is an array - statusCodes, ok := rule["response_status_codes"].([]interface{}) - require.True(t, ok, "response_status_codes should be an array, got: %T (%v)", rule["response_status_codes"], rule["response_status_codes"]) - assert.Len(t, statusCodes, 4, "Should have 4 status codes") - - // Check the actual values (they could be strings or numbers depending on API) - codes := make([]string, len(statusCodes)) - for i, c := range statusCodes { - codes[i] = strings.TrimSpace(strings.Replace(strings.Replace(strings.Replace(c.(string), " ", "", -1), "\t", "", -1), "\n", "", -1)) - } - assert.Contains(t, codes, "500") - assert.Contains(t, codes, "502") - assert.Contains(t, codes, "503") - assert.Contains(t, codes, "504") - break - } - } - assert.True(t, foundRetry, "Should have a retry rule after upsert") - - t.Logf("Successfully upserted connection %s with retry status codes as array", connID) -} - -// TestConnectionUpsertBug3_RetryStatusCodesViaUpdate tests the same bug via the -// update command path, since buildConnectionRules is shared. -func TestConnectionUpsertBug3_RetryStatusCodesViaUpdate(t *testing.T) { - if testing.Short() { - t.Skip("Skipping acceptance test in short mode") - } - - cli := NewCLIRunner(t) - - connID := createTestConnection(t, cli) - require.NotEmpty(t, connID, "Connection ID should not be empty") - - t.Cleanup(func() { - deleteConnection(t, cli, connID) - }) - - // Update with retry rule that includes response status codes - var updated Connection - err := cli.RunJSON(&updated, - "gateway", "connection", "update", connID, - "--rule-retry-strategy", "linear", - "--rule-retry-count", "3", - "--rule-retry-interval", "5000", - "--rule-retry-response-status-codes", "500,502,503", - ) - require.NoError(t, err, "Should update connection with retry response status codes") - require.NotEmpty(t, updated.Rules, "Connection should have rules") - - // Find the retry rule and verify status codes are an array - foundRetry := false - for _, rule := range updated.Rules { - if rule["type"] == "retry" { - foundRetry = true - - statusCodes, ok := rule["response_status_codes"].([]interface{}) - require.True(t, ok, "response_status_codes should be an array, got: %T (%v)", rule["response_status_codes"], rule["response_status_codes"]) - assert.Len(t, statusCodes, 3, "Should have 3 status codes") - break - } - } - assert.True(t, foundRetry, "Should have a retry rule") - - t.Logf("Successfully verified retry status codes are sent as array via update command") -} diff --git a/test/acceptance/connection_upsert_test.go b/test/acceptance/connection_upsert_test.go index 3a68c2e..fc6702f 100644 --- a/test/acceptance/connection_upsert_test.go +++ b/test/acceptance/connection_upsert_test.go @@ -1,6 +1,7 @@ package acceptance import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -241,4 +242,203 @@ func TestConnectionUpsertPartialUpdates(t *testing.T) { t.Logf("Successfully updated connection %s source config", connID) }) + + // Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1: + // Upserting with only rule flags on a connection with destination auth should NOT + // send auth_type without credentials. + t.Run("UpsertRulesOnlyPreservesDestinationAuth", func(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + timestamp := generateTimestamp() + + connName := "test-upsert-rules-auth-" + timestamp + sourceName := "test-upsert-src-ra-" + timestamp + destName := "test-upsert-dst-ra-" + timestamp + + // Create a connection WITH destination authentication (bearer token) + var createResp map[string]interface{} + err := cli.RunJSON(&createResp, + "gateway", "connection", "create", + "--name", connName, + "--source-type", "WEBHOOK", + "--source-name", sourceName, + "--destination-type", "HTTP", + "--destination-name", destName, + "--destination-url", "https://api.example.com/webhook", + "--destination-auth-method", "bearer", + "--destination-bearer-token", "test_secret_token_123", + ) + require.NoError(t, err, "Should create connection with bearer auth") + + connID, ok := createResp["id"].(string) + require.True(t, ok && connID != "", "Expected connection ID") + + t.Cleanup(func() { + deleteConnection(t, cli, connID) + }) + + // Verify auth was set + dest, ok := createResp["destination"].(map[string]interface{}) + require.True(t, ok, "Expected destination object") + destConfig, ok := dest["config"].(map[string]interface{}) + require.True(t, ok, "Expected destination config") + assert.Equal(t, "BEARER_TOKEN", destConfig["auth_type"], "Auth type should be BEARER_TOKEN after creation") + + // Upsert with ONLY rule flags (no source/destination flags) + var upsertResp map[string]interface{} + err = cli.RunJSON(&upsertResp, + "gateway", "connection", "upsert", connName, + "--rule-retry-strategy", "linear", + "--rule-retry-count", "3", + "--rule-retry-interval", "5000", + ) + require.NoError(t, err, "Should upsert with only rule flags without auth_type error") + + // Verify rules were applied + rules, ok := upsertResp["rules"].([]interface{}) + require.True(t, ok, "Expected rules array") + + foundRetry := false + for _, r := range rules { + rule, ok := r.(map[string]interface{}) + if ok && rule["type"] == "retry" { + foundRetry = true + assert.Equal(t, "linear", rule["strategy"]) + break + } + } + assert.True(t, foundRetry, "Should have a retry rule") + + // Verify auth was preserved + upsertDest, ok := upsertResp["destination"].(map[string]interface{}) + require.True(t, ok, "Expected destination in upsert response") + upsertDestConfig, ok := upsertDest["config"].(map[string]interface{}) + require.True(t, ok, "Expected destination config in upsert response") + assert.Equal(t, "BEARER_TOKEN", upsertDestConfig["auth_type"], "Auth type should still be BEARER_TOKEN") + + t.Logf("Successfully upserted connection %s with only rule flags, auth preserved", connID) + }) + + // Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3: + // --rule-retry-response-status-codes must be sent as an array, not a string. + t.Run("UpsertRetryResponseStatusCodesAsArray", func(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + timestamp := generateTimestamp() + + connName := "test-upsert-statuscodes-" + timestamp + sourceName := "test-upsert-src-sc-" + timestamp + destName := "test-upsert-dst-sc-" + timestamp + + // Create a connection with full source/dest so the upsert provides all required fields + var upsertResp map[string]interface{} + err := cli.RunJSON(&upsertResp, + "gateway", "connection", "upsert", connName, + "--source-name", sourceName, + "--source-type", "WEBHOOK", + "--destination-name", destName, + "--destination-type", "HTTP", + "--destination-url", "https://api.example.com/webhook", + "--rule-retry-strategy", "linear", + "--rule-retry-count", "3", + "--rule-retry-interval", "5000", + "--rule-retry-response-status-codes", "500,502,503,504", + ) + require.NoError(t, err, "Should upsert with retry response status codes as array") + + connID, _ := upsertResp["id"].(string) + t.Cleanup(func() { + if connID != "" { + deleteConnection(t, cli, connID) + } + }) + + // Verify the retry rule has status codes as an array + rules, ok := upsertResp["rules"].([]interface{}) + require.True(t, ok, "Expected rules array") + + foundRetry := false + for _, r := range rules { + rule, ok := r.(map[string]interface{}) + if ok && rule["type"] == "retry" { + foundRetry = true + + statusCodes, ok := rule["response_status_codes"].([]interface{}) + require.True(t, ok, "response_status_codes should be array, got: %T (%v)", rule["response_status_codes"], rule["response_status_codes"]) + assert.Len(t, statusCodes, 4, "Should have 4 status codes") + + codes := make([]string, len(statusCodes)) + for i, c := range statusCodes { + codes[i] = strings.TrimSpace(c.(string)) + } + assert.Contains(t, codes, "500") + assert.Contains(t, codes, "502") + assert.Contains(t, codes, "503") + assert.Contains(t, codes, "504") + break + } + } + assert.True(t, foundRetry, "Should have a retry rule") + + t.Logf("Successfully verified retry status codes sent as array") + }) + + // Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 2: + // Upserting with --source-name alone (without --source-type) should work for + // existing connections (the existing type is preserved). + t.Run("UpsertSourceNameWithoutTypeOnExistingConnection", func(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + timestamp := generateTimestamp() + + connName := "test-upsert-srconly-" + timestamp + sourceName := "test-upsert-src-so-" + timestamp + destName := "test-upsert-dst-so-" + timestamp + newSourceName := "test-upsert-newsrc-" + timestamp + + // Create a connection first + var createResp map[string]interface{} + err := cli.RunJSON(&createResp, + "gateway", "connection", "create", + "--name", connName, + "--source-type", "WEBHOOK", + "--source-name", sourceName, + "--destination-type", "HTTP", + "--destination-name", destName, + "--destination-url", "https://api.example.com/webhook", + ) + require.NoError(t, err, "Should create connection") + + connID, ok := createResp["id"].(string) + require.True(t, ok && connID != "", "Expected connection ID") + + t.Cleanup(func() { + deleteConnection(t, cli, connID) + }) + + // Upsert with --source-name only (no --source-type) + // Previously this failed with "both --source-name and --source-type are required" + var upsertResp map[string]interface{} + err = cli.RunJSON(&upsertResp, + "gateway", "connection", "upsert", connName, + "--source-name", newSourceName, + ) + require.NoError(t, err, "Should upsert with --source-name only on existing connection") + + // Verify the source was updated + upsertSource, ok := upsertResp["source"].(map[string]interface{}) + require.True(t, ok, "Expected source in upsert response") + assert.Equal(t, newSourceName, upsertSource["name"], "Source name should be updated") + + t.Logf("Successfully upserted connection %s with source-name only", connID) + }) } From 15d39b4306aa6fa67c911fe01d68c34366dede3d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 25 Feb 2026 22:56:02 +0000 Subject: [PATCH 3/3] ci: add unit tests to CI and fix broken TestSourceCreateRequiresName - Add unit-test job to test.yml that runs `go test -short ./pkg/...` on every PR. Previously only GoReleaser builds ran, so unit test failures were never caught. - Fix TestSourceCreateRequiresName: the old test executed the real root command which hit ValidateAPIKey() before cobra's required-flag check, causing a misleading "not authenticated" error. Replaced with a direct check that --name and --type have the required-flag annotation. - Add TestConnectionCreateRetryResponseStatusCodes acceptance test for Bug 3 coverage on the create command path (update and upsert were already covered). https://claude.ai/code/session_01D8k5gyzeqzjZUanVrwv5dL --- .github/workflows/test.yml | 12 +++++++ pkg/cmd/source_create_update_test.go | 28 ++++++++++----- test/acceptance/connection_test.go | 54 ++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d49dfec..36a84e0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,6 +10,18 @@ on: - main jobs: + unit-test: + runs-on: ubuntu-latest + steps: + - name: Code checkout + uses: actions/checkout@v4 + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 1.24.9 + - name: Run unit tests + run: go test -short ./pkg/... + build-mac: runs-on: macos-latest steps: diff --git a/pkg/cmd/source_create_update_test.go b/pkg/cmd/source_create_update_test.go index 61059ab..9a081e2 100644 --- a/pkg/cmd/source_create_update_test.go +++ b/pkg/cmd/source_create_update_test.go @@ -1,21 +1,31 @@ package cmd import ( - "strings" "testing" "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -// TestSourceCreateRequiresName asserts that create without --name fails (Cobra required-flag validation). -func TestSourceCreateRequiresName(t *testing.T) { - rootCmd.SetArgs([]string{"gateway", "source", "create", "--type", "WEBHOOK"}) - err := rootCmd.Execute() - require.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "name") || strings.Contains(err.Error(), "required"), - "error should mention name or required, got: %s", err.Error()) +// TestSourceCreateRequiresNameAndType verifies that the source create command +// marks --name and --type as required flags via cobra's MarkFlagRequired. +func TestSourceCreateRequiresNameAndType(t *testing.T) { + sc := newSourceCreateCmd() + + nameFlag := sc.cmd.Flags().Lookup("name") + assert.NotNil(t, nameFlag, "--name flag should exist") + + typeFlag := sc.cmd.Flags().Lookup("type") + assert.NotNil(t, typeFlag, "--type flag should exist") + + // Cobra marks required flags with the "required" annotation + nameAnnotations := nameFlag.Annotations + assert.Contains(t, nameAnnotations, "cobra_annotation_bash_completion_one_required_flag", + "--name should be marked as required") + + typeAnnotations := typeFlag.Annotations + assert.Contains(t, typeAnnotations, "cobra_annotation_bash_completion_one_required_flag", + "--type should be marked as required") } // TestSourceUpdateRequestEmpty asserts the "no updates specified" logic for update. diff --git a/test/acceptance/connection_test.go b/test/acceptance/connection_test.go index 6b3ddf5..7ab68e3 100644 --- a/test/acceptance/connection_test.go +++ b/test/acceptance/connection_test.go @@ -2195,6 +2195,60 @@ func TestConnectionCreateOutputStructure(t *testing.T) { t.Logf("Successfully verified connection create output structure") } +// TestConnectionCreateRetryResponseStatusCodes verifies that creating a connection +// with --rule-retry-response-status-codes sends the codes as an array to the API. +// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3. +func TestConnectionCreateRetryResponseStatusCodes(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + timestamp := generateTimestamp() + + connName := "test-create-statuscodes-" + timestamp + sourceName := "test-src-sc-" + timestamp + destName := "test-dst-sc-" + timestamp + + var conn Connection + err := cli.RunJSON(&conn, + "gateway", "connection", "create", + "--name", connName, + "--source-name", sourceName, + "--source-type", "WEBHOOK", + "--destination-name", destName, + "--destination-type", "HTTP", + "--destination-url", "https://api.example.com/webhook", + "--rule-retry-strategy", "linear", + "--rule-retry-count", "3", + "--rule-retry-interval", "5000", + "--rule-retry-response-status-codes", "500,502,503,504", + ) + require.NoError(t, err, "Should create connection with retry response status codes") + require.NotEmpty(t, conn.ID, "Connection should have an ID") + + t.Cleanup(func() { + deleteConnection(t, cli, conn.ID) + }) + + require.NotEmpty(t, conn.Rules, "Connection should have rules") + + foundRetry := false + for _, rule := range conn.Rules { + if rule["type"] == "retry" { + foundRetry = true + + statusCodes, ok := rule["response_status_codes"].([]interface{}) + require.True(t, ok, "response_status_codes should be an array, got: %T (%v)", rule["response_status_codes"], rule["response_status_codes"]) + assert.Len(t, statusCodes, 4, "Should have 4 status codes") + break + } + } + assert.True(t, foundRetry, "Should have a retry rule") + + t.Logf("Successfully created connection %s with retry status codes as array", conn.ID) +} + // TestConnectionWithDestinationPathForwarding tests path_forwarding_disabled and http_method fields func TestConnectionWithDestinationPathForwarding(t *testing.T) { if testing.Short() {