From d34a98d34bcd77e600f20eb260758364bdd2e1da Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 9 Oct 2026 03:27:42 +0000 Subject: [PATCH] Precompute canonical paths before sorting --- docs/roadmap/implementation.md | 2 +- internal/engine/diagnostic.go | 21 +++++++++++-- internal/engine/diagnostic_test.go | 20 ++++++++++++ internal/engine/document_compare.go | 17 ++++++++-- internal/engine/overlay.go | 19 +++++++++-- internal/engine/overlay_test.go | 23 ++++++++++++++ internal/engine/override_apply.go | 49 ++++++++++++++--------------- internal/engine/override_test.go | 37 ++++++++++++++++++++++ internal/engine/projection_test.go | 15 +++++++++ internal/engine/resolution.go | 20 ++++++++---- internal/engine/resolution_test.go | 25 +++++++++++++++ 11 files changed, 209 insertions(+), 39 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 83f93d2..8d7f9dc 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -156,7 +156,7 @@ Roadmap candidate 2. Owners: engine `path.go`, `resolution.go`, `metadata.go`. Validate: `go test ./internal/engine -run 'Path|Metadata|Resolution|Override|Inheritance'` and the everyday check. Public `Path.String` stays unchanged. -## Stage 5: Format sorting keys once per item +## Stage 5: Format sorting keys once per item — Complete Roadmap candidate 2. Owners: engine resolution, overlay, override application, document comparison, and diagnostics. diff --git a/internal/engine/diagnostic.go b/internal/engine/diagnostic.go index dd25d33..37e37cd 100644 --- a/internal/engine/diagnostic.go +++ b/internal/engine/diagnostic.go @@ -85,15 +85,32 @@ func canonicalDiagnostics(diagnostics []Diagnostic) []Diagnostic { for i := range canonical { slices.SortFunc(canonical[i].Related, compareLocation) } - slices.SortFunc(canonical, compareDiagnostic) + type orderedDiagnostic struct { + diagnostic Diagnostic + pointer string + } + ordered := make([]orderedDiagnostic, len(canonical)) + for i, diagnostic := range canonical { + ordered[i] = orderedDiagnostic{diagnostic: diagnostic, pointer: diagnostic.Path.String()} + } + slices.SortFunc(ordered, func(left, right orderedDiagnostic) int { + return compareDiagnosticWithPaths(left.diagnostic, right.diagnostic, left.pointer, right.pointer) + }) + for i, item := range ordered { + canonical[i] = item.diagnostic + } return canonical } func compareDiagnostic(left, right Diagnostic) int { + return compareDiagnosticWithPaths(left, right, left.Path.String(), right.Path.String()) +} + +func compareDiagnosticWithPaths(left, right Diagnostic, leftPath, rightPath string) int { if order := compareOptionalLocation(left.Primary, right.Primary); order != 0 { return order } - if order := cmp.Compare(left.Path.String(), right.Path.String()); order != 0 { + if order := cmp.Compare(leftPath, rightPath); order != 0 { return order } if order := cmp.Compare(left.Code, right.Code); order != 0 { diff --git a/internal/engine/diagnostic_test.go b/internal/engine/diagnostic_test.go index 0200745..518071e 100644 --- a/internal/engine/diagnostic_test.go +++ b/internal/engine/diagnostic_test.go @@ -90,6 +90,26 @@ func TestDiagnosticsAreCanonicalAndDetached(t *testing.T) { } } +func TestCanonicalDiagnosticsSortEscapedPaths(t *testing.T) { + primary := &Location{SourceID: "source-1", Line: 1, Column: 1} + diagnostics := []Diagnostic{ + {Code: "same", Severity: SeverityError, Path: Keys("a/b"), Message: "same", Primary: primary}, + {Code: "same", Severity: SeverityError, Path: Keys("a~b"), Message: "same", Primary: primary}, + {Code: "z", Severity: SeverityError, Path: Keys("same"), Message: "same", Primary: primary}, + {Code: "a", Severity: SeverityError, Path: Keys("same"), Message: "same", Primary: primary}, + } + canonical := canonicalDiagnostics(diagnostics) + want := []string{"/a~0b", "/a~1b", "/same", "/same"} + for i, diagnostic := range canonical { + if got := diagnostic.Path.String(); got != want[i] { + t.Fatalf("diagnostic[%d] path = %q, want %q", i, got, want[i]) + } + } + if canonical[2].Code != "a" || canonical[3].Code != "z" { + t.Fatalf("same-path code order = %q, %q", canonical[2].Code, canonical[3].Code) + } +} + func TestWriteDiagnosticsEscapesControls(t *testing.T) { t.Parallel() diff --git a/internal/engine/document_compare.go b/internal/engine/document_compare.go index 68286d3..f6e90cf 100644 --- a/internal/engine/document_compare.go +++ b/internal/engine/document_compare.go @@ -24,12 +24,23 @@ type documentDelta struct { func compareConfigurationValues(before, after *document.Node) []documentDelta { var deltas []documentDelta compareConfigurationValue(before, after, Path{}, &deltas) - slices.SortFunc(deltas, func(left, right documentDelta) int { - if order := cmp.Compare(left.path.String(), right.path.String()); order != 0 { + type orderedDelta struct { + delta documentDelta + pointer string + } + ordered := make([]orderedDelta, len(deltas)) + for i, delta := range deltas { + ordered[i] = orderedDelta{delta: delta, pointer: delta.path.String()} + } + slices.SortFunc(ordered, func(left, right orderedDelta) int { + if order := cmp.Compare(left.pointer, right.pointer); order != 0 { return order } - return int(left.kind) - int(right.kind) + return int(left.delta.kind) - int(right.delta.kind) }) + for i, item := range ordered { + deltas[i] = item.delta + } return deltas } diff --git a/internal/engine/overlay.go b/internal/engine/overlay.go index 6689ed9..81e8d0e 100644 --- a/internal/engine/overlay.go +++ b/internal/engine/overlay.go @@ -131,9 +131,24 @@ func previousOrigins(origin Origin) []Origin { func displacedOrigins(node *document.Node, origins map[string]Origin, path Path) []displacedDefinition { var displaced []displacedDefinition collectDisplacedOrigins(&displaced, node, origins, path, Origin{}) - slices.SortFunc(displaced, func(left, right displacedDefinition) int { - return strings.Compare(left.path.String(), right.path.String()) + return sortDisplacedDefinitions(displaced) +} + +func sortDisplacedDefinitions(displaced []displacedDefinition) []displacedDefinition { + type orderedDefinition struct { + definition displacedDefinition + pointer string + } + ordered := make([]orderedDefinition, len(displaced)) + for i, definition := range displaced { + ordered[i] = orderedDefinition{definition: definition, pointer: definition.path.String()} + } + slices.SortFunc(ordered, func(left, right orderedDefinition) int { + return strings.Compare(left.pointer, right.pointer) }) + for i, item := range ordered { + displaced[i] = item.definition + } return displaced } diff --git a/internal/engine/overlay_test.go b/internal/engine/overlay_test.go index 6b052be..89ee714 100644 --- a/internal/engine/overlay_test.go +++ b/internal/engine/overlay_test.go @@ -117,3 +117,26 @@ func TestOwnedOverlayKeepsCompositeContributorsAndAcquiredInputs(t *testing.T) { t.Fatal("working tree mutation changed an acquired input") } } + +func TestDisplacedOriginsSortEscapedPointers(t *testing.T) { + input := parseCompositionDocument(t, "source-1", `values: + z: old + a/b: slash + a~b: tilde + nested: {x: old} +`) + composed, diagnostics := composeDocuments(input) + if len(diagnostics) != 0 { + t.Fatal(diagnostics) + } + displaced := displacedOrigins(composed.root, composed.origins, Path{}) + want := []string{"", "/values", "/values/a~0b", "/values/a~1b", "/values/nested", "/values/nested/x", "/values/z"} + if len(displaced) != len(want) { + t.Fatalf("displaced definitions = %d, want %d", len(displaced), len(want)) + } + for i, definition := range displaced { + if got := definition.path.String(); got != want[i] { + t.Fatalf("displaced[%d] path = %q, want %q", i, got, want[i]) + } + } +} diff --git a/internal/engine/override_apply.go b/internal/engine/override_apply.go index 64b5c19..3ff7f72 100644 --- a/internal/engine/override_apply.go +++ b/internal/engine/override_apply.go @@ -1,9 +1,6 @@ package engine import ( - "slices" - "strings" - "gitea.maximumdirect.net/eric/ConfigKit/internal/document" ) @@ -86,26 +83,28 @@ func applyPreparedOverride(result *composedDocument, override preparedOverride, } func affectedOverrideOrigins(existing, incoming *document.Node, origins map[string]Origin, path Path, index *mappingIndex) []displacedDefinition { - displaced := []displacedDefinition{{path: path.clone(), origin: cloneOrigin(origins[path.String()])}} - if existing.Kind == document.Mapping && incoming.Kind == document.Mapping { - for _, entry := range incoming.Mapping { - position := index.find(existing, entry.Key) - if position < 0 { - continue - } - childPath := path.Key(entry.Key) - childExisting := existing.Mapping[position].Value - if childExisting.Kind == document.Mapping && entry.Value.Kind == document.Mapping { - displaced = append(displaced, affectedOverrideOrigins(childExisting, entry.Value, origins, childPath, index)...) - } else { - displaced = append(displaced, displacedOrigins(childExisting, origins, childPath)...) - } - } - } else { - displaced = displacedOrigins(existing, origins, path) - } - slices.SortFunc(displaced, func(left, right displacedDefinition) int { - return strings.Compare(left.path.String(), right.path.String()) - }) - return displaced + var displaced []displacedDefinition + collectAffectedOverrideOrigins(&displaced, existing, incoming, origins, path, index) + return sortDisplacedDefinitions(displaced) +} + +func collectAffectedOverrideOrigins(displaced *[]displacedDefinition, existing, incoming *document.Node, origins map[string]Origin, path Path, index *mappingIndex) { + if existing.Kind != document.Mapping || incoming.Kind != document.Mapping { + collectDisplacedOrigins(displaced, existing, origins, path, Origin{}) + return + } + *displaced = append(*displaced, displacedDefinition{path: path.clone(), origin: cloneOrigin(origins[path.String()])}) + for _, entry := range incoming.Mapping { + position := index.find(existing, entry.Key) + if position < 0 { + continue + } + childPath := path.Key(entry.Key) + childExisting := existing.Mapping[position].Value + if childExisting.Kind == document.Mapping && entry.Value.Kind == document.Mapping { + collectAffectedOverrideOrigins(displaced, childExisting, entry.Value, origins, childPath, index) + } else { + collectDisplacedOrigins(displaced, childExisting, origins, childPath, Origin{}) + } + } } diff --git a/internal/engine/override_test.go b/internal/engine/override_test.go index 738732d..ea3b8f1 100644 --- a/internal/engine/override_test.go +++ b/internal/engine/override_test.go @@ -271,6 +271,43 @@ func TestOverrideEqualReplacementStillAppearsInHistory(t *testing.T) { } } +func TestAffectedOverrideOriginsSortEscapedPointers(t *testing.T) { + existingInput := parseCompositionDocument(t, "source-1", `values: + z: old + a/b: slash + a~b: tilde + nested: {x: old} +`) + existing, diagnostics := composeDocuments(existingInput) + if len(diagnostics) != 0 { + t.Fatal(diagnostics) + } + incoming := parseCompositionDocument(t, "source-2", `values: + z: new + a/b: new + a~b: new + nested: {x: new} +`) + existingValues, ok := documentAtPath(existing.root, Keys("values")) + if !ok { + t.Fatal("existing values mapping is missing") + } + incomingValues, ok := documentAtPath(incoming, Keys("values")) + if !ok { + t.Fatal("incoming values mapping is missing") + } + displaced := affectedOverrideOrigins(existingValues, incomingValues, existing.origins, Keys("values"), &mappingIndex{}) + want := []string{"/values", "/values/a~0b", "/values/a~1b", "/values/nested", "/values/nested/x", "/values/z"} + if len(displaced) != len(want) { + t.Fatalf("displaced definitions = %d, want %d", len(displaced), len(want)) + } + for i, definition := range displaced { + if got := definition.path.String(); got != want[i] { + t.Fatalf("displaced[%d] path = %q, want %q", i, got, want[i]) + } + } +} + func TestOverrideAggregateLimitsFailBeforeApplication(t *testing.T) { base, diagnostics := composeDocuments(&document.Node{Kind: document.Mapping}) if len(diagnostics) != 0 { diff --git a/internal/engine/projection_test.go b/internal/engine/projection_test.go index 3009aa5..162e85f 100644 --- a/internal/engine/projection_test.go +++ b/internal/engine/projection_test.go @@ -357,6 +357,21 @@ func TestCompareDocumentTreesTracksSemanticChanges(t *testing.T) { } } +func TestCompareConfigurationValuesSortEscapedPointers(t *testing.T) { + before := parseCompositionDocument(t, "before", `values: {z: old, 'a/b': old, 'a~b': old, nested: {x: old}}`) + after := parseCompositionDocument(t, "after", `values: {z: new, 'a/b': new, 'a~b': new, nested: {x: new}}`) + deltas := compareConfigurationValues(before, after) + want := []string{"/values/a~0b", "/values/a~1b", "/values/nested/x", "/values/z"} + if len(deltas) != len(want) { + t.Fatalf("deltas = %#v", deltas) + } + for i, delta := range deltas { + if got := delta.path.String(); got != want[i] { + t.Fatalf("delta[%d] path = %q, want %q", i, got, want[i]) + } + } +} + func mustSchemaFor[T any](t *testing.T) *schemaDescriptor { t.Helper() schema, issue := testBuildSchema(reflect.TypeFor[T]()) diff --git a/internal/engine/resolution.go b/internal/engine/resolution.go index 12cb1c9..c96de3d 100644 --- a/internal/engine/resolution.go +++ b/internal/engine/resolution.go @@ -313,14 +313,22 @@ func runResolution[T any](configuration *T, resolver func(*T, *Resolution) error } func orderedResolutionRecords(records map[string]*resolutionRecord) []*resolutionRecord { - ordered := make([]*resolutionRecord, 0, len(records)) - for _, record := range records { - ordered = append(ordered, record) + type orderedRecord struct { + record *resolutionRecord + pointer string } - slices.SortFunc(ordered, func(left, right *resolutionRecord) int { - return strings.Compare(left.path.String(), right.path.String()) + ordered := make([]orderedRecord, 0, len(records)) + for _, record := range records { + ordered = append(ordered, orderedRecord{record: record, pointer: record.path.String()}) + } + slices.SortFunc(ordered, func(left, right orderedRecord) int { + return strings.Compare(left.pointer, right.pointer) }) - return ordered + result := make([]*resolutionRecord, len(ordered)) + for i, item := range ordered { + result[i] = item.record + } + return result } func coveringResolutionRecord(records []*resolutionRecord, path Path) *resolutionRecord { diff --git a/internal/engine/resolution_test.go b/internal/engine/resolution_test.go index f752377..19bef35 100644 --- a/internal/engine/resolution_test.go +++ b/internal/engine/resolution_test.go @@ -206,6 +206,31 @@ func TestResolutionSequenceReplacementUpdatesDescendantOrigins(t *testing.T) { } } +func TestOrderedResolutionRecordsSortEscapedPointers(t *testing.T) { + parsedIndex, err := ParsePath("/items/0") + if err != nil { + t.Fatal(err) + } + records := map[string]*resolutionRecord{ + "tilde": {path: Keys("a~b")}, + "slash": {path: Keys("a/b")}, + "nested": {path: Keys("a", "b")}, + "index": {path: Keys("items").Index(0)}, + "parsed": {path: parsedIndex}, + "last": {path: Keys("z")}, + } + ordered := orderedResolutionRecords(records) + want := []string{"/a/b", "/a~0b", "/a~1b", "/items/0", "/items/0", "/z"} + if len(ordered) != len(want) { + t.Fatalf("ordered records = %d, want %d", len(ordered), len(want)) + } + for i, record := range ordered { + if got := record.path.String(); got != want[i] { + t.Fatalf("record[%d] path = %q, want %q", i, got, want[i]) + } + } +} + func TestResolutionRejectsUnrecordedAndMismatchedMutations(t *testing.T) { t.Parallel()