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/REFERENCE.md b/REFERENCE.md index b018b14c..ba436a18 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 1ad78943..94411147 100644 --- a/pkg/cmd/connection_common.go +++ b/pkg/cmd/connection_common.go @@ -135,9 +135,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..690ce693 --- /dev/null +++ b/pkg/cmd/connection_transform_rule.go @@ -0,0 +1,132 @@ +package cmd + +import ( + "context" + "fmt" + "strconv" + "strings" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" +) + +// 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 +) + +// 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) + } + } + + params := map[string]string{ + "name": nameOrID, + "limit": strconv.Itoa(transformationLookupPageLimit), + } + 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 "", 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 +// 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..374192d7 --- /dev/null +++ b/pkg/cmd/connection_transform_rule_test.go @@ -0,0 +1,225 @@ +package cmd + +import ( + "context" + "errors" + "fmt" + "strconv" + "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") +} + +// 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") +} 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 0ee84ef7..e1561680 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") + } + }) + } +} + // TestConnectionCreateRuleOrderFollowsFlags verifies that filter and transform rules // are stored in the order their flags were given. Filter, transform and deduplicate // rules run in array order, so filter-then-transform must survive the round trip. diff --git a/test/acceptance/connection_update_test.go b/test/acceptance/connection_update_test.go index 8f3d1109..83e0ec38 100644 --- a/test/acceptance/connection_update_test.go +++ b/test/acceptance/connection_update_test.go @@ -436,3 +436,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 c8ed0012..d7b54716 100644 --- a/test/acceptance/connection_upsert_test.go +++ b/test/acceptance/connection_upsert_test.go @@ -850,3 +850,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") + }) + } +}