Fix underlay provenance and resource path handling
This commit is contained in:
@@ -6,6 +6,28 @@ contracts in the [roadmap](sources-and-layering.md) and
|
|||||||
[implementation plan](implementation.md). No implementation fixes are included
|
[implementation plan](implementation.md). No implementation fixes are included
|
||||||
in this review.
|
in this review.
|
||||||
|
|
||||||
|
## Resolution status
|
||||||
|
|
||||||
|
All three findings were addressed on 2026-09-29:
|
||||||
|
|
||||||
|
- Composition now fills sequence-descendant origins separately from atomic
|
||||||
|
conflict claims, so initial underlay attribution reaches every surviving node.
|
||||||
|
- History reconciliation checks contributing origins and recognizes ancestor
|
||||||
|
replacement events without duplicating their descendant contributions.
|
||||||
|
- Native contexts use `fs.ValidPath`; host contexts use `filepath.Localize`
|
||||||
|
before the existing canonicalization and containment checks.
|
||||||
|
|
||||||
|
`facade_underlay_origins_test.go` covers imported sequence descendants in first
|
||||||
|
and later tiers, resolver/result/explanation agreement, and later mapping
|
||||||
|
contributors. `TestResourceContextsPreserveLiteralBackslashes` covers native
|
||||||
|
and host resource reads and directory listing. The new tests reproduced all
|
||||||
|
three failures before the fixes. The findings below retain the original review
|
||||||
|
evidence and acceptance requirements.
|
||||||
|
|
||||||
|
Post-fix validation passed: `./scripts/check.sh` and
|
||||||
|
`GOPROXY=off go test -race ./...`. Existing sequence replacement, provenance,
|
||||||
|
history, and resource-containment tests remain passing.
|
||||||
|
|
||||||
## 1. Preserve underlay attribution for sequence descendants (P2)
|
## 1. Preserve underlay attribution for sequence descendants (P2)
|
||||||
|
|
||||||
**Owners:** `internal/engine/load_request.go:174`,
|
**Owners:** `internal/engine/load_request.go:174`,
|
||||||
|
|||||||
107
facade_underlay_origins_test.go
Normal file
107
facade_underlay_origins_test.go
Normal file
@@ -0,0 +1,107 @@
|
|||||||
|
package configkit_test
|
||||||
|
|
||||||
|
import (
|
||||||
|
"reflect"
|
||||||
|
"testing"
|
||||||
|
"testing/fstest"
|
||||||
|
|
||||||
|
configkit "gitea.maximumdirect.net/eric/ConfigKit"
|
||||||
|
)
|
||||||
|
|
||||||
|
func TestUnderlaySequenceDescendantOrigins(t *testing.T) {
|
||||||
|
for _, later := range []bool{false, true} {
|
||||||
|
name := "first tier"
|
||||||
|
if later {
|
||||||
|
name = "later tier"
|
||||||
|
}
|
||||||
|
t.Run(name, func(t *testing.T) {
|
||||||
|
bundle := configkit.NewFilesystem("bundle", fstest.MapFS{
|
||||||
|
"main.yml": &fstest.MapFile{Data: []byte("configkit: {imports: [{path: items.yml}]}\n")},
|
||||||
|
"items.yml": &fstest.MapFile{Data: []byte("items: [{name: first, nested: [false]}]\n")},
|
||||||
|
})
|
||||||
|
tiers := []configkit.Underlay{}
|
||||||
|
if later {
|
||||||
|
tiers = append(tiers, configkit.Underlay{Inputs: []configkit.Input{{Source: configkit.Mapping("lower", map[string]any{"other": true})}}})
|
||||||
|
}
|
||||||
|
tiers = append(tiers, configkit.Underlay{Inputs: []configkit.Input{{Source: bundle.File("main.yml"), Imports: true}}})
|
||||||
|
paths := []string{"/items", "/items/0", "/items/0/name", "/items/0/nested", "/items/0/nested/0"}
|
||||||
|
entry := map[string]configkit.Origin{}
|
||||||
|
result, err := configkit.LoadLayered[map[string]any](configkit.Input{Source: configkit.Mapping("main", map[string]any{"main": true})}, configkit.LayeredOptions[map[string]any]{
|
||||||
|
Underlays: tiers,
|
||||||
|
Definition: configkit.Definition[map[string]any]{Resolve: func(_ *map[string]any, resolution *configkit.Resolution) error {
|
||||||
|
for _, pointer := range paths {
|
||||||
|
path, _ := configkit.ParsePath(pointer)
|
||||||
|
info, ok := resolution.InputInfo(path)
|
||||||
|
if !ok {
|
||||||
|
t.Errorf("missing resolver input %s", pointer)
|
||||||
|
}
|
||||||
|
entry[pointer] = info.Origin
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}},
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
var sourceID string
|
||||||
|
for _, source := range result.Sources() {
|
||||||
|
if source.Path == "items.yml" {
|
||||||
|
sourceID = source.ID
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if sourceID == "" {
|
||||||
|
t.Fatal("missing imported source")
|
||||||
|
}
|
||||||
|
for _, pointer := range paths {
|
||||||
|
path, _ := configkit.ParsePath(pointer)
|
||||||
|
info, ok := result.Metadata().Lookup(path)
|
||||||
|
if !ok || info.Origin.Layer != configkit.LayerUnderlay || len(info.Origin.Locations) != 1 || info.Origin.Locations[0].SourceID != sourceID {
|
||||||
|
t.Errorf("%s origin = %+v", pointer, info.Origin)
|
||||||
|
}
|
||||||
|
if !reflect.DeepEqual(entry[pointer], info.Origin) {
|
||||||
|
t.Errorf("%s resolver origin differs", pointer)
|
||||||
|
}
|
||||||
|
explanation, ok := result.Explain(path)
|
||||||
|
if !ok || !reflect.DeepEqual(explanation.Origin, info.Origin) {
|
||||||
|
t.Errorf("%s explanation origin differs", pointer)
|
||||||
|
}
|
||||||
|
history := result.Metadata().History(path)
|
||||||
|
if len(history) == 0 || !reflect.DeepEqual(history[len(history)-1].Origin, info.Origin) {
|
||||||
|
t.Errorf("%s history = %+v", pointer, history)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestUnderlayHistoryRetainsLaterMappingContributors(t *testing.T) {
|
||||||
|
result, err := configkit.LoadLayered[map[string]any](configkit.Input{Source: configkit.NoSource()}, configkit.LayeredOptions[map[string]any]{Underlays: []configkit.Underlay{
|
||||||
|
{Inputs: []configkit.Input{{Source: configkit.Mapping("one", map[string]any{"x": 1})}}},
|
||||||
|
{Inputs: []configkit.Input{{Source: configkit.Mapping("two", map[string]any{"x": map[string]any{}})}}},
|
||||||
|
{Inputs: []configkit.Input{{Source: configkit.Mapping("three", map[string]any{"x": map[string]any{"y": 2}})}}},
|
||||||
|
}})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
path := configkit.Keys("x")
|
||||||
|
info, _ := result.Metadata().Lookup(path)
|
||||||
|
wantOrigin := configkit.Origin{Layer: configkit.LayerUnderlay, Locations: []configkit.Location{{SourceID: "source-2"}, {SourceID: "source-3"}}}
|
||||||
|
if !reflect.DeepEqual(info.Origin, wantOrigin) {
|
||||||
|
t.Fatalf("effective origin = %+v, want %+v", info.Origin, wantOrigin)
|
||||||
|
}
|
||||||
|
history := result.Metadata().History(path)
|
||||||
|
if len(history) != 3 || !reflect.DeepEqual(history[len(history)-1].Origin, info.Origin) {
|
||||||
|
t.Fatalf("history = %+v, effective origin = %+v", history, info.Origin)
|
||||||
|
}
|
||||||
|
explanation, ok := result.Explain(path)
|
||||||
|
if !ok || !reflect.DeepEqual(explanation.Origin, info.Origin) {
|
||||||
|
t.Fatal("explanation and metadata disagree")
|
||||||
|
}
|
||||||
|
if len(explanation.History) != 3 {
|
||||||
|
t.Fatalf("explanation history = %+v", explanation.History)
|
||||||
|
}
|
||||||
|
last := explanation.History[len(explanation.History)-1].Origin
|
||||||
|
if last.Layer != configkit.LayerUnderlay || len(last.Locations) != 1 || last.Locations[0].SourceID != "source-3" {
|
||||||
|
t.Fatalf("last explanation origin = %+v", last)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -92,14 +92,35 @@ func composeDocuments(inputs ...*document.Node) (*composedDocument, []Diagnostic
|
|||||||
for _, input := range inputs {
|
for _, input := range inputs {
|
||||||
applications = append(applications, historyApplication{layer: LayerBase, value: input})
|
applications = append(applications, historyApplication{layer: LayerBase, value: input})
|
||||||
}
|
}
|
||||||
|
origins := compositionOrigins(collected.ordered)
|
||||||
|
completeCompositionOrigins(root, Path{}, origins)
|
||||||
return &composedDocument{
|
return &composedDocument{
|
||||||
root: root,
|
root: root,
|
||||||
claims: cloneCompositionClaims(collected.ordered),
|
claims: cloneCompositionClaims(collected.ordered),
|
||||||
origins: compositionOrigins(collected.ordered),
|
origins: origins,
|
||||||
applications: applications,
|
applications: applications,
|
||||||
}, nil
|
}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Conflict claims treat sequences atomically, but inspection needs origins for
|
||||||
|
// every descendant. Preserve composite mapping claims while filling those gaps.
|
||||||
|
func completeCompositionOrigins(node *document.Node, path Path, origins map[string]Origin) {
|
||||||
|
pointer := path.String()
|
||||||
|
if _, present := origins[pointer]; !present {
|
||||||
|
origins[pointer] = Origin{Layer: LayerBase, Locations: []Location{publicLocation(node.Location)}}
|
||||||
|
}
|
||||||
|
switch node.Kind {
|
||||||
|
case document.Mapping:
|
||||||
|
for _, entry := range node.Mapping {
|
||||||
|
completeCompositionOrigins(entry.Value, path.Key(entry.Key), origins)
|
||||||
|
}
|
||||||
|
case document.Sequence:
|
||||||
|
for index, child := range node.Sequence {
|
||||||
|
completeCompositionOrigins(child, path.Index(index), origins)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func (c *compositionClaims) append(node *document.Node, path Path) {
|
func (c *compositionClaims) append(node *document.Node, path Path) {
|
||||||
claim := compositionClaim{
|
claim := compositionClaim{
|
||||||
path: path.clone(),
|
path: path.clone(),
|
||||||
|
|||||||
@@ -125,9 +125,6 @@ func applyOverlayMetadata(metadata *Metadata, records []overlayRecord) {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
if history := events[pointer]; len(history) != 0 && history[len(history)-1].Origin.Layer == info.Origin.Layer && history[len(history)-1].Origin.Operation == info.Origin.Operation {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
appendInputOriginEvent(events, baseline, pointer, path, info.Origin)
|
appendInputOriginEvent(events, baseline, pointer, path, info.Origin)
|
||||||
}
|
}
|
||||||
metadata.history = make(map[string][]Change, len(baseline)+len(events))
|
metadata.history = make(map[string][]Change, len(baseline)+len(events))
|
||||||
@@ -144,12 +141,37 @@ func appendInputOriginEvent(events map[string][]Change, baseline map[string][]Ch
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
history := events[pointer]
|
history := events[pointer]
|
||||||
if len(history) != 0 && originContainedBy(history[len(history)-1].Origin, origin) {
|
if len(history) != 0 && originRepresentedByChange(path, origin, history[len(history)-1]) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
appendInputChangeEvent(events, baseline, pointer, Change{Path: path.clone(), Origin: cloneOrigin(origin)})
|
appendInputChangeEvent(events, baseline, pointer, Change{Path: path.clone(), Origin: cloneOrigin(origin)})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func originRepresentedByChange(path Path, origin Origin, change Change) bool {
|
||||||
|
if change.Path.String() == path.String() {
|
||||||
|
return originContainedBy(origin, change.Origin)
|
||||||
|
}
|
||||||
|
if !pathContains(change.Path, path) || origin.Layer != change.Origin.Layer || origin.Operation != change.Origin.Operation {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
// A subtree replacement already records its descendants' contributions.
|
||||||
|
// Their coordinates differ from the container's, but their sources must be
|
||||||
|
// covered by the operation; matching only the precedence role is insufficient.
|
||||||
|
for _, location := range origin.Locations {
|
||||||
|
found := false
|
||||||
|
for _, recorded := range change.Origin.Locations {
|
||||||
|
if location.SourceID == recorded.SourceID {
|
||||||
|
found = true
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if !found {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
func appendInputChangeEvent(events map[string][]Change, baseline map[string][]Change, pointer string, value Change) {
|
func appendInputChangeEvent(events map[string][]Change, baseline map[string][]Change, pointer string, value Change) {
|
||||||
change := cloneChange(value)
|
change := cloneChange(value)
|
||||||
history := events[pointer]
|
history := events[pointer]
|
||||||
|
|||||||
@@ -36,7 +36,7 @@ type rootedNativeFS struct {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (r rootedNativeFS) name(name string) (string, error) {
|
func (r rootedNativeFS) name(name string) (string, error) {
|
||||||
if !fs.ValidPath(name) || strings.Contains(name, "\\") {
|
if !fs.ValidPath(name) {
|
||||||
return "", &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid}
|
return "", &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid}
|
||||||
}
|
}
|
||||||
joined := path.Join(r.root, name)
|
joined := path.Join(r.root, name)
|
||||||
@@ -75,7 +75,8 @@ type rootedHostFS struct {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (r rootedHostFS) Open(name string) (fs.File, error) {
|
func (r rootedHostFS) Open(name string) (fs.File, error) {
|
||||||
if !fs.ValidPath(name) || strings.Contains(name, "\\") {
|
localName, err := filepath.Localize(name)
|
||||||
|
if err != nil {
|
||||||
return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid}
|
return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid}
|
||||||
}
|
}
|
||||||
opener := r.openRoot
|
opener := r.openRoot
|
||||||
@@ -86,7 +87,7 @@ func (r rootedHostFS) Open(name string) (fs.File, error) {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, &fs.PathError{Op: "open", Path: name, Err: err}
|
return nil, &fs.PathError{Op: "open", Path: name, Err: err}
|
||||||
}
|
}
|
||||||
canonical, err := canonicalExistingPath(filepath.Join(r.root, filepath.FromSlash(name)))
|
canonical, err := canonicalExistingPath(filepath.Join(r.root, localName))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
_ = root.Close()
|
_ = root.Close()
|
||||||
return nil, &fs.PathError{Op: "open", Path: name, Err: err}
|
return nil, &fs.PathError{Op: "open", Path: name, Err: err}
|
||||||
|
|||||||
@@ -5,10 +5,46 @@ import (
|
|||||||
"io/fs"
|
"io/fs"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"runtime"
|
||||||
"testing"
|
"testing"
|
||||||
"testing/fstest"
|
"testing/fstest"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
func TestResourceContextsPreserveLiteralBackslashes(t *testing.T) {
|
||||||
|
const directory = `assets\literal`
|
||||||
|
const name = directory + `/text\file`
|
||||||
|
check := func(t *testing.T, filesystem fs.FS) {
|
||||||
|
t.Helper()
|
||||||
|
data, err := fs.ReadFile(filesystem, name)
|
||||||
|
if err != nil || string(data) != "content" {
|
||||||
|
t.Fatalf("read = %q, %v", data, err)
|
||||||
|
}
|
||||||
|
entries, err := fs.ReadDir(filesystem, directory)
|
||||||
|
if err != nil || len(entries) != 1 || entries[0].Name() != `text\file` {
|
||||||
|
t.Fatalf("entries = %v, %v", entries, err)
|
||||||
|
}
|
||||||
|
if _, err := filesystem.Open("../outside"); !errors.Is(err, fs.ErrInvalid) {
|
||||||
|
t.Fatalf("escape = %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
t.Run("native", func(t *testing.T) {
|
||||||
|
check(t, rootedNativeFS{filesystem: fstest.MapFS{"bundle/" + name: &fstest.MapFile{Data: []byte("content")}}, root: "bundle"})
|
||||||
|
})
|
||||||
|
t.Run("host", func(t *testing.T) {
|
||||||
|
if runtime.GOOS == "windows" {
|
||||||
|
t.Skip("backslash is a host separator on Windows; native case remains covered")
|
||||||
|
}
|
||||||
|
root := t.TempDir()
|
||||||
|
if err := os.Mkdir(filepath.Join(root, directory), 0o700); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if err := os.WriteFile(filepath.Join(root, filepath.FromSlash(name)), []byte("content"), 0o600); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
check(t, rootedHostFS{root: root})
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
type trackedHostRoot struct {
|
type trackedHostRoot struct {
|
||||||
root *os.Root
|
root *os.Root
|
||||||
closed *int
|
closed *int
|
||||||
|
|||||||
Reference in New Issue
Block a user