diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 4f81dab..a550bde 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -78,6 +78,9 @@ race, consumer, and documentation validation is specified in Stage 33. Finding: **A01**; also the `schemaPath` portion of **F06**. Owners: `internal/engine/schema.go`, `decode.go`, `secret.go`, and their tests. +Status: **complete** (A01; schema-path portion of F06). Validation passed: +`GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test ./internal/engine -run 'Schema|Decode|Resolution|Secret|PointerOnly' -count=1`, the specified focused check, and `GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test ./internal/engine -count=1`. + 1. Introduce one private pointer-descriptor terminal lookup that detects a repeated descriptor, using a traversal-local visited set. Return failure for a pointer-only loop; do not reject that type during schema construction. diff --git a/internal/engine/decode.go b/internal/engine/decode.go index 43f1f48..63ea900 100644 --- a/internal/engine/decode.go +++ b/internal/engine/decode.go @@ -91,19 +91,30 @@ func decodeValue(node *document.Node, base reflect.Value, descriptor *typeDescri } } if descriptor.kind == descriptorPointer { - var elementBase reflect.Value - if base.IsNil() { - elementBase = reflect.Zero(descriptor.typeOf.Elem()) - } else { - elementBase = base.Elem() + terminal, ok := terminalDescriptor(descriptor) + if !ok { + return reflect.Value{}, []decodeIssue{newDecodeIssue(CodeDecodeType, path, node, "configuration type cannot represent a non-null value")} } - decoded, issues := decodeValue(node, elementBase, descriptor.element, path, schema, context) + var pointers []*typeDescriptor + currentBase := base + for current := descriptor; current != terminal; current = current.element { + pointers = append(pointers, current) + if currentBase.IsNil() { + currentBase = reflect.Zero(current.typeOf.Elem()) + } else { + currentBase = currentBase.Elem() + } + } + decoded, issues := decodeValue(node, currentBase, terminal, path, schema, context) if len(issues) != 0 { return reflect.Value{}, issues } - result := reflect.New(descriptor.typeOf.Elem()) - result.Elem().Set(decoded) - return result, nil + for i := len(pointers) - 1; i >= 0; i-- { + result := reflect.New(pointers[i].typeOf.Elem()) + result.Elem().Set(decoded) + decoded = result + } + return decoded, nil } switch descriptor.kind { @@ -392,10 +403,11 @@ func decodeDynamic(node *document.Node, base reflect.Value, path Path, schema *s } func acceptsMapping(descriptor *typeDescriptor) bool { - for descriptor != nil && descriptor.kind == descriptorPointer { - descriptor = descriptor.element + terminal, ok := terminalDescriptor(descriptor) + if !ok { + return false } - return descriptor != nil && (descriptor.kind == descriptorStruct || descriptor.kind == descriptorMap || descriptor.kind == descriptorDynamic) + return terminal.kind == descriptorStruct || terminal.kind == descriptorMap || terminal.kind == descriptorDynamic } func fieldForDecode(root reflect.Value, index []int, allocate bool) (reflect.Value, bool) { diff --git a/internal/engine/pointer_descriptor_test.go b/internal/engine/pointer_descriptor_test.go new file mode 100644 index 0000000..e0fa035 --- /dev/null +++ b/internal/engine/pointer_descriptor_test.go @@ -0,0 +1,112 @@ +package engine + +import ( + "context" + "errors" + "os" + "os/exec" + "reflect" + "testing" + "time" +) + +type pointerOnlyLoop *pointerOnlyLoop + +type pointerOnlyConfig struct { + Loop pointerOnlyLoop `yaml:"loop"` +} + +func TestPointerOnlyDescriptorsTerminate(t *testing.T) { + if mode := os.Getenv("CONFIGKIT_POINTER_DESCRIPTOR_TEST_MODE"); mode != "" { + switch mode { + case "decode": + _, err := testLoad[pointerOnlyConfig](Bytes("config.yml", []byte("loop: {}\n"))) + var loadErr *LoadError + if !errors.As(err, &loadErr) { + t.Fatalf("load error = %v, want a safe decode failure", err) + } + for _, diagnostic := range loadErr.Diagnostics() { + if diagnostic.Code == CodeDecodeType && diagnostic.Path.String() == "/loop" { + return + } + } + t.Fatalf("diagnostics = %#v, want decode type at /loop", loadErr.Diagnostics()) + case "paths": + schema, issue := testBuildSchema(reflect.TypeFor[pointerOnlyConfig]()) + if issue != nil { + t.Fatalf("schema issue = %#v", issue) + } + if _, ok := resolveSchemaPath(schema, Keys("loop", "child")); ok { + t.Fatal("descendant below a pointer-only loop was accepted") + } + if _, ok := schemaSecretRoot(schema, Keys("loop", "child")); ok { + t.Fatal("secret path below a pointer-only loop was accepted") + } + if acceptsMapping(schema.root.fields[schema.root.fieldByName["loop"]].descriptor) { + t.Fatal("pointer-only loop was accepted as a mapping") + } + default: + t.Fatalf("unknown test mode %q", mode) + } + return + } + + for _, mode := range []string{"decode", "paths"} { + t.Run(mode, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + command := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestPointerOnlyDescriptorsTerminate$") + command.Env = append(os.Environ(), "CONFIGKIT_POINTER_DESCRIPTOR_TEST_MODE="+mode) + output, err := command.CombinedOutput() + if ctx.Err() != nil { + t.Fatalf("pointer descriptor %s did not terminate: %s", mode, output) + } + if err != nil { + t.Fatalf("pointer descriptor %s failed: %v, %s", mode, err, output) + } + }) + } +} + +func TestPointerOnlyNullableFieldAndRecursiveValues(t *testing.T) { + for name, source := range map[string]Source{ + "omitted": NoSource(), + "null": Bytes("config.yml", []byte("loop: null\n")), + } { + t.Run(name, func(t *testing.T) { + result, err := testLoad[pointerOnlyConfig](source) + if err != nil || result.Config.Loop != nil { + t.Fatalf("result = %#v, error = %v", result, err) + } + }) + } + + result, err := testLoad[recursiveConfig](Bytes("config.yml", []byte("name: root\nnext:\n name: child\n next:\n name: leaf\n"))) + if err != nil { + t.Fatal(err) + } + if result.Config.Name != "root" || result.Config.Next == nil || result.Config.Next.Name != "child" || result.Config.Next.Next == nil || result.Config.Next.Next.Name != "leaf" { + t.Fatalf("recursive value = %#v", result.Config) + } + + schema, issue := testBuildSchema(reflect.TypeFor[pointerOnlyConfig]()) + if issue != nil { + t.Fatal(issue.message) + } + resolved, ok := resolveSchemaPath(schema, Keys("loop")) + if !ok || resolved.descriptor.kind != descriptorPointer { + t.Fatalf("nullable pointer field resolution = %#v, %v", resolved, ok) + } + + called := false + nullableResult, err := testLoad[pointerOnlyConfig](NoSource(), Options[pointerOnlyConfig]{Definition: Definition[pointerOnlyConfig]{ + Resolve: func(config *pointerOnlyConfig, resolution *Resolution) error { + called = true + Normalize(resolution, Keys("loop"), &config.Loop, nil, "clear nullable field") + return nil + }, + }}) + if err != nil || !called || nullableResult.Config.Loop != nil { + t.Fatalf("nullable field operation = %#v, called=%v, error=%v", nullableResult, called, err) + } +} diff --git a/internal/engine/schema.go b/internal/engine/schema.go index acffcd5..b00ef0a 100644 --- a/internal/engine/schema.go +++ b/internal/engine/schema.go @@ -334,8 +334,21 @@ func prependIndex(index int, nested []int) []int { type schemaPath struct { descriptor *typeDescriptor - field *fieldDescriptor - dynamic bool +} + +// terminalDescriptor follows a pointer-only descriptor chain without entering +// any value. Recursive pointer aliases can refer back to an earlier descriptor, +// so callers must handle a repeated descriptor as an unaddressable chain. +func terminalDescriptor(descriptor *typeDescriptor) (*typeDescriptor, bool) { + visited := make(map[*typeDescriptor]struct{}) + for descriptor != nil && descriptor.kind == descriptorPointer { + if _, exists := visited[descriptor]; exists { + return nil, false + } + visited[descriptor] = struct{}{} + descriptor = descriptor.element + } + return descriptor, descriptor != nil } func resolveSchemaPath(schema *schemaDescriptor, path Path) (schemaPath, bool) { @@ -343,10 +356,11 @@ func resolveSchemaPath(schema *schemaDescriptor, path Path) (schemaPath, bool) { return schemaPath{}, false } current := schema.root - var field *fieldDescriptor for _, segment := range path.segments { - for current.kind == descriptorPointer { - current = current.element + var ok bool + current, ok = terminalDescriptor(current) + if !ok { + return schemaPath{}, false } switch current.kind { case descriptorStruct: @@ -355,12 +369,10 @@ func resolveSchemaPath(schema *schemaDescriptor, path Path) (schemaPath, bool) { } position, ok := current.fieldByName[segment.key] if ok { - field = ¤t.fields[position] - current = field.descriptor + current = current.fields[position].descriptor continue } if current.inlineMap != nil { - field = current.inlineMap current = current.inlineMap.descriptor.element continue } @@ -369,26 +381,20 @@ func resolveSchemaPath(schema *schemaDescriptor, path Path) (schemaPath, bool) { if segment.kind == pathSegmentIndex { return schemaPath{}, false } - field = nil current = current.element case descriptorSlice, descriptorArray: index, ok := schemaIndex(segment) if !ok || (current.kind == descriptorArray && index >= current.length) { return schemaPath{}, false } - field = nil current = current.element case descriptorDynamic: - return schemaPath{descriptor: current, field: field, dynamic: true}, true + return schemaPath{descriptor: current}, true default: return schemaPath{}, false } } - dynamic := current - for dynamic.kind == descriptorPointer { - dynamic = dynamic.element - } - return schemaPath{descriptor: current, field: field, dynamic: dynamic.kind == descriptorDynamic}, true + return schemaPath{descriptor: current}, true } func schemaIndex(segment pathSegment) (int, bool) { diff --git a/internal/engine/secret.go b/internal/engine/secret.go index d7aafc7..c38735b 100644 --- a/internal/engine/secret.go +++ b/internal/engine/secret.go @@ -168,8 +168,10 @@ func schemaSecretRoot(schema *schemaDescriptor, path Path) (Path, bool) { walked := Path{} for _, segment := range path.segments { walked.segments = append(walked.segments, segment) - for current.kind == descriptorPointer { - current = current.element + var ok bool + current, ok = terminalDescriptor(current) + if !ok { + return Path{}, false } switch current.kind { case descriptorStruct: