From eac4f89a714aacee5f82185282c45108ff027b0d Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Fri, 2 Oct 2026 12:39:48 +0100 Subject: [PATCH 1/4] fix(connection): resolve --rule-transform-name as a name or ID --rule-transform-name said "Name or ID" but always sent the value as transformation.name. The API matches a transform rule's transformation by name and creates one when the name is new, even without code. So an ID, or a mistyped name, returned 200 and attached a new, empty transformation that failed every event with "TypeError: undefined requires parameter 1 to be a string". Resolve the value first, on create, update and upsert: - found by ID or exact name: reference it by transformation_id, sending any --rule-transform-code/--rule-transform-env as an update to it - not found, with code: create a new transformation with that name, as before (an ID is an error) - not found, without code: error, pointing at transformation list and --rule-transform-code Update and upsert now also require --rule-transform-name with transform code or env, as create already did. --rules and --rules-file are sent as given. Help text and REFERENCE.md say that code and env replace an existing transformation's. Co-Authored-By: Claude Opus 5.5 --- REFERENCE.md | 18 +-- pkg/cmd/connection_common.go | 6 +- pkg/cmd/connection_create.go | 3 + pkg/cmd/connection_transform_rule.go | 110 ++++++++++++++++ pkg/cmd/connection_transform_rule_test.go | 145 ++++++++++++++++++++++ pkg/cmd/connection_update.go | 3 + pkg/cmd/connection_upsert.go | 5 + test/acceptance/connection_test.go | 62 +++++++++ 8 files changed, 340 insertions(+), 12 deletions(-) create mode 100644 pkg/cmd/connection_transform_rule.go create mode 100644 pkg/cmd/connection_transform_rule_test.go diff --git a/REFERENCE.md b/REFERENCE.md index 362c7082..d4f0f2a1 100644 --- a/REFERENCE.md +++ b/REFERENCE.md @@ -402,9 +402,9 @@ hookdeck gateway connection create [flags] | `--rule-retry-interval` | `int` | Interval between retries in milliseconds (default "0") | | `--rule-retry-response-status-codes` | `string` | Comma-separated HTTP status codes to retry on | | `--rule-retry-strategy` | `string` | Retry strategy (linear, exponential) | -| `--rule-transform-code` | `string` | Transformation code (if creating inline) | -| `--rule-transform-env` | `string` | JSON string representing environment variables for transformation | -| `--rule-transform-name` | `string` | Name or ID of the transformation to apply | +| `--rule-transform-code` | `string` | Transformation code. Creates the transformation named by `--rule-transform-name`, or replaces its code if it exists | +| `--rule-transform-env` | `string` | JSON string representing environment variables for transformation. Replaces the env of an existing transformation | +| `--rule-transform-name` | `string` | Name or ID of an existing transformation to apply. With `--rule-transform-code`, an existing transformation's code is replaced, or a new transformation is created with this name | | `--rules` | `string` | JSON string representing the entire rules array | | `--rules-file` | `string` | Path to a JSON file containing the rules array | | `--source-allowed-http-methods` | `string` | Comma-separated list of allowed HTTP methods (GET, POST, PUT, PATCH, DELETE) | @@ -521,9 +521,9 @@ hookdeck gateway connection update [flags] | `--rule-retry-interval` | `int` | Interval between retries in milliseconds (default "0") | | `--rule-retry-response-status-codes` | `string` | Comma-separated HTTP status codes to retry on | | `--rule-retry-strategy` | `string` | Retry strategy (linear, exponential) | -| `--rule-transform-code` | `string` | Transformation code (if creating inline) | -| `--rule-transform-env` | `string` | JSON string representing environment variables for transformation | -| `--rule-transform-name` | `string` | Name or ID of the transformation to apply | +| `--rule-transform-code` | `string` | Transformation code. Creates the transformation named by `--rule-transform-name`, or replaces its code if it exists | +| `--rule-transform-env` | `string` | JSON string representing environment variables for transformation. Replaces the env of an existing transformation | +| `--rule-transform-name` | `string` | Name or ID of an existing transformation to apply. With `--rule-transform-code`, an existing transformation's code is replaced, or a new transformation is created with this name | | `--rules` | `string` | JSON string representing the entire rules array | | `--rules-file` | `string` | Path to a JSON file containing the rules array | | `--source-id` | `string` | Update source by ID | @@ -661,9 +661,9 @@ hookdeck gateway connection upsert [flags] | `--rule-retry-interval` | `int` | Interval between retries in milliseconds (default "0") | | `--rule-retry-response-status-codes` | `string` | Comma-separated HTTP status codes to retry on | | `--rule-retry-strategy` | `string` | Retry strategy (linear, exponential) | -| `--rule-transform-code` | `string` | Transformation code (if creating inline) | -| `--rule-transform-env` | `string` | JSON string representing environment variables for transformation | -| `--rule-transform-name` | `string` | Name or ID of the transformation to apply | +| `--rule-transform-code` | `string` | Transformation code. Creates the transformation named by `--rule-transform-name`, or replaces its code if it exists | +| `--rule-transform-env` | `string` | JSON string representing environment variables for transformation. Replaces the env of an existing transformation | +| `--rule-transform-name` | `string` | Name or ID of an existing transformation to apply. With `--rule-transform-code`, an existing transformation's code is replaced, or a new transformation is created with this name | | `--rules` | `string` | JSON string representing the entire rules array | | `--rules-file` | `string` | Path to a JSON file containing the rules array | | `--source-allowed-http-methods` | `string` | Comma-separated list of allowed HTTP methods (GET, POST, PUT, PATCH, DELETE) | diff --git a/pkg/cmd/connection_common.go b/pkg/cmd/connection_common.go index 0029d16b..9af86284 100644 --- a/pkg/cmd/connection_common.go +++ b/pkg/cmd/connection_common.go @@ -82,9 +82,9 @@ func addConnectionRuleFlags(cmd *cobra.Command, f *connectionRuleFlags) { cmd.Flags().StringVar(&f.RuleFilterQuery, "rule-filter-query", "", "Filter on request query parameters using Hookdeck filter syntax (JSON)") cmd.Flags().StringVar(&f.RuleFilterPath, "rule-filter-path", "", "Filter on request path using Hookdeck filter syntax (JSON)") - cmd.Flags().StringVar(&f.RuleTransformName, "rule-transform-name", "", "Name or ID of the transformation to apply") - cmd.Flags().StringVar(&f.RuleTransformCode, "rule-transform-code", "", "Transformation code (if creating inline)") - cmd.Flags().StringVar(&f.RuleTransformEnv, "rule-transform-env", "", "JSON string representing environment variables for transformation") + cmd.Flags().StringVar(&f.RuleTransformName, "rule-transform-name", "", "Name or ID of an existing transformation to apply. With --rule-transform-code, an existing transformation's code is replaced, or a new transformation is created with this name") + cmd.Flags().StringVar(&f.RuleTransformCode, "rule-transform-code", "", "Transformation code. Creates the transformation named by --rule-transform-name, or replaces its code if it exists") + cmd.Flags().StringVar(&f.RuleTransformEnv, "rule-transform-env", "", "JSON string representing environment variables for transformation. Replaces the env of an existing transformation") cmd.Flags().IntVar(&f.RuleDelay, "rule-delay", 0, "Delay in milliseconds") diff --git a/pkg/cmd/connection_create.go b/pkg/cmd/connection_create.go index a20bb7f0..8485fd1c 100644 --- a/pkg/cmd/connection_create.go +++ b/pkg/cmd/connection_create.go @@ -459,6 +459,9 @@ func (cc *connectionCreateCmd) runConnectionCreateCmd(cmd *cobra.Command, args [ if err != nil { return err } + if err := resolveRuleTransformation(context.Background(), client, &cc.connectionRuleFlags, rules); err != nil { + return err + } if len(rules) > 0 { req.Rules = &rules } diff --git a/pkg/cmd/connection_transform_rule.go b/pkg/cmd/connection_transform_rule.go new file mode 100644 index 00000000..34f23d7d --- /dev/null +++ b/pkg/cmd/connection_transform_rule.go @@ -0,0 +1,110 @@ +package cmd + +import ( + "context" + "fmt" + "strings" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" +) + +// transformationLookup is the subset of the API client used to resolve +// --rule-transform-name. +type transformationLookup interface { + GetTransformation(ctx context.Context, id string) (*hookdeck.Transformation, error) + ListTransformations(ctx context.Context, params map[string]string) (*hookdeck.TransformationListResponse, error) +} + +// findTransformationID returns the ID of the transformation whose ID or exact +// name is nameOrID, or "" when there is none. +func findTransformationID(ctx context.Context, client transformationLookup, nameOrID string) (string, error) { + if strings.HasPrefix(nameOrID, "trs_") { + trn, err := client.GetTransformation(ctx, nameOrID) + if err == nil { + return trn.ID, nil + } + if !hookdeck.IsNotFoundError(err) { + return "", fmt.Errorf("failed to look up transformation '%s': %w", nameOrID, err) + } + } + + result, err := client.ListTransformations(ctx, map[string]string{"name": nameOrID}) + if err != nil { + return "", fmt.Errorf("failed to look up transformation '%s': %w", nameOrID, err) + } + for _, trn := range result.Models { + if trn.Name == nameOrID { + return trn.ID, nil + } + } + return "", nil +} + +// resolveRuleTransformation makes --rule-transform-name accept a transformation +// name or ID, as its help text says. +// +// The API matches a transform rule's transformation by name and creates one when +// the name is new, even without code. Sent as a name, an ID or a mistyped name +// therefore created an empty transformation that failed every event. So the value +// is resolved first: +// - found: the rule references it by transformation_id; --rule-transform-code and +// --rule-transform-env, if given, update that transformation +// - not found, with --rule-transform-code: a new transformation is created with +// that name, as before (not allowed for an ID) +// - not found, without --rule-transform-code: an error +// +// Rules from --rules or --rules-file are sent as given. +func resolveRuleTransformation(ctx context.Context, client transformationLookup, f *connectionRuleFlags, rules []hookdeck.Rule) error { + if f.Rules != "" || f.RulesFile != "" { + return nil + } + if f.RuleTransformName == "" { + if f.RuleTransformCode != "" || f.RuleTransformEnv != "" { + return fmt.Errorf("--rule-transform-name is required when using transform rule flags") + } + return nil + } + + idx := -1 + for i, r := range rules { + if r["type"] == "transform" { + idx = i + break + } + } + if idx == -1 { + return nil + } + + nameOrID := f.RuleTransformName + id, err := findTransformationID(ctx, client, nameOrID) + if err != nil { + return err + } + + if id == "" { + if strings.HasPrefix(nameOrID, "trs_") { + return fmt.Errorf("transformation '%s' not found. Check the ID with 'hookdeck gateway transformation list'", nameOrID) + } + if f.RuleTransformCode == "" { + return fmt.Errorf("transformation '%s' not found. Check the name with 'hookdeck gateway transformation list', or pass --rule-transform-code to create a new transformation with this name", nameOrID) + } + // New transformation: keep name and code so the API creates it + return nil + } + + rule := hookdeck.Rule{"type": "transform", "transformation_id": id} + if existing, ok := rules[idx]["transformation"].(map[string]interface{}); ok { + update := make(map[string]interface{}) + for _, key := range []string{"code", "env"} { + if v, ok := existing[key]; ok { + update[key] = v + } + } + if len(update) > 0 { + rule["transformation"] = update + } + } + rules[idx] = rule + return nil +} diff --git a/pkg/cmd/connection_transform_rule_test.go b/pkg/cmd/connection_transform_rule_test.go new file mode 100644 index 00000000..b7b8f3bf --- /dev/null +++ b/pkg/cmd/connection_transform_rule_test.go @@ -0,0 +1,145 @@ +package cmd + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" +) + +// fakeTransformations serves GetTransformation by ID and ListTransformations by +// name the way the API does: list may return near matches, so callers must +// compare names exactly. +type fakeTransformations struct { + byID map[string]hookdeck.Transformation + listErr error + calls int +} + +func (f *fakeTransformations) GetTransformation(_ context.Context, id string) (*hookdeck.Transformation, error) { + f.calls++ + if trn, ok := f.byID[id]; ok { + return &trn, nil + } + return nil, &hookdeck.APIError{StatusCode: 404, Message: "not found"} +} + +func (f *fakeTransformations) ListTransformations(_ context.Context, params map[string]string) (*hookdeck.TransformationListResponse, error) { + f.calls++ + if f.listErr != nil { + return nil, f.listErr + } + resp := &hookdeck.TransformationListResponse{} + for _, trn := range f.byID { + if len(params["name"]) > 0 && len(trn.Name) >= len(params["name"]) && trn.Name[:len(params["name"])] == params["name"] { + resp.Models = append(resp.Models, trn) + } + } + return resp, nil +} + +func newFakeTransformations() *fakeTransformations { + return &fakeTransformations{byID: map[string]hookdeck.Transformation{ + "trs_abc123": {ID: "trs_abc123", Name: "orders-transform"}, + }} +} + +func buildTransformRules(t *testing.T, f *connectionRuleFlags) []hookdeck.Rule { + t.Helper() + rules, err := buildConnectionRules(f) + require.NoError(t, err) + return rules +} + +func TestResolveRuleTransformationAttachesExisting(t *testing.T) { + for _, nameOrID := range []string{"trs_abc123", "orders-transform"} { + t.Run(nameOrID, func(t *testing.T) { + f := &connectionRuleFlags{RuleTransformName: nameOrID, RuleFilterBody: `{"type":"order"}`} + rules := buildTransformRules(t, f) + + require.NoError(t, resolveRuleTransformation(context.Background(), newFakeTransformations(), f, rules)) + + assert.Equal(t, hookdeck.Rule{"type": "transform", "transformation_id": "trs_abc123"}, rules[0], + "an existing transformation is referenced by ID, never sent as a name") + assert.Equal(t, "filter", rules[1]["type"], "other rules are untouched") + }) + } +} + +func TestResolveRuleTransformationUpdatesExistingWithCodeAndEnv(t *testing.T) { + f := &connectionRuleFlags{ + RuleTransformName: "trs_abc123", + RuleTransformCode: `addHandler("transform", (request) => request);`, + RuleTransformEnv: `{"KEY":"value"}`, + } + rules := buildTransformRules(t, f) + + require.NoError(t, resolveRuleTransformation(context.Background(), newFakeTransformations(), f, rules)) + + assert.Equal(t, "trs_abc123", rules[0]["transformation_id"]) + update, ok := rules[0]["transformation"].(map[string]interface{}) + require.True(t, ok, "code and env are sent as an update to the existing transformation") + assert.Equal(t, f.RuleTransformCode, update["code"]) + assert.Equal(t, map[string]interface{}{"KEY": "value"}, update["env"]) + assert.NotContains(t, update, "name", "the ID must not be sent as the transformation's new name") +} + +func TestResolveRuleTransformationNotFound(t *testing.T) { + t.Run("name without code is an error", func(t *testing.T) { + // "orders" is a prefix of an existing name, so only an exact match may count + f := &connectionRuleFlags{RuleTransformName: "orders"} + err := resolveRuleTransformation(context.Background(), newFakeTransformations(), f, buildTransformRules(t, f)) + require.Error(t, err) + assert.Contains(t, err.Error(), "transformation 'orders' not found") + assert.Contains(t, err.Error(), "--rule-transform-code") + }) + + t.Run("ID is an error, even with code", func(t *testing.T) { + f := &connectionRuleFlags{RuleTransformName: "trs_missing", RuleTransformCode: "x"} + err := resolveRuleTransformation(context.Background(), newFakeTransformations(), f, buildTransformRules(t, f)) + require.Error(t, err) + assert.Contains(t, err.Error(), "transformation 'trs_missing' not found") + }) + + t.Run("name with code creates a new transformation", func(t *testing.T) { + f := &connectionRuleFlags{RuleTransformName: "new-transform", RuleTransformCode: "x"} + rules := buildTransformRules(t, f) + require.NoError(t, resolveRuleTransformation(context.Background(), newFakeTransformations(), f, rules)) + assert.Equal(t, map[string]interface{}{"name": "new-transform", "code": "x"}, rules[0]["transformation"]) + assert.NotContains(t, rules[0], "transformation_id") + }) +} + +func TestResolveRuleTransformationRequiresName(t *testing.T) { + f := &connectionRuleFlags{RuleTransformCode: "x"} + err := resolveRuleTransformation(context.Background(), newFakeTransformations(), f, buildTransformRules(t, f)) + require.Error(t, err) + assert.Contains(t, err.Error(), "--rule-transform-name is required") +} + +func TestResolveRuleTransformationLeavesRulesJSONAlone(t *testing.T) { + f := &connectionRuleFlags{ + Rules: `[{"type":"transform","transformation":{"name":"trs_abc123"}}]`, + RuleTransformName: "ignored", + } + rules := buildTransformRules(t, f) + fake := newFakeTransformations() + + require.NoError(t, resolveRuleTransformation(context.Background(), fake, f, rules)) + assert.Equal(t, map[string]interface{}{"name": "trs_abc123"}, rules[0]["transformation"]) + assert.Zero(t, fake.calls, "--rules is sent as given, without lookups") +} + +func TestResolveRuleTransformationReturnsLookupErrors(t *testing.T) { + fake := newFakeTransformations() + fake.listErr = errors.New("boom") + f := &connectionRuleFlags{RuleTransformName: "orders-transform"} + + err := resolveRuleTransformation(context.Background(), fake, f, buildTransformRules(t, f)) + require.Error(t, err) + assert.Contains(t, err.Error(), "boom") +} diff --git a/pkg/cmd/connection_update.go b/pkg/cmd/connection_update.go index 8728d460..5c1534c7 100644 --- a/pkg/cmd/connection_update.go +++ b/pkg/cmd/connection_update.go @@ -118,6 +118,9 @@ func (cu *connectionUpdateCmd) runConnectionUpdateCmd(cmd *cobra.Command, args [ if err != nil { return err } + if err := resolveRuleTransformation(ctx, client, &cu.connectionRuleFlags, rules); err != nil { + return err + } // Keyed off whether the flag was given rather than whether it produced any // rules, so that an explicit empty array reaches the API and clears the // ruleset instead of being read as "no rules mentioned". diff --git a/pkg/cmd/connection_upsert.go b/pkg/cmd/connection_upsert.go index 84e3c388..04b484fb 100644 --- a/pkg/cmd/connection_upsert.go +++ b/pkg/cmd/connection_upsert.go @@ -373,6 +373,11 @@ func (cu *connectionUpsertCmd) runConnectionUpsertCmd(cmd *cobra.Command, args [ if err != nil { return err } + if req.Rules != nil { + if err := resolveRuleTransformation(context.Background(), client, &cu.connectionCreateCmd.connectionRuleFlags, *req.Rules); err != nil { + return err + } + } // For dry-run mode, preview changes without applying if cu.dryRun { diff --git a/test/acceptance/connection_test.go b/test/acceptance/connection_test.go index 8f6a4ea4..b45b9c89 100644 --- a/test/acceptance/connection_test.go +++ b/test/acceptance/connection_test.go @@ -1391,6 +1391,68 @@ func TestConnectionWithTransformRule(t *testing.T) { t.Logf("Successfully created and verified connection with transform rule: %s", conn.ID) } +// TestConnectionTransformRuleNameOrID verifies that --rule-transform-name attaches an +// existing transformation by ID or by name, and that an unknown value without +// --rule-transform-code is an error instead of an empty transformation being created. +func TestConnectionTransformRuleNameOrID(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + trnID := createTestTransformation(t, cli) + t.Cleanup(func() { deleteTransformation(t, cli, trnID) }) + + var trn Transformation + require.NoError(t, cli.RunJSON(&trn, "gateway", "transformation", "get", trnID)) + require.NotEmpty(t, trn.Name, "transformation name") + + createArgs := func(suffix string) []string { + timestamp := generateTimestamp() + return []string{ + "gateway", "connection", "create", + "--name", "test-trn-ref-" + suffix + "-" + timestamp, + "--source-name", "test-trn-ref-src-" + timestamp, + "--source-type", "WEBHOOK", + "--destination-name", "test-trn-ref-dst-" + timestamp, + "--destination-type", "CLI", + "--destination-cli-path", "/webhooks", + } + } + + for _, tc := range []struct{ name, value string }{{"by ID", trnID}, {"by name", trn.Name}} { + t.Run("attaches existing "+tc.name, func(t *testing.T) { + var conn Connection + err := cli.RunJSON(&conn, append(createArgs("ok"), "--rule-transform-name", tc.value)...) + require.NoError(t, err, "Should create connection referencing an existing transformation") + t.Cleanup(func() { deleteConnection(t, cli, conn.ID) }) + + var getConn Connection + require.NoError(t, cli.RunJSON(&getConn, "gateway", "connection", "get", conn.ID)) + require.Len(t, getConn.Rules, 1, "Connection should have one rule") + assert.Equal(t, trnID, getConn.Rules[0]["transformation_id"], + "Rule should reference the existing transformation, not a new one") + }) + } + + for _, value := range []string{"trs_doesnotexist0", "test-trn-missing-" + generateTimestamp()} { + t.Run("rejects unknown "+value, func(t *testing.T) { + stdout, stderr, err := cli.Run(append(createArgs("missing"), "--rule-transform-name", value)...) + require.Error(t, err, "Unknown transformation without code should fail\nstdout: %s", stdout) + assert.Contains(t, stderr+stdout, "transformation '"+value+"' not found") + + // Nothing should have been created with that name + var list struct { + Models []Transformation `json:"models"` + } + require.NoError(t, cli.RunJSON(&list, "gateway", "transformation", "list", "--name", value)) + for _, m := range list.Models { + assert.NotEqual(t, value, m.Name, "No transformation should be created for an unknown value") + } + }) + } +} + // TestConnectionWithDelayRule tests creating a connection with a delay rule func TestConnectionWithDelayRule(t *testing.T) { if testing.Short() { From 3f575d60d16e8c855fb76d35fd2c1f26be7291f7 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Thu, 5 Nov 2026 12:36:29 +0000 Subject: [PATCH 2/4] fix(connection): follow pagination when resolving --rule-transform-name The name filter on GET /transformations can return near matches, so the exact name may not be on the first page. The lookup now asks for the largest page the API allows (255) and follows pagination.next, up to 20 pages, then errors and asks for the ID. Addresses Copilot's review finding on #468. Co-Authored-By: Claude Opus 5.5 --- pkg/cmd/connection_transform_rule.go | 35 ++++++++-- pkg/cmd/connection_transform_rule_test.go | 80 +++++++++++++++++++++++ 2 files changed, 108 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/connection_transform_rule.go b/pkg/cmd/connection_transform_rule.go index 34f23d7d..b414d0f5 100644 --- a/pkg/cmd/connection_transform_rule.go +++ b/pkg/cmd/connection_transform_rule.go @@ -3,11 +3,20 @@ package cmd import ( "context" "fmt" + "strconv" "strings" "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" ) +// The name filter on GET /transformations can return near matches, so the +// exact name is not necessarily on the first page. The lookup asks for the +// largest page the API allows and follows pagination.next, up to a bound. +const ( + transformationLookupPageLimit = 255 + transformationLookupMaxPages = 20 +) + // transformationLookup is the subset of the API client used to resolve // --rule-transform-name. type transformationLookup interface { @@ -28,16 +37,28 @@ func findTransformationID(ctx context.Context, client transformationLookup, name } } - result, err := client.ListTransformations(ctx, map[string]string{"name": nameOrID}) - if err != nil { - return "", fmt.Errorf("failed to look up transformation '%s': %w", nameOrID, err) + params := map[string]string{ + "name": nameOrID, + "limit": strconv.Itoa(transformationLookupPageLimit), } - for _, trn := range result.Models { - if trn.Name == nameOrID { - return trn.ID, nil + for page := 0; page < transformationLookupMaxPages; page++ { + result, err := client.ListTransformations(ctx, params) + if err != nil { + return "", fmt.Errorf("failed to look up transformation '%s': %w", nameOrID, err) + } + for _, trn := range result.Models { + if trn.Name == nameOrID { + return trn.ID, nil + } + } + next := result.Pagination.Next + if next == nil || *next == "" { + return "", nil } + params["next"] = *next } - return "", nil + return "", fmt.Errorf("failed to look up transformation '%s': more than %d transformations match the name filter; pass its ID instead", + nameOrID, transformationLookupPageLimit*transformationLookupMaxPages) } // resolveRuleTransformation makes --rule-transform-name accept a transformation diff --git a/pkg/cmd/connection_transform_rule_test.go b/pkg/cmd/connection_transform_rule_test.go index b7b8f3bf..374192d7 100644 --- a/pkg/cmd/connection_transform_rule_test.go +++ b/pkg/cmd/connection_transform_rule_test.go @@ -3,6 +3,8 @@ package cmd import ( "context" "errors" + "fmt" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -143,3 +145,81 @@ func TestResolveRuleTransformationReturnsLookupErrors(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "boom") } + +// pagedTransformations serves ListTransformations one page at a time, the way +// the API pages a name filter that matches more than one page of near matches. +type pagedTransformations struct { + pages [][]hookdeck.Transformation + params []map[string]string +} + +func (p *pagedTransformations) GetTransformation(_ context.Context, id string) (*hookdeck.Transformation, error) { + return nil, &hookdeck.APIError{StatusCode: 404, Message: "not found"} +} + +func (p *pagedTransformations) ListTransformations(_ context.Context, params map[string]string) (*hookdeck.TransformationListResponse, error) { + copied := map[string]string{} + for k, v := range params { + copied[k] = v + } + p.params = append(p.params, copied) + + page := 0 + if next := params["next"]; next != "" { + fmt.Sscanf(next, "page-%d", &page) + } + resp := &hookdeck.TransformationListResponse{} + if page < len(p.pages) { + resp.Models = p.pages[page] + } + if page+1 < len(p.pages) { + next := fmt.Sprintf("page-%d", page+1) + resp.Pagination.Next = &next + } + return resp, nil +} + +func TestFindTransformationIDFollowsPagination(t *testing.T) { + p := &pagedTransformations{pages: [][]hookdeck.Transformation{ + {{ID: "trs_near1", Name: "orders-transform-v1"}, {ID: "trs_near2", Name: "orders-transform-v2"}}, + {{ID: "trs_near3", Name: "orders-transform-v3"}}, + {{ID: "trs_exact", Name: "orders-transform"}}, + }} + + id, err := findTransformationID(context.Background(), p, "orders-transform") + require.NoError(t, err) + assert.Equal(t, "trs_exact", id, "the exact match on the third page should be found") + + require.Len(t, p.params, 3, "one request per page") + for _, params := range p.params { + assert.Equal(t, "orders-transform", params["name"]) + assert.Equal(t, strconv.Itoa(transformationLookupPageLimit), params["limit"], "each page asks for the largest page size") + } + assert.Equal(t, "", p.params[0]["next"]) + assert.Equal(t, "page-1", p.params[1]["next"]) + assert.Equal(t, "page-2", p.params[2]["next"]) +} + +func TestFindTransformationIDStopsWhenNoNextPage(t *testing.T) { + p := &pagedTransformations{pages: [][]hookdeck.Transformation{ + {{ID: "trs_near1", Name: "orders-transform-v1"}}, + }} + + id, err := findTransformationID(context.Background(), p, "orders-transform") + require.NoError(t, err) + assert.Equal(t, "", id, "no exact match means not found") + assert.Len(t, p.params, 1, "no further request without pagination.next") +} + +func TestFindTransformationIDBoundsPagination(t *testing.T) { + pages := make([][]hookdeck.Transformation, transformationLookupMaxPages+1) + for i := range pages { + pages[i] = []hookdeck.Transformation{{ID: fmt.Sprintf("trs_near%d", i), Name: fmt.Sprintf("orders-transform-%d", i)}} + } + p := &pagedTransformations{pages: pages} + + _, err := findTransformationID(context.Background(), p, "orders-transform") + require.Error(t, err) + assert.Contains(t, err.Error(), "pass its ID instead") + assert.Len(t, p.params, transformationLookupMaxPages, "stops at the page bound") +} From 0cd040ef321bf29dad19feadba2bb082ebe53120 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Thu, 5 Nov 2026 12:36:30 +0000 Subject: [PATCH 3/4] test(acceptance): cover --rule-transform-name on connection update and upsert Both commands have their own wiring path, which the create-only acceptance test did not exercise. Each new test attaches an existing transformation by ID and by exact name, checks the stored transformation_id, and checks that an unknown value fails. The update test also checks that code passed with an ID replaces that transformation's code without renaming it. Addresses Copilot's review findings on #468. Co-Authored-By: Claude Opus 5.5 --- test/acceptance/connection_update_test.go | 71 +++++++++++++++++++++++ test/acceptance/connection_upsert_test.go | 56 ++++++++++++++++++ 2 files changed, 127 insertions(+) diff --git a/test/acceptance/connection_update_test.go b/test/acceptance/connection_update_test.go index 671b7b41..72e529c9 100644 --- a/test/acceptance/connection_update_test.go +++ b/test/acceptance/connection_update_test.go @@ -392,3 +392,74 @@ func TestConnectionUpdateWithRulesJSON(t *testing.T) { t.Logf("Connection update with --rules JSON verified: %s", connID) } + +// TestConnectionUpdateTransformRuleNameOrID checks that `connection update` +// resolves --rule-transform-name as an ID or an exact name, attaching the +// existing transformation by transformation_id instead of creating a new one. +func TestConnectionUpdateTransformRuleNameOrID(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + trnID := createTestTransformation(t, cli) + t.Cleanup(func() { deleteTransformation(t, cli, trnID) }) + + var trn Transformation + require.NoError(t, cli.RunJSON(&trn, "gateway", "transformation", "get", trnID)) + require.NotEmpty(t, trn.Name, "transformation name") + + assertAttached := func(t *testing.T, connID string) { + t.Helper() + var fetched Connection + require.NoError(t, cli.RunJSON(&fetched, "gateway", "connection", "get", connID)) + require.Len(t, fetched.Rules, 1, "Connection should have one rule") + assert.Equal(t, "transform", fetched.Rules[0]["type"]) + assert.Equal(t, trnID, fetched.Rules[0]["transformation_id"], + "Rule should reference the existing transformation, not a new one") + } + + for _, tc := range []struct{ name, value string }{{"by ID", trnID}, {"by name", trn.Name}} { + t.Run("attaches existing "+tc.name, func(t *testing.T) { + connID := createTestConnection(t, cli) + t.Cleanup(func() { deleteConnection(t, cli, connID) }) + + var updated Connection + require.NoError(t, cli.RunJSON(&updated, + "gateway", "connection", "update", connID, + "--rule-transform-name", tc.value, + ), "Should update connection to reference an existing transformation") + assertAttached(t, connID) + }) + } + + t.Run("code with an ID updates that transformation", func(t *testing.T) { + connID := createTestConnection(t, cli) + t.Cleanup(func() { deleteConnection(t, cli, connID) }) + + newCode := `addHandler("transform", (request, context) => { request.headers["x-updated"] = "1"; return request; });` + var updated Connection + require.NoError(t, cli.RunJSON(&updated, + "gateway", "connection", "update", connID, + "--rule-transform-name", trnID, + "--rule-transform-code", newCode, + ), "Should update connection with an existing transformation ID and new code") + assertAttached(t, connID) + + var after Transformation + require.NoError(t, cli.RunJSON(&after, "gateway", "transformation", "get", trnID)) + assert.Equal(t, newCode, after.Code, "The referenced transformation's code should be replaced") + assert.Equal(t, trn.Name, after.Name, "The ID must not be sent as a new name") + }) + + for _, value := range []string{"trs_doesnotexist0", "test-trn-missing-" + generateTimestamp()} { + t.Run("rejects unknown "+value, func(t *testing.T) { + connID := createTestConnection(t, cli) + t.Cleanup(func() { deleteConnection(t, cli, connID) }) + + stdout, stderr, err := cli.Run("gateway", "connection", "update", connID, "--rule-transform-name", value) + require.Error(t, err, "Unknown transformation without code should fail\nstdout: %s", stdout) + assert.Contains(t, stderr+stdout, "transformation '"+value+"' not found") + }) + } +} diff --git a/test/acceptance/connection_upsert_test.go b/test/acceptance/connection_upsert_test.go index 29908e69..946e45d1 100644 --- a/test/acceptance/connection_upsert_test.go +++ b/test/acceptance/connection_upsert_test.go @@ -802,3 +802,59 @@ func TestConnectionUpsertRejectsEmptySourceWebhookSecret(t *testing.T) { assert.Contains(t, stdout+stderr, "--source-webhook-secret") }) } + +// TestConnectionUpsertTransformRuleNameOrID checks that `connection upsert` +// resolves --rule-transform-name as an ID or an exact name, attaching the +// existing transformation by transformation_id instead of creating a new one. +func TestConnectionUpsertTransformRuleNameOrID(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + + cli := NewCLIRunner(t) + trnID := createTestTransformation(t, cli) + t.Cleanup(func() { deleteTransformation(t, cli, trnID) }) + + var trn Transformation + require.NoError(t, cli.RunJSON(&trn, "gateway", "transformation", "get", trnID)) + require.NotEmpty(t, trn.Name, "transformation name") + + existingConnection := func(t *testing.T) Connection { + t.Helper() + connID := createTestConnection(t, cli) + t.Cleanup(func() { deleteConnection(t, cli, connID) }) + var conn Connection + require.NoError(t, cli.RunJSON(&conn, "gateway", "connection", "get", connID)) + require.NotEmpty(t, conn.Name, "connection name") + return conn + } + + for _, tc := range []struct{ name, value string }{{"by ID", trnID}, {"by name", trn.Name}} { + t.Run("attaches existing "+tc.name, func(t *testing.T) { + conn := existingConnection(t) + + var upserted Connection + require.NoError(t, cli.RunJSON(&upserted, + "gateway", "connection", "upsert", conn.Name, + "--rule-transform-name", tc.value, + ), "Should upsert connection to reference an existing transformation") + + var fetched Connection + require.NoError(t, cli.RunJSON(&fetched, "gateway", "connection", "get", conn.ID)) + require.Len(t, fetched.Rules, 1, "Connection should have one rule") + assert.Equal(t, "transform", fetched.Rules[0]["type"]) + assert.Equal(t, trnID, fetched.Rules[0]["transformation_id"], + "Rule should reference the existing transformation, not a new one") + }) + } + + for _, value := range []string{"trs_doesnotexist0", "test-trn-missing-" + generateTimestamp()} { + t.Run("rejects unknown "+value, func(t *testing.T) { + conn := existingConnection(t) + + stdout, stderr, err := cli.Run("gateway", "connection", "upsert", conn.Name, "--rule-transform-name", value) + require.Error(t, err, "Unknown transformation without code should fail\nstdout: %s", stdout) + assert.Contains(t, stderr+stdout, "transformation '"+value+"' not found") + }) + } +} From a1ec85037bfbd7945f8308a03b3c25e9649da128 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Thu, 5 Nov 2026 15:21:02 +0000 Subject: [PATCH 4/4] docs(known-issues): record --rule-transform-name attaching empty transformations Symptom, cause, affected versions (v3.1.0 and earlier), the fix version, a --rules workaround, and how to find and delete the empty transformations it left behind. Also stops the lookup's comment claiming the API's name filter returns near matches, which hasn't been verified. The exact compare and pagination hold either way. Co-Authored-By: Claude Opus 5.5 --- KNOWN_ISSUES.md | 27 +++++++++++++++++++++++++++ pkg/cmd/connection_transform_rule.go | 7 ++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/KNOWN_ISSUES.md b/KNOWN_ISSUES.md index d1ec420a..a0865235 100644 --- a/KNOWN_ISSUES.md +++ b/KNOWN_ISSUES.md @@ -1,5 +1,32 @@ # Known Issues +## `--rule-transform-name` with an ID or a mistyped name attaches an empty transformation + +**Symptom:** `hookdeck gateway connection create`, `update` or `upsert` with `--rule-transform-name` +set to a transformation ID, or to a name that doesn't exist, succeeds, but every event on the +connection then fails its transformation with a `TypeError`. + +**Why:** The CLI always sent the value as a transformation name. When no transformation has that +name, the API creates one, even with no code, so the connection got a new, empty transformation +instead of the one you meant. + +**Affected:** v3.1.0 and earlier. + +**Recommended fix:** Update to **v3.2.0 or later**. The value is then resolved as an ID or an exact +name, and an unknown value without `--rule-transform-code` is an error. + +- npm: `npm install -g hookdeck-cli@latest` +- Homebrew: `brew upgrade hookdeck` + +**Workaround (until you update):** Pass the rule as JSON with `--rules` or `--rules-file`, +referencing the transformation by `transformation_id`. + +**Cleanup:** If you hit this, you have empty transformations named after an ID (`trs_...`) or a +typo. Find them with `hookdeck gateway transformation list`. Point any connection that uses one at +the right transformation, then delete it with `hookdeck gateway transformation delete `. + +**Tracking:** #468 + ## Connection rules built from `--rule-*` flags run in a fixed order, not the order you wrote them **Symptom:** A connection created, updated or upserted with `--rule-*` flags runs its rules in a diff --git a/pkg/cmd/connection_transform_rule.go b/pkg/cmd/connection_transform_rule.go index b414d0f5..690ce693 100644 --- a/pkg/cmd/connection_transform_rule.go +++ b/pkg/cmd/connection_transform_rule.go @@ -9,9 +9,10 @@ import ( "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" ) -// The name filter on GET /transformations can return near matches, so the -// exact name is not necessarily on the first page. The lookup asks for the -// largest page the API allows and follows pagination.next, up to a bound. +// The lookup doesn't rely on the name filter on GET /transformations being an +// exact match. It compares names exactly and, in case the filter matches more +// than one page, asks for the largest page the API allows and follows +// pagination.next, up to a bound. const ( transformationLookupPageLimit = 255 transformationLookupMaxPages = 20