From 63ca2b19ca564ea8a7bd19fcf6cf1d971a408c13 Mon Sep 17 00:00:00 2001 From: Mike Sawka Date: Mon, 28 Sep 2026 22:33:21 -0700 Subject: [PATCH] feat: add ordered --add field mutations --- AGENTS.md | 6 ++ pkg/cmdctx/cmdctx.go | 48 +++++++------- pkg/registry/buildctx.go | 54 ++++++++-------- pkg/registry/buildctx_workflow_test.go | 9 +-- pkg/registry/endpoint.go | 30 ++++++++- pkg/registry/endpoint_test.go | 29 +++++++++ pkg/registry/fieldtype.go | 49 ++++++++++++-- pkg/registry/fieldtype_test.go | 2 +- pkg/registry/flag.go | 1 + pkg/registry/mutation.go | 40 ++++++++---- pkg/registry/mutation_flags.go | 88 ++++++++++++++++++++++++++ pkg/registry/mutation_test.go | 51 +++++++++++++++ pkg/registry/registry.go | 14 +--- pkg/spec/spec.go | 4 +- 14 files changed, 338 insertions(+), 87 deletions(-) create mode 100644 pkg/registry/mutation_flags.go diff --git a/AGENTS.md b/AGENTS.md index 0ac710a..549b375 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -231,6 +231,12 @@ harness list pr_activity / The CLI reads auth from the active profile (typically `~/.harness/profiles.yaml`). For endpoint-backed create/update/execute commands, append the hidden `--preview-request` flag to inspect the assembled URL and body without sending the write (an update may still perform a preparatory GET). +## Test selection + +Add tests for behavior with a plausible, non-obvious failure mode: interacting operations, ordering, branching rules, boundary cases, error handling, and exact request-body transformations. Prefer compact direct/table tests at the layer that owns that logic; run existing suites for the rest. + +Do not add tests for mechanical pass-through or framework guarantees (e.g. Cobra accepting a registered flag, a loop forwarding an element, or a handler receiving an operation explicitly supplied by the test). Do not build mock commands or HTTP servers just to reassert a body shape already covered by a direct mutation test. If you cannot name a realistic mistake the test would catch, omit it; redundant tests add maintenance and context cost. + ## Current spec files | File | Commands | diff --git a/pkg/cmdctx/cmdctx.go b/pkg/cmdctx/cmdctx.go index 7f84bb6..26defd2 100644 --- a/pkg/cmdctx/cmdctx.go +++ b/pkg/cmdctx/cmdctx.go @@ -122,6 +122,7 @@ type MutationKind string const ( MutationSet MutationKind = "set" + MutationAdd MutationKind = "add" MutationDelete MutationKind = "del" ) @@ -225,29 +226,30 @@ type PageMeta struct { // Auth is nil for management commands (version, etc.) that do not require credentials. // When Auth is non-nil, OrgID and ProjectID already reflect any --org/--project overrides. type Ctx struct { - Context context.Context - CancelFn context.CancelCauseFunc - Auth *auth.ResolvedAuth - Verb string - VerbHandler string // behavioral dispatch verb; defaults to Verb when verb_handler is unset in spec - Noun string - FieldsNoun string // overrides Noun for field lookup when set (from spec fields_noun) - Id string - ParentId string // optional parent-id arg for list commands (e.g. pipeline ID on "list execution") - MigrateFrom string // --from flag value (pair verbs, e.g. migrate: identifies the source endpoint) - MigrateTo string // --to flag value (pair verbs, e.g. migrate: identifies the destination endpoint) - SetArgs map[string]string // --set key=value pairs for update verb (when HasSetArg set on spec) - DelArgs []string // --del key targets for update verb (when HasSetArg set on spec) - MutationFlags []FieldMutation // set flags, positional sets, then delete flags; not original argv interleaving - Args []string // extra positional args beyond [id] (when HasArgs set on spec) - IdParts []string // id split on "/" when id_parts > 1 on spec; length equals the number of actual parts - Level string // scope level: "account", "org", or "project" (empty when flag not present) - IsPty bool // true when stdout is an interactive terminal - IsCompletion bool // true when this ctx was built for a shell completion request - Resolver Resolver - GlobalFlags GlobalFlags - FormatFlags FormatFlags - PagingFlags PagingFlags + Context context.Context + CancelFn context.CancelCauseFunc + Auth *auth.ResolvedAuth + Verb string + VerbHandler string // behavioral dispatch verb; defaults to Verb when verb_handler is unset in spec + Noun string + FieldsNoun string // overrides Noun for field lookup when set (from spec fields_noun) + Id string + ParentId string // optional parent-id arg for list commands (e.g. pipeline ID on "list execution") + MigrateFrom string // --from flag value (pair verbs, e.g. migrate: identifies the source endpoint) + MigrateTo string // --to flag value (pair verbs, e.g. migrate: identifies the destination endpoint) + SetArgs map[string]string // --set key=value pairs for update verb (when HasSetArg set on spec) + DelArgs []string // --del key targets for update verb (when HasSetArg set on spec) + MutationFlags []FieldMutation // explicit flags in parse order, followed by positional sets + MutationOrderCaptured bool // use MutationFlags rather than the legacy SetArgs/DelArgs views + Args []string // extra positional args beyond [id] (when HasArgs set on spec) + IdParts []string // id split on "/" when id_parts > 1 on spec; length equals the number of actual parts + Level string // scope level: "account", "org", or "project" (empty when flag not present) + IsPty bool // true when stdout is an interactive terminal + IsCompletion bool // true when this ctx was built for a shell completion request + Resolver Resolver + GlobalFlags GlobalFlags + FormatFlags FormatFlags + PagingFlags PagingFlags // FlagValues holds typed flag values for this command, keyed by flag name. It contains: // - all flags declared in the spec (cs.Flags), typed as string/bool/[]string // - "page" int (0-indexed) when the spec declares builtin_flags.page diff --git a/pkg/registry/buildctx.go b/pkg/registry/buildctx.go index 9597808..f37b364 100644 --- a/pkg/registry/buildctx.go +++ b/pkg/registry/buildctx.go @@ -265,36 +265,40 @@ func buildCtx(cmd *cobra.Command, cs *spec.CommandSpec, args []string, r *Regist } ctx.Args = extra } - if cs.BuiltinFlags.Set { - setVals, _ := cmd.Flags().GetStringArray("set") - // positional args after the id are also treated as key=value pairs - positional := args - if consumedIdArg { - positional = args[1:] - } - all := append(setVals, positional...) - if len(all) > 0 { - ctx.SetArgs = make(map[string]string, len(all)) - for _, kv := range all { - k, v, ok := strings.Cut(kv, "=") - if !ok && !isBareSetField(k, cs, r) { - return nil, fmt.Errorf("invalid value %q: expected key=value format", kv) + if cs.BuiltinFlags.Set || cs.BuiltinFlags.Del { + ctx.MutationOrderCaptured = true + for _, op := range capturedMutationFlags(cmd) { + op.Key, op.Value, op.HasValue = strings.Cut(op.Raw, "=") + switch op.Kind { + case cmdctx.MutationSet: + if !op.HasValue && !isBareSetField(op.Key, cs, r) { + return nil, fmt.Errorf("invalid value %q: expected key=value format", op.Raw) } - ctx.MutationFlags = append(ctx.MutationFlags, cmdctx.FieldMutation{ - Kind: cmdctx.MutationSet, Raw: kv, Key: k, Value: v, HasValue: ok, - }) - ctx.SetArgs[k] = v + if ctx.SetArgs == nil { + ctx.SetArgs = map[string]string{} + } + ctx.SetArgs[op.Key] = op.Value + case cmdctx.MutationDelete: + ctx.DelArgs = append(ctx.DelArgs, op.Raw) } + ctx.MutationFlags = append(ctx.MutationFlags, op) } - } - if cs.BuiltinFlags.Del { - delVals, _ := cmd.Flags().GetStringArray("del") - if len(delVals) > 0 { - ctx.DelArgs = delVals - for _, raw := range delVals { + if cs.BuiltinFlags.Set { + positional := args + if consumedIdArg { + positional = args[1:] + } + for _, raw := range positional { key, value, hasValue := strings.Cut(raw, "=") + if !hasValue && !isBareSetField(key, cs, r) { + return nil, fmt.Errorf("invalid value %q: expected key=value format", raw) + } + if ctx.SetArgs == nil { + ctx.SetArgs = map[string]string{} + } + ctx.SetArgs[key] = value ctx.MutationFlags = append(ctx.MutationFlags, cmdctx.FieldMutation{ - Kind: cmdctx.MutationDelete, Raw: raw, Key: key, Value: value, HasValue: hasValue, + Kind: cmdctx.MutationSet, Raw: raw, Key: key, Value: value, HasValue: hasValue, }) } } diff --git a/pkg/registry/buildctx_workflow_test.go b/pkg/registry/buildctx_workflow_test.go index cee509a..d17b3ab 100644 --- a/pkg/registry/buildctx_workflow_test.go +++ b/pkg/registry/buildctx_workflow_test.go @@ -754,7 +754,7 @@ func TestBuildCtx_MutationFlagsPreserveRawOperands(t *testing.T) { t.Fatal(err) } cs := &spec.CommandSpec{Command: "update widget", Verb: VerbUpdate, Noun: "widget", - NoAuth: true, BuiltinFlags: spec.BuiltinFlags{Set: true, Del: true}} + NoAuth: true, BuiltinFlags: spec.BuiltinFlags{Set: true}} cmd := &cobra.Command{Use: "widget"} if workflow { cs.HandlerType = spec.HandlerWorkflow @@ -765,7 +765,7 @@ func TestBuildCtx_MutationFlagsPreserveRawOperands(t *testing.T) { } cmd.Flags().Float64("timeout", 0, "Command timeout in seconds") if err := cmd.ParseFlags([]string{"id", "--set", "modules.CD", "--set=modules.CD=", "--set", "owner=user:a@x.com", - "--set", "name=a=b", "--del", "owners=", "--del=modules.CD", "description=positional"}); err != nil { + "--del", "owners=", "--add=modules.CI", "--set", "name=a=b", "--del=modules.CD", "description=positional"}); err != nil { t.Fatal(err) } ctx, err := buildCtx(cmd, cs, cmd.Flags().Args(), r) @@ -776,10 +776,11 @@ func TestBuildCtx_MutationFlagsPreserveRawOperands(t *testing.T) { {Kind: cmdctx.MutationSet, Raw: "modules.CD", Key: "modules.CD"}, {Kind: cmdctx.MutationSet, Raw: "modules.CD=", Key: "modules.CD", HasValue: true}, {Kind: cmdctx.MutationSet, Raw: "owner=user:a@x.com", Key: "owner", Value: "user:a@x.com", HasValue: true}, - {Kind: cmdctx.MutationSet, Raw: "name=a=b", Key: "name", Value: "a=b", HasValue: true}, - {Kind: cmdctx.MutationSet, Raw: "description=positional", Key: "description", Value: "positional", HasValue: true}, {Kind: cmdctx.MutationDelete, Raw: "owners=", Key: "owners", HasValue: true}, + {Kind: cmdctx.MutationAdd, Raw: "modules.CI", Key: "modules.CI"}, + {Kind: cmdctx.MutationSet, Raw: "name=a=b", Key: "name", Value: "a=b", HasValue: true}, {Kind: cmdctx.MutationDelete, Raw: "modules.CD", Key: "modules.CD"}, + {Kind: cmdctx.MutationSet, Raw: "description=positional", Key: "description", Value: "positional", HasValue: true}, } if !reflect.DeepEqual(ctx.MutationFlags, want) { t.Fatalf("MutationFlags = %#v, want %#v", ctx.MutationFlags, want) diff --git a/pkg/registry/endpoint.go b/pkg/registry/endpoint.go index 9a3c48b..e710493 100644 --- a/pkg/registry/endpoint.go +++ b/pkg/registry/endpoint.go @@ -856,9 +856,30 @@ func runGetThenPutKV(ctx *cmdctx.Ctx, ep *spec.EndpointSpec, c *client.Client, p } } - maps.Copy(kvMap, ctx.SetArgs) - for _, k := range ctx.DelArgs { - delete(kvMap, k) + if ctx.MutationOrderCaptured { + for _, op := range ctx.MutationFlags { + switch op.Kind { + case cmdctx.MutationSet: + kvMap[op.Key] = op.Value + case cmdctx.MutationDelete: + delete(kvMap, op.Raw) + case cmdctx.MutationAdd: + if !op.HasValue { + return nil, fmt.Errorf("--add %s: expected key=value", op.Key) + } + if existing, found := kvMap[op.Key]; found && existing != op.Value { + return nil, fmt.Errorf("--add %s: key already exists with a different value; use --set to overwrite it", op.Key) + } + kvMap[op.Key] = op.Value + default: + return nil, fmt.Errorf("unknown mutation operation %q", op.Kind) + } + } + } else { + maps.Copy(kvMap, ctx.SetArgs) + for _, k := range ctx.DelArgs { + delete(kvMap, k) + } } // Rebuild as [{key, value}, ...]. @@ -909,6 +930,9 @@ func runSetFields(ctx *cmdctx.Ctx, ep *spec.EndpointSpec, c *client.Client, path func mutationBodyForCtx(ctx *cmdctx.Ctx, base map[string]any, wrap string, extra map[string]any) (map[string]any, error) { ops := effectiveMutations(ctx.SetArgs, ctx.DelArgs, ctx.MutationFlags) + if ctx.MutationOrderCaptured { + ops = ctx.MutationFlags + } if len(ops) > 0 && ctx.Resolver == nil { return nil, fmt.Errorf("field type resolver is not available") } diff --git a/pkg/registry/endpoint_test.go b/pkg/registry/endpoint_test.go index 894f58d..cf7bd0d 100644 --- a/pkg/registry/endpoint_test.go +++ b/pkg/registry/endpoint_test.go @@ -658,8 +658,10 @@ func TestCallEndpointFull_Priority2_GetThenPutKV(t *testing.T) { name string setArgs map[string]string delArgs []string + mutations []cmdctx.FieldMutation wantPresent map[string]string wantAbsent []string + wantError string }{ { name: "upsert_existing_key", @@ -677,6 +679,25 @@ func TestCallEndpointFull_Priority2_GetThenPutKV(t *testing.T) { wantPresent: map[string]string{"env": "prod"}, wantAbsent: []string{"team"}, }, + { + name: "ordered_add_delete_and_set", + mutations: []cmdctx.FieldMutation{ + {Kind: cmdctx.MutationAdd, Key: "region", Value: "us", HasValue: true}, + {Kind: cmdctx.MutationAdd, Key: "env", Value: "prod", HasValue: true}, + {Kind: cmdctx.MutationDelete, Key: "team", Raw: "team"}, + {Kind: cmdctx.MutationSet, Key: "env", Value: "stage", HasValue: true}, + }, + wantPresent: map[string]string{"env": "stage", "region": "us"}, + wantAbsent: []string{"team"}, + }, + { + name: "add_conflict_prevents_write", + mutations: []cmdctx.FieldMutation{ + {Kind: cmdctx.MutationAdd, Key: "env", Value: "stage", HasValue: true}, + {Kind: cmdctx.MutationDelete, Key: "env", Raw: "env"}, + }, + wantError: "key already exists", + }, } for _, tc := range tests { @@ -685,6 +706,8 @@ func TestCallEndpointFull_Priority2_GetThenPutKV(t *testing.T) { ctx := testCtx(srv.URL, nil) ctx.SetArgs = tc.setArgs ctx.DelArgs = tc.delArgs + ctx.MutationFlags = tc.mutations + ctx.MutationOrderCaptured = len(tc.mutations) > 0 ctx.Resolver = testNounRegistry(t) ep := &spec.EndpointSpec{ @@ -692,6 +715,12 @@ func TestCallEndpointFull_Priority2_GetThenPutKV(t *testing.T) { UpdateStrategy: spec.UpdateStrategyGetThenPutKV, UpdateBodyWrap: "metadata", } _, _, err := callEndpointFull(ctx, ep, nil) + if tc.wantError != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantError) || len(*caps) != 1 { + t.Fatalf("error = %v, server requests = %d; want GET only and %q", err, len(*caps), tc.wantError) + } + return + } if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/registry/fieldtype.go b/pkg/registry/fieldtype.go index 9e77513..06b099c 100644 --- a/pkg/registry/fieldtype.go +++ b/pkg/registry/fieldtype.go @@ -37,19 +37,38 @@ func (r *Registry) ResolveFieldType(id string) (cmdctx.FieldTypeHandler, bool) { } func mutateScalar(_ spec.FieldDef, _ any, op cmdctx.FieldMutation) (any, bool, error) { - if op.Kind == cmdctx.MutationDelete { + switch op.Kind { + case cmdctx.MutationSet: + return op.Value, true, nil + case cmdctx.MutationDelete: + if op.HasValue { + return nil, false, fmt.Errorf("--del %s: scalar fields do not take a value", op.Raw) + } return nil, true, nil + case cmdctx.MutationAdd: + return nil, false, fmt.Errorf("--add %s: scalar fields do not support addition", op.Key) + default: + return nil, false, fmt.Errorf("unknown mutation operation %q", op.Kind) } - return op.Value, true, nil } func mutateTags(_ spec.FieldDef, current any, op cmdctx.FieldMutation) (any, bool, error) { - _, tag, found := strings.Cut(op.Key, ".") + if op.Kind != cmdctx.MutationSet && op.Kind != cmdctx.MutationAdd && op.Kind != cmdctx.MutationDelete { + return nil, false, fmt.Errorf("unknown mutation operation %q", op.Kind) + } + selector := op.Key + if op.Kind == cmdctx.MutationDelete && op.HasValue { + selector = op.Raw + } + _, tag, found := strings.Cut(selector, ".") if !found { if op.Kind == cmdctx.MutationDelete { return nil, false, fmt.Errorf("--del %s: tag fields require a key (e.g. --del tags.key)", op.Key) } - return nil, false, fmt.Errorf("--set %s: tag fields require a key (e.g. --set tags.key=value)", op.Key) + return nil, false, fmt.Errorf("--%s %s: tag fields require a key (e.g. --%s tags.key=value)", op.Kind, op.Key, op.Kind) + } + if op.Kind == cmdctx.MutationAdd && !op.HasValue { + return nil, false, fmt.Errorf("--add %s: tag fields require key=value", op.Key) } tags, _ := current.(map[string]any) if op.Kind == cmdctx.MutationDelete { @@ -64,16 +83,34 @@ func mutateTags(_ spec.FieldDef, current any, op cmdctx.FieldMutation) (any, boo if op.Kind == cmdctx.MutationDelete { delete(next, tag) } else { + if op.Kind == cmdctx.MutationAdd { + if existing, found := next[tag]; found { + if existing == op.Value { + return nil, false, nil + } + return nil, false, fmt.Errorf("--add %s: key already exists with a different value; use --set to overwrite it", op.Key) + } + } next[tag] = op.Value } return next, true, nil } func mutateStringSet(_ spec.FieldDef, current any, op cmdctx.FieldMutation) (any, bool, error) { - _, member, found := strings.Cut(op.Key, ".") - if !found || (op.Kind == cmdctx.MutationSet && member == "") { + if op.Kind != cmdctx.MutationSet && op.Kind != cmdctx.MutationAdd && op.Kind != cmdctx.MutationDelete { + return nil, false, fmt.Errorf("unknown mutation operation %q", op.Kind) + } + selector := op.Key + if op.Kind == cmdctx.MutationDelete && op.HasValue { + selector = op.Raw + } + _, member, found := strings.Cut(selector, ".") + if !found || member == "" { return nil, false, fmt.Errorf("--%s %s: set fields require a member (e.g. --%s modules.CD)", op.Kind, op.Key, op.Kind) } + if op.Kind == cmdctx.MutationAdd && op.HasValue { + return nil, false, fmt.Errorf("--add %s: set members do not take a value", op.Key) + } var arr []any switch values := current.(type) { case []any: diff --git a/pkg/registry/fieldtype_test.go b/pkg/registry/fieldtype_test.go index 8acebe8..d25d65f 100644 --- a/pkg/registry/fieldtype_test.go +++ b/pkg/registry/fieldtype_test.go @@ -134,7 +134,7 @@ func TestApplyMutations_ExistingNoWriteCases(t *testing.T) { } } -func TestApplyMutations_StillSetsBeforeDeleting(t *testing.T) { +func TestLegacyMutationsStillSetBeforeDeleting(t *testing.T) { r := New() field := spec.FieldDef{ID: "name", Expr: "it.name", MutablePath: "name"} m := map[string]any{"name": "old"} diff --git a/pkg/registry/flag.go b/pkg/registry/flag.go index 4a57f42..a50440d 100644 --- a/pkg/registry/flag.go +++ b/pkg/registry/flag.go @@ -129,6 +129,7 @@ var coreFlagTable = []CoreFlag{ {Name: "ui", Bool: true}, {Name: "force", Bool: true}, {Name: "set"}, + {Name: "add"}, {Name: "del"}, {Name: "preview-request", Bool: true}, {Name: "from"}, diff --git a/pkg/registry/mutation.go b/pkg/registry/mutation.go index 6cc7cec..96d1fe0 100644 --- a/pkg/registry/mutation.go +++ b/pkg/registry/mutation.go @@ -50,6 +50,10 @@ func effectiveMutations(sets map[string]string, dels []string, captured []cmdctx // buildMutationBody constructs a new JSON-shaped body without modifying its inputs. func buildMutationBody(base map[string]any, fields map[string]spec.FieldDef, handlers map[string]cmdctx.FieldTypeHandler, ops []cmdctx.FieldMutation, wrap string, extra map[string]any) (map[string]any, error) { mutable := cloneMutationMap(base) + working := map[string]any{} + initialized := map[string]bool{} + touched := map[string]bool{} + var touchOrder []string for _, op := range ops { fieldID, _, _ := strings.Cut(op.Key, ".") field, found := fields[fieldID] @@ -60,23 +64,37 @@ func buildMutationBody(base map[string]any, fields map[string]spec.FieldDef, han if !found || handler.Mutate == nil { return nil, fmt.Errorf("field %q: field_type %q has no registered mutator", field.ID, field.FieldType) } - current := cloneMutationValue(getDotPathValue(mutable, field.MutablePath)) - if handler.Normalize != nil { - var err error - current, err = handler.Normalize(field, current) - if err != nil { - return nil, err + current := working[fieldID] + if !initialized[fieldID] { + current = cloneMutationValue(getDotPathValue(mutable, field.MutablePath)) + if handler.Normalize != nil { + var err error + current, err = handler.Normalize(field, current) + if err != nil { + return nil, err + } } + working[fieldID] = current + initialized[fieldID] = true } - next, write, err := handler.Mutate(field, current, op) + next, write, err := handler.Mutate(field, cloneMutationValue(current), op) if err != nil { return nil, err } - if !write { - continue + if write { + working[fieldID] = next + if !touched[fieldID] { + touchOrder = append(touchOrder, fieldID) + } + touched[fieldID] = true } - if handler.Encode != nil { - next, err = handler.Encode(field, next) + } + for _, fieldID := range touchOrder { + field := fields[fieldID] + next := working[fieldID] + if handler := handlers[field.FieldType]; handler.Encode != nil { + var err error + next, err = handler.Encode(field, cloneMutationValue(next)) if err != nil { return nil, err } diff --git a/pkg/registry/mutation_flags.go b/pkg/registry/mutation_flags.go new file mode 100644 index 0000000..f3da822 --- /dev/null +++ b/pkg/registry/mutation_flags.go @@ -0,0 +1,88 @@ +// Copyright © 2026 Harness Inc. +// SPDX-License-Identifier: Apache-2.0 + +package registry + +import ( + "github.com/spf13/cobra" + "github.com/spf13/pflag" + + "github.com/harness/cli/v3/pkg/cmdctx" + "github.com/harness/cli/v3/pkg/spec" +) + +type mutationFlagLog struct { + records []cmdctx.FieldMutation +} + +type mutationFlagValue struct { + pflag.Value + slice pflag.SliceValue + kind cmdctx.MutationKind + log *mutationFlagLog +} + +func (v *mutationFlagValue) Set(raw string) error { + if err := v.Value.Set(raw); err != nil { + return err + } + v.log.records = append(v.log.records, cmdctx.FieldMutation{Kind: v.kind, Raw: raw}) + return nil +} + +func (v *mutationFlagValue) Append(raw string) error { + if err := v.slice.Append(raw); err != nil { + return err + } + v.log.records = append(v.log.records, cmdctx.FieldMutation{Kind: v.kind, Raw: raw}) + return nil +} + +func (v *mutationFlagValue) Replace(raw []string) error { + if err := v.slice.Replace(raw); err != nil { + return err + } + records := v.log.records[:0] + for _, record := range v.log.records { + if record.Kind != v.kind { + records = append(records, record) + } + } + v.log.records = records + for _, value := range raw { + v.log.records = append(v.log.records, cmdctx.FieldMutation{Kind: v.kind, Raw: value}) + } + return nil +} + +func (v *mutationFlagValue) GetSlice() []string { return v.slice.GetSlice() } + +func registerMutationFlags(cmd *cobra.Command, cs *spec.CommandSpec) { + if !cs.BuiltinFlags.Set && !cs.BuiltinFlags.Del { + return + } + log := &mutationFlagLog{} + register := func(name, usage string, kind cmdctx.MutationKind) { + cmd.Flags().StringArray(name, nil, usage) + flag := cmd.Flags().Lookup(name) + flag.Value = &mutationFlagValue{Value: flag.Value, slice: flag.Value.(pflag.SliceValue), kind: kind, log: log} + } + if cs.BuiltinFlags.Set { + register("set", "Set a field as key=value or a set member as field.member (repeatable)", cmdctx.MutationSet) + register("add", "Add a field member without replacing an existing value (repeatable)", cmdctx.MutationAdd) + } + if cs.BuiltinFlags.Set || cs.BuiltinFlags.Del { + register("del", "Delete a field or field member (repeatable)", cmdctx.MutationDelete) + } +} + +func capturedMutationFlags(cmd *cobra.Command) []cmdctx.FieldMutation { + for _, name := range []string{"set", "del"} { + if flag := cmd.Flags().Lookup(name); flag != nil { + if value, ok := flag.Value.(*mutationFlagValue); ok { + return append([]cmdctx.FieldMutation(nil), value.log.records...) + } + } + } + return nil +} diff --git a/pkg/registry/mutation_test.go b/pkg/registry/mutation_test.go index 22e560d..e599146 100644 --- a/pkg/registry/mutation_test.go +++ b/pkg/registry/mutation_test.go @@ -63,6 +63,9 @@ func TestBuildMutationBody(t *testing.T) { set := func(key, value string) cmdctx.FieldMutation { return cmdctx.FieldMutation{Kind: cmdctx.MutationSet, Raw: key + "=" + value, Key: key, Value: value, HasValue: true} } + add := func(key, value string, hasValue bool) cmdctx.FieldMutation { + return cmdctx.FieldMutation{Kind: cmdctx.MutationAdd, Key: key, Value: value, HasValue: hasValue} + } del := func(key string) cmdctx.FieldMutation { return cmdctx.FieldMutation{Kind: cmdctx.MutationDelete, Raw: key, Key: key} } @@ -88,6 +91,19 @@ func TestBuildMutationBody(t *testing.T) { ops: []cmdctx.FieldMutation{set("nested.env", "staging")}, want: map[string]any{"config": map[string]any{"other": "keep", "tags": map[string]any{"env": "staging"}}}}, {name: "set_then_delete", base: map[string]any{"modules": []any{"CI"}}, ops: []cmdctx.FieldMutation{set("modules.CD", ""), del("modules.CD")}, want: map[string]any{"modules": []any{"CI"}}}, {name: "tag_value_with_equals", ops: []cmdctx.FieldMutation{set("tags.env", "a=b")}, want: map[string]any{"tags": map[string]any{"env": "a=b"}}}, + {name: "add_tags_and_set_members", base: base, ops: []cmdctx.FieldMutation{add("tags.region", "us", true), add("modules.CD", "", false), add("modules.CE", "", false)}, + want: map[string]any{"name": "keep", "tags": map[string]any{"env": "prod", "team": "ops", "region": "us"}, "modules": []any{"CI", "CE", "CD"}}}, + {name: "add_existing_same_value", base: base, ops: []cmdctx.FieldMutation{add("tags.env", "prod", true), add("modules.CI", "", false)}, want: base}, + {name: "add_empty_map_value", ops: []cmdctx.FieldMutation{add("tags.note", "", true)}, want: map[string]any{"tags": map[string]any{"note": ""}}}, + {name: "add_map_conflict", base: base, ops: []cmdctx.FieldMutation{add("tags.env", "staging", true)}, wantError: "key already exists"}, + {name: "add_map_missing_value", ops: []cmdctx.FieldMutation{add("tags.env", "", false)}, wantError: "key=value"}, + {name: "add_set_with_value", ops: []cmdctx.FieldMutation{add("modules.CD", "x", true)}, wantError: "do not take a value"}, + {name: "add_set_missing_member", ops: []cmdctx.FieldMutation{add("modules", "", false)}, wantError: "require a member"}, + {name: "add_scalar", ops: []cmdctx.FieldMutation{add("name", "new", true)}, wantError: "do not support addition"}, + {name: "delete_then_add", base: base, ops: []cmdctx.FieldMutation{del("tags.env"), add("tags.env", "staging", true)}, + want: map[string]any{"name": "keep", "tags": map[string]any{"env": "staging", "team": "ops"}, "modules": []any{"CI", "CE"}}}, + {name: "add_then_delete", base: base, ops: []cmdctx.FieldMutation{add("modules.CD", "", false), del("modules.CD")}, want: base}, + {name: "intermediate_add_conflict_aborts", base: base, ops: []cmdctx.FieldMutation{add("tags.env", "wrong", true), del("tags.env")}, wantError: "key already exists"}, {name: "unknown_field", ops: []cmdctx.FieldMutation{set("bad", "x")}, wantError: "unknown or read-only"}, {name: "invalid_tag_set", base: base, ops: []cmdctx.FieldMutation{set("tags", "x")}, wantError: "tag fields require a key"}, {name: "invalid_tag_delete", base: base, ops: []cmdctx.FieldMutation{del("tags")}, wantError: "tag fields require a key"}, @@ -141,6 +157,41 @@ func TestBuildMutationBodyIsolatesHandlersAndResult(t *testing.T) { } } +func TestBuildMutationBodyNormalizesAndEncodesOnce(t *testing.T) { + field := spec.FieldDef{ID: "items", MutablePath: "items", FieldType: "example:items"} + base := map[string]any{"items": []any{map[string]any{"name": "seed", "id": "read-only"}}} + normalizes, encodes := 0, 0 + handlers := map[string]cmdctx.FieldTypeHandler{"example:items": { + Normalize: func(_ spec.FieldDef, current any) (any, error) { + normalizes++ + if normalizes > 1 { + t.Fatal("normalized an already-mutated field") + } + return []any{current.([]any)[0].(map[string]any)["name"]}, nil + }, + Mutate: func(_ spec.FieldDef, current any, op cmdctx.FieldMutation) (any, bool, error) { + return append(current.([]any), op.Value), true, nil + }, + Encode: func(_ spec.FieldDef, current any) (any, error) { + encodes++ + out := []any{} + for _, name := range current.([]any) { + out = append(out, map[string]any{"name": name}) + } + return out, nil + }, + }} + ops := []cmdctx.FieldMutation{{Kind: cmdctx.MutationAdd, Key: "items", Value: "one"}, {Kind: cmdctx.MutationAdd, Key: "items", Value: "two"}} + got, err := buildMutationBody(base, map[string]spec.FieldDef{"items": field}, handlers, ops, "", nil) + want := map[string]any{"items": []any{map[string]any{"name": "seed"}, map[string]any{"name": "one"}, map[string]any{"name": "two"}}} + if err != nil || !reflect.DeepEqual(got, want) || normalizes != 1 || encodes != 1 { + t.Fatalf("body = %#v, error = %v, normalizes = %d, encodes = %d", got, err, normalizes, encodes) + } + if !reflect.DeepEqual(base, map[string]any{"items": []any{map[string]any{"name": "seed", "id": "read-only"}}}) { + t.Fatalf("input mutated: %#v", base) + } +} + func TestMutationBodyForCtxResolvesRegisteredTypes(t *testing.T) { r := New() if err := r.RegisterNoun(spec.NounDef{Noun: "widget", Fields: []spec.FieldDef{ diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index 2c1218d..6e451ba 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -990,12 +990,7 @@ func (r *Registry) bindWorkflowCmd(cmd *cobra.Command, cs *spec.CommandSpec, fn cmd.Flags().String("to", "", cs.MigrateTo.EffectiveLabel("Destination identifier to migrate to")) } } - if cs.BuiltinFlags.Set { - cmd.Flags().StringArray("set", nil, "Set a field value as key=value (repeatable)") - } - if cs.BuiltinFlags.Del { - cmd.Flags().StringArray("del", nil, "Delete a field or field member (repeatable)") - } + registerMutationFlags(cmd, cs) if cs.BuiltinFlags.UI { addFlag(cmd.Flags(), specUI) } @@ -1084,12 +1079,7 @@ func (r *Registry) bindEndpointCmdFlags(cmd *cobra.Command, cs *spec.CommandSpec addFlags(cmd.Flags(), specFormat, specJson) } addFlag(cmd.Flags(), specOut) - if cs.BuiltinFlags.Set { - cmd.Flags().StringArray("set", nil, "Set a field as key=value or a set member as field.member (repeatable)") - } - if cs.BuiltinFlags.Del { - cmd.Flags().StringArray("del", nil, "Delete a field or field member (repeatable)") - } + registerMutationFlags(cmd, cs) if ep.FileBody == spec.FileBodyRequired { addFlag(cmd.Flags(), specFile) cmd.MarkFlagRequired("file") diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 8b34a27..8f2bebb 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -150,8 +150,8 @@ func (m *MigrateFlag) UsageFragment(name string) string { // BuiltinFlags enables predefined system flags that have fixed registration and dispatch behavior. type BuiltinFlags struct { Page bool `yaml:"page,omitempty"` // --page N (1-indexed); exposed in expr as integer flags.page = N-1 - Set bool `yaml:"set,omitempty"` // --set key=value (repeatable); parsed into ctx.SetArgs - Del bool `yaml:"del,omitempty"` // --del key (repeatable); parsed into ctx.DelArgs + Set bool `yaml:"set,omitempty"` // Enables --set, --del, and --add; handlers decide which operations are valid. + Del bool `yaml:"del,omitempty"` // Retained for commands that only enable --del. UI bool `yaml:"ui,omitempty"` // --ui launch interactive TUI (requires both stdin and stdout to be a TTY) }