From deb9aa5c69dd616e6db305098803b0de7856cd75 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 25 Oct 2026 21:20:41 +0000 Subject: [PATCH] Roll back failed dynamic descriptor builds --- docs/roadmap/implementation.md | 3 + .../dynamic_descriptor_transaction_test.go | 95 +++++++++++++++++++ internal/engine/schema.go | 9 ++ internal/engine/value_operation.go | 2 + 4 files changed, 109 insertions(+) create mode 100644 internal/engine/dynamic_descriptor_transaction_test.go diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index a550bde..7bd91c3 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -112,6 +112,9 @@ Focused check: `go test ./internal/engine -run 'Schema|Decode|Resolution|Secret' Finding: **A02**. Owners: `internal/engine/schema.go`, `value_operation.go`, projection/resolution tests. +Status: **complete** (A02). Previous completed unit: `2736ddf`. Validation +passed: `GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test ./internal/engine -run 'Dynamic|Projection|Clone|Resolution' -count=1` and `GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test ./internal/engine -count=1`. + 1. Give `schemaBuilder` a journal of types newly inserted during a build. Record insertion only after a cache miss; cached, completed descriptors are borrowed and not mutated. Recursive new descriptors may refer to one another diff --git a/internal/engine/dynamic_descriptor_transaction_test.go b/internal/engine/dynamic_descriptor_transaction_test.go new file mode 100644 index 0000000..2c68377 --- /dev/null +++ b/internal/engine/dynamic_descriptor_transaction_test.go @@ -0,0 +1,95 @@ +package engine + +import ( + "errors" + "reflect" + "testing" +) + +type unsupportedDynamicRoot struct { + Child unsupportedDynamicChild `yaml:"child"` + Invalid chan int `yaml:"invalid"` +} + +type unsupportedDynamicChild struct { + Parent *unsupportedDynamicRoot `yaml:"parent"` +} + +type secretDynamicRoot struct { + Nested secretDynamicNested `yaml:"nested"` +} + +type secretDynamicNested struct { + Token Secret `yaml:"token"` +} + +type dynamicTransactionConfig struct { + First any `yaml:"first"` + Second any `yaml:"second"` + Good any `yaml:"good"` +} + +func TestDynamicDescriptorFailuresRollBackProvisionalTypes(t *testing.T) { + called := false + result, err := testLoad[dynamicTransactionConfig](NoSource(), Options[dynamicTransactionConfig]{Definition: Definition[dynamicTransactionConfig]{ + Resolve: func(config *dynamicTransactionConfig, resolution *Resolution) error { + called = true + Normalize(resolution, Keys("first"), &config.First, any((*unsupportedDynamicRoot)(nil)), "try typed nil") + Normalize(resolution, Keys("second"), &config.Second, any(&unsupportedDynamicRoot{}), "try non-nil") + Normalize(resolution, Keys("good"), &config.Good, any(map[string]string{"valid": "value"}), "continue with supported value") + return nil + }, + }}) + var loadErr *LoadError + if !called || result != nil || !errors.As(err, &loadErr) { + t.Fatalf("result = %#v, called=%v, error=%v", result, called, err) + } + paths := make(map[string]Code) + for _, diagnostic := range loadErr.Diagnostics() { + paths[diagnostic.Path.String()] = diagnostic.Code + } + for _, path := range []string{"/first", "/second"} { + if paths[path] != CodeResolveInvalidValue { + t.Errorf("diagnostic at %s = %q, want safe invalid-value failure", path, paths[path]) + } + } + + schema := mustSchemaFor[dynamicTransactionConfig](t) + operation := valueOperation{schema: schema} + prior, issue := operation.dynamicDescriptor(reflect.TypeFor[map[string]string](), Keys("prior")) + if issue != nil { + t.Fatalf("prior valid dynamic descriptor issue = %#v", issue) + } + if _, issue := operation.dynamicDescriptor(reflect.TypeFor[unsupportedDynamicRoot](), Keys("failure")); issue == nil || issue.code != CodeSchemaDynamicType { + t.Fatalf("unsupported dynamic descriptor issue = %#v", issue) + } + if schema.types[reflect.TypeFor[map[string]string]()] != prior { + t.Fatal("failed build removed a previously completed descriptor") + } + for _, current := range []reflect.Type{ + reflect.TypeFor[unsupportedDynamicRoot](), + reflect.TypeFor[unsupportedDynamicChild](), + reflect.TypeFor[*unsupportedDynamicRoot](), + } { + if _, exists := schema.types[current]; exists { + t.Errorf("failed build retained provisional descriptor for %v", current) + } + } + if _, issue := operation.dynamicDescriptor(reflect.TypeFor[unsupportedDynamicRoot](), Keys("again")); issue == nil || issue.path.String() != "/again" { + t.Fatalf("repeated failure issue = %#v", issue) + } + + secretSchema, schemaIssue := buildSchema(reflect.TypeFor[dynamicTransactionConfig](), testSecretBinding()) + if schemaIssue != nil { + t.Fatalf("secret schema issue = %#v", schemaIssue) + } + secretOperation := valueOperation{schema: secretSchema} + if _, issue := secretOperation.dynamicDescriptor(reflect.TypeFor[secretDynamicRoot](), Keys("secret")); issue == nil || issue.code != CodeSchemaDynamicType { + t.Fatalf("dynamic secret eligibility issue = %#v", issue) + } + for _, current := range []reflect.Type{reflect.TypeFor[secretDynamicRoot](), reflect.TypeFor[secretDynamicNested](), reflect.TypeFor[Secret]()} { + if _, exists := secretSchema.types[current]; exists { + t.Errorf("secret eligibility failure retained provisional descriptor for %v", current) + } + } +} diff --git a/internal/engine/schema.go b/internal/engine/schema.go index b00ef0a..84427a6 100644 --- a/internal/engine/schema.go +++ b/internal/engine/schema.go @@ -113,6 +113,7 @@ func buildSchema(root reflect.Type, secrets SecretBinding) (*schemaDescriptor, * type schemaBuilder struct { cache map[reflect.Type]*typeDescriptor building map[reflect.Type]bool + journal []reflect.Type secrets SecretBinding } @@ -122,6 +123,7 @@ func (b *schemaBuilder) describe(current reflect.Type, path Path) (*typeDescript } descriptor := &typeDescriptor{typeOf: current} b.cache[current] = descriptor + b.journal = append(b.journal, current) b.building[current] = true defer delete(b.building, current) @@ -210,6 +212,13 @@ func (b *schemaBuilder) describe(current reflect.Type, path Path) (*typeDescript return descriptor, nil } +func (b *schemaBuilder) rollback() { + for _, current := range b.journal { + delete(b.cache, current) + } + b.journal = nil +} + func (binding SecretBinding) validate() { if binding.Type == nil { return diff --git a/internal/engine/value_operation.go b/internal/engine/value_operation.go index 31872bd..acdd437 100644 --- a/internal/engine/value_operation.go +++ b/internal/engine/value_operation.go @@ -40,9 +40,11 @@ func (o *valueOperation) dynamicDescriptor(current reflect.Type, path Path) (*ty builder := schemaBuilder{cache: o.schema.types, building: make(map[reflect.Type]bool), secrets: o.schema.secrets} descriptor, issue := builder.describe(current, path) if issue != nil { + builder.rollback() return nil, &projectionIssue{code: CodeSchemaDynamicType, path: path.clone(), message: "dynamic configuration value has an unsupported type"} } if descriptorContainsSecret(descriptor, make(map[*typeDescriptor]bool)) { + builder.rollback() return nil, &projectionIssue{code: CodeSchemaDynamicType, path: path.clone(), message: "dynamic configuration values cannot establish secret-reference semantics"} } return descriptor, nil