diff --git a/docs/adr/0014-sequence-placement-and-composition.md b/docs/adr/0014-sequence-placement-and-composition.md new file mode 100644 index 0000000..8ecfcec --- /dev/null +++ b/docs/adr/0014-sequence-placement-and-composition.md @@ -0,0 +1,73 @@ +# ADR 0014: Place and Compose Sequence Items Explicitly + +## Status + +Accepted. Implementation status and sequencing are tracked in the +[sequence composition implementation plan](../roadmap/implementation.md). + +## Date + +2026-10-16 + +## Context + +ConfigKit strictly composes independent documents by recursively combining +mappings and rejecting other overlapping values. This makes two independently +authored sequence fragments conflict, even when their records are distinct. +Precedence assembly has a different purpose: a higher-priority present sequence +replaces the lower-priority sequence as one value. Those existing rules are +predictable, but they do not express the separate operation of collecting +independent records while rejecting duplicate identities. + +Likewise, mapping destinations place a source beneath literal mapping keys, but +there is no explicit way to state that a source mapping represents one item in a +sequence. Inferring that intent from document shape or fields would make source +meaning implicit and change existing placement behavior. + +## Decision + +Keep mapping placement as the zero-value behavior and add an explicit item +placement choice for a source whose mapping should become one element at a +non-root sequence destination. Item placement provides structure only; it does +not infer or synthesize identity values. + +Allow a definition to register literal mapping paths and one literal identity +field for sequences at those paths. Within each strict composition tier, a +registered sequence accepts mapping records with a present string identity, +appends distinct records in contribution order, and rejects repeated exact +identity strings. Identical registrations coalesce; conflicting identity fields +for the same path are invalid. Registration does not recursively select paths +inside sequence records. + +Keep precedence assembly unchanged. A present higher-priority sequence replaces +the lower-priority value wholesale, including at a registered path. Registration +changes strict composition only; it does not turn precedence into append or +identity-based merging. + +This decision preserves ConfigKit's distinction between independent +same-priority contributions and intentional precedence. Explicit source +placement and definition-owned composition policy make the two required choices +visible while keeping the existing zero-value load behavior. + +## Alternatives Considered + +- **Infer item placement from a source mapping or identity field.** Shape-based + inference would make ordinary mapping placement depend on document contents + and would leave the intended destination ambiguous. +- **Append all overlapping sequences during strict composition.** This would + change existing conflict semantics at unregistered paths and would not detect + duplicate records. +- **Merge matching records recursively by identity.** This would add a second + recursive merge rule with different conflict and precedence behavior from + ConfigKit's existing mapping composition and overlay operations. +- **Use identity to merge sequences across precedence tiers.** This would + change established whole-value replacement semantics and conflate strict + composition with precedence. + +## Consequences + +Consumers can explicitly place mappings as sequence items and opt selected +sequences into duplicate-rejecting strict composition. Unregistered sequences +retain their current strict-conflict behavior, and higher-priority sequences +continue to replace lower-priority ones. The implementation plan owns the exact +API, YAML syntax, diagnostics, and implementation status. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index bdf451e..5c4be5c 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -213,6 +213,24 @@ Scope: one ADR and engine option normalization; no new public fields yet. Validation: focused option tests and `./scripts/check.sh`. Completion: existing loads remain unchanged, with only private registered-policy input newly accepted. +Completion evidence: added Accepted ADR 0014 and private engine registration +normalization. Ordinary and layered entry points compile/coalesce policy and +reject invalid registrations before source reads, then carry the compiled policy +through the load request while clearing raw registration slices from internal +handoffs. No public API or composition behavior was added. Focused coverage +checks duplicate/conflicting definitions, empty identity names, numeric and +escaped literal keys, invalid explicit indexes, unused/root registrations, +zero-option behavior, and reader preservation on preflight failure. These checks +passed with Go `go1.26.9 linux/amd64`: + +- `GOTOOLCHAIN=local GOPROXY=off go test ./internal/engine -run 'SequenceRegistration' -count=1` +- `GOTOOLCHAIN=local ./scripts/check.sh` +- `git diff --check` + +The compiled registration policy is intentionally not consumed by composition +until the next implementation unit. No environmental limitation affected +validation; checks ran on Linux/amd64. + ## Stage 2 — Implement registered strict sequence validation and assembly Scope: `internal/engine/compose.go` and focused composition tests. diff --git a/internal/engine/load.go b/internal/engine/load.go index 7886127..0bdbd30 100644 --- a/internal/engine/load.go +++ b/internal/engine/load.go @@ -16,6 +16,8 @@ type Definition[T any] struct { Validate func(*T, *Validation) Version *VersionRule Secret func(Path) bool + + sequenceRegistrations []sequenceRegistration } // Options contains the application definition and per-load input selections. @@ -137,12 +139,16 @@ func Load[T any](source Source, secrets SecretBinding, supplied ...Options[T]) ( func loadWithSecretLookup[T any](source Source, secrets SecretBinding, lookup secretLookup, supplied ...Options[T]) (*Loaded[T], error) { options, diagnostics := selectOptions(source, supplied) + policy, policyDiagnostics := compileSequencePolicy(options.Definition.sequenceRegistrations) + diagnostics = append(diagnostics, policyDiagnostics...) if len(diagnostics) != 0 { return nil, (&diagnosticAccumulator{diagnostics: diagnostics}).loadError() } + definition := options.Definition + definition.sequenceRegistrations = nil request := loadRequest[T]{ main: loadInput{source: source, imports: options.Imports, filesystem: options.Filesystem}, - definition: options.Definition, overlay: options.Overlay, + definition: definition, sequencePolicy: policy, overlay: options.Overlay, overrides: options.Overrides, ordinary: true, sourceVersionMode: options.SourceVersionMode, effectiveVersion: effectiveSourceVersionRule(options.Definition.Version, Path{}, options.SourceVersionMode), diff --git a/internal/engine/load_request.go b/internal/engine/load_request.go index 0561643..1865dcb 100644 --- a/internal/engine/load_request.go +++ b/internal/engine/load_request.go @@ -10,6 +10,7 @@ type loadRequest[T any] struct { main loadInput underlays []loadGroup definition Definition[T] + sequencePolicy sequencePolicy sourceVersionMode sourceVersionMode effectiveVersion *VersionRule overlay string @@ -37,6 +38,10 @@ type loadGroup struct{ inputs []loadInput } func loadGroupedWithSecretLookup[T any](request loadRequest[T], secrets SecretBinding, lookup secretLookup) (*Loaded[T], error) { request.ordinary = false var diagnostics []Diagnostic + policy, policyDiagnostics := compileSequencePolicy(request.definition.sequenceRegistrations) + request.sequencePolicy = policy + diagnostics = append(diagnostics, policyDiagnostics...) + request.definition.sequenceRegistrations = nil if normalized, err := normalizeDestination(request.main.destination); err != nil { diagnostics = append(diagnostics, invalidDestinationDiagnostic(request.main.destination)) } else { diff --git a/internal/engine/sequence_registration.go b/internal/engine/sequence_registration.go new file mode 100644 index 0000000..ad9a057 --- /dev/null +++ b/internal/engine/sequence_registration.go @@ -0,0 +1,38 @@ +package engine + +type sequenceRegistration struct { + path Path + identityField string +} + +type sequencePolicy struct { + identityFields map[string]string +} + +func compileSequencePolicy(registrations []sequenceRegistration) (sequencePolicy, []Diagnostic) { + policy := sequencePolicy{identityFields: make(map[string]string, len(registrations))} + var diagnostics []Diagnostic + for _, registration := range registrations { + path, err := normalizeDestination(registration.path) + if err != nil { + diagnostics = append(diagnostics, Diagnostic{ + Code: CodeOptionsInvalid, Severity: SeverityError, + Path: registration.path.clone(), Message: "sequence registration path must contain only non-reserved mapping keys", + }) + continue + } + + key := path.String() + if current, ok := policy.identityFields[key]; ok { + if current != registration.identityField { + diagnostics = append(diagnostics, Diagnostic{ + Code: CodeOptionsInvalid, Severity: SeverityError, + Path: path, Message: "sequence registrations at one path must use the same identity field", + }) + } + continue + } + policy.identityFields[key] = registration.identityField + } + return policy, canonicalDiagnostics(diagnostics) +} diff --git a/internal/engine/sequence_registration_test.go b/internal/engine/sequence_registration_test.go new file mode 100644 index 0000000..47603ea --- /dev/null +++ b/internal/engine/sequence_registration_test.go @@ -0,0 +1,127 @@ +package engine + +import ( + "errors" + "strings" + "testing" +) + +func TestCompileSequencePolicyNormalizesRegistrations(t *testing.T) { + parsed, err := ParsePath("/items/0/a~1b") + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + registrations []sequenceRegistration + want map[string]string + wantError bool + }{ + { + name: "identical registrations coalesce", + registrations: []sequenceRegistration{ + {path: Keys("items"), identityField: "id"}, + {path: Keys("items"), identityField: "id"}, + }, + want: map[string]string{"/items": "id"}, + }, + { + name: "different identity fields conflict", + registrations: []sequenceRegistration{ + {path: Keys("items"), identityField: "id"}, + {path: Keys("items"), identityField: "name"}, + }, + wantError: true, + }, + { + name: "empty identity field is literal", + registrations: []sequenceRegistration{{path: Keys("items"), identityField: ""}}, + want: map[string]string{"/items": ""}, + }, + { + name: "numeric and escaped tokens remain literal keys", + registrations: []sequenceRegistration{{path: parsed, identityField: "id"}}, + want: map[string]string{"/items/0/a~1b": "id"}, + }, + { + name: "explicit index is invalid", + registrations: []sequenceRegistration{{path: Keys("items").Index(0), identityField: "id"}}, + wantError: true, + }, + { + name: "unused and root registrations are accepted", + registrations: []sequenceRegistration{ + {path: Path{}, identityField: "id"}, + {path: Keys("unused"), identityField: "name"}, + }, + want: map[string]string{"": "id", "/unused": "name"}, + }, + {name: "zero option policy", want: map[string]string{}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + policy, diagnostics := compileSequencePolicy(test.registrations) + if (len(diagnostics) != 0) != test.wantError { + t.Fatalf("diagnostics = %#v, wantError = %t", diagnostics, test.wantError) + } + if test.wantError { + if diagnostics[0].Code != CodeOptionsInvalid { + t.Fatalf("diagnostic code = %q", diagnostics[0].Code) + } + return + } + if len(policy.identityFields) != len(test.want) { + t.Fatalf("compiled fields = %#v, want %#v", policy.identityFields, test.want) + } + for path, field := range test.want { + if got := policy.identityFields[path]; got != field { + t.Errorf("identity field at %q = %q, want %q", path, got, field) + } + } + }) + } +} + +func TestSequenceRegistrationPreflightDoesNotReadSources(t *testing.T) { + t.Parallel() + type config struct { + Value string `yaml:"value"` + } + + ordinaryReader := strings.NewReader("value: ordinary\n") + _, err := Load[config](Reader("ordinary.yml", ordinaryReader), SecretBinding{}, Options[config]{ + Definition: Definition[config]{sequenceRegistrations: []sequenceRegistration{{path: Keys("items").Index(0), identityField: "id"}}}, + }) + requireSequenceRegistrationOptionError(t, err) + if ordinaryReader.Len() == 0 { + t.Fatal("ordinary reader was consumed before invalid registration was rejected") + } + + mainReader := strings.NewReader("value: main\n") + underlayReader := strings.NewReader("value: underlay\n") + _, err = LoadLayered[config](Input{Source: Reader("main.yml", mainReader)}, SecretBinding{}, LayeredOptions[config]{ + Definition: Definition[config]{sequenceRegistrations: []sequenceRegistration{{path: Keys("items").Index(0), identityField: "id"}}}, + Underlays: []Underlay{{Inputs: []Input{{Source: Reader("underlay.yml", underlayReader)}}}}, + }) + requireSequenceRegistrationOptionError(t, err) + if mainReader.Len() == 0 || underlayReader.Len() == 0 { + t.Fatal("layered source was consumed before invalid registration was rejected") + } +} + +func requireSequenceRegistrationOptionError(t *testing.T, err error) { + t.Helper() + if err == nil { + t.Fatal("expected invalid sequence registration error") + } + var loadErr *LoadError + if !errors.As(err, &loadErr) { + t.Fatalf("error type = %T, want *LoadError", err) + } + diagnostics := loadErr.Diagnostics() + if len(diagnostics) != 1 || diagnostics[0].Code != CodeOptionsInvalid { + t.Fatalf("diagnostics = %#v, want one options diagnostic", diagnostics) + } +}