From 4eb6f950bf12423894789985213538b559926883 Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Fri, 29 May 2026 09:04:09 +0300 Subject: [PATCH 1/5] feat(entity,flags,rpc): Add filter lifting, HTTP request population, and UI parameter roles Introduce three major capabilities: 1. Filter lifting (entity.go): Add Filterable interface and LiftFilters function to allow subcommand options structs to expose typed filter lookups that diverge from parent entity filters. This enables fine-grained filter control when positional arguments pin certain identifier filters. 2. HTTP request population (flags/request.go): Add PopulateFromRequest function to populate struct fields from HTTP request data without shared pflag state. This fixes state leakage across concurrent requests where slice values would accumulate across calls. Includes support for CSV expansion, file loading, and type parsing (int, bool, duration, time). 3. OpenAPI parameter roles (rpc/openapi.go): Add ClickyParameterMeta and paramRole function to classify query parameters for UI routing (pagination, time-range, filters) instead of generic text inputs. Roles are assigned based on parameter name conventions and operation capabilities. 4. Hierarchical subcommand paths (sub_command.go): Extend RegisterSubCommand and RegisterSubCommandFn to accept slash-delimited paths (e.g., "billing/policy") for nested command registration, creating missing intermediates and reusing existing nodes. Includes comprehensive test coverage for concurrent request isolation, CSV parsing, default values, file loading, parameter role classification, and path-aware subcommand registration. --- entity.go | 49 +++++++++++ flags/request.go | 185 ++++++++++++++++++++++++++++++++++++++++++ flags/request_test.go | 175 +++++++++++++++++++++++++++++++++++++++ rpc/openapi.go | 72 ++++++++++++++-- rpc/openapi_test.go | 98 ++++++++++++++++++++++ sub_command.go | 27 +++++- sub_command_test.go | 46 +++++++++++ 7 files changed, 644 insertions(+), 8 deletions(-) create mode 100644 flags/request.go create mode 100644 flags/request_test.go diff --git a/entity.go b/entity.go index c4eb1077..ce76d7dd 100644 --- a/entity.go +++ b/entity.go @@ -133,6 +133,55 @@ type Filter[ListOpts any] interface { Options(opts ListOpts) map[string]api.Textable } +// Filterable is implemented by AddNamedCommand options structs that want to +// expose typed filter lookups (dropdowns/typeaheads on the web UI, shell +// completions on the CLI) on the subcommand itself rather than inheriting +// only the parent entity's Filters slice. The same Filter[T] values that back +// an Entity's Filters slice work here; AddNamedCommand stores a LookupFunc +// in the lookupFuncRegistry and binds completions whenever an opts struct +// satisfies this interface. +// +// Use this whenever a subcommand's flag surface diverges from its parent +// list view — typically when a positional argument pins one or more +// identifier filters and the subcommand only exposes the orthogonal +// (status, type, date range) lenses. +type Filterable[T any] interface { + Filters() []Filter[T] +} + +// LiftFilters adapts a Filter[Inner] slice so the same picker definitions +// work against an outer struct that embeds (or otherwise reaches) the inner +// filter struct. The project func extracts a *Inner from any *Outer the +// filter system hands us; lookup keys, labels, options, and selected-value +// reads/writes flow through unchanged. Use this to keep picker constructors +// scoped to the inner filter type while letting subcommand options structs — +// which also carry positional args, behaviour flags, etc. — satisfy +// Filterable[Outer]. +func LiftFilters[Outer any, Inner any]( + filters []Filter[Inner], + project func(*Outer) *Inner, +) []Filter[Outer] { + out := make([]Filter[Outer], 0, len(filters)) + for _, f := range filters { + out = append(out, liftedFilter[Outer, Inner]{inner: f, project: project}) + } + return out +} + +type liftedFilter[Outer any, Inner any] struct { + inner Filter[Inner] + project func(*Outer) *Inner +} + +func (l liftedFilter[Outer, Inner]) Key() string { return l.inner.Key() } +func (l liftedFilter[Outer, Inner]) Label() string { return l.inner.Label() } +func (l liftedFilter[Outer, Inner]) Lookup(opts *Outer) (map[string]api.Textable, error) { + return l.inner.Lookup(l.project(opts)) +} +func (l liftedFilter[Outer, Inner]) Options(opts Outer) map[string]api.Textable { + return l.inner.Options(*l.project(&opts)) +} + // EntityAction is the type-erased registration surface for custom entity // actions. Use Action or ActionWithFlags to construct values. type EntityAction interface { diff --git a/flags/request.go b/flags/request.go new file mode 100644 index 00000000..27cf6594 --- /dev/null +++ b/flags/request.go @@ -0,0 +1,185 @@ +package flags + +import ( + "encoding/csv" + "fmt" + "reflect" + "strconv" + "strings" + + "github.com/flanksource/commons/duration" +) + +// PopulateFromRequest fills optsValue by parsing flagMap and args directly, +// without touching any shared pflag pointer. This is the entry point used by +// the HTTP data-func dispatcher in cobra_command.go. +// +// The CLI path keeps the existing pflag-backed pipeline (AssignFieldValue); +// only the HTTP path benefits from per-request isolation. Concurrent requests +// allocate their own optsValue, so no locking is required. +// +// Precedence per field: explicit flagMap[FlagName] → args (for the IsArgs +// field) → DefaultValue → type zero value. Stdin is intentionally skipped on +// this path — HTTP requests have no terminal. +func PopulateFromRequest(optsValue reflect.Value, fields []FieldInfo, flagMap map[string]string, args []string) error { + for _, info := range fields { + fieldValue := GetFieldByPath(optsValue, info.FieldPath) + if !fieldValue.IsValid() || !fieldValue.CanSet() { + return fmt.Errorf("cannot set field %s", info.FieldName) + } + + raw, hasRaw := pickRawValue(info, flagMap) + if err := assignFieldFromRequest(fieldValue, info, raw, hasRaw, args); err != nil { + return fmt.Errorf("field %s: %w", info.FieldName, err) + } + } + return nil +} + +// pickRawValue chooses the string input for a field, honouring the precedence +// flagMap → DefaultValue. Args and stdin are handled inside the per-type +// branch because their semantics differ (args may be a slice; stdin is +// off for HTTP). +func pickRawValue(info FieldInfo, flagMap map[string]string) (string, bool) { + if info.FlagName != "" { + if v, ok := flagMap[info.FlagName]; ok { + return v, true + } + } + if info.DefaultValue != "" { + return info.DefaultValue, true + } + return "", false +} + +func assignFieldFromRequest(fieldValue reflect.Value, info FieldInfo, raw string, hasRaw bool, args []string) error { + switch info.FieldType.Kind() { + case reflect.String: + val := raw + if !hasRaw && info.IsArgs && len(args) > 0 { + val = args[0] + } + loaded, err := loadFromFileOrURL(val) + if err != nil { + return err + } + fieldValue.SetString(loaded) + return nil + + case reflect.Int: + if !hasRaw { + fieldValue.SetInt(0) + return nil + } + n, err := strconv.Atoi(strings.TrimSpace(raw)) + if err != nil { + return fmt.Errorf("parsing int: %w", err) + } + fieldValue.SetInt(int64(n)) + return nil + + case reflect.Bool: + if !hasRaw { + fieldValue.SetBool(false) + return nil + } + b, err := strconv.ParseBool(strings.TrimSpace(raw)) + if err != nil { + return fmt.Errorf("parsing bool: %w", err) + } + fieldValue.SetBool(b) + return nil + + case reflect.Slice: + return assignSliceFromRequest(fieldValue, info, raw, hasRaw, args) + + default: + switch info.FieldType.String() { + case "duration.Duration": + if !hasRaw { + fieldValue.Set(reflect.ValueOf(duration.Duration(0))) + return nil + } + d, err := duration.ParseDuration(strings.TrimSpace(raw)) + if err != nil { + return fmt.Errorf("parsing duration: %w", err) + } + fieldValue.Set(reflect.ValueOf(d)) + return nil + + case "time.Time": + if !hasRaw { + return nil + } + t, err := parseTime(raw) + if err != nil { + return fmt.Errorf("parsing time: %w", err) + } + fieldValue.Set(reflect.ValueOf(t)) + return nil + } + } + return nil +} + +func assignSliceFromRequest(fieldValue reflect.Value, info FieldInfo, raw string, hasRaw bool, args []string) error { + elemKind := info.FieldType.Elem().Kind() + + var tokens []string + switch { + case hasRaw: + parsed, err := readAsCSVRecord(raw) + if err != nil { + return fmt.Errorf("parsing CSV: %w", err) + } + tokens = parsed + case info.IsArgs && len(args) > 0: + tokens = args + } + + switch elemKind { + case reflect.String: + if len(tokens) == 1 { + lines, err := loadLinesFromFileOrURL(tokens[0]) + if err != nil { + return err + } + tokens = lines + } + if tokens == nil { + fieldValue.Set(reflect.Zero(info.FieldType)) + return nil + } + return assignValue(fieldValue, tokens) + + case reflect.Int: + if tokens == nil { + fieldValue.Set(reflect.Zero(info.FieldType)) + return nil + } + out := make([]int, 0, len(tokens)) + for _, tok := range tokens { + n, err := strconv.Atoi(strings.TrimSpace(tok)) + if err != nil { + return fmt.Errorf("parsing int slice element %q: %w", tok, err) + } + out = append(out, n) + } + return assignValue(fieldValue, out) + } + return nil +} + +// readAsCSVRecord mirrors pflag.stringSliceValue.Set: parse the raw string as +// one CSV record. An empty string yields nil so the field is left at the zero +// value rather than a one-element [""] slice. +func readAsCSVRecord(raw string) ([]string, error) { + if raw == "" { + return nil, nil + } + rec, err := csv.NewReader(strings.NewReader(raw)).Read() + if err != nil { + return nil, err + } + return rec, nil +} diff --git a/flags/request_test.go b/flags/request_test.go new file mode 100644 index 00000000..2582aae5 --- /dev/null +++ b/flags/request_test.go @@ -0,0 +1,175 @@ +package flags + +import ( + "fmt" + "os" + "path/filepath" + "reflect" + "sync" + "testing" + "time" + + "github.com/flanksource/commons/duration" +) + +type reqOpts struct { + Slice []string `flag:"slice"` + IntList []int `flag:"ints"` + Name string `flag:"name"` + Count int `flag:"count" default:"42"` + Verbose bool `flag:"verbose"` + MaxAge duration.Duration `flag:"max-age" default:"7d"` + Since time.Time `flag:"since"` +} + +func populate(t *testing.T, optsValue reflect.Value, flagMap map[string]string, args []string) { + t.Helper() + fields, err := ParseStructFields(reflect.TypeOf(reqOpts{})) + if err != nil { + t.Fatalf("ParseStructFields: %v", err) + } + if err := PopulateFromRequest(optsValue, fields, flagMap, args); err != nil { + t.Fatalf("PopulateFromRequest: %v", err) + } +} + +// TestPopulateFromRequest_NoStateLeakAcrossCalls is the regression test for +// the bug that motivated this file: pflag's shared slice value appended on +// successive Set() calls, so a sequence of HTTP requests carried the union +// of every product/plan GUID ever supplied. The new path allocates a fresh +// opts struct per call, so a later call with no slice value must observe +// an empty slice rather than the prior call's tokens. +func TestPopulateFromRequest_NoStateLeakAcrossCalls(t *testing.T) { + var first, second, third reqOpts + populate(t, reflect.ValueOf(&first).Elem(), map[string]string{"slice": "A"}, nil) + populate(t, reflect.ValueOf(&second).Elem(), map[string]string{"slice": "B"}, nil) + populate(t, reflect.ValueOf(&third).Elem(), map[string]string{}, nil) + + if got, want := first.Slice, []string{"A"}; !reflect.DeepEqual(got, want) { + t.Errorf("first.Slice = %v; want %v", got, want) + } + if got, want := second.Slice, []string{"B"}; !reflect.DeepEqual(got, want) { + t.Errorf("second.Slice = %v; want %v (must replace, not append)", got, want) + } + if third.Slice != nil { + t.Errorf("third.Slice = %v; want nil (must reset to zero)", third.Slice) + } +} + +func TestPopulateFromRequest_CSVExpansion(t *testing.T) { + var opts reqOpts + populate(t, reflect.ValueOf(&opts).Elem(), map[string]string{"slice": "A,B,C"}, nil) + if got, want := opts.Slice, []string{"A", "B", "C"}; !reflect.DeepEqual(got, want) { + t.Errorf("Slice = %v; want %v", got, want) + } +} + +func TestPopulateFromRequest_IntSliceCSV(t *testing.T) { + var opts reqOpts + populate(t, reflect.ValueOf(&opts).Elem(), map[string]string{"ints": "1,2,3"}, nil) + if got, want := opts.IntList, []int{1, 2, 3}; !reflect.DeepEqual(got, want) { + t.Errorf("IntList = %v; want %v", got, want) + } +} + +func TestPopulateFromRequest_ScalarParsing(t *testing.T) { + var opts reqOpts + populate(t, reflect.ValueOf(&opts).Elem(), map[string]string{ + "name": "moshe", + "count": "7", + "verbose": "true", + }, nil) + if opts.Name != "moshe" { + t.Errorf("Name = %q; want moshe", opts.Name) + } + if opts.Count != 7 { + t.Errorf("Count = %d; want 7", opts.Count) + } + if !opts.Verbose { + t.Errorf("Verbose = false; want true") + } +} + +func TestPopulateFromRequest_DefaultsApply(t *testing.T) { + var opts reqOpts + populate(t, reflect.ValueOf(&opts).Elem(), nil, nil) + if opts.Count != 42 { + t.Errorf("Count = %d; want default 42", opts.Count) + } + expectedMaxAge, _ := duration.ParseDuration("7d") + if opts.MaxAge != expectedMaxAge { + t.Errorf("MaxAge = %v; want default 7d (%v)", opts.MaxAge, expectedMaxAge) + } +} + +func TestPopulateFromRequest_AtFileResolution(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "items.txt") + if err := os.WriteFile(path, []byte("alpha\nbeta\ngamma\n"), 0o600); err != nil { + t.Fatalf("write tmp file: %v", err) + } + + var opts reqOpts + populate(t, reflect.ValueOf(&opts).Elem(), map[string]string{"slice": "@" + path}, nil) + if got, want := opts.Slice, []string{"alpha", "beta", "gamma"}; !reflect.DeepEqual(got, want) { + t.Errorf("Slice = %v; want %v (file loading must be preserved)", got, want) + } +} + +// TestPopulateFromRequest_Concurrent fires the data-path under -race with +// interleaved payloads. Every result must reflect only its own input — if +// any goroutine sees another's tokens or scalar values, the race detector +// or the equality check will fail. 200 iterations × 2 workers is enough to +// catch a pflag-style shared-pointer regression every time on a typical +// laptop without dragging the suite out. +func TestPopulateFromRequest_Concurrent(t *testing.T) { + const iterations = 200 + var wg sync.WaitGroup + wg.Add(2) + + check := func(want []string, scalarName string, scalarCount int) { + defer wg.Done() + for i := 0; i < iterations; i++ { + var opts reqOpts + populate(t, reflect.ValueOf(&opts).Elem(), map[string]string{ + "slice": want[0], + "name": scalarName, + "count": fmt.Sprintf("%d", scalarCount), + }, nil) + if !reflect.DeepEqual(opts.Slice, want) { + t.Errorf("iter %d: Slice = %v; want %v", i, opts.Slice, want) + return + } + if opts.Name != scalarName { + t.Errorf("iter %d: Name = %q; want %q", i, opts.Name, scalarName) + return + } + if opts.Count != scalarCount { + t.Errorf("iter %d: Count = %d; want %d", i, opts.Count, scalarCount) + return + } + } + } + + go check([]string{"alpha"}, "worker-A", 1) + go check([]string{"omega"}, "worker-B", 2) + wg.Wait() +} + +type argsOpts struct { + IDs []string `flag:"ids" args:"true"` +} + +func TestPopulateFromRequest_ArgsFallThrough(t *testing.T) { + fields, err := ParseStructFields(reflect.TypeOf(argsOpts{})) + if err != nil { + t.Fatalf("ParseStructFields: %v", err) + } + var opts argsOpts + if err := PopulateFromRequest(reflect.ValueOf(&opts).Elem(), fields, nil, []string{"a", "b"}); err != nil { + t.Fatalf("PopulateFromRequest: %v", err) + } + if got, want := opts.IDs, []string{"a", "b"}; !reflect.DeepEqual(got, want) { + t.Errorf("IDs = %v; want %v", got, want) + } +} diff --git a/rpc/openapi.go b/rpc/openapi.go index d7592a63..ad26ff1e 100644 --- a/rpc/openapi.go +++ b/rpc/openapi.go @@ -76,12 +76,30 @@ type OpenAPIOperation struct { // OpenAPIParameter represents a parameter in the OpenAPI spec type OpenAPIParameter struct { - Name string `json:"name"` - In string `json:"in"` - Description string `json:"description,omitempty"` - Required bool `json:"required,omitempty"` - Schema *OpenAPISchema `json:"schema,omitempty"` - Example interface{} `json:"example,omitempty"` + Name string `json:"name"` + In string `json:"in"` + Description string `json:"description,omitempty"` + Required bool `json:"required,omitempty"` + Schema *OpenAPISchema `json:"schema,omitempty"` + Example interface{} `json:"example,omitempty"` + Clicky *ClickyParameterMeta `json:"x-clicky,omitempty"` +} + +// ClickyParameterMeta carries UI hints for a single OpenAPI parameter so the +// explorer can route it to the right widget instead of treating every query +// param as a free-text input. Role is derived from the parameter name plus +// the operation's lookup capability — see paramRole. +type ClickyParameterMeta struct { + // Role tells the UI how to render this parameter. Empty means "no + // special handling". Recognised values: + // - "filter" — show as a filter pill in the DataTable's FilterBar + // (entity .Filters(...) keys or any list-op query param + // when the op declares SupportsLookup). + // - "limit" — drives DataTable pagination's pageSize. + // - "offset" — drives DataTable pagination's page. + // - "time-from" — left edge of a time-range picker. + // - "time-to" — right edge of a time-range picker. + Role string `json:"role,omitempty"` } // OpenAPIRequestBody represents a request body in the OpenAPI spec @@ -317,6 +335,8 @@ func (g *OpenAPIGenerator) convertOperationToOpenAPI(op RPCOperation) OpenAPIOpe } // Convert parameters + supportsLookup := op.Clicky != nil && op.Clicky.SupportsLookup + isListOp := op.Clicky != nil && op.Clicky.Verb == "list" for _, param := range op.Parameters { openAPIParam := OpenAPIParameter{ Name: param.Name, @@ -329,6 +349,9 @@ func (g *OpenAPIGenerator) convertOperationToOpenAPI(op RPCOperation) OpenAPIOpe Default: param.Default, }), } + if role := paramRole(param, supportsLookup, isListOp); role != "" { + openAPIParam.Clicky = &ClickyParameterMeta{Role: role} + } openAPIOp.Parameters = append(openAPIOp.Parameters, openAPIParam) } @@ -375,6 +398,43 @@ func (g *OpenAPIGenerator) convertOperationToOpenAPI(op RPCOperation) OpenAPIOpe return openAPIOp } +// paramRole classifies a parameter so the UI can route it to the right widget +// (pagination footer, time-range picker, filter pill) instead of falling back +// to a generic text input. Returns "" when the parameter has no special role. +// +// Pagination roles ("limit", "offset") apply only on list operations because +// non-list ops that happen to take a literal `limit` flag would otherwise be +// hijacked. Time-range roles trigger on the conventional `since`/`from` and +// `to`/`until` names used across the codebase. +// +// "filter" is assigned to any remaining query-string parameter on a list op +// that declares SupportsLookup — that flag is the signal that the entity +// registered explicit Filter[ListOpts] entries, so every non-pagination query +// param maps to a filter chip. Non-query params (path/body/header) and params +// on non-lookup ops get no role. +func paramRole(param RPCParameter, supportsLookup, isListOp bool) string { + if param.In != "query" { + return "" + } + name := strings.ToLower(param.Name) + if isListOp { + switch name { + case "limit": + return "limit" + case "offset": + return "offset" + case "since", "from": + return "time-from" + case "to", "until": + return "time-to" + } + } + if isListOp && supportsLookup { + return "filter" + } + return "" +} + func (g *OpenAPIGenerator) executionResponseSchema() *OpenAPISchema { return g.convertGoTypeToOpenAPI(reflect.TypeOf(ExecutionResponse{})) } diff --git a/rpc/openapi_test.go b/rpc/openapi_test.go index b6aa9e16..3e1aa46e 100644 --- a/rpc/openapi_test.go +++ b/rpc/openapi_test.go @@ -680,3 +680,101 @@ func getMethodNames(pathItem OpenAPIPath) []string { } return methods } + +func TestParamRole(t *testing.T) { + tests := []struct { + name string + param RPCParameter + supportsLookup bool + isListOp bool + want string + }{ + { + name: "limit query on list op", + param: RPCParameter{Name: "limit", In: "query"}, + isListOp: true, + want: "limit", + }, + { + name: "offset query on list op", + param: RPCParameter{Name: "offset", In: "query"}, + isListOp: true, + want: "offset", + }, + { + name: "since query on list op", + param: RPCParameter{Name: "since", In: "query"}, + isListOp: true, + want: "time-from", + }, + { + name: "from query on list op", + param: RPCParameter{Name: "from", In: "query"}, + isListOp: true, + want: "time-from", + }, + { + name: "to query on list op", + param: RPCParameter{Name: "to", In: "query"}, + isListOp: true, + want: "time-to", + }, + { + name: "until query on list op", + param: RPCParameter{Name: "until", In: "query"}, + isListOp: true, + want: "time-to", + }, + { + name: "named query on lookup list op becomes filter", + param: RPCParameter{Name: "name", In: "query"}, + supportsLookup: true, + isListOp: true, + want: "filter", + }, + { + name: "named query on list op without lookup gets no role", + param: RPCParameter{Name: "name", In: "query"}, + isListOp: true, + want: "", + }, + { + name: "named query on non-list op with lookup gets no role", + param: RPCParameter{Name: "name", In: "query"}, + supportsLookup: true, + want: "", + }, + { + name: "limit query on non-list op gets no role", + param: RPCParameter{Name: "limit", In: "query"}, + isListOp: false, + want: "", + }, + { + name: "path parameter gets no role even when filterable name", + param: RPCParameter{Name: "name", In: "path"}, + supportsLookup: true, + isListOp: true, + want: "", + }, + { + name: "body parameter gets no role", + param: RPCParameter{Name: "limit", In: "body"}, + supportsLookup: true, + isListOp: true, + want: "", + }, + { + name: "uppercase LIMIT still detected", + param: RPCParameter{Name: "LIMIT", In: "query"}, + isListOp: true, + want: "limit", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := paramRole(tt.param, tt.supportsLookup, tt.isListOp) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/sub_command.go b/sub_command.go index 9ef0df33..1ad947e4 100644 --- a/sub_command.go +++ b/sub_command.go @@ -1,6 +1,7 @@ package clicky import ( + "strings" "sync" "github.com/spf13/cobra" @@ -19,9 +20,14 @@ type pendingSubCommand struct { // RegisterSubCommand defers attaching cmd as a child of a parent cobra command // identified by parentName. The attachment is performed by GenerateCLI after -// all entity parents have been created. If no command named parentName exists +// all entity parents have been created. If no command at parentName exists // after entity generation, a thin parent command is created to host cmd. // +// parentName may be a bare command name ("policy") or a slash-delimited path +// ("billing/policy"). A path is resolved one segment at a time beneath root; +// missing intermediates are created as thin grouping commands, and existing +// nodes (including entity-generated parents) are reused. +// // Use this for non-entity cobra commands that should live under an // entity-generated parent (or under an arbitrary grouping command). func RegisterSubCommand(parentName string, cmd *cobra.Command) { @@ -37,6 +43,8 @@ func RegisterSubCommand(parentName string, cmd *cobra.Command) { // identified by parentName. Use this when the subcommand needs to be constructed // lazily — e.g. AddNamedCommand which both builds and attaches in one call. // +// parentName accepts the same bare-name-or-slash-path form as RegisterSubCommand. +// // Example: // // clicky.RegisterSubCommandFn("correspondence", func(parent *cobra.Command) { @@ -58,7 +66,7 @@ func flushPendingSubCommands(root *cobra.Command) { defer pendingSubCommandsMu.Unlock() for _, p := range pendingSubCommands { - parent := findOrCreateChild(root, p.parentName) + parent := findOrCreateAtPath(root, p.parentName) if p.build != nil { p.build(parent) } else { @@ -74,3 +82,18 @@ func flushPendingSubCommands(root *cobra.Command) { } pendingSubCommands = nil } + +// findOrCreateAtPath resolves a slash-delimited path beneath root, creating +// any missing nodes via findOrCreateChild. A path with no separator behaves +// exactly like findOrCreateChild(root, path). Empty segments (leading/trailing +// or doubled slashes) are skipped. +func findOrCreateAtPath(root *cobra.Command, path string) *cobra.Command { + cur := root + for _, segment := range strings.Split(path, "/") { + if segment == "" { + continue + } + cur = findOrCreateChild(cur, segment) + } + return cur +} diff --git a/sub_command_test.go b/sub_command_test.go index 0656b3d5..9b85faa5 100644 --- a/sub_command_test.go +++ b/sub_command_test.go @@ -122,3 +122,49 @@ func TestRegisterSubCommandCreatesParent(t *testing.T) { t.Fatalf("expected process under lazily-created correspondence, got err=%v", err) } } + +// TestRegisterSubCommandPathAware exercises the slash-delimited parentName +// form: the subcommand must attach to the grandchild named by the path, +// reusing entity-generated intermediates rather than creating siblings. +func TestRegisterSubCommandPathAware(t *testing.T) { + resetEntityRegistry(t) + defer resetEntityRegistry(t) + + RegisterEntity(Entity[nestedTestEntity, nestedTestOpts, nestedTestEntity]{ + Name: "accounts", + Parent: "xero", + List: func(_ nestedTestOpts) ([]nestedTestEntity, error) { + return nil, nil + }, + }) + + create := &cobra.Command{Use: "create"} + RegisterSubCommand("xero/accounts", create) + + autoCreated := &cobra.Command{Use: "leaf"} + RegisterSubCommand("billing/policy", autoCreated) + + root := &cobra.Command{Use: "root"} + GenerateCLI(root) + + found, _, err := root.Find([]string{"xero", "accounts", "create"}) + if err != nil || found == nil || found.Name() != "create" { + t.Fatalf("expected create under xero/accounts, got %v err=%v", found, err) + } + + // Reuses the entity-generated xero parent, doesn't shadow it with a sibling. + xeroChildren := 0 + for _, c := range root.Commands() { + if c.Name() == "xero" { + xeroChildren++ + } + } + if xeroChildren != 1 { + t.Fatalf("expected exactly one xero child of root, got %d", xeroChildren) + } + + leaf, _, err := root.Find([]string{"billing", "policy", "leaf"}) + if err != nil || leaf == nil || leaf.Name() != "leaf" { + t.Fatalf("expected leaf under auto-created billing/policy, got %v err=%v", leaf, err) + } +} From 28da416d91e0a6370eb08766158ce381f5fe4b7c Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Fri, 29 May 2026 09:04:32 +0300 Subject: [PATCH 2/5] feat(api): add Admonition and Keyed types for structured content rendering Introduce Admonition type to support callout blocks with severity levels (note, info, tip, warning, danger) and multi-format rendering (text, ANSI, HTML, Markdown). Add Keyed wrapper type to associate stable identifiers with Textable values for structured JSON output without affecting other formats. Also fix Markdown table rendering to escape pipe characters in cell content, preventing GFM table layout corruption. Add shell completion support for Filterable subcommands and refactor RPC data function to use PopulateFromRequest for proper concurrent request handling. Add WithKey helper function to format.go for convenient Keyed wrapper creation. --- api/admonition.go | 134 +++++++++++++++++++++++++++++++++++++++++ api/admonition_test.go | 61 +++++++++++++++++++ api/keyed.go | 66 ++++++++++++++++++++ api/keyed_test.go | 39 ++++++++++++ api/table.go | 9 ++- api/table_test.go | 14 +++++ cobra_command.go | 55 +++++++++++------ format.go | 7 +++ 8 files changed, 366 insertions(+), 19 deletions(-) create mode 100644 api/admonition.go create mode 100644 api/admonition_test.go create mode 100644 api/keyed.go create mode 100644 api/keyed_test.go diff --git a/api/admonition.go b/api/admonition.go new file mode 100644 index 00000000..d0a4c5e6 --- /dev/null +++ b/api/admonition.go @@ -0,0 +1,134 @@ +package api + +import ( + "fmt" + "strings" + + "github.com/flanksource/clicky/api/icons" +) + +// Severity classifies an Admonition callout. The zero value is SeverityNote. +type Severity int + +const ( + SeverityNote Severity = iota + SeverityInfo + SeverityTip + SeverityWarning + SeverityDanger +) + +// String returns the canonical lower-case severity keyword, used both as the +// admonition header word and the HTML class suffix. +func (s Severity) String() string { + switch s { + case SeverityInfo: + return "info" + case SeverityTip: + return "tip" + case SeverityWarning: + return "warning" + case SeverityDanger: + return "danger" + default: + return "note" + } +} + +// Icon returns the status icon conventionally paired with the severity. +func (s Severity) Icon() icons.Icon { + switch s { + case SeverityInfo: + return icons.Info + case SeverityTip: + return icons.Success + case SeverityWarning: + return icons.Warning + case SeverityDanger: + return icons.Error + default: + return icons.Info + } +} + +// ParseSeverity maps an author-supplied severity word to a Severity. Unknown +// words fall through to SeverityNote so callers preserve the raw text via the +// admonition body rather than failing. +func ParseSeverity(s string) Severity { + switch strings.ToLower(strings.TrimSpace(s)) { + case "info", "information", "note-info": + return SeverityInfo + case "tip", "success", "hint": + return SeverityTip + case "warning", "warn", "caution": + return SeverityWarning + case "danger", "error", "critical", "failure": + return SeverityDanger + default: + return SeverityNote + } +} + +// Admonition is a callout block (`!!! `) with an indented +// body. Title and Body are each a single Textable; Title may be nil (header +// shows the severity word only) and Body may be nil (header only). +type Admonition struct { + Severity Severity + Title Textable + Body Textable +} + +// header returns the plain `!!! <severity> <title-text>` line used by the +// text-oriented renderers. +func (a Admonition) header() string { + h := "!!! " + a.Severity.String() + if a.Title != nil { + if title := a.Title.String(); title != "" { + h += " " + title + } + } + return h +} + +func indentLines(s, prefix string) string { + lines := strings.Split(s, "\n") + for i, l := range lines { + lines[i] = prefix + l + } + return strings.Join(lines, "\n") +} + +func (a Admonition) String() string { + if a.Body == nil { + return a.header() + } + return a.header() + "\n" + indentLines(a.Body.String(), " ") +} + +func (a Admonition) ANSI() string { + head := Text{Content: a.header(), Style: "font-bold"}.ANSI() + if a.Body == nil { + return head + } + return head + "\n" + indentLines(a.Body.ANSI(), " ") +} + +func (a Admonition) HTML() string { + title := "" + if a.Title != nil { + title = a.Title.HTML() + } + body := "" + if a.Body != nil { + body = a.Body.HTML() + } + return fmt.Sprintf("<div class=%q><p>%s</p>%s</div>", + "admonition admonition-"+a.Severity.String(), title, body) +} + +func (a Admonition) Markdown() string { + if a.Body == nil { + return a.header() + } + return a.header() + "\n" + indentLines(a.Body.Markdown(), " ") +} diff --git a/api/admonition_test.go b/api/admonition_test.go new file mode 100644 index 00000000..fe07990d --- /dev/null +++ b/api/admonition_test.go @@ -0,0 +1,61 @@ +package api + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Severity", func() { + It("round-trips known severity words through ParseSeverity/String", func() { + for word, sev := range map[string]Severity{ + "note": SeverityNote, + "info": SeverityInfo, + "tip": SeverityTip, + "warning": SeverityWarning, + "danger": SeverityDanger, + "error": SeverityDanger, + "critical": SeverityDanger, + } { + Expect(ParseSeverity(word)).To(Equal(sev), "ParseSeverity(%q)", word) + } + Expect(SeverityWarning.String()).To(Equal("warning")) + }) + + It("falls back to note for unknown words", func() { + Expect(ParseSeverity("nonsense")).To(Equal(SeverityNote)) + }) +}) + +var _ = Describe("Admonition", func() { + build := func() Admonition { + return Admonition{ + Severity: SeverityWarning, + Title: Text{Content: "Net income changed sign"}, + Body: Text{Content: "review the statement"}, + } + } + + It("renders the !!! header with severity and title in text formats", func() { + a := build() + Expect(a.String()).To(HavePrefix("!!! warning Net income changed sign")) + Expect(a.Markdown()).To(HavePrefix("!!! warning Net income changed sign")) + Expect(a.ANSI()).To(ContainSubstring("Net income changed sign")) + }) + + It("indents the body four spaces in text and markdown", func() { + a := build() + Expect(a.String()).To(ContainSubstring(" review the statement")) + Expect(a.Markdown()).To(ContainSubstring(" review the statement")) + }) + + It("renders an admonition div keyed by severity in HTML", func() { + a := build() + Expect(a.HTML()).To(ContainSubstring(`class="admonition admonition-warning"`)) + Expect(a.HTML()).To(ContainSubstring("Net income changed sign")) + }) + + It("renders header only when body is nil", func() { + a := Admonition{Severity: SeverityNote, Title: Text{Content: "heads up"}} + Expect(a.String()).To(Equal("!!! note heads up")) + }) +}) diff --git a/api/keyed.go b/api/keyed.go new file mode 100644 index 00000000..a8ed38a6 --- /dev/null +++ b/api/keyed.go @@ -0,0 +1,66 @@ +package api + +import "encoding/json" + +// Keyed wraps a Textable with a data key. Rendering passes through to the +// wrapped value unchanged in every format; only JSON serialization differs — +// the value marshals under Key as a single-field object. Use it when a block in +// a document needs a stable identifier for structured output (e.g. a table keyed +// by its heading) without altering how it renders. +type Keyed struct { + Key string + Value Textable +} + +func (k Keyed) String() string { return textableString(k.Value) } +func (k Keyed) ANSI() string { return textableANSI(k.Value) } +func (k Keyed) HTML() string { return textableHTML(k.Value) } +func (k Keyed) Markdown() string { return textableMarkdown(k.Value) } + +// MarshalJSON emits {Key: Value}. The wrapped value serializes via its own +// MarshalJSON when it implements json.Marshaler, otherwise via its rendered +// string. +func (k Keyed) MarshalJSON() ([]byte, error) { + return json.Marshal(map[string]any{k.Key: jsonValue(k.Value)}) +} + +func textableString(t Textable) string { + if t == nil { + return "" + } + return t.String() +} + +func textableANSI(t Textable) string { + if t == nil { + return "" + } + return t.ANSI() +} + +func textableHTML(t Textable) string { + if t == nil { + return "" + } + return t.HTML() +} + +func textableMarkdown(t Textable) string { + if t == nil { + return "" + } + return t.Markdown() +} + +// jsonValue returns the wrapped value as-is when it can marshal itself (so a +// TextTable serializes to its row array, an Amount to its formatted string), +// falling back to the rendered string otherwise. +func jsonValue(t Textable) any { + if t == nil { + return nil + } + if _, ok := t.(json.Marshaler); ok { + return t + } + return t.String() +} diff --git a/api/keyed_test.go b/api/keyed_test.go new file mode 100644 index 00000000..38851f2a --- /dev/null +++ b/api/keyed_test.go @@ -0,0 +1,39 @@ +package api + +import ( + "encoding/json" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Keyed", func() { + It("renders identically to the wrapped value in every format", func() { + inner := Text{Content: "hello", Style: "font-bold"} + k := Keyed{Key: "greeting", Value: inner} + Expect(k.String()).To(Equal(inner.String())) + Expect(k.ANSI()).To(Equal(inner.ANSI())) + Expect(k.HTML()).To(Equal(inner.HTML())) + Expect(k.Markdown()).To(Equal(inner.Markdown())) + }) + + It("marshals JSON as a single-field object keyed by Key", func() { + k := Keyed{Key: "greeting", Value: Text{Content: "hi"}} + raw, err := json.Marshal(k) + Expect(err).ToNot(HaveOccurred()) + Expect(string(raw)).To(Equal(`{"greeting":"hi"}`)) + }) + + It("delegates JSON to a wrapped value that marshals itself", func() { + table := TextTable{ + Headers: TextList{Text{Content: "A"}}, + FieldNames: []string{"a"}, + Rows: []TableRow{{"a": TypedValue{Textable: Text{Content: "1"}}}}, + } + k := Keyed{Key: "rows", Value: table} + raw, err := json.Marshal(k) + Expect(err).ToNot(HaveOccurred()) + Expect(string(raw)).To(ContainSubstring(`"rows":[`)) + Expect(string(raw)).To(ContainSubstring(`"a":"1"`)) + }) +}) diff --git a/api/table.go b/api/table.go index a8264376..dff80736 100644 --- a/api/table.go +++ b/api/table.go @@ -506,7 +506,7 @@ func (t TextTable) Markdown() string { values = append(values, "") continue } - values = append(values, TransformerMarkdown(cell)) + values = append(values, escapeMarkdownPipes(TransformerMarkdown(cell))) } if err := table.Append(values...); err != nil { @@ -522,6 +522,13 @@ func (t TextTable) Markdown() string { return "\n" + buf.String() } +// escapeMarkdownPipes escapes a literal pipe so a cell value does not break the +// GFM table layout. The tablewriter markdown renderer does not escape pipes, so +// callers that put `|` in cell content rely on this. +func escapeMarkdownPipes(s string) string { + return strings.ReplaceAll(s, "|", "\\|") +} + type TextTransformer func(t Textable) string var TransformerANSI TextTransformer = func(t Textable) string { diff --git a/api/table_test.go b/api/table_test.go index b0835031..a26304d1 100644 --- a/api/table_test.go +++ b/api/table_test.go @@ -90,3 +90,17 @@ var _ = Describe("WithoutEmptyColumns", func() { Expect(func() { table.ANSI() }).NotTo(Panic()) }) }) + +var _ = Describe("TextTable Markdown pipe escaping", func() { + It("escapes a literal pipe in a cell so the GFM table stays intact", func() { + t := TextTable{ + Headers: TextList{Text{Content: "A"}, Text{Content: "B"}}, + FieldNames: []string{"a", "b"}, + Rows: []TableRow{ + {"a": TypedValue{Textable: Text{Content: "x|y"}}, "b": TypedValue{Textable: Text{Content: "ok"}}}, + }, + } + Expect(t.Markdown()).To(ContainSubstring(`x\|y`)) + Expect(t.Markdown()).NotTo(ContainSubstring(`| x|y |`)) + }) +}) diff --git a/cobra_command.go b/cobra_command.go index 321ce0e6..00a52d3e 100644 --- a/cobra_command.go +++ b/cobra_command.go @@ -120,8 +120,24 @@ func AddNamedCommand[T any, R any](name string, parent *cobra.Command, opts T, f } parent.AddCommand(cmd) SetCommandResponseMeta(cmd, ResponseOpenAPIMeta{Type: responseTypeOf[R]()}) + + // Filterable opts contribute the same lookup/completion surface as an + // Entity's Filters slice — typed dropdowns / typeaheads on the web UI and + // shell completions on the CLI — but scoped to one subcommand. Subcommand + // pages on the entity explorer share the entity's Filters by default; this + // hook lets a subcommand publish a *different* filter set when its CLI + // flag surface differs from the parent list view (e.g. a + // `<entity> activities` listing that drops identifier filters its parent + // list exposes, because they are already pinned by the positional id). + var subFilters []Filter[T] + if carrier, ok := optsValue.Interface().(Filterable[T]); ok { + subFilters = carrier.Filters() + } if meta := GetCommandOpenAPIMeta(parent); meta != nil { - annotateEntityOperationCommand(cmd, parent, "action", "", "collection", name, "", false, false) + annotateEntityOperationCommand(cmd, parent, "action", "", "collection", name, "", len(subFilters) > 0, false) + } + if len(subFilters) > 0 { + lookupFuncRegistry.Store(cmd, buildLookupFunc[T](subFilters)) } if namer, ok := optsValue.Interface().(Name); ok { @@ -191,26 +207,29 @@ func AddNamedCommand[T any, R any](name string, parent *cobra.Command, opts T, f cmd.Args = cobra.NoArgs } - // Register data func for RPC direct invocation (bypasses stdout capture) - dataFuncRegistry.Store(cmd, func(flagMap map[string]string, args []string) (any, error) { - // Set cobra flags from the map (same as executor.ExecuteCommand does) - for k, v := range flagMap { - if flag := cmd.Flags().Lookup(k); flag != nil { - _ = flag.Value.Set(v) - flag.Changed = true - } + // Wire shell completions for any Filterable subcommand. The binder skips + // flag names the opts struct does not actually expose, so a filter list + // can over-declare without breaking less-restrictive subcommands. + if len(subFilters) > 0 { + if binder := buildFilterCompletionBinder[T](subFilters); binder != nil { + binder(cmd) } + } - // Build opts struct from the now-set flags (same as RunE does) + // Register data func for RPC direct invocation (bypasses stdout capture). + // + // The HTTP path parses each request directly into a fresh opts struct + // rather than routing through the cobra command's pflag pointers. Pflag + // pointers are shared across every invocation of the same command, and + // pflag slice values append on Set once flag.Changed is true — so the + // shared path would leak state and corrupt concurrent requests. The + // CLI path (cmd.RunE below) keeps pflag because cobra owns its + // lifecycle and one process invocation == one Run. + capturedFields := append([]flags.FieldInfo(nil), fieldInfos...) + dataFuncRegistry.Store(cmd, func(flagMap map[string]string, args []string) (any, error) { optsValue := reflect.New(optsType).Elem() - for _, fv := range flagValues { - argsToPass := []string(nil) - if fv.IsArgs { - argsToPass = args - } - if err := flags.AssignFieldValue(optsValue, fv, argsToPass, false); err != nil { - return nil, err - } + if err := flags.PopulateFromRequest(optsValue, capturedFields, flagMap, args); err != nil { + return nil, err } return fn(optsValue.Interface().(T)) }) diff --git a/format.go b/format.go index 424e267a..c9953563 100644 --- a/format.go +++ b/format.go @@ -160,6 +160,13 @@ func Textf(content string, args ...any) api.Text { } } +// WithKey wraps a Textable with a data key. The result renders identically to +// value in every format but serializes to JSON as {key: value}. Callers reading +// the key for their own serialization can type-assert to api.Keyed. +func WithKey(key string, value api.Textable) api.Keyed { + return api.Keyed{Key: key, Value: value} +} + func Collapsed(label string, content api.Textable, styles ...string) api.Collapsed { return api.Collapsed{ Label: label, From c4bcbe34599b139085714592d7f1899c21c00588 Mon Sep 17 00:00:00 2001 From: Moshe Immerman <moshe@flanksource.com> Date: Sun, 31 May 2026 13:30:18 +0300 Subject: [PATCH 3/5] fix(task): enforce group concurrency at dequeue time instead of task body Move semaphore acquisition from task function wrapper to worker dequeue time. This prevents worker slots from being blocked on group semaphores, allowing independent tasks to run while a concurrency-limited group is busy. The parent group is now set via withParent option before task enqueue, ensuring the worker can access the group's semaphore at dequeue time. This fixes a deadlock risk when concurrency=1 and prevents worker starvation. Adds TestGroupConcurrencySlots to verify independent tasks can run concurrently with a saturated serial group. --- task/group.go | 28 ++++++------------- task/group_concurrency_test.go | 50 ++++++++++++++++++++++++++++++++++ task/options.go | 9 ++++++ task/task.go | 12 ++++++++ task/worker.go | 15 ++++++++++ 5 files changed, 95 insertions(+), 19 deletions(-) diff --git a/task/group.go b/task/group.go index 733a6ed6..31707c84 100644 --- a/task/group.go +++ b/task/group.go @@ -43,28 +43,18 @@ func (g TypedGroup[T]) Add(name string, taskFunc func(flanksourceContext.Context g.mu.Lock() defer g.mu.Unlock() - // Wrap the task function with semaphore acquire/release if concurrency is limited - wrappedTaskFunc := taskFunc - if g.sem != nil { - wrappedTaskFunc = func(ctx flanksourceContext.Context, t *Task) (T, error) { - // Acquire semaphore permit - if err := g.sem.Acquire(ctx, 1); err != nil { - var zero T - return zero, err - } - defer g.sem.Release(1) - - // Execute the original task function - return taskFunc(ctx, t) - } - } - - // Create the task using the group's manager with wrapped function - task := StartTask(name, wrappedTaskFunc, opts...) + // Concurrency is enforced by the worker at dequeue time via the group's + // semaphore (see worker.run / Task.groupSem); wrapping the task function to + // acquire here would double-acquire the same permit (deadlock at N=1). + // + // The parent must be set BEFORE StartTask enqueues the task, otherwise a + // worker could dequeue and run it ungated while parent is still nil. The + // withParent option is applied inside newTask, before enqueue; it is + // prepended so a caller-supplied option cannot override it. + task := StartTask(name, taskFunc, append([]Option{withParent(g.Group)}, opts...)...) // Add to the group's items g.Items = append(g.Items, task) - task.parent = g.Group // Update start time if this is the first item or it started earlier task.mu.Lock() diff --git a/task/group_concurrency_test.go b/task/group_concurrency_test.go index 8e685616..39339871 100644 --- a/task/group_concurrency_test.go +++ b/task/group_concurrency_test.go @@ -61,6 +61,56 @@ func TestGroupConcurrency(t *testing.T) { assert.Equal(t, 5, len(results), "Expected all 5 tasks to complete") } +// TestGroupConcurrencySlots proves the concurrency limit is enforced at +// dequeue time, not inside the task body: a saturated concurrency=1 group must +// not hold worker slots blocked on its semaphore. We assert (1) only one serial +// body runs at a time and (2) an independent task still runs WHILE the serial +// group is busy — which fails if all workers are stuck blocking on the serial +// semaphore (the pre-fix behavior). +func TestGroupConcurrencySlots(t *testing.T) { + serial := StartGroup[int]("Serial", WithConcurrency(1)) + + var serialActive, serialMaxActive int64 + var serialRunning atomic.Bool + for i := 0; i < 4; i++ { + serial.Add( + "S"+string(rune('A'+i)), + func(ctx flanksourceContext.Context, t *Task) (int, error) { + a := atomic.AddInt64(&serialActive, 1) + for { + c := atomic.LoadInt64(&serialMaxActive) + if a <= c || atomic.CompareAndSwapInt64(&serialMaxActive, c, a) { + break + } + } + serialRunning.Store(true) + time.Sleep(100 * time.Millisecond) + atomic.AddInt64(&serialActive, -1) + return 0, nil + }, + ) + } + + var otherRanDuringSerial atomic.Bool + other := StartGroup[int]("Other") + other.Add( + "Other", + func(ctx flanksourceContext.Context, t *Task) (int, error) { + // Let the serial group claim its single permit and start first. + time.Sleep(20 * time.Millisecond) + otherRanDuringSerial.Store(serialRunning.Load()) + return 0, nil + }, + ) + + serial.WaitFor() + other.WaitFor() + + assert.Equal(t, int64(1), serialMaxActive, "serial group must run exactly 1 body at a time") + assert.True(t, otherRanDuringSerial.Load(), + "independent task must run while the serial group is busy; workers must not be blocked on the serial semaphore") +} + func TestGroupNoConcurrencyLimit(t *testing.T) { // Create a group without concurrency limit (default behavior) group := StartGroup[int]("No Limit Test") diff --git a/task/options.go b/task/options.go index fdfe8b09..78c9a6ce 100644 --- a/task/options.go +++ b/task/options.go @@ -66,6 +66,15 @@ func WithFunc(fn func(flanksourceContext.Context, *Task) error) Option { } } +// withParent links a task to its group before it is enqueued, so the worker can +// read the group's concurrency semaphore (Task.groupSem) at dequeue time. It is +// unexported because only TypedGroup.Add should set the parent. +func withParent(g *Group) Option { + return func(t *Task) { + t.parent = g + } +} + // WithModel sets the model name for the task func WithModel(modelName string) Option { return func(t *Task) { diff --git a/task/task.go b/task/task.go index cb199c34..fc86314a 100644 --- a/task/task.go +++ b/task/task.go @@ -13,6 +13,7 @@ import ( "github.com/flanksource/commons/logger" "github.com/flanksource/commons/text" "github.com/samber/lo" + "golang.org/x/sync/semaphore" "github.com/flanksource/clicky/api" "github.com/flanksource/clicky/formatters" @@ -610,6 +611,17 @@ func (t *Task) IsGroup() bool { return false } +// groupSem returns the concurrency semaphore of the task's parent group, or nil +// if the task is ungrouped or its group has no concurrency limit. Group.sem is +// assigned once in StartGroup before any task is added and never mutated, so no +// lock is needed. +func (t *Task) groupSem() *semaphore.Weighted { + if t.parent == nil { + return nil + } + return t.parent.sem +} + // getDuration returns formatted duration string func (t *Task) getDuration() string { if t.status == StatusPending || t.startTime.IsZero() { diff --git a/task/worker.go b/task/worker.go index d28dbb19..05d904ba 100644 --- a/task/worker.go +++ b/task/worker.go @@ -51,9 +51,24 @@ func (w *worker) run() { continue } + // Gate on group concurrency at dequeue time. If the group's permit + // pool is saturated, do not occupy a worker slot — re-enqueue with a + // short delay and let this worker pick up other work. + sem := task.groupSem() + if sem != nil && !sem.TryAcquire(1) { + w.manager.taskQueue.EnqueueWithDelay(task, 50*time.Millisecond) + continue + } + w.manager.workersActive.Add(1) func() { + // Release the group permit on ALL exit paths (success, failure, + // retry, timeout, panic). Registered first so it runs last, + // after the recover defer records terminal status. + if sem != nil { + defer sem.Release(1) + } defer func() { if r := recover(); r != nil { task.mu.Lock() From 3d9720bec33ae060220ecba3859802b157ea1bca Mon Sep 17 00:00:00 2001 From: Moshe Immerman <moshe@flanksource.com> Date: Sun, 31 May 2026 13:30:40 +0300 Subject: [PATCH 4/5] feat(formatters): Add ClickyDocument and ClickyText for direct Textable serialization Introduce NewClickyDocument() to build clicky documents directly from Textable objects, bypassing PrettyData conversion. This preserves rich formatting (code blocks, collapsibles, tables) verbatim without flattening through PrettyData. Add ClickyText wrapper type that implements json.Marshaler to emit structured clicky documents instead of plain strings, enabling use in struct fields with `json:"field"` tags. Export previously private clicky* types (ClickyDocument, ClickyNode, ClickyStyle, ClickyField, ClickyColumn, ClickyRow, ClickyTreeItem, ClickyStackFrame) to support external consumption and round-tripping through encoding/json. Add comprehensive tests verifying code blocks, collapsed sections, and JSON round-trip fidelity. --- formatters/clicky_document.go | 40 +++++ formatters/clicky_document_test.go | 113 +++++++++++++ formatters/html_react_formatter.go | 200 ++++++++++++------------ formatters/html_react_formatter_test.go | 98 ++++++++++++ 4 files changed, 354 insertions(+), 97 deletions(-) create mode 100644 formatters/clicky_document.go create mode 100644 formatters/clicky_document_test.go diff --git a/formatters/clicky_document.go b/formatters/clicky_document.go new file mode 100644 index 00000000..ad9d1a35 --- /dev/null +++ b/formatters/clicky_document.go @@ -0,0 +1,40 @@ +package formatters + +import ( + "encoding/json" + + "github.com/flanksource/clicky/api" +) + +// NewClickyDocument builds the {version:1, node:{…}} document directly from a +// Textable, reusing the same node converter the clicky-json / html-react +// formatters use. Unlike those formatters it does not route through PrettyData, +// so a hand-built rich Text (code blocks, collapsibles, tables, …) is encoded +// verbatim. The result is the shape consumed by @flanksource/clicky-ui's +// <Clicky data={…} /> component and round-trips through encoding/json. +func NewClickyDocument(t api.Textable) ClickyDocument { + if t == nil { + return ClickyDocument{Version: 1, Node: clickyTextNode("")} + } + return ClickyDocument{Version: 1, Node: convertTextable(t)} +} + +// ClickyText wraps a Textable so json.Marshal emits the structured clicky +// document (NewClickyDocument) instead of the flattened plain string that +// api.Text.MarshalJSON produces. Drop it into any struct field whose JSON should +// be rendered by <Clicky/>: +// +// type Result struct { +// Detail ClickyText `json:"detail"` +// } +// +// The transported/persisted form is the concrete ClickyDocument, which is what +// unmarshals — ClickyText is producer-side only. +type ClickyText struct { + api.Textable +} + +// MarshalJSON renders the wrapped Textable as a ClickyDocument. +func (c ClickyText) MarshalJSON() ([]byte, error) { + return json.Marshal(NewClickyDocument(c.Textable)) +} diff --git a/formatters/clicky_document_test.go b/formatters/clicky_document_test.go new file mode 100644 index 00000000..ef72153d --- /dev/null +++ b/formatters/clicky_document_test.go @@ -0,0 +1,113 @@ +package formatters + +import ( + "encoding/json" + "reflect" + "testing" + + "github.com/flanksource/clicky/api" +) + +// richDoc builds a Text carrying a labelled YAML code block wrapped in a +// collapsible — the shape the apply suite attaches as a test's source view. +func richDoc() api.Text { + body := api.Text{Content: "source"}. + NewLine(). + Add(api.CodeBlock("yaml", "kind: TestPlan\nproduct: Acme\n")) + return api.Text{Content: "Document"}. + Add(api.Collapsed{Label: "details", Content: body}) +} + +func TestNewClickyDocumentEncodesCodeAndCollapsed(t *testing.T) { + doc := NewClickyDocument(richDoc()) + + if doc.Version != 1 { + t.Fatalf("version = %d, want 1", doc.Version) + } + if doc.Node.Kind != "text" { + t.Fatalf("root kind = %q, want text", doc.Node.Kind) + } + + collapsed := findNode(doc.Node, "collapsed") + if collapsed == nil { + t.Fatal("no collapsed node in document") + } + if collapsed.Content == nil { + t.Fatal("collapsed node has no content") + } + + code := findNode(doc.Node, "code") + if code == nil { + t.Fatal("no code node in document") + } + if code.Language != "yaml" { + t.Fatalf("code language = %q, want yaml", code.Language) + } + if code.Source != "kind: TestPlan\nproduct: Acme\n" { + t.Fatalf("code source = %q, want the yaml body", code.Source) + } +} + +func TestClickyDocumentRoundTrips(t *testing.T) { + want := NewClickyDocument(richDoc()) + + raw, err := json.Marshal(want) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var got ClickyDocument + if err := json.Unmarshal(raw, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if !reflect.DeepEqual(want, got) { + t.Fatalf("round-trip mismatch:\n want %#v\n got %#v", want, got) + } +} + +func TestClickyTextMarshalsAsDocument(t *testing.T) { + type holder struct { + Detail ClickyText `json:"detail"` + } + + raw, err := json.Marshal(holder{Detail: ClickyText{Textable: richDoc()}}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var decoded struct { + Detail ClickyDocument `json:"detail"` + } + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if decoded.Detail.Version != 1 { + t.Fatalf("wrapped version = %d, want 1", decoded.Detail.Version) + } + if findNode(decoded.Detail.Node, "code") == nil { + t.Fatal("wrapped document lost its code block") + } +} + +// findNode returns the first node of the given kind in a depth-first walk over +// the node tree (children, items, content, label). +func findNode(node ClickyNode, kind string) *ClickyNode { + if node.Kind == kind { + return &node + } + candidates := append([]ClickyNode{}, node.Children...) + candidates = append(candidates, node.Items...) + if node.Content != nil { + candidates = append(candidates, *node.Content) + } + if node.Label != nil { + candidates = append(candidates, *node.Label) + } + for _, c := range candidates { + if found := findNode(c, kind); found != nil { + return found + } + } + return nil +} diff --git a/formatters/html_react_formatter.go b/formatters/html_react_formatter.go index b221d406..0659b476 100644 --- a/formatters/html_react_formatter.go +++ b/formatters/html_react_formatter.go @@ -26,12 +26,18 @@ type HTMLReactFormatter struct{} // payload is the same shape consumed by @flanksource/clicky-ui's <Clicky data={...} />. type ClickyJSONFormatter struct{} -type clickyDocument struct { +// ClickyDocument is the {version:1, node:{…}} envelope consumed by +// @flanksource/clicky-ui's <Clicky data={…} /> component. It is built from a +// PrettyData tree (clicky-json / html-react formatters) or directly from a +// Textable via NewClickyDocument. Every field below is concrete (no interfaces), +// so the document round-trips through encoding/json (Marshal and Unmarshal) with +// the stock codec — callers can persist it and reload it without loss. +type ClickyDocument struct { Version int `json:"version"` - Node clickyNode `json:"node"` + Node ClickyNode `json:"node"` } -type clickyStyle struct { +type ClickyStyle struct { ClassName string `json:"className,omitempty"` Color string `json:"color,omitempty"` BackgroundColor string `json:"backgroundColor,omitempty"` @@ -47,31 +53,31 @@ type clickyStyle struct { Monospace bool `json:"monospace,omitempty"` } -type clickyField struct { +type ClickyField struct { Name string `json:"name"` Label string `json:"label,omitempty"` - Value clickyNode `json:"value"` + Value ClickyNode `json:"value"` } -type clickyColumn struct { +type ClickyColumn struct { Name string `json:"name"` Label string `json:"label,omitempty"` - Header *clickyNode `json:"header,omitempty"` + Header *ClickyNode `json:"header,omitempty"` Align string `json:"align,omitempty"` } -type clickyRow struct { - Cells map[string]clickyNode `json:"cells"` - Detail *clickyNode `json:"detail,omitempty"` +type ClickyRow struct { + Cells map[string]ClickyNode `json:"cells"` + Detail *ClickyNode `json:"detail,omitempty"` } -type clickyTreeItem struct { +type ClickyTreeItem struct { ID string `json:"id"` - Label clickyNode `json:"label"` - Children []clickyTreeItem `json:"children,omitempty"` + Label ClickyNode `json:"label"` + Children []ClickyTreeItem `json:"children,omitempty"` } -type clickyStackFrame struct { +type ClickyStackFrame struct { FunctionName string `json:"functionName,omitempty"` DisplayName string `json:"displayName,omitempty"` File string `json:"file,omitempty"` @@ -89,25 +95,25 @@ type clickyStackFrame struct { SourceLanguage string `json:"sourceLanguage,omitempty"` } -type clickyNode struct { +type ClickyNode struct { Kind string `json:"kind"` Plain string `json:"plain,omitempty"` - Style *clickyStyle `json:"style,omitempty"` + Style *ClickyStyle `json:"style,omitempty"` Text string `json:"text,omitempty"` - Children []clickyNode `json:"children,omitempty"` - Tooltip *clickyNode `json:"tooltip,omitempty"` + Children []ClickyNode `json:"children,omitempty"` + Tooltip *ClickyNode `json:"tooltip,omitempty"` HTML string `json:"html,omitempty"` Inline bool `json:"inline,omitempty"` Ordered bool `json:"ordered,omitempty"` Unstyled bool `json:"unstyled,omitempty"` - Bullet *clickyNode `json:"bullet,omitempty"` - Items []clickyNode `json:"items,omitempty"` - Fields []clickyField `json:"fields,omitempty"` - Columns []clickyColumn `json:"columns,omitempty"` - Rows []clickyRow `json:"rows,omitempty"` - Roots []clickyTreeItem `json:"roots,omitempty"` - Label *clickyNode `json:"label,omitempty"` - Content *clickyNode `json:"content,omitempty"` + Bullet *ClickyNode `json:"bullet,omitempty"` + Items []ClickyNode `json:"items,omitempty"` + Fields []ClickyField `json:"fields,omitempty"` + Columns []ClickyColumn `json:"columns,omitempty"` + Rows []ClickyRow `json:"rows,omitempty"` + Roots []ClickyTreeItem `json:"roots,omitempty"` + Label *ClickyNode `json:"label,omitempty"` + Content *ClickyNode `json:"content,omitempty"` Href string `json:"href,omitempty"` Target string `json:"target,omitempty"` Command string `json:"command,omitempty"` @@ -125,7 +131,7 @@ type clickyNode struct { ExceptionClass string `json:"exceptionClass,omitempty"` Message string `json:"message,omitempty"` CausedBy []string `json:"causedBy,omitempty"` - Frames []clickyStackFrame `json:"frames,omitempty"` + Frames []ClickyStackFrame `json:"frames,omitempty"` // Badge fields — set for Kind == "badge" (LabelBadge). Rendered by // clicky-ui as <Badge variant="label" label={Value1} value={Value2} />. BadgeLabel string `json:"badgeLabel,omitempty"` @@ -177,14 +183,14 @@ func clickyDocumentJSON(data any, opts FormatOptions, indent bool) ([]byte, erro return json.Marshal(payload) } -func convertPrettyData(pd *api.PrettyData) clickyDocument { - return clickyDocument{ +func convertPrettyData(pd *api.PrettyData) ClickyDocument { + return ClickyDocument{ Version: 1, Node: convertTypedValue(&pd.TypedValue, pd.Schema), } } -func convertTypedValue(tv *api.TypedValue, schema *api.PrettyObject) clickyNode { +func convertTypedValue(tv *api.TypedValue, schema *api.PrettyObject) ClickyNode { if tv == nil { return clickyTextNode("") } @@ -212,7 +218,7 @@ func convertTypedValue(tv *api.TypedValue, schema *api.PrettyObject) clickyNode return clickyTextNode(tv.String()) } -func convertTextable(t api.Textable) clickyNode { +func convertTextable(t api.Textable) ClickyNode { switch v := t.(type) { case api.Text: return convertText(v) @@ -279,11 +285,11 @@ func convertTextable(t api.Textable) clickyNode { case *api.ButtonGroup: return convertButtonGroup(*v) case api.HtmlElement: - return clickyNode{Kind: "html", Plain: v.String(), HTML: v.HTML()} + return ClickyNode{Kind: "html", Plain: v.String(), HTML: v.HTML()} case *api.HtmlElement: - return clickyNode{Kind: "html", Plain: v.String(), HTML: v.HTML()} + return ClickyNode{Kind: "html", Plain: v.String(), HTML: v.HTML()} case api.Comment: - return clickyNode{Kind: "comment", Plain: string(v), Text: string(v)} + return ClickyNode{Kind: "comment", Plain: string(v), Text: string(v)} case api.KeyValuePair: return convertKeyValuePair(v) case *api.KeyValuePair: @@ -301,16 +307,16 @@ func convertTextable(t api.Textable) clickyNode { case *api.LabelBadge: return convertLabelBadge(*v) default: - return clickyNode{Kind: "html", Plain: t.String(), HTML: t.HTML()} + return ClickyNode{Kind: "html", Plain: t.String(), HTML: t.HTML()} } } -func convertText(text api.Text) clickyNode { +func convertText(text api.Text) ClickyNode { return convertInlineTextNode("text", text) } -func convertInlineTextNode(kind string, text api.Text) clickyNode { - node := clickyNode{ +func convertInlineTextNode(kind string, text api.Text) ClickyNode { + node := ClickyNode{ Kind: kind, Plain: text.String(), Text: text.Content, @@ -329,14 +335,14 @@ func convertInlineTextNode(kind string, text api.Text) clickyNode { return node } -func convertLink(link api.Link) clickyNode { +func convertLink(link api.Link) ClickyNode { node := convertInlineTextNode("link", link.Content) node.Href = link.Href node.Target = string(link.Target) return node } -func convertLinkCommand(link api.LinkCommand) clickyNode { +func convertLinkCommand(link api.LinkCommand) ClickyNode { node := convertInlineTextNode("link-command", link.Content) node.Command = link.Command node.Target = string(link.Target) @@ -353,8 +359,8 @@ func convertLinkCommand(link api.LinkCommand) clickyNode { return node } -func convertList(list api.List) clickyNode { - node := clickyNode{ +func convertList(list api.List) ClickyNode { + node := ClickyNode{ Kind: "list", Plain: list.String(), Ordered: list.Numbered || list.Ordered, @@ -374,72 +380,72 @@ func convertList(list api.List) clickyNode { return node } -func convertTextList(list api.TextList) clickyNode { - node := clickyNode{Kind: "list", Plain: list.String()} +func convertTextList(list api.TextList) ClickyNode { + node := ClickyNode{Kind: "list", Plain: list.String()} for _, item := range list { node.Items = append(node.Items, convertTextable(item)) } return node } -func convertTextMap(tm api.TextMap, schema *api.PrettyObject) clickyNode { - fields := make([]clickyField, 0, len(tm)) +func convertTextMap(tm api.TextMap, schema *api.PrettyObject) ClickyNode { + fields := make([]ClickyField, 0, len(tm)) for _, key := range orderedFieldNames(schema, mapKeysTextMap(tm)) { value, ok := tm[key] if !ok { continue } - fields = append(fields, clickyField{ + fields = append(fields, ClickyField{ Name: key, Label: clickySchemaFieldLabel(schema, key), Value: convertTextable(value), }) } - return clickyNode{Kind: "map", Plain: tm.String(), Fields: fields} + return ClickyNode{Kind: "map", Plain: tm.String(), Fields: fields} } -func convertTypedMap(tm *api.TypedMap, schema *api.PrettyObject) clickyNode { +func convertTypedMap(tm *api.TypedMap, schema *api.PrettyObject) ClickyNode { if tm == nil { - return clickyNode{Kind: "map"} + return ClickyNode{Kind: "map"} } - fields := make([]clickyField, 0, len(*tm)) + fields := make([]ClickyField, 0, len(*tm)) for _, key := range orderedFieldNames(schema, mapKeysTypedMap(*tm)) { value, ok := (*tm)[key] if !ok { continue } - fields = append(fields, clickyField{ + fields = append(fields, ClickyField{ Name: key, Label: clickySchemaFieldLabel(schema, key), Value: convertTypedValue(&value, nil), }) } - return clickyNode{Kind: "map", Plain: tm.String(), Fields: fields} + return ClickyNode{Kind: "map", Plain: tm.String(), Fields: fields} } -func convertTypedList(tl *api.TypedList) clickyNode { +func convertTypedList(tl *api.TypedList) ClickyNode { if tl == nil { - return clickyNode{Kind: "list"} + return ClickyNode{Kind: "list"} } - node := clickyNode{Kind: "list", Plain: tl.String()} + node := ClickyNode{Kind: "list", Plain: tl.String()} for _, item := range *tl { node.Items = append(node.Items, convertTypedValue(&item, nil)) } return node } -func convertTable(table *api.TextTable) clickyNode { +func convertTable(table *api.TextTable) ClickyNode { if table == nil { - return clickyNode{Kind: "table"} + return ClickyNode{Kind: "table"} } - node := clickyNode{Kind: "table", Plain: table.String()} + node := ClickyNode{Kind: "table", Plain: table.String()} if len(table.Headers) > 0 { for i, header := range table.Headers { - column := clickyColumn{} + column := ClickyColumn{} if i < len(table.FieldNames) && table.FieldNames[i] != "" { column.Name = table.FieldNames[i] } else if i < len(table.Columns) && table.Columns[i].Name != "" { @@ -462,7 +468,7 @@ func convertTable(table *api.TextTable) clickyNode { } } else { for _, columnDef := range table.Columns { - column := clickyColumn{ + column := ClickyColumn{ Name: columnDef.Name, Label: columnDef.Label, Align: alignFromStyle(columnDef.Style), @@ -475,7 +481,7 @@ func convertTable(table *api.TextTable) clickyNode { } for rowIndex, row := range table.Rows { - rowNode := clickyRow{Cells: map[string]clickyNode{}} + rowNode := ClickyRow{Cells: map[string]ClickyNode{}} for _, column := range node.Columns { if cell, ok := row[column.Name]; ok { rowNode.Cells[column.Name] = convertTypedValue(&cell, nil) @@ -505,12 +511,12 @@ func convertTable(table *api.TextTable) clickyNode { return node } -func convertTree(tree *api.TextTree) clickyNode { +func convertTree(tree *api.TextTree) ClickyNode { if tree == nil { - return clickyNode{Kind: "tree"} + return ClickyNode{Kind: "tree"} } - node := clickyNode{Kind: "tree", Plain: tree.String()} + node := ClickyNode{Kind: "tree", Plain: tree.String()} if tree.Node == nil { for index, child := range tree.Children { node.Roots = append(node.Roots, convertTreeItem(&child, fmt.Sprintf("root-%d", index))) @@ -522,8 +528,8 @@ func convertTree(tree *api.TextTree) clickyNode { return node } -func convertTreeItem(tree *api.TextTree, id string) clickyTreeItem { - item := clickyTreeItem{ +func convertTreeItem(tree *api.TextTree, id string) ClickyTreeItem { + item := ClickyTreeItem{ ID: id, Label: convertTextable(tree.Node), } @@ -535,8 +541,8 @@ func convertTreeItem(tree *api.TextTree, id string) clickyTreeItem { return item } -func convertCode(code api.Code) clickyNode { - return clickyNode{ +func convertCode(code api.Code) ClickyNode { + return ClickyNode{ Kind: "code", Plain: code.String(), Style: convertTextStyle(code.Style, api.Class{}, true), @@ -546,8 +552,8 @@ func convertCode(code api.Code) clickyNode { } } -func convertStackTrace(trace api.StackTrace) clickyNode { - node := clickyNode{ +func convertStackTrace(trace api.StackTrace) ClickyNode { + node := ClickyNode{ Kind: "stacktrace", Plain: trace.String(), ExceptionClass: trace.ExceptionClass, @@ -578,7 +584,7 @@ func convertStackTrace(trace api.StackTrace) clickyNode { if frame.Runtime { kind = "runtime" } - node.Frames = append(node.Frames, clickyStackFrame{ + node.Frames = append(node.Frames, ClickyStackFrame{ FunctionName: functionName, DisplayName: functionName, File: frame.File, @@ -599,18 +605,18 @@ func convertStackTrace(trace api.StackTrace) clickyNode { return node } -func convertCollapsed(collapsed api.Collapsed) clickyNode { - label := clickyNode{ +func convertCollapsed(collapsed api.Collapsed) ClickyNode { + label := ClickyNode{ Kind: "text", Plain: collapsed.Label, Text: collapsed.Label, Style: convertTextStyle(collapsed.Style, api.Class{}, false), } if collapsed.Icon != nil { - label.Children = append([]clickyNode{convertIcon(*collapsed.Icon)}, label.Children...) + label.Children = append([]ClickyNode{convertIcon(*collapsed.Icon)}, label.Children...) } - node := clickyNode{ + node := ClickyNode{ Kind: "collapsed", Plain: collapsed.String(), Label: &label, @@ -624,9 +630,9 @@ func convertCollapsed(collapsed api.Collapsed) clickyNode { return node } -func convertButton(button api.Button) clickyNode { +func convertButton(button api.Button) ClickyNode { label := clickyTextNode(button.Label) - return clickyNode{ + return ClickyNode{ Kind: "button", Plain: button.String(), Label: &label, @@ -637,16 +643,16 @@ func convertButton(button api.Button) clickyNode { } } -func convertButtonGroup(group api.ButtonGroup) clickyNode { - node := clickyNode{Kind: "button-group", Plain: group.String()} +func convertButtonGroup(group api.ButtonGroup) ClickyNode { + node := ClickyNode{Kind: "button-group", Plain: group.String()} for _, button := range group.Buttons { node.Items = append(node.Items, convertButton(button)) } return node } -func convertIcon(icon apiicons.Icon) clickyNode { - return clickyNode{ +func convertIcon(icon apiicons.Icon) ClickyNode { + return ClickyNode{ Kind: "icon", Plain: icon.String(), Style: convertTextStyle(icon.Style, api.Class{}, false), @@ -655,14 +661,14 @@ func convertIcon(icon apiicons.Icon) clickyNode { } } -func convertKeyValuePair(pair api.KeyValuePair) clickyNode { +func convertKeyValuePair(pair api.KeyValuePair) ClickyNode { if pair.IsEmpty() { - return clickyNode{Kind: "map"} + return ClickyNode{Kind: "map"} } - return clickyNode{ + return ClickyNode{ Kind: "map", Plain: pair.String(), - Fields: []clickyField{ + Fields: []ClickyField{ { Name: pair.Key, Label: pair.Key, @@ -672,8 +678,8 @@ func convertKeyValuePair(pair api.KeyValuePair) clickyNode { } } -func convertLabelBadge(b api.LabelBadge) clickyNode { - return clickyNode{ +func convertLabelBadge(b api.LabelBadge) ClickyNode { + return ClickyNode{ Kind: "badge", Plain: b.String(), BadgeLabel: b.Label, @@ -685,13 +691,13 @@ func convertLabelBadge(b api.LabelBadge) clickyNode { } } -func convertDescriptionList(list api.DescriptionList) clickyNode { - node := clickyNode{Kind: "map", Plain: list.String()} +func convertDescriptionList(list api.DescriptionList) ClickyNode { + node := ClickyNode{Kind: "map", Plain: list.String()} for _, item := range list.Items { if item.IsEmpty() { continue } - node.Fields = append(node.Fields, clickyField{ + node.Fields = append(node.Fields, ClickyField{ Name: item.Key, Label: item.Key, Value: convertAnyToNode(item.Value), @@ -700,19 +706,19 @@ func convertDescriptionList(list api.DescriptionList) clickyNode { return node } -func convertAnyToNode(value any) clickyNode { +func convertAnyToNode(value any) ClickyNode { if typed := api.TryTypedValue(value); typed != nil { return convertTypedValue(typed, nil) } return clickyTextNode(fmt.Sprintf("%v", value)) } -func clickyTextNode(text string) clickyNode { - return clickyNode{Kind: "text", Plain: text, Text: text} +func clickyTextNode(text string) ClickyNode { + return ClickyNode{Kind: "text", Plain: text, Text: text} } -func convertTextStyle(styleStr string, class api.Class, monospace bool) *clickyStyle { - style := clickyStyle{ +func convertTextStyle(styleStr string, class api.Class, monospace bool) *ClickyStyle { + style := ClickyStyle{ ClassName: strings.TrimSpace(styleStr), Monospace: monospace || strings.Contains(styleStr, "font-mono"), } @@ -749,7 +755,7 @@ func convertTextStyle(styleStr string, class api.Class, monospace bool) *clickyS } } - if style == (clickyStyle{}) { + if style == (ClickyStyle{}) { return nil } diff --git a/formatters/html_react_formatter_test.go b/formatters/html_react_formatter_test.go index 610c9499..696d083c 100644 --- a/formatters/html_react_formatter_test.go +++ b/formatters/html_react_formatter_test.go @@ -365,3 +365,101 @@ func TestHTMLReactBuildHTML(t *testing.T) { } } } + +// entityLinkLike mirrors models.EntityLink: it implements BOTH Pretty (which +// returns a Link) and Textable. When it is a struct field it must serialize as +// a single link node, not a {kind,guid,name} field map. +type entityLinkLike struct { + Kind string `json:"kind"` + GUID string `json:"guid"` + Name string `json:"name"` +} + +func (l entityLinkLike) Pretty() api.Text { + return api.Text{}.Add(api.Link{Href: "/entity/" + l.Kind + "/" + l.GUID, Content: api.Text{Content: l.Name}}) +} +func (l entityLinkLike) String() string { return l.Name } +func (l entityLinkLike) ANSI() string { return l.Name } +func (l entityLinkLike) HTML() string { return l.Pretty().HTML() } +func (l entityLinkLike) Markdown() string { return l.Pretty().Markdown() } + +// TestClickyJSONEntityLinkFieldRendersAsLink pins the EntityLink fix: a struct +// field whose type implements both Pretty and Textable serializes as a "link" +// node (honoring Pretty), not a nested "map" of its struct fields. +func TestClickyJSONEntityLinkFieldRendersAsLink(t *testing.T) { + type detail struct { + Name string `json:"name"` + ClientEntityLink entityLinkLike `json:"clientEntityLink"` + } + d := detail{ + Name: "Scheme", + ClientEntityLink: entityLinkLike{Kind: "client", GUID: "abc-123", Name: "GL Scheme G0796016"}, + } + + out, err := (&ClickyJSONFormatter{}).Format(d, FormatOptions{}) + if err != nil { + t.Fatalf("format: %v", err) + } + if !strings.Contains(out, `"link"`) { + t.Errorf("expected a link node, got:\n%s", out) + } + if !strings.Contains(out, "/entity/client/abc-123") { + t.Errorf("expected clientEntityLink href in output, got:\n%s", out) + } + if !strings.Contains(out, "GL Scheme G0796016") { + t.Errorf("expected link label in output, got:\n%s", out) + } + // The field must NOT degrade to a map exposing its kind/guid/name fields. + if strings.Contains(out, `"guid"`) { + t.Errorf("clientEntityLink leaked struct fields (rendered as a map), got:\n%s", out) + } +} + +// shortLinkLike implements PrettyShort (a compact self-link) AND Pretty (a +// fuller block). The `short` tag must select PrettyShort. +type shortLinkLike struct { + Kind string `json:"kind"` + GUID string `json:"guid"` + Name string `json:"name"` +} + +func (l shortLinkLike) PrettyShort() api.Textable { + return api.Text{}.Add(api.Link{Href: "/entity/" + l.Kind + "/" + l.GUID, Content: api.Text{Content: l.Name}}) +} +func (l shortLinkLike) Pretty() api.Text { + return api.Text{Content: "FULL-DETAIL-BLOCK-" + l.Name} +} + +// TestClickyJSONShortTagRendersPrettyShort pins the `short` pretty-tag: a field +// tagged `pretty:",short"` renders via PrettyShort() (a link node), while the +// same value without the tag renders via Pretty() (the full block). +func TestClickyJSONShortTagRendersPrettyShort(t *testing.T) { + type shortDetail struct { + Plan shortLinkLike `json:"plan" pretty:",short"` + } + out, err := (&ClickyJSONFormatter{}).Format( + shortDetail{Plan: shortLinkLike{Kind: "plan", GUID: "p-1", Name: "Life Plan"}}, + FormatOptions{}) + if err != nil { + t.Fatalf("format: %v", err) + } + if !strings.Contains(out, `"link"`) || !strings.Contains(out, "/entity/plan/p-1") { + t.Errorf("short tag: expected a link node to the plan, got:\n%s", out) + } + if strings.Contains(out, "FULL-DETAIL-BLOCK") { + t.Errorf("short tag: expected PrettyShort, not Pretty's full block, got:\n%s", out) + } + + type plainDetail struct { + Plan shortLinkLike `json:"plan"` + } + plain, err := (&ClickyJSONFormatter{}).Format( + plainDetail{Plan: shortLinkLike{Kind: "plan", GUID: "p-1", Name: "Life Plan"}}, + FormatOptions{}) + if err != nil { + t.Fatalf("format: %v", err) + } + if !strings.Contains(plain, "FULL-DETAIL-BLOCK-Life Plan") { + t.Errorf("without short tag: expected Pretty's full block, got:\n%s", plain) + } +} From 5c5c40842c57225ee7abb3e0a2547e157fa2b645 Mon Sep 17 00:00:00 2001 From: Moshe Immerman <moshe@flanksource.com> Date: Sun, 31 May 2026 13:30:54 +0300 Subject: [PATCH 5/5] feat(api): Add PrettyShort interface for compact cell rendering and Link JSON payload support Introduce PrettyShort interface to enable objects to provide compact, single-line representations for table cells (typically self-links), distinct from Pretty()'s fuller detail view. Cell renderers now prefer PrettyShort() over Pretty() when available. Add JSON field to Link struct with WithJSON() builder method and MarshalJSON() implementation to serialize links as structured objects with href, text, tooltip, and optional payload. This allows structured-JSON consumers to render links as anchors while preserving navigation metadata. Add `short` pretty-tag flag to FieldMeta to control field rendering via PrettyShort() in structs, tables, and clicky+json printers. Update TryTypedValue to check Pretty before Textable, ensuring types implementing both (e.g. EntityLink) honor their Pretty() method rather than serializing as bare structs. Includes comprehensive tests for Link.MarshalJSON, PrettyShort precedence, and Pretty/Textable interaction. --- api/column.go | 2 ++ api/link.go | 42 ++++++++++++++++++++++- api/link_test.go | 86 +++++++++++++++++++++++++++++++++++++++++++++++ api/meta.go | 10 ++++-- api/meta_test.go | 32 ++++++++++++++++++ api/parse_tags.go | 2 ++ api/parser.go | 34 +++++++++++++++++++ api/types.go | 14 ++++++++ entity.go | 2 ++ 9 files changed, 221 insertions(+), 3 deletions(-) create mode 100644 api/link_test.go diff --git a/api/column.go b/api/column.go index 0521c53c..ad765ee5 100644 --- a/api/column.go +++ b/api/column.go @@ -194,6 +194,8 @@ func convertToTextable(val any) Textable { } switch v := val.(type) { + case PrettyShort: + return v.PrettyShort() case Textable: return v case Pretty: diff --git a/api/link.go b/api/link.go index d8733aba..c7fd33a0 100644 --- a/api/link.go +++ b/api/link.go @@ -1,6 +1,9 @@ package api -import "fmt" +import ( + "encoding/json" + "fmt" +) type LinkTarget string @@ -18,12 +21,25 @@ type Link struct { Href string Target LinkTarget Content Text + // JSON is an optional structured payload carried alongside the link. Unlike + // Text, a Link serializes to a structured object (see MarshalJSON), so this + // payload survives JSON encoding and reaches structured-JSON consumers (e.g. + // a web UI that renders an <a> from the href and uses the payload for + // client-side navigation). It is ignored by String/ANSI/Markdown/HTML. + JSON any } func NewLink(href string) Link { return Link{Href: href} } +// WithJSON attaches a structured payload to the link. The payload is emitted by +// MarshalJSON under the "json" key; it does not affect text rendering. +func (l Link) WithJSON(v any) Link { + l.JSON = v + return l +} + func (l Link) Add(child Textable) Link { l.Content = l.Content.Add(child) return l @@ -119,6 +135,30 @@ func (l Link) HTML() string { return fmt.Sprintf(`<a href="%s"%s%s>%s</a>`, htmlEscapeString(l.Href), target, rel, content) } +// MarshalJSON serializes a Link as a structured object so its href and payload +// survive JSON encoding. This is deliberately richer than Text.MarshalJSON +// (which flattens to a plain string): a structured-JSON consumer can render the +// link as an anchor and use the payload for navigation. Only non-empty fields +// are emitted. +func (l Link) MarshalJSON() ([]byte, error) { + out := map[string]any{"text": l.Content.String()} + if l.Href != "" { + out["href"] = l.Href + } + if l.Target != "" { + out["target"] = string(l.Target) + } + if l.Content.Tooltip != nil { + if tip := l.Content.Tooltip.String(); tip != "" { + out["tooltip"] = tip + } + } + if l.JSON != nil { + out["json"] = l.JSON + } + return json.Marshal(out) +} + type LinkCommand struct { Command string Args []string diff --git a/api/link_test.go b/api/link_test.go new file mode 100644 index 00000000..067410d2 --- /dev/null +++ b/api/link_test.go @@ -0,0 +1,86 @@ +package api + +import ( + "encoding/json" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// shortAndLong implements both PrettyShort and Pretty; cells must pick the short form. +type shortAndLong struct{} + +func (shortAndLong) PrettyShort() Textable { return Text{Content: "short"} } +func (shortAndLong) Pretty() Text { return Text{Content: "long"} } + +// longOnly implements only Pretty; cells fall back to it. +type longOnly struct{} + +func (longOnly) Pretty() Text { return Text{Content: "long"} } + +var _ = Describe("Link", func() { + Describe("MarshalJSON", func() { + It("emits a structured object with href, text, and payload", func() { + link := NewLink("/entity/plan/abc"). + Text("Life Plan"). + WithJSON(map[string]any{"id": "abc", "name": "Life Plan", "kind": "plan"}) + + raw, err := json.Marshal(link) + Expect(err).NotTo(HaveOccurred()) + + var shape map[string]any + Expect(json.Unmarshal(raw, &shape)).To(Succeed()) + Expect(shape["href"]).To(Equal("/entity/plan/abc")) + Expect(shape["text"]).To(Equal("Life Plan")) + payload, ok := shape["json"].(map[string]any) + Expect(ok).To(BeTrue()) + Expect(payload["id"]).To(Equal("abc")) + Expect(payload["name"]).To(Equal("Life Plan")) + Expect(payload["kind"]).To(Equal("plan")) + }) + + It("omits href, target, tooltip, and json when unset", func() { + raw, err := json.Marshal(NewLink("").Text("plain")) + Expect(err).NotTo(HaveOccurred()) + + var shape map[string]any + Expect(json.Unmarshal(raw, &shape)).To(Succeed()) + Expect(shape).To(HaveKeyWithValue("text", "plain")) + Expect(shape).NotTo(HaveKey("href")) + Expect(shape).NotTo(HaveKey("target")) + Expect(shape).NotTo(HaveKey("tooltip")) + Expect(shape).NotTo(HaveKey("json")) + }) + + It("emits the tooltip when one is set", func() { + link := NewLink("/x").Text("name").WithTooltip(Text{Content: "the-guid"}) + raw, err := json.Marshal(link) + Expect(err).NotTo(HaveOccurred()) + + var shape map[string]any + Expect(json.Unmarshal(raw, &shape)).To(Succeed()) + Expect(shape["tooltip"]).To(Equal("the-guid")) + }) + }) + + Describe("PrettyShort cell preference", func() { + It("prefers PrettyShort over Pretty for a cell value", func() { + Expect(convertToTextable(shortAndLong{}).String()).To(Equal("short")) + }) + + It("falls back to Pretty when PrettyShort is not implemented", func() { + Expect(convertToTextable(longOnly{}).String()).To(Equal("long")) + }) + }) + + Describe("text rendering is unaffected by the payload", func() { + It("renders String/Markdown/HTML the same with or without JSON", func() { + base := NewLink("/x").Text("name") + withJSON := base.WithJSON(map[string]any{"id": "1"}) + + Expect(withJSON.String()).To(Equal(base.String())) + Expect(withJSON.Markdown()).To(Equal(base.Markdown())) + Expect(withJSON.HTML()).To(Equal(base.HTML())) + }) + }) +}) diff --git a/api/meta.go b/api/meta.go index 405e168b..9012bed9 100644 --- a/api/meta.go +++ b/api/meta.go @@ -379,6 +379,7 @@ type TextMap map[string]Textable type FieldMeta struct { Name string CompactItems bool + Short bool Format string } @@ -504,14 +505,19 @@ func TryTypedValue(o any) *TypedValue { return &TypedValue{TypedMap: &v} case TypedList: return &TypedValue{TypedList: &v} - case Textable: - return &TypedValue{Textable: v} case TreeNode: return &TypedValue{Tree: lo.ToPtr(NewTree(v))} case TreeMixin: return &TypedValue{Tree: lo.ToPtr(NewTree(v.Tree()))} + // Pretty must be checked before Textable: a type implementing both (e.g. + // an EntityLink whose Pretty() returns a Link) controls its own rendered + // node via Pretty(), so honor that rather than treating the bare value as + // Textable and serializing it as a struct/map. This mirrors the slice path + // below, which already checks Pretty before Textable. case Pretty: return &TypedValue{Textable: v.Pretty()} + case Textable: + return &TypedValue{Textable: v} case []TableMixin: return &TypedValue{Table: lo.ToPtr(NewTable(v))} case []TableRowMixin2: diff --git a/api/meta_test.go b/api/meta_test.go index e2929fae..14e0b9df 100644 --- a/api/meta_test.go +++ b/api/meta_test.go @@ -88,6 +88,23 @@ func (t testTextable) ANSI() string { return t.Value } func (t testTextable) HTML() string { return t.Value } func (t testTextable) Markdown() string { return t.Value } +// testPrettyLink implements BOTH Pretty and Textable, with Pretty() returning +// a Link. This mirrors models.EntityLink: TryTypedValue must honor Pretty() +// (yielding the Link) rather than treating the value as a bare Textable that +// would serialize as a struct/map node. +type testPrettyLink struct { + Href string + Label string +} + +func (t testPrettyLink) Pretty() Text { + return Text{}.Add(Link{Href: t.Href, Content: Text{Content: t.Label}}) +} +func (t testPrettyLink) String() string { return t.Label } +func (t testPrettyLink) ANSI() string { return t.Label } +func (t testPrettyLink) HTML() string { return t.Pretty().HTML() } +func (t testPrettyLink) Markdown() string { return t.Pretty().Markdown() } + var _ = Describe("PrettyData", func() { Describe("IsEmpty", func() { It("returns true for nil PrettyData", func() { @@ -308,4 +325,19 @@ var _ = Describe("TextTable marshaling", func() { Expect(string(data)).To(Equal("[]\n")) }) }) + + Describe("Pretty precedence over Textable", func() { + It("uses Pretty() for a value implementing both Pretty and Textable", func() { + // EntityLink-style value: Pretty() returns a Link. The result must + // carry the rendered Link, not the bare struct, so it serializes as + // a link node rather than a {href,label} field map. + result := TryTypedValue(testPrettyLink{Href: "/entity/client/abc", Label: "GL Scheme G0796016"}) + + Expect(result).NotTo(BeNil()) + Expect(result.Textable).NotTo(BeNil()) + html := result.Textable.HTML() + Expect(html).To(ContainSubstring(`href="/entity/client/abc"`)) + Expect(html).To(ContainSubstring("GL Scheme G0796016")) + }) + }) }) diff --git a/api/parse_tags.go b/api/parse_tags.go index ba9ae8c1..1a756117 100644 --- a/api/parse_tags.go +++ b/api/parse_tags.go @@ -102,6 +102,8 @@ func ParsePrettyTagWithName(fieldName, tag string) PrettyField { field.FormatOptions["dir"] = part case "compact": field.CompactItems = true + case "short": + field.Short = true case "no_icons": if field.TreeOptions == nil { field.TreeOptions = DefaultTreeOptions() diff --git a/api/parser.go b/api/parser.go index 3060378d..ee89ce6c 100644 --- a/api/parser.go +++ b/api/parser.go @@ -23,6 +23,24 @@ func SafeDerefPointer(val reflect.Value) (reflect.Value, bool) { return val.Elem(), false } +// shortTextable returns the PrettyShort() rendering of a field value when it +// implements PrettyShort, or nil otherwise. It is the render hook for the +// `short` pretty-tag. Nil pointers (including a typed nil whose PrettyShort has +// a value receiver) return nil rather than panicking, so a `short`-tagged +// relation that is absent renders as empty. +func shortTextable(val reflect.Value) Textable { + if !val.IsValid() || !val.CanInterface() { + return nil + } + if val.Kind() == reflect.Ptr && val.IsNil() { + return nil + } + if ps, ok := val.Interface().(PrettyShort); ok { + return ps.PrettyShort() + } + return nil +} + // jsonFieldName returns the JSON field name from a struct field's tag, // falling back to the struct field name if no valid json tag is present. func jsonFieldName(f reflect.StructField) string { @@ -315,6 +333,15 @@ func (p *StructParser) ParseDataWithSchema(data interface{}, schema *PrettyObjec fieldVal = fieldVal.Elem() } + // `short` tag: render the field via its value's PrettyShort() (a + // compact self-link) instead of Pretty()/Textable. + if field.Short { + if short := shortTextable(fieldVal); short != nil { + values[field.Name] = TypedValue{Textable: short} + continue + } + } + // Try TryTypedValue first - handles TableProvider, TreeNode, Textable, etc. if fieldVal.CanInterface() { if tv := TryTypedValue(fieldVal.Interface()); tv != nil { @@ -484,6 +511,13 @@ func (p *StructParser) parseTableData(val reflect.Value, field PrettyField) Text if fieldVal.Kind() == reflect.Interface && !fieldVal.IsNil() { fieldVal = fieldVal.Elem() } + // `short` tag: render the cell via its value's PrettyShort(). + if tableField.Short { + if short := shortTextable(fieldVal); short != nil { + row[tableField.Name] = TypedValue{Textable: short} + continue + } + } // Use tableField.Parse to apply type-specific formatting (dates, currency, etc.) fieldValue, err := tableField.Parse(fieldVal.Interface()) if err != nil { diff --git a/api/types.go b/api/types.go index bdb3143f..dd2730bb 100644 --- a/api/types.go +++ b/api/types.go @@ -24,6 +24,16 @@ type PrettyFull interface { PrettyFull() Textable } +// PrettyShort enables objects to provide a compact, single-line representation +// for use in table cells (typically a self-link), distinct from Pretty()'s +// fuller detail view. Cell renderers prefer PrettyShort() over Pretty() when a +// value implements it; everything else renders via Pretty()/Textable unchanged. +// It returns a Textable so an implementer can hand back a value type (e.g. a +// link struct that itself implements Textable) without an extra render call. +type PrettyShort interface { + PrettyShort() Textable +} + // PrettyRow enables structs to provide custom table row representation with fine-grained control // over columns and cell formatting based on output format options. // The opts parameter should be of type formatters.FormatOptions. @@ -61,6 +71,10 @@ type PrettyField struct { // For custom rendering RenderFunc RenderFunc `json:"-" yaml:"-"` CompactItems bool `json:"compact_items,omitempty" yaml:"compact_items,omitempty"` + // Short renders the field via its value's PrettyShort() (a compact + // self-link, typically) instead of Pretty()/Textable, in the struct, + // table, and clicky+json printers. Set by the `short` pretty-tag flag. + Short bool `json:"short,omitempty" yaml:"short,omitempty"` } // TableOptions configures tabular data presentation including column definitions, diff --git a/entity.go b/entity.go index ce76d7dd..e3561042 100644 --- a/entity.go +++ b/entity.go @@ -1412,6 +1412,8 @@ func columnsAndRowFromStruct(inner any) ([]api.ColumnDef, map[string]any, bool) func addPrettyRowValue(cell api.Text, value any) api.Text { switch v := value.(type) { + case api.PrettyShort: + return cell.Add(v.PrettyShort()) case api.Textable: return cell.Add(v) case api.Pretty: