From 9448d209615507679333d12484fae88331cfff3c Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Mon, 15 Jun 2026 15:29:20 -0400 Subject: [PATCH 1/7] refactor(validate): consume cli's structured validation API crossplane/cli#66 lands a structured-result validate API at pkg/validate.SchemaValidate, replacing the previous cmd/crossplane/validate.SchemaValidation entry point that produced its results by writing prefix-marked lines to a writer. Switch schema_validator.go to the new API so that: - Validation errors come from typed FieldValidationError records rather than parsed [x]/[!] lines, removing the extractValidationErrors stdout-parsing helper and its test. - The bytes.Buffer + io.MultiWriter + loggerwriter indirection used to capture rendered output is gone; per-resource debug logging is now explicit via logResultDetails. - Failure detection runs through pkg/validate.ResultError, which preserves the historical error-on-missing-schemas semantic. The previous SchemaValidation fused defaulting with validation, mutating the caller's resources in place. SchemaValidate deep-copies its inputs, so defaulting no longer happens as a side-effect. Apply defaults explicitly via pkg/xr.ApplyCRDDefaults before validation to preserve the downstream invariant that the diff calculator sees fully-defaulted resources (covered by TestDefaultSchemaValidator_ValidateResources_AppliesDefaults). Also rename the call site in diff_processor.go from render.DefaultValues to xr.ApplyCRDDefaults: the helper kept its signature but was promoted out of cmd/crossplane/render into the new pkg/xr library package in the same upstream cleanup wave. go.mod pins crossplane/cli to v2.4.0-rc.0.0.20260615182009-ba59fbfac34b (the merge commit of crossplane/cli#66) and pulls crossplane-runtime/v2 and crossplane/apis/v2 forward to v2.4.0-rc.0 to satisfy the cli's transitive constraints. Bump to a tagged release once one is cut. Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/diff_processor.go | 8 +- cmd/diff/diffprocessor/schema_validator.go | 159 +++++++++++++----- .../diffprocessor/schema_validator_test.go | 131 ++++++++++++--- go.mod | 8 +- go.sum | 12 +- 5 files changed, 236 insertions(+), 82 deletions(-) diff --git a/cmd/diff/diffprocessor/diff_processor.go b/cmd/diff/diffprocessor/diff_processor.go index b61824de..51468715 100644 --- a/cmd/diff/diffprocessor/diff_processor.go +++ b/cmd/diff/diffprocessor/diff_processor.go @@ -18,6 +18,7 @@ import ( dt "github.com/crossplane-contrib/crossplane-diff/cmd/diff/renderer/types" "github.com/crossplane-contrib/crossplane-diff/cmd/diff/types" "github.com/crossplane/cli/v2/cmd/crossplane/render" + clixr "github.com/crossplane/cli/v2/pkg/xr" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -1557,7 +1558,10 @@ func (p *DefaultDiffProcessor) applyXRDDefaults(ctx context.Context, xr *cmp.Uns return errors.Wrapf(err, "cannot find CRD for XRD %s (resource %s)", xrdName, resourceID) } - // Apply defaults using the render.DefaultValues function + // Apply defaults using the cli's pkg/xr defaulting helper. The + // previous home, render.DefaultValues, was renamed to + // xr.ApplyCRDDefaults and moved out of cmd/crossplane/render in + // crossplane/cli when that package was promoted to a library. apiVersion := xr.GetAPIVersion() xrContent := xr.UnstructuredContent() @@ -1566,7 +1570,7 @@ func (p *DefaultDiffProcessor) applyXRDDefaults(ctx context.Context, xr *cmp.Uns "apiVersion", apiVersion, "crdName", crdForDefaults.Name) - err = render.DefaultValues(xrContent, apiVersion, *crdForDefaults) + err = clixr.ApplyCRDDefaults(xrContent, apiVersion, *crdForDefaults) if err != nil { return errors.Wrapf(err, "cannot apply default values for XR %s", resourceID) } diff --git a/cmd/diff/diffprocessor/schema_validator.go b/cmd/diff/diffprocessor/schema_validator.go index c2e65334..3e5b6474 100644 --- a/cmd/diff/diffprocessor/schema_validator.go +++ b/cmd/diff/diffprocessor/schema_validator.go @@ -1,16 +1,14 @@ package diffprocessor import ( - "bytes" "context" "fmt" - "io" "strings" xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" k8 "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/kubernetes" - "github.com/crossplane/cli/v2/cmd/crossplane/common/loggerwriter" - "github.com/crossplane/cli/v2/cmd/crossplane/validate" + pkgvalidate "github.com/crossplane/cli/v2/pkg/validate" + clixr "github.com/crossplane/cli/v2/pkg/xr" extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" @@ -108,32 +106,32 @@ func (v *DefaultSchemaValidator) ValidateResources(ctx context.Context, xr *un.U return errors.Wrap(err, "unable to ensure CRDs") } - // Create a buffer to capture validation output for error messages, - // and a MultiWriter to also send output to debug logs - var validationOutput bytes.Buffer - - multiWriter := io.MultiWriter(&validationOutput, loggerwriter.NewLoggerWriter(v.logger)) - - // SchemaValidation applies defaults IN-PLACE to the resources it sees, - // mutating the underlying Object map. For each managed resource we wrap - // `&un.Unstructured{Object: composed[i].UnstructuredContent()}` — the - // embedded unstructured.Unstructured returns its Object map by reference, - // so defaults applied here propagate back to the caller's - // `composed[i]` via the shared map. That preserves pre-existing - // defaulting behaviour for downstream diff calculation. - // - // The XR is passed by pointer; defaults applied to it land on the - // caller's object, which is then used downstream for diffing against - // cluster state. The real composite reconciler in the render pipeline - // already applied XRD schema defaults before we got here, so this is - // belt-and-braces. + // Apply CRD-level defaults in place before handing resources off to + // the diff calculator. The structured SchemaValidate API + // intentionally deep-copies inputs and does not surface defaults + // to its caller, so without this step composed resources would + // reach diff calculation undefaulted and produce spurious diffs + // for fields the cluster's defaulter would have populated. The + // previous SchemaValidation entry point fused this defaulting + // into validation; doing it explicitly here makes the + // dependency obvious. + if err := v.applyCRDDefaults(ctx, resources); err != nil { + return err + } + + // SchemaValidate is the structured-result API: it returns a + // *ValidationResult that callers inspect directly. v.logger.Debug("Performing schema validation", "resourceCount", len(resources)) - err = validate.SchemaValidation(ctx, resources, v.schemaClient.GetAllCRDs(), true, true, multiWriter) + result, err := pkgvalidate.SchemaValidate(ctx, resources, v.schemaClient.GetAllCRDs()) if err != nil { - // Parse and extract only the error lines from validation output - details := extractValidationErrors(validationOutput.String()) - return NewSchemaValidationError("", details, err) + return errors.Wrap(err, "schema validation failed") + } + + v.logResultDetails(result) + + if rerr := pkgvalidate.ResultError(result, true); rerr != nil { + return NewSchemaValidationError("", formatValidationErrors(result), rerr) } // Additionally validate resource scope constraints (namespace requirements and cross-namespace refs) @@ -259,25 +257,102 @@ func (v *DefaultSchemaValidator) ValidateScopeConstraints(ctx context.Context, r return nil } -// extractValidationErrors parses validation output and returns clean error messages. -// It extracts lines starting with [x] (validation errors) and [!] (warnings/missing schemas), -// stripping the prefixes for cleaner display. -func extractValidationErrors(output string) string { - var validationErrs []string - - for line := range strings.SplitSeq(output, "\n") { - line = strings.TrimSpace(line) - // Use CutPrefix to check for prefix and strip it in one operation - if cleaned, found := strings.CutPrefix(line, "[x]"); found { - validationErrs = append(validationErrs, strings.TrimSpace(cleaned)) - } else if cleaned, found := strings.CutPrefix(line, "[!]"); found { - validationErrs = append(validationErrs, strings.TrimSpace(cleaned)) +// applyCRDDefaults applies CRD-derived defaults to each resource in +// place. Built-in types and resources whose CRD is unknown are +// skipped; in both cases the diff calculator already handles them +// without server-side defaulting. We treat a CRD lookup error here +// as a no-op rather than a failure: EnsureComposedResourceCRDs is +// the canonical gate on missing CRDs, and the subsequent SchemaValidate +// call surfaces the same condition through ValidationStatusMissingSchema. +func (v *DefaultSchemaValidator) applyCRDDefaults(ctx context.Context, resources []*un.Unstructured) error { + for _, r := range resources { + gvk := r.GroupVersionKind() + if !v.schemaClient.IsCRDRequired(ctx, gvk) { + continue + } + + crd, err := v.schemaClient.GetCRD(ctx, gvk) + if err != nil { + v.logger.Debug("skipping defaulting; CRD not found", "gvk", gvk.String(), "error", err) + continue + } + + if err := clixr.ApplyCRDDefaults(r.Object, r.GetAPIVersion(), *crd); err != nil { + return errors.Wrapf(err, "cannot apply CRD defaults for %s/%s", gvk.String(), r.GetName()) + } + } + + return nil +} + +// logResultDetails emits per-resource debug logging for a validation +// result. This replaces the incidental logging that the previous +// stdout-capturing implementation produced via a loggerwriter, and +// surfaces operational signals (defaulting failures, missing schemas) +// even when ResultError reports success overall. +func (v *DefaultSchemaValidator) logResultDetails(result *pkgvalidate.ValidationResult) { + for _, r := range result.Resources { + v.logger.Debug("validation result", + "apiVersion", r.APIVersion, + "kind", r.Kind, + "name", r.Name, + "status", string(r.Status), + "errors", len(r.Errors), + ) + + for _, e := range r.Errors { + v.logger.Debug("validation error", + "apiVersion", r.APIVersion, + "kind", r.Kind, + "name", r.Name, + "type", string(e.Type), + "field", e.Field, + "message", e.Message, + ) + } + } +} + +// formatValidationErrors produces a single-line, semicolon-joined string +// describing the failures in a *ValidationResult. It mirrors the +// previously extracted text-renderer output (one entry per +// FieldValidationError, with a "could not find CRD/XRD for ..." entry +// per missing-schema resource) so existing callers and tests that +// match on these substrings keep working. +func formatValidationErrors(result *pkgvalidate.ValidationResult) string { + var msgs []string + + for _, r := range result.Resources { + gvk := fmt.Sprintf("%s, Kind=%s", r.APIVersion, r.Kind) + + switch r.Status { + case pkgvalidate.ValidationStatusMissingSchema: + msgs = append(msgs, "could not find CRD/XRD for: "+gvk) + case pkgvalidate.ValidationStatusInvalid: + for _, e := range r.Errors { + if e.Type == pkgvalidate.FieldErrorTypeDefaulting { + // ResultError treats a defaulting error as a + // failure only when accompanied by a schema-class + // error on the same resource. The schema-class + // error already gets its own entry; emitting the + // defaulting line too would just be noise. + continue + } + + msgs = append(msgs, fmt.Sprintf("schema validation error %s, %s : %s", gvk, r.Name, e.Message)) + } + case pkgvalidate.ValidationStatusValid, + pkgvalidate.ValidationStatusDefaultingFailed: + // Valid: nothing to report. DefaultingFailed-only resources + // are reported by ResultError as success, so this function + // never sees them via the failure path; the case is here + // only to keep the switch exhaustive. } } - if len(validationErrs) == 0 { + if len(msgs) == 0 { return "schema validation failed" } - return strings.Join(validationErrs, "; ") + return strings.Join(msgs, "; ") } diff --git a/cmd/diff/diffprocessor/schema_validator_test.go b/cmd/diff/diffprocessor/schema_validator_test.go index d41cd13e..164cc673 100644 --- a/cmd/diff/diffprocessor/schema_validator_test.go +++ b/cmd/diff/diffprocessor/schema_validator_test.go @@ -7,6 +7,7 @@ import ( xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" tu "github.com/crossplane-contrib/crossplane-diff/cmd/diff/testutils" + pkgvalidate "github.com/crossplane/cli/v2/pkg/validate" "github.com/google/go-cmp/cmp" extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -501,7 +502,7 @@ func TestDefaultSchemaValidator_ValidateResources_AppliesDefaults(t *testing.T) } // Verify defaults were applied to the ORIGINAL resource - // The defaults are applied in-place by validate.SchemaValidation, so they persist + // The defaults are applied in-place by applyCRDDefaults before validation, so they persist deletionPolicy, found, err := un.NestedString(managedResource.Object, "spec", "deletionPolicy") if err != nil { t.Fatalf("Failed to get deletionPolicy: %v", err) @@ -534,50 +535,126 @@ func TestDefaultSchemaValidator_ValidateResources_AppliesDefaults(t *testing.T) // The compositionRevisionRef remains in the original resource, which is correct behavior. } -func TestExtractValidationErrors(t *testing.T) { +func TestFormatValidationErrors(t *testing.T) { tests := map[string]struct { - input string + result *pkgvalidate.ValidationResult expected string }{ - "SingleValidationError": { - input: "[x] schema validation error example.org/v1, my-xr : spec.region: Required value\nTotal 1 resources: 0 missing schemas, 0 success cases, 1 failure cases", - expected: "schema validation error example.org/v1, my-xr : spec.region: Required value", + "SingleSchemaError": { + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Field: "spec.region", + Message: "spec.region: Required value", + }}, + }}, + }, + expected: "schema validation error example.org/v1, Kind=XR, my-xr : spec.region: Required value", }, "SingleMissingSchema": { - input: "[!] could not find CRD/XRD for: other.org/v1, Kind=SomeResource\nTotal 1 resources: 1 missing schemas, 0 success cases, 0 failure cases", + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "other.org/v1", + Kind: "SomeResource", + Name: "thing", + Status: pkgvalidate.ValidationStatusMissingSchema, + }}, + }, expected: "could not find CRD/XRD for: other.org/v1, Kind=SomeResource", }, - "MultipleErrors": { - input: "[x] schema validation error example.org/v1, my-xr : spec.region: Required value\n[!] could not find CRD/XRD for: other.org/v1\nTotal 2 resources: 1 missing schemas, 0 success cases, 1 failure cases", - expected: "schema validation error example.org/v1, my-xr : spec.region: Required value; could not find CRD/XRD for: other.org/v1", + "MultipleErrorsAcrossResources": { + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{ + { + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Message: "spec.region: Required value", + }}, + }, + { + APIVersion: "other.org/v1", + Kind: "SomeResource", + Name: "thing", + Status: pkgvalidate.ValidationStatusMissingSchema, + }, + }, + }, + expected: "schema validation error example.org/v1, Kind=XR, my-xr : spec.region: Required value; could not find CRD/XRD for: other.org/v1, Kind=SomeResource", }, - "MixedWithSuccessLines": { - input: "[✓] example.org/v1, good-xr validated successfully\n[x] schema validation error example.org/v1, bad-xr : spec.field: Invalid value\nTotal 2 resources: 0 missing schemas, 1 success cases, 1 failure cases", - expected: "schema validation error example.org/v1, bad-xr : spec.field: Invalid value", + "ValidResourcesIgnored": { + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{ + { + APIVersion: "example.org/v1", + Kind: "XR", + Name: "good-xr", + Status: pkgvalidate.ValidationStatusValid, + }, + { + APIVersion: "example.org/v1", + Kind: "XR", + Name: "bad-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Message: "spec.field: Invalid value", + }}, + }, + }, + }, + expected: "schema validation error example.org/v1, Kind=XR, bad-xr : spec.field: Invalid value", }, - "EmptyInput": { - input: "", + "EmptyResult": { + result: &pkgvalidate.ValidationResult{}, expected: "schema validation failed", }, - "NoErrorsOnlySuccess": { - input: "[✓] example.org/v1, my-xr validated successfully\nTotal 1 resources: 0 missing schemas, 1 success cases, 0 failure cases", + "OnlyValidResources": { + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusValid, + }}, + }, expected: "schema validation failed", }, - "WhitespaceHandling": { - input: " [x] error message with leading spaces \n", - expected: "error message with leading spaces", - }, - "MultipleValidationErrors": { - input: "[x] error one\n[x] error two\n[x] error three", - expected: "error one; error two; error three", + "DefaultingErrorAccompanyingSchemaError": { + // A defaulting failure that co-occurs with a schema-class + // error must not produce a separate entry: the schema entry + // already conveys the failure, and dropping the defaulting + // line keeps the error string focused on the actionable + // problem. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{ + {Type: pkgvalidate.FieldErrorTypeDefaulting, Message: "cannot apply defaults"}, + {Type: pkgvalidate.FieldErrorTypeSchema, Message: "spec.field: Required value"}, + }, + }}, + }, + expected: "schema validation error example.org/v1, Kind=XR, my-xr : spec.field: Required value", }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - result := extractValidationErrors(tt.input) - if result != tt.expected { - t.Errorf("extractValidationErrors() = %q, want %q", result, tt.expected) + got := formatValidationErrors(tt.result) + if got != tt.expected { + t.Errorf("formatValidationErrors() = %q, want %q", got, tt.expected) } }) } diff --git a/go.mod b/go.mod index e8ac0937..73b0cb72 100644 --- a/go.mod +++ b/go.mod @@ -6,9 +6,9 @@ require ( dario.cat/mergo v1.0.2 github.com/Masterminds/semver v1.5.0 github.com/alecthomas/kong v1.15.0 - github.com/crossplane/cli/v2 v2.3.2 - github.com/crossplane/crossplane-runtime/v2 v2.3.2 - github.com/crossplane/crossplane/apis/v2 v2.3.2 + github.com/crossplane/cli/v2 v2.4.0-rc.0.0.20260615182009-ba59fbfac34b + github.com/crossplane/crossplane-runtime/v2 v2.4.0-rc.0 + github.com/crossplane/crossplane/apis/v2 v2.4.0-rc.0 github.com/crossplane/crossplane/v2 v2.3.2 github.com/docker/docker v28.5.2+incompatible github.com/google/go-cmp v0.7.0 @@ -32,7 +32,6 @@ require ( require ( cel.dev/expr v0.25.1 // indirect github.com/MakeNowJust/heredoc v1.0.0 // indirect - github.com/Masterminds/semver/v3 v3.4.0 // indirect github.com/antlr4-go/antlr/v4 v4.13.1 // indirect github.com/blang/semver/v4 v4.0.0 // indirect github.com/chai2010/gettext-go v1.0.2 // indirect @@ -121,7 +120,6 @@ require ( github.com/google/uuid v1.6.0 github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/json-iterator/go v1.1.12 // indirect - github.com/klauspost/compress v1.18.6 // indirect github.com/mattn/go-colorable v0.1.14 // indirect github.com/mattn/go-isatty v0.0.22 // indirect github.com/moby/spdystream v0.5.1 // indirect diff --git a/go.sum b/go.sum index 7f86603e..77dadda5 100644 --- a/go.sum +++ b/go.sum @@ -47,12 +47,12 @@ github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSV github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= -github.com/crossplane/cli/v2 v2.3.2 h1:9zDVZN9uHanIjp5wCzAe8NshyxrAuQWW/4me4VkNtH4= -github.com/crossplane/cli/v2 v2.3.2/go.mod h1:r0lKyzouUxl89nXLdgdRWh5vEcynpP3CDP/jmmfAKnI= -github.com/crossplane/crossplane-runtime/v2 v2.3.2 h1:gjfJmr0PTf3/Ccg4iasogXKIRjYdEMILduiP/IZN260= -github.com/crossplane/crossplane-runtime/v2 v2.3.2/go.mod h1:POGt8DSTcxQJlTww+3yGeeXuEdLyjZ61vZ3ap5tTxhE= -github.com/crossplane/crossplane/apis/v2 v2.3.2 h1:Drs3xz59qT3zFfaszxQWqr51a0leAx20DBL4TqMnqi0= -github.com/crossplane/crossplane/apis/v2 v2.3.2/go.mod h1:o+D0ktZQKJCFcpfzMKA4n53aTo2sFqqDsADBNIRuIyE= +github.com/crossplane/cli/v2 v2.4.0-rc.0.0.20260615182009-ba59fbfac34b h1:Ol4CZMZvcS/m3IYRz4YceZGj6pCF1TLyN+nTpgMgDPg= +github.com/crossplane/cli/v2 v2.4.0-rc.0.0.20260615182009-ba59fbfac34b/go.mod h1:TVfHZdpkSlrfkE6V8VLlTOS/DT4Qsxc+ybMcUnZz3ko= +github.com/crossplane/crossplane-runtime/v2 v2.4.0-rc.0 h1:Zgiq+hrh9lbjWtv8ECCLd1A0I9knt3c8ZUELExw6M1w= +github.com/crossplane/crossplane-runtime/v2 v2.4.0-rc.0/go.mod h1:PAo3zIfmMzrS18HGyHJLXCeXIp0nFW2Md2Fn9gocMaU= +github.com/crossplane/crossplane/apis/v2 v2.4.0-rc.0 h1:4PBahj+tnK9RwSZm1bYGvOkHOU+1CSHjJF2PoPzBMD0= +github.com/crossplane/crossplane/apis/v2 v2.4.0-rc.0/go.mod h1:xaQozPfGYv6ut6yZP8maDQm7ZTynHAGUffecZ5hqmhg= github.com/crossplane/crossplane/v2 v2.3.2 h1:tRkV1QjXBbEkohlfBWyZ6hoe1UQJcD9bwPz43mTxUuU= github.com/crossplane/crossplane/v2 v2.3.2/go.mod h1:lx6VH4QhRhWDmLsd59QXv2dp9KrBrClX7NR28QQKvTM= github.com/crossplane/function-sdk-go v0.7.1 h1:BEM2b7nAr8rJYc9tjwnoMfVVy1veCA/juD8APYCVLi0= From 9985534fd39c2ca76a588743f107dda0866167a2 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Mon, 15 Jun 2026 17:23:05 -0400 Subject: [PATCH 2/7] fix: emit defaulting-only errors instead of dropping them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review feedback on PR #347. formatValidationErrors previously dropped every FieldErrorTypeDefaulting entry on Invalid resources unconditionally. Upstream's statusFromErrors guarantees an Invalid resource carries at least one non-defaulting error today, so in practice this produced the right output — but if that contract ever shifted, an Invalid resource whose Errors are all defaulting would silently produce zero per-resource messages and the formatter would fall through to the generic "schema validation failed" string, hiding the real cause. Suppress defaulting only when there is another actionable (schema / CEL / unknown-field) error on the same resource. With only defaulting errors present, emit them as-is. Tested by the new InvalidWithOnlyDefaultingErrorsStillEmitted case. Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/schema_validator.go | 27 ++++++++++++++----- .../diffprocessor/schema_validator_test.go | 19 +++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/cmd/diff/diffprocessor/schema_validator.go b/cmd/diff/diffprocessor/schema_validator.go index 3e5b6474..71ec722d 100644 --- a/cmd/diff/diffprocessor/schema_validator.go +++ b/cmd/diff/diffprocessor/schema_validator.go @@ -329,13 +329,28 @@ func formatValidationErrors(result *pkgvalidate.ValidationResult) string { case pkgvalidate.ValidationStatusMissingSchema: msgs = append(msgs, "could not find CRD/XRD for: "+gvk) case pkgvalidate.ValidationStatusInvalid: + // Suppress defaulting entries only when there are other + // actionable (schema / CEL / unknown-field) errors on the + // same resource: those already convey the failure, and the + // defaulting line would just be noise. Upstream's + // statusFromErrors today guarantees that an Invalid + // resource has at least one non-defaulting error, but we + // don't rely on that here — if a future change ever + // produced an Invalid resource whose Errors are all + // defaulting, we still emit them rather than silently drop + // everything and fall through to the generic "schema + // validation failed" message. + hasActionable := false + + for _, e := range r.Errors { + if e.Type != pkgvalidate.FieldErrorTypeDefaulting { + hasActionable = true + break + } + } + for _, e := range r.Errors { - if e.Type == pkgvalidate.FieldErrorTypeDefaulting { - // ResultError treats a defaulting error as a - // failure only when accompanied by a schema-class - // error on the same resource. The schema-class - // error already gets its own entry; emitting the - // defaulting line too would just be noise. + if hasActionable && e.Type == pkgvalidate.FieldErrorTypeDefaulting { continue } diff --git a/cmd/diff/diffprocessor/schema_validator_test.go b/cmd/diff/diffprocessor/schema_validator_test.go index 164cc673..53922a7d 100644 --- a/cmd/diff/diffprocessor/schema_validator_test.go +++ b/cmd/diff/diffprocessor/schema_validator_test.go @@ -648,6 +648,25 @@ func TestFormatValidationErrors(t *testing.T) { }, expected: "schema validation error example.org/v1, Kind=XR, my-xr : spec.field: Required value", }, + "InvalidWithOnlyDefaultingErrorsStillEmitted": { + // Defensive: upstream's statusFromErrors today won't return + // Invalid for a resource whose Errors are all defaulting, + // but if that ever changed we'd rather surface the + // defaulting message than silently fall back to the generic + // "schema validation failed" string. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{ + {Type: pkgvalidate.FieldErrorTypeDefaulting, Message: "cannot apply defaults"}, + }, + }}, + }, + expected: "schema validation error example.org/v1, Kind=XR, my-xr : cannot apply defaults", + }, } for name, tt := range tests { From d036e49c901291cc5e649d46ff21677b04cbc7d5 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Mon, 15 Jun 2026 23:55:12 -0400 Subject: [PATCH 3/7] refactor(validate): redesign error format around structured per-resource blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous formatValidationErrors mirrored the upstream text renderer's prefix-marked output (`[x] schema validation error , : `) because the original implementation literally parsed that text. With structured FieldValidationError records now in hand, the mirroring is gone: the format is multi-line, grouped per resource, and surfaces information the line-parser couldn't. Output shape: / /: [ (got )] [] ... /: missing schema Per-resource grouping replaces a flat list with N entries duplicating the GVK on each line. The error type ([schema] / [cel] / [unknownField] / [defaulting]) makes the failure class visible without re-parsing the message. Bad values from FieldValidationError.Value are appended as "(got )" only when they aren't already substring-present in the message — k8s-derived errors typically embed the bad value in the message text, so this avoids duplication. The "schema validation failed" fallback is gone: formatValidationErrors is invoked only after pkgvalidate.ResultError has flagged a failure, so an empty result is unreachable in production. Returning "" lets the wrapping SchemaValidationError carry the underlying ResultError message instead of substituting a misleading generic string. The integration TestDiffIntegration/SchemaValidationError previously asserted on `Contains "schema validation"`, which only passed because the inner formatter happened to repeat that phrase. The wrap prefix "cannot validate resources" from diff_processor.go is the stable semantic anchor; assertion updated. Address Copilot review feedback as part of this redesign: defaulting-only Invalid resources (currently unreachable per upstream statusFromErrors, but defensible as a future-proofing measure) emit their defaulting messages rather than producing an empty block. Signed-off-by: Jonathan Ogilvie --- cmd/diff/diff_integration_test.go | 9 +- cmd/diff/diffprocessor/schema_validator.go | 139 ++++++++++++------ .../diffprocessor/schema_validator_test.go | 121 ++++++++++++--- 3 files changed, 207 insertions(+), 62 deletions(-) diff --git a/cmd/diff/diff_integration_test.go b/cmd/diff/diff_integration_test.go index b71223f8..ae1e821c 100644 --- a/cmd/diff/diff_integration_test.go +++ b/cmd/diff/diff_integration_test.go @@ -1150,9 +1150,12 @@ Summary: 2 modified, 2 removed`, inputFiles: []string{ "testdata/diff/invalid-schema-xr.yaml", }, - expectedError: true, - expectedExitCode: dp.ExitCodeSchemaValidation, - expectedErrorContains: "schema validation", + expectedError: true, + expectedExitCode: dp.ExitCodeSchemaValidation, + // The wrap prefix from diff_processor.go is the stable + // anchor here; the inner per-resource formatter no longer + // repeats "schema validation" in its rendered text. + expectedErrorContains: "cannot validate resources", noColor: true, }, "NewClaimShowsDiff": { diff --git a/cmd/diff/diffprocessor/schema_validator.go b/cmd/diff/diffprocessor/schema_validator.go index 71ec722d..53a01d7a 100644 --- a/cmd/diff/diffprocessor/schema_validator.go +++ b/cmd/diff/diffprocessor/schema_validator.go @@ -313,61 +313,118 @@ func (v *DefaultSchemaValidator) logResultDetails(result *pkgvalidate.Validation } } -// formatValidationErrors produces a single-line, semicolon-joined string -// describing the failures in a *ValidationResult. It mirrors the -// previously extracted text-renderer output (one entry per -// FieldValidationError, with a "could not find CRD/XRD for ..." entry -// per missing-schema resource) so existing callers and tests that -// match on these substrings keep working. +// formatValidationErrors produces a multi-line, per-resource breakdown +// of the failures in a *ValidationResult. Each resource that has +// something to report contributes a header line " :" +// followed by one indented line per FieldValidationError, with the +// error type appended in brackets and the structured Value rendered as +// "(got )" when it isn't already quoted into the message text. +// Missing-schema resources collapse to a single " : missing +// schema" line. +// +// Callers invoke this only after pkgvalidate.ResultError has already +// returned a non-nil error, so an empty result means "no per-resource +// detail to report" — we return "" and let the wrapping +// SchemaValidationError carry the underlying ResultError message. func formatValidationErrors(result *pkgvalidate.ValidationResult) string { - var msgs []string + var blocks []string for _, r := range result.Resources { - gvk := fmt.Sprintf("%s, Kind=%s", r.APIVersion, r.Kind) - switch r.Status { case pkgvalidate.ValidationStatusMissingSchema: - msgs = append(msgs, "could not find CRD/XRD for: "+gvk) + blocks = append(blocks, formatMissingSchemaBlock(r)) case pkgvalidate.ValidationStatusInvalid: - // Suppress defaulting entries only when there are other - // actionable (schema / CEL / unknown-field) errors on the - // same resource: those already convey the failure, and the - // defaulting line would just be noise. Upstream's - // statusFromErrors today guarantees that an Invalid - // resource has at least one non-defaulting error, but we - // don't rely on that here — if a future change ever - // produced an Invalid resource whose Errors are all - // defaulting, we still emit them rather than silently drop - // everything and fall through to the generic "schema - // validation failed" message. - hasActionable := false - - for _, e := range r.Errors { - if e.Type != pkgvalidate.FieldErrorTypeDefaulting { - hasActionable = true - break - } - } - - for _, e := range r.Errors { - if hasActionable && e.Type == pkgvalidate.FieldErrorTypeDefaulting { - continue - } - - msgs = append(msgs, fmt.Sprintf("schema validation error %s, %s : %s", gvk, r.Name, e.Message)) - } + blocks = append(blocks, formatInvalidBlock(r)) case pkgvalidate.ValidationStatusValid, pkgvalidate.ValidationStatusDefaultingFailed: // Valid: nothing to report. DefaultingFailed-only resources // are reported by ResultError as success, so this function - // never sees them via the failure path; the case is here + // never sees them via the failure path; the cases are here // only to keep the switch exhaustive. } } - if len(msgs) == 0 { - return "schema validation failed" + return strings.Join(blocks, "\n") +} + +// resourceHeader formats the per-resource header used by both the +// missing-schema and invalid blocks. Cluster-scoped resources omit the +// namespace prefix; namespaced resources produce "/". +func resourceHeader(r pkgvalidate.ResourceValidationResult) string { + name := r.Name + if r.Namespace != "" { + name = r.Namespace + "/" + r.Name + } + + if name == "" { + return fmt.Sprintf("%s/%s", r.APIVersion, r.Kind) + } + + return fmt.Sprintf("%s/%s %s", r.APIVersion, r.Kind, name) +} + +// formatMissingSchemaBlock renders a single line for a resource whose +// CRD/XRD wasn't found. The resource was never validated, so there are +// no per-error lines to indent under it. +func formatMissingSchemaBlock(r pkgvalidate.ResourceValidationResult) string { + return resourceHeader(r) + ": missing schema" +} + +// formatInvalidBlock renders a header line plus one indented line per +// surfaced error. +// +// Defaulting entries are suppressed only when an actionable (schema / +// CEL / unknown-field) error is present on the same resource: those +// already convey the failure and the defaulting line would just be +// noise. Upstream's statusFromErrors today guarantees an Invalid +// resource has at least one non-defaulting error, but if a future +// change ever produced an Invalid resource whose Errors are all +// defaulting we'd rather emit them than silently drop everything. +func formatInvalidBlock(r pkgvalidate.ResourceValidationResult) string { + hasActionable := false + + for _, e := range r.Errors { + if e.Type != pkgvalidate.FieldErrorTypeDefaulting { + hasActionable = true + break + } + } + + lines := []string{resourceHeader(r) + ":"} + + for _, e := range r.Errors { + if hasActionable && e.Type == pkgvalidate.FieldErrorTypeDefaulting { + continue + } + + lines = append(lines, " "+formatErrorLine(e)) + } + + return strings.Join(lines, "\n") +} + +// formatErrorLine renders one FieldValidationError as +// "[ (got )] []". The bad value tail is omitted +// when Value is nil or already substring-present in the message, so +// k8s-derived errors (which embed the value in their text) don't +// produce duplicate output. +func formatErrorLine(e pkgvalidate.FieldValidationError) string { + msg := e.Message + if rendered := renderBadValue(e.Value); rendered != "" && !strings.Contains(msg, rendered) { + msg = fmt.Sprintf("%s (got %s)", msg, rendered) + } + + return fmt.Sprintf("%s [%s]", msg, e.Type) +} + +// renderBadValue formats a FieldValidationError.Value for display. +// Returns the empty string for nil so callers can use it as a presence +// check; non-empty results are suitable for direct concatenation into +// an error message. +func renderBadValue(value any) string { + if value == nil { + return "" } - return strings.Join(msgs, "; ") + return fmt.Sprintf("%v", value) } diff --git a/cmd/diff/diffprocessor/schema_validator_test.go b/cmd/diff/diffprocessor/schema_validator_test.go index 53922a7d..1a2984f9 100644 --- a/cmd/diff/diffprocessor/schema_validator_test.go +++ b/cmd/diff/diffprocessor/schema_validator_test.go @@ -163,7 +163,7 @@ func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { composed: []cpd.Unstructured{*composedResource1, *composedResource2}, preloadedCRDs: []*extv1.CustomResourceDefinition{createCRDWithStringField(xrCRD)}, expectedErr: true, - expectedErrMsg: "could not find CRD/XRD", + expectedErrMsg: "missing schema", }, } @@ -554,7 +554,7 @@ func TestFormatValidationErrors(t *testing.T) { }}, }}, }, - expected: "schema validation error example.org/v1, Kind=XR, my-xr : spec.region: Required value", + expected: "example.org/v1/XR my-xr:\n spec.region: Required value [schema]", }, "SingleMissingSchema": { result: &pkgvalidate.ValidationResult{ @@ -565,9 +565,93 @@ func TestFormatValidationErrors(t *testing.T) { Status: pkgvalidate.ValidationStatusMissingSchema, }}, }, - expected: "could not find CRD/XRD for: other.org/v1, Kind=SomeResource", + expected: "other.org/v1/SomeResource thing: missing schema", }, - "MultipleErrorsAcrossResources": { + "MissingSchemaWithoutName": { + // We may not always have a name (e.g. when the user feeds a + // resource without metadata.name). Header collapses to just + // the GVK rather than producing a trailing space. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "other.org/v1", + Kind: "SomeResource", + Status: pkgvalidate.ValidationStatusMissingSchema, + }}, + }, + expected: "other.org/v1/SomeResource: missing schema", + }, + "NamespacedResource": { + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "Thing", + Namespace: "production", + Name: "my-thing", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Message: "spec.size: Required value", + }}, + }}, + }, + expected: "example.org/v1/Thing production/my-thing:\n spec.size: Required value [schema]", + }, + "MultipleErrorsGroupedUnderResource": { + // Two errors on the same resource should be grouped under a + // single header, not produce a header per error. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{ + {Type: pkgvalidate.FieldErrorTypeSchema, Message: "spec.region: Required value"}, + {Type: pkgvalidate.FieldErrorTypeCEL, Message: "spec.replicas: must be > 0"}, + }, + }}, + }, + expected: "example.org/v1/XR my-xr:\n spec.region: Required value [schema]\n spec.replicas: must be > 0 [cel]", + }, + "BadValueAppendedWhenAbsentFromMessage": { + // CEL-style errors don't always embed the bad value into + // the message; the structured Value should be rendered as + // "(got )" so users can see what was rejected. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeCEL, + Message: "spec.replicas: must be > 0", + Value: -1, + }}, + }}, + }, + expected: "example.org/v1/XR my-xr:\n spec.replicas: must be > 0 (got -1) [cel]", + }, + "BadValueOmittedWhenAlreadyInMessage": { + // k8s field errors typically embed the bad value as a + // quoted string in the message text. Avoid duplicating it + // when the message already contains the rendered form. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Message: `spec.replicas: Invalid value: "five": must be integer`, + Value: "five", + }}, + }}, + }, + expected: `example.org/v1/XR my-xr:` + "\n" + ` spec.replicas: Invalid value: "five": must be integer [schema]`, + }, + "MultipleResourcesEachOnTheirOwnBlock": { result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{ { @@ -588,9 +672,11 @@ func TestFormatValidationErrors(t *testing.T) { }, }, }, - expected: "schema validation error example.org/v1, Kind=XR, my-xr : spec.region: Required value; could not find CRD/XRD for: other.org/v1, Kind=SomeResource", + expected: "example.org/v1/XR my-xr:\n spec.region: Required value [schema]\nother.org/v1/SomeResource thing: missing schema", }, - "ValidResourcesIgnored": { + "ValidResourcesProduceNoBlock": { + // A resource that passed validation shouldn't appear in the + // output at all; only the failing resource does. result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{ { @@ -611,13 +697,13 @@ func TestFormatValidationErrors(t *testing.T) { }, }, }, - expected: "schema validation error example.org/v1, Kind=XR, bad-xr : spec.field: Invalid value", - }, - "EmptyResult": { - result: &pkgvalidate.ValidationResult{}, - expected: "schema validation failed", + expected: "example.org/v1/XR bad-xr:\n spec.field: Invalid value [schema]", }, - "OnlyValidResources": { + "NoApplicableEntriesReturnsEmpty": { + // formatValidationErrors is only invoked after ResultError + // has flagged a failure, so an empty/all-valid input is + // unreachable in production. We pin the contract anyway: + // no entries → empty string, not a misleading fallback. result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -626,13 +712,13 @@ func TestFormatValidationErrors(t *testing.T) { Status: pkgvalidate.ValidationStatusValid, }}, }, - expected: "schema validation failed", + expected: "", }, "DefaultingErrorAccompanyingSchemaError": { // A defaulting failure that co-occurs with a schema-class // error must not produce a separate entry: the schema entry // already conveys the failure, and dropping the defaulting - // line keeps the error string focused on the actionable + // line keeps the error block focused on the actionable // problem. result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ @@ -646,14 +732,13 @@ func TestFormatValidationErrors(t *testing.T) { }, }}, }, - expected: "schema validation error example.org/v1, Kind=XR, my-xr : spec.field: Required value", + expected: "example.org/v1/XR my-xr:\n spec.field: Required value [schema]", }, "InvalidWithOnlyDefaultingErrorsStillEmitted": { // Defensive: upstream's statusFromErrors today won't return // Invalid for a resource whose Errors are all defaulting, // but if that ever changed we'd rather surface the - // defaulting message than silently fall back to the generic - // "schema validation failed" string. + // defaulting message than emit an empty block. result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -665,7 +750,7 @@ func TestFormatValidationErrors(t *testing.T) { }, }}, }, - expected: "schema validation error example.org/v1, Kind=XR, my-xr : cannot apply defaults", + expected: "example.org/v1/XR my-xr:\n cannot apply defaults [defaulting]", }, } From 86ca63d596fbdc40b069566e90c3b30238b1a4a4 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 16 Jun 2026 10:41:58 -0400 Subject: [PATCH 4/7] feat(validate): expose typed validation failures in JSON / YAML output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Schema validation now surfaces structured per-resource failure detail through OutputError.ValidationFailures so machine consumers of crossplane-diff's JSON / YAML output don't have to parse the human-readable Message string. The string is still emitted alongside, unchanged, for operators reading the JSON directly. Wire shape: { "errors": [{ "resourceID": "XNopResource/my-xr", "message": "...", "validationFailures": [ { "apiVersion": "ns.example.org/v1alpha1", "kind": "XNopResource", "name": "my-xr", "namespace": "default", "status": "invalid", "errors": [ { "type": "schema", "field": "spec.coolField", "message": "spec.coolField: Invalid value: \"number\": ...", "value": "number" } ] } ] }] } ResourceID and ValidationFailures play complementary roles: ResourceID identifies which user input the diff was processing (one entry per batched run), while ValidationFailures lists every resource inside that input's render tree that failed validation, with full GVK, namespace, and typed errors. They overlap on Kind+Name when the input itself is among the failing resources — that's intentional: consumers iterating ValidationFailures should never miss an XR-level rejection. Wire types live in renderer/types/types.go (ResourceValidationFailure, FieldValidationError) so crossplane-diff owns its public schema rather than re-exporting crossplane/cli's pkg/validate types — the upstream shape can evolve independently of ours. SchemaValidationError gains an optional Result field carrying the *pkgvalidate.ValidationResult that produced it; ValidateResources attaches it. NewOutputError handles the type assertion + conversion in one place, used at both XR diff and comp diff error-construction sites. Non-validation paths (scope check, tool errors, IO errors) leave ValidationFailures nil. The integration test infra splits stdout/stderr into separate buffers (previously combined via kong.Writers(&stdout, &stdout), which broke JSON parsing as soon as the renderer wrote a stderr ERROR line) and allows expectedStructuredOutput to coexist with expectedError when no expectedErrorContains is set. ExpectedDiff gains a fluent WithError / WithValidationFailure / WithFieldError builder chain so tests assert on validationFailures structurally. The SchemaValidationError integration test switches from substring matching the human-readable error string to JSON-mode + structured assertion: it now pins the failing GVKs, namespaces, statuses, error types, and field paths for both the failing XR and its composed resource. The k8s-emitted message wording in between is intentionally not asserted so apimachinery upgrades can shift the phrasing without breaking the test. Signed-off-by: Jonathan Ogilvie --- cmd/diff/diff_integration_test.go | 73 +++-- cmd/diff/diffprocessor/comp_processor.go | 9 +- cmd/diff/diffprocessor/diff_processor.go | 10 +- cmd/diff/diffprocessor/errors.go | 122 +++++++- cmd/diff/diffprocessor/errors_test.go | 321 ++++++++++++++++++++ cmd/diff/diffprocessor/schema_validator.go | 2 +- cmd/diff/renderer/types/types.go | 69 ++++- cmd/diff/testutils/structured_assertions.go | 303 +++++++++++++++++- 8 files changed, 874 insertions(+), 35 deletions(-) diff --git a/cmd/diff/diff_integration_test.go b/cmd/diff/diff_integration_test.go index ae1e821c..96a67d85 100644 --- a/cmd/diff/diff_integration_test.go +++ b/cmd/diff/diff_integration_test.go @@ -220,8 +220,11 @@ func runIntegrationTest(t *testing.T, testType DiffTestType, tt IntegrationTestC // so we can pass paths directly without special handling testFiles = append(testFiles, tt.inputFiles...) - // Create a buffer to capture the output - var stdout bytes.Buffer + // Capture stdout and stderr into separate buffers. The diff command + // writes structured output (JSON / YAML / text diff) to stdout and + // human-readable error lines to stderr; mixing them defeats JSON + // parsing for tests that drive the structured assertion path. + var stdout, stderr bytes.Buffer // Create command line args that match your pre-populated struct args := []string{ @@ -304,7 +307,7 @@ func runIntegrationTest(t *testing.T, testType DiffTestType, tt IntegrationTestC // Create a Kong context with stdout parser, err := kong.New(cmd, - kong.Writers(&stdout, &stdout), + kong.Writers(&stdout, &stderr), kong.Bind(appCtx), kong.Bind(exitCode), kong.BindTo(logger, (*logging.Logger)(nil)), @@ -333,19 +336,35 @@ func runIntegrationTest(t *testing.T, testType DiffTestType, tt IntegrationTestC t.Fatalf("expected no error but got: %v", err) } - // Check for specific error message if expected - if err != nil { - if tt.expectedErrorContains != "" && strings.Contains(err.Error(), tt.expectedErrorContains) { - // This is an expected error with the expected message - t.Logf("Got expected error containing: %s", tt.expectedErrorContains) - } else { - t.Errorf("Expected no error or specific error message, got: %v", err) + // Check for specific error message if expected. + // + // Three modes: + // 1. expectedError + expectedErrorContains: substring-check the error. + // 2. expectedError + structured assertion: any error is fine here; + // the structured assertion below validates errors[]. + // 3. unexpected error: already caught by the t.Fatalf above. + if err != nil && tt.expectedError { + hasStructuredAssertion := tt.expectedStructuredOutput != nil || tt.expectedStructuredCompOutput != nil + + switch { + case tt.expectedErrorContains != "": + if strings.Contains(err.Error(), tt.expectedErrorContains) { + t.Logf("Got expected error containing: %s", tt.expectedErrorContains) + } else { + t.Errorf("Expected error containing %q, got: %v", tt.expectedErrorContains, err) + } + case hasStructuredAssertion: + t.Logf("Got expected error; structured assertion will validate errors[]") + default: + t.Errorf("expectedError set without expectedErrorContains or structured assertion; got: %v", err) } } - // For expected errors with specific messages, we've already checked above - if tt.expectedError && tt.expectedErrorContains != "" { - // Skip output check for expected error cases + // Skip stdout check for expected-error cases that use the legacy + // substring assertion. Cases that pin a structured assertion fall + // through so the JSON path is exercised against errors[] / changes. + if tt.expectedError && tt.expectedErrorContains != "" && + tt.expectedStructuredOutput == nil && tt.expectedStructuredCompOutput == nil { return } @@ -1141,7 +1160,7 @@ Summary: 2 modified, 2 removed`, noColor: true, }, "SchemaValidationError": { - reason: "Validates exit code 2 for schema validation errors", + reason: "Validates exit code 2 for schema validation errors and structured per-resource failure detail in JSON output", setupFiles: []string{ "testdata/diff/resources/xrd.yaml", "testdata/diff/resources/composition.yaml", @@ -1150,13 +1169,29 @@ Summary: 2 modified, 2 removed`, inputFiles: []string{ "testdata/diff/invalid-schema-xr.yaml", }, + outputFormat: "json", expectedError: true, expectedExitCode: dp.ExitCodeSchemaValidation, - // The wrap prefix from diff_processor.go is the stable - // anchor here; the inner per-resource formatter no longer - // repeats "schema validation" in its rendered text. - expectedErrorContains: "cannot validate resources", - noColor: true, + // Structured assertion: lock down the typed + // validationFailures payload for both the failing XR and + // its composed resource. Field paths and error types are + // ours to assert; the embedded k8s message wording is + // left unasserted (apimachinery upgrades can shift its + // phrasing without changing our output contract). + expectedStructuredOutput: tu.ExpectDiff(). + WithError("XNopResource/invalid-schema-xr"). + WithValidationFailure("ns.diff.example.org/v1alpha1", "XNopResource", "invalid-schema-xr", "default"). + WithStatus("invalid"). + WithFieldError("schema", "spec.coolField"). + AndFieldError(). + AndValidationFailure(). + WithValidationFailure("ns.nop.example.org/v1alpha1", "XDownstreamResource", "invalid-schema-xr", "default"). + WithStatus("invalid"). + WithFieldError("schema", "spec.forProvider.configData"). + AndFieldError(). + AndValidationFailure(). + AndError(), + noColor: true, }, "NewClaimShowsDiff": { reason: "Shows diff for new claim", diff --git a/cmd/diff/diffprocessor/comp_processor.go b/cmd/diff/diffprocessor/comp_processor.go index 097cdd3a..ac79454a 100644 --- a/cmd/diff/diffprocessor/comp_processor.go +++ b/cmd/diff/diffprocessor/comp_processor.go @@ -261,10 +261,11 @@ func (p *DefaultCompDiffProcessor) DiffComposition(ctx context.Context, composit for _, impact := range comp.ImpactAnalysis { if impact.Status == renderer.XRStatusError && impact.Error != nil { resourceID := fmt.Sprintf("%s/%s", impact.Kind, impact.Name) - output.Errors = append(output.Errors, dt.OutputError{ - ResourceID: resourceID, - Message: impact.Error.Error(), - }) + // NewOutputError surfaces typed validation failures + // via OutputError.ValidationFailures when impact.Error + // wraps a SchemaValidationError carrying a structured + // Result. + output.Errors = append(output.Errors, NewOutputError(resourceID, impact.Error)) } } } diff --git a/cmd/diff/diffprocessor/diff_processor.go b/cmd/diff/diffprocessor/diff_processor.go index 51468715..d432ff72 100644 --- a/cmd/diff/diffprocessor/diff_processor.go +++ b/cmd/diff/diffprocessor/diff_processor.go @@ -231,11 +231,11 @@ func (p *DefaultDiffProcessor) PerformDiff(ctx context.Context, resources []*un. "error", err) errs = append(errs, errors.Wrapf(err, "unable to process resource %s", resourceID)) - // Collect error for structured output - outputErrors = append(outputErrors, dt.OutputError{ - ResourceID: resourceID, - Message: err.Error(), - }) + // Collect error for structured output. NewOutputError + // surfaces typed validation failures via + // OutputError.ValidationFailures when err wraps a + // SchemaValidationError that carries a structured Result. + outputErrors = append(outputErrors, NewOutputError(resourceID, err)) } else { // Only merge diffs on success - we don't emit partial results for a single XR maps.Copy(allDiffs, diffs) diff --git a/cmd/diff/diffprocessor/errors.go b/cmd/diff/diffprocessor/errors.go index 8f15c484..410afccc 100644 --- a/cmd/diff/diffprocessor/errors.go +++ b/cmd/diff/diffprocessor/errors.go @@ -19,6 +19,9 @@ package diffprocessor import ( "errors" "fmt" + + dt "github.com/crossplane-contrib/crossplane-diff/cmd/diff/renderer/types" + pkgvalidate "github.com/crossplane/cli/v2/pkg/validate" ) // Exit codes for crossplane-diff CLI. @@ -37,11 +40,21 @@ const ( ) // SchemaValidationError indicates schema validation failed. -// Used to distinguish validation errors from other tool errors for exit code handling. +// Used to distinguish validation errors from other tool errors for exit +// code handling. +// +// Result, when non-nil, carries the structured per-resource validation +// outcome that produced this error. Output renderers use it to surface +// typed FieldValidationError records under OutputError.ValidationFailures +// in JSON / YAML output. It is left nil for paths that fail validation +// without a *pkgvalidate.ValidationResult in hand — for example +// scope-validation errors raised after schema validation succeeded — so +// the absence of structured detail is observable rather than fabricated. type SchemaValidationError struct { ResourceID string Message string Err error + Result *pkgvalidate.ValidationResult } // Error implements the error interface. @@ -58,7 +71,9 @@ func (e *SchemaValidationError) Unwrap() error { return e.Err } -// NewSchemaValidationError creates a new SchemaValidationError. +// NewSchemaValidationError creates a new SchemaValidationError without +// a structured Result. Use WithResult on the returned value to attach +// one when the failure originated from pkg/validate.SchemaValidate. func NewSchemaValidationError(resourceID, message string, err error) *SchemaValidationError { return &SchemaValidationError{ ResourceID: resourceID, @@ -67,6 +82,109 @@ func NewSchemaValidationError(resourceID, message string, err error) *SchemaVali } } +// WithResult attaches the structured *pkgvalidate.ValidationResult +// that produced this error so downstream renderers can emit typed +// per-resource failures alongside the human-readable Message. Returns +// the receiver for fluent chaining. +func (e *SchemaValidationError) WithResult(result *pkgvalidate.ValidationResult) *SchemaValidationError { + e.Result = result + return e +} + +// NewOutputError builds a structured-output entry for err, tagged with +// resourceID. When err contains a *SchemaValidationError that carries a +// pkgvalidate.ValidationResult, the returned OutputError also exposes a +// typed per-resource breakdown via ValidationFailures so machine +// consumers don't need to parse Message. Non-validation errors return +// an OutputError with only ResourceID and Message populated. +func NewOutputError(resourceID string, err error) dt.OutputError { + out := dt.OutputError{ + ResourceID: resourceID, + Message: err.Error(), + } + + var sve *SchemaValidationError + if errors.As(err, &sve) && sve.Result != nil { + out.ValidationFailures = validationFailuresFromResult(sve.Result) + } + + return out +} + +// validationFailuresFromResult maps a pkgvalidate.ValidationResult into +// the wire types crossplane-diff exposes through OutputError. Resources +// with status Valid are filtered out — ValidationFailures is "what went +// wrong", not "the full audit log". Resources with status +// DefaultingFailed are filtered out too: pkgvalidate.ResultError treats +// defaulting-only failures as success, so a SchemaValidationError +// reaching this code path should not advertise them as failures. +func validationFailuresFromResult(result *pkgvalidate.ValidationResult) []dt.ResourceValidationFailure { + if result == nil { + return nil + } + + var out []dt.ResourceValidationFailure + + for _, r := range result.Resources { + switch r.Status { + case pkgvalidate.ValidationStatusValid, + pkgvalidate.ValidationStatusDefaultingFailed: + continue + case pkgvalidate.ValidationStatusInvalid, + pkgvalidate.ValidationStatusMissingSchema: + // fall through + } + + out = append(out, dt.ResourceValidationFailure{ + APIVersion: r.APIVersion, + Kind: r.Kind, + Name: r.Name, + Namespace: r.Namespace, + Status: string(r.Status), + Errors: fieldValidationErrorsFromUpstream(r.Errors), + }) + } + + return out +} + +// fieldValidationErrorsFromUpstream converts the cli's per-field error +// slice into our wire shape. Defaulting entries are filtered out when +// at least one actionable (schema / CEL / unknown-field) error is +// present on the same resource, mirroring the suppression policy of +// formatValidationErrors so the typed and human-readable views agree. +func fieldValidationErrorsFromUpstream(errs []pkgvalidate.FieldValidationError) []dt.FieldValidationError { + if len(errs) == 0 { + return nil + } + + hasActionable := false + + for _, e := range errs { + if e.Type != pkgvalidate.FieldErrorTypeDefaulting { + hasActionable = true + break + } + } + + out := make([]dt.FieldValidationError, 0, len(errs)) + + for _, e := range errs { + if hasActionable && e.Type == pkgvalidate.FieldErrorTypeDefaulting { + continue + } + + out = append(out, dt.FieldValidationError{ + Type: string(e.Type), + Field: e.Field, + Message: e.Message, + Value: e.Value, + }) + } + + return out +} + // IsSchemaValidationError checks if any error in the chain is a schema validation error. func IsSchemaValidationError(err error) bool { var sve *SchemaValidationError diff --git a/cmd/diff/diffprocessor/errors_test.go b/cmd/diff/diffprocessor/errors_test.go index fc484a82..60a3e7f4 100644 --- a/cmd/diff/diffprocessor/errors_test.go +++ b/cmd/diff/diffprocessor/errors_test.go @@ -21,6 +21,8 @@ import ( "fmt" "testing" + dt "github.com/crossplane-contrib/crossplane-diff/cmd/diff/renderer/types" + pkgvalidate "github.com/crossplane/cli/v2/pkg/validate" gcmp "github.com/google/go-cmp/cmp" xperrors "github.com/crossplane/crossplane-runtime/v2/pkg/errors" @@ -135,6 +137,325 @@ func TestNewSchemaValidationError(t *testing.T) { if !errors.Is(err.Err, wrappedErr) { t.Errorf("Err = %v, want %v", err.Err, wrappedErr) } + + if err.Result != nil { + t.Errorf("Result = %v, want nil for the basic constructor", err.Result) + } +} + +func TestSchemaValidationError_WithResult(t *testing.T) { + result := &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "x", + Status: pkgvalidate.ValidationStatusInvalid, + }}, + } + + err := NewSchemaValidationError("", "msg", errors.New("inner")).WithResult(result) + if err.Result != result { + t.Errorf("Result = %v, want %v", err.Result, result) + } +} + +func TestNewOutputError(t *testing.T) { + tests := map[string]struct { + resourceID string + err error + want dt.OutputError + }{ + "NonValidationError_NoFailuresAttached": { + // A plain error has no structured slot to populate, so + // ValidationFailures stays nil. + resourceID: "XR/my-xr", + err: errors.New("kube unreachable"), + want: dt.OutputError{ + ResourceID: "XR/my-xr", + Message: "kube unreachable", + }, + }, + "SchemaValidationError_WithoutResult_NoFailuresAttached": { + // A SchemaValidationError that wasn't given a Result (e.g., + // scope-validation failure path) only contributes Message. + resourceID: "XR/my-xr", + err: NewSchemaValidationError("", "scope failure", errors.New("inner")), + want: dt.OutputError{ + ResourceID: "XR/my-xr", + Message: "scope failure", + }, + }, + "SchemaValidationError_WithResult_ExposesTypedFailures": { + resourceID: "XR/my-xr", + err: NewSchemaValidationError("", "msg", errors.New("inner")).WithResult( + &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Field: "spec.region", + Message: "spec.region: Required value", + }}, + }}, + }), + want: dt.OutputError{ + ResourceID: "XR/my-xr", + Message: "msg", + ValidationFailures: []dt.ResourceValidationFailure{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: "invalid", + Errors: []dt.FieldValidationError{{ + Type: "schema", + Field: "spec.region", + Message: "spec.region: Required value", + }}, + }}, + }, + }, + "WrappedSchemaValidationError_StillSurfacesFailures": { + // errors.As walks the chain, so a SchemaValidationError + // hidden behind errors.Wrap should still surface its Result. + resourceID: "XR/my-xr", + err: xperrors.Wrap( + NewSchemaValidationError("", "msg", errors.New("inner")).WithResult( + &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "other.org/v1", + Kind: "Thing", + Name: "thing", + Status: pkgvalidate.ValidationStatusMissingSchema, + }}, + }), + "cannot validate resources"), + want: dt.OutputError{ + ResourceID: "XR/my-xr", + Message: "cannot validate resources: msg", + ValidationFailures: []dt.ResourceValidationFailure{{ + APIVersion: "other.org/v1", + Kind: "Thing", + Name: "thing", + Status: "missingSchema", + }}, + }, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := NewOutputError(tc.resourceID, tc.err) + if diff := gcmp.Diff(tc.want, got); diff != "" { + t.Errorf("NewOutputError() mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestValidationFailuresFromResult(t *testing.T) { + tests := map[string]struct { + result *pkgvalidate.ValidationResult + want []dt.ResourceValidationFailure + }{ + "NilResultReturnsNil": { + result: nil, + want: nil, + }, + "OnlyValidResourcesReturnsNil": { + // Valid status entries are filtered: ValidationFailures is + // "what failed", not the full audit log. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "ok", + Status: pkgvalidate.ValidationStatusValid, + }}, + }, + want: nil, + }, + "DefaultingFailedFiltered": { + // pkgvalidate.ResultError treats DefaultingFailed as + // success, so a SchemaValidationError that reaches us + // shouldn't advertise these resources as failures. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "x", + Status: pkgvalidate.ValidationStatusDefaultingFailed, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeDefaulting, + Message: "cannot apply defaults", + }}, + }}, + }, + want: nil, + }, + "InvalidWithMixedErrorsSuppressesDefaulting": { + // Mirror the human formatter: when actionable errors are + // present, defaulting entries are dropped so the typed and + // rendered views agree on the failure detail. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Namespace: "production", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{ + {Type: pkgvalidate.FieldErrorTypeDefaulting, Message: "cannot apply defaults"}, + {Type: pkgvalidate.FieldErrorTypeSchema, Field: "spec.region", Message: "spec.region: Required value"}, + }, + }}, + }, + want: []dt.ResourceValidationFailure{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Namespace: "production", + Status: "invalid", + Errors: []dt.FieldValidationError{{ + Type: "schema", + Field: "spec.region", + Message: "spec.region: Required value", + }}, + }}, + }, + "InvalidWithOnlyDefaultingErrorsKeepsThem": { + // Defensive: if a future upstream change ever produced an + // Invalid resource whose Errors are all defaulting, we'd + // rather emit the entries than produce an invalid resource + // row with no errors. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeDefaulting, + Message: "cannot apply defaults", + }}, + }}, + }, + want: []dt.ResourceValidationFailure{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: "invalid", + Errors: []dt.FieldValidationError{{ + Type: "defaulting", + Message: "cannot apply defaults", + }}, + }}, + }, + "MissingSchemaSurfacedWithoutErrors": { + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "other.org/v1", + Kind: "SomeResource", + Name: "thing", + Status: pkgvalidate.ValidationStatusMissingSchema, + }}, + }, + want: []dt.ResourceValidationFailure{{ + APIVersion: "other.org/v1", + Kind: "SomeResource", + Name: "thing", + Status: "missingSchema", + }}, + }, + "BadValuePropagated": { + // The structured Value should travel through unchanged, + // preserving its Go type (here: a string), so JSON output + // renders it without forcing callers to parse it back. + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Field: "spec.replicas", + Message: `spec.replicas: Invalid value: "five"`, + Value: "five", + }}, + }}, + }, + want: []dt.ResourceValidationFailure{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: "invalid", + Errors: []dt.FieldValidationError{{ + Type: "schema", + Field: "spec.replicas", + Message: `spec.replicas: Invalid value: "five"`, + Value: "five", + }}, + }}, + }, + "MultipleResourcesPreserveOrder": { + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{ + { + APIVersion: "example.org/v1", + Kind: "XR", + Name: "first", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Message: "first error", + }}, + }, + { + APIVersion: "example.org/v1", + Kind: "XR", + Name: "ok-skipped", + Status: pkgvalidate.ValidationStatusValid, + }, + { + APIVersion: "other.org/v1", + Kind: "Thing", + Name: "third", + Status: pkgvalidate.ValidationStatusMissingSchema, + }, + }, + }, + want: []dt.ResourceValidationFailure{ + { + APIVersion: "example.org/v1", + Kind: "XR", + Name: "first", + Status: "invalid", + Errors: []dt.FieldValidationError{{ + Type: "schema", + Message: "first error", + }}, + }, + { + APIVersion: "other.org/v1", + Kind: "Thing", + Name: "third", + Status: "missingSchema", + }, + }, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := validationFailuresFromResult(tc.result) + if diff := gcmp.Diff(tc.want, got); diff != "" { + t.Errorf("validationFailuresFromResult() mismatch (-want +got):\n%s", diff) + } + }) + } } func TestIsOnlySchemaValidationErrors(t *testing.T) { diff --git a/cmd/diff/diffprocessor/schema_validator.go b/cmd/diff/diffprocessor/schema_validator.go index 53a01d7a..4dfdd571 100644 --- a/cmd/diff/diffprocessor/schema_validator.go +++ b/cmd/diff/diffprocessor/schema_validator.go @@ -131,7 +131,7 @@ func (v *DefaultSchemaValidator) ValidateResources(ctx context.Context, xr *un.U v.logResultDetails(result) if rerr := pkgvalidate.ResultError(result, true); rerr != nil { - return NewSchemaValidationError("", formatValidationErrors(result), rerr) + return NewSchemaValidationError("", formatValidationErrors(result), rerr).WithResult(result) } // Additionally validate resource scope constraints (namespace requirements and cross-namespace refs) diff --git a/cmd/diff/renderer/types/types.go b/cmd/diff/renderer/types/types.go index 3fbfd794..862665a8 100644 --- a/cmd/diff/renderer/types/types.go +++ b/cmd/diff/renderer/types/types.go @@ -212,9 +212,74 @@ func MakeDiffKeyFromResource(res *un.Unstructured) string { // OutputError represents an error in structured output. // Used consistently by both XR diff and comp diff for machine-readable error handling. // Note: Only JSON tags are used because sigs.k8s.io/yaml uses JSON tags for YAML serialization. +// +// ResourceID and ValidationFailures play complementary roles: +// +// - ResourceID identifies the input the diff command was processing +// (the XR or claim file the user fed in). It is "/" +// so machine consumers can correlate an error to a specific +// input across batched runs. +// +// - ValidationFailures, when non-empty, gives a structured +// per-resource breakdown of *what* the schema validator rejected +// within that input's render tree. It contains one entry per +// resource (the input itself plus any composed resource) that +// failed validation, with full GVK / namespace / typed errors, +// so consumers can drive UI or programmatic decisions without +// parsing the human-readable Message. +// +// The two fields therefore partially overlap when the input itself is +// among the failing resources — that's intentional. ValidationFailures +// is the complete failure list (so consumers iterating it never miss +// an XR-level rejection); ResourceID independently anchors the error +// to a user-supplied input. ValidationFailures is set only for schema +// validation errors; it is nil for tool, IO, and render errors. type OutputError struct { - ResourceID string `json:"resourceID,omitempty"` - Message string `json:"message"` + ResourceID string `json:"resourceID,omitempty"` + Message string `json:"message"` + ValidationFailures []ResourceValidationFailure `json:"validationFailures,omitempty"` +} + +// ResourceValidationFailure is the per-resource view inside an +// OutputError.ValidationFailures slice. It mirrors the shape of +// crossplane/cli's pkg/validate.ResourceValidationResult so the +// information transfers cleanly, but is defined here so crossplane-diff's +// JSON output schema is owned by us — consumers depend on this struct, +// not the upstream type. +type ResourceValidationFailure struct { + APIVersion string `json:"apiVersion"` + Kind string `json:"kind"` + Name string `json:"name,omitempty"` + Namespace string `json:"namespace,omitempty"` + // Status is the validator-assigned outcome for this resource. + // Today the surfaced values are "invalid" and "missingSchema"; + // "valid" entries are filtered out by the converter so machine + // consumers iterating ValidationFailures see only the failure rows. + Status string `json:"status"` + Errors []FieldValidationError `json:"errors,omitempty"` +} + +// FieldValidationError is the wire shape for a single field-level +// validation error. Mirrors pkg/validate.FieldValidationError; defined +// here so consumers of our JSON output bind to a stable schema we +// control rather than the upstream type. +type FieldValidationError struct { + // Type categorizes the error: "schema", "cel", "unknownField", + // or "defaulting". + Type string `json:"type"` + // Field is the JSONPath of the offending field (e.g. + // "spec.forProvider.region"), set when the validator can pinpoint + // a path. Empty for errors with no field locality (e.g. + // defaulting failures). + Field string `json:"field,omitempty"` + // Message is the validator-emitted human-readable description. + // For k8s-derived schema errors this typically embeds the field + // path and bad value already. + Message string `json:"message"` + // Value, when set, is the offending value as the validator saw + // it. Type is preserved (string, number, bool, struct), so JSON + // consumers can present or compare it directly. + Value any `json:"value,omitempty"` } // FormatError returns a human-readable error string. diff --git a/cmd/diff/testutils/structured_assertions.go b/cmd/diff/testutils/structured_assertions.go index 81e65301..a4a37031 100644 --- a/cmd/diff/testutils/structured_assertions.go +++ b/cmd/diff/testutils/structured_assertions.go @@ -48,6 +48,7 @@ type DiffExpectation interface { type ExpectedDiff struct { summary *expectedSummary resources []*ResourceExpectation + errors []*ErrorExpectation } func (e *ExpectedDiff) expectation() *ExpectedDiff { return e } @@ -127,6 +128,145 @@ func (e *ExpectedDiff) WithModifiedResource(kind, name, namespace string) *Resou return r } +// ErrorExpectation describes one expected entry in the structured +// errors[] payload. Match is by ResourceID; ValidationFailures, when +// non-nil, asserts the per-resource typed validation breakdown. +type ErrorExpectation struct { + parent *ExpectedDiff + resourceID string + messageContains string + validationFailures []*ValidationFailureExpectation +} + +func (e *ErrorExpectation) expectation() *ExpectedDiff { return e.parent } + +// ValidationFailureExpectation describes one expected entry in +// errors[].validationFailures[]. Match is by APIVersion/Kind/Name; +// Errors asserts on the per-field validation entries within. +type ValidationFailureExpectation struct { + parent *ErrorExpectation + apiVersion string + kind string + name string + namespace string + status string // optional; if "", no assertion + errors []*FieldErrorExpectation // optional; nil = no assertion on per-field errors +} + +func (v *ValidationFailureExpectation) expectation() *ExpectedDiff { return v.parent.parent } + +// FieldErrorExpectation describes one expected entry in +// errors[].validationFailures[].errors[]. Match is by Type+Field +// (Field may be empty for non-localized errors); messageContains and +// value are optional refinements. +type FieldErrorExpectation struct { + parent *ValidationFailureExpectation + errType string + field string + messageContains string + hasValue bool + value any +} + +func (f *FieldErrorExpectation) expectation() *ExpectedDiff { return f.parent.parent.parent } + +// WithError attaches an expectation for an entry in the structured +// errors[] payload, matched by resourceID. Use the returned +// ErrorExpectation to add validationFailures expectations. +func (e *ExpectedDiff) WithError(resourceID string) *ErrorExpectation { + er := &ErrorExpectation{ + parent: e, + resourceID: resourceID, + } + e.errors = append(e.errors, er) + + return er +} + +// WithMessageContaining adds a substring assertion on the error's +// Message field. Optional — useful when machine consumers care about +// the human-readable message too. +func (e *ErrorExpectation) WithMessageContaining(s string) *ErrorExpectation { + e.messageContains = s + return e +} + +// WithValidationFailure adds an expectation for one entry in the +// error's validationFailures[] slice, matched by APIVersion/Kind/Name. +// namespace is optional (empty string skips the namespace assertion); +// pass the actual namespace to lock down namespaced resources. +func (e *ErrorExpectation) WithValidationFailure(apiVersion, kind, name, namespace string) *ValidationFailureExpectation { + v := &ValidationFailureExpectation{ + parent: e, + apiVersion: apiVersion, + kind: kind, + name: name, + namespace: namespace, + } + e.validationFailures = append(e.validationFailures, v) + + return v +} + +// AndError returns to the parent ExpectedDiff to chain another +// WithError call, mirroring the AndXR / AndComp helpers elsewhere in +// this file. +func (e *ErrorExpectation) AndError() *ExpectedDiff { + return e.parent +} + +// WithStatus pins the validation status on this resource entry +// ("invalid", "missingSchema"). Optional; empty status skips the +// assertion. +func (v *ValidationFailureExpectation) WithStatus(status string) *ValidationFailureExpectation { + v.status = status + return v +} + +// WithFieldError adds an expectation for one entry in the failure's +// errors[] slice, matched by errType ("schema", "cel", +// "unknownField", "defaulting") and field (empty matches +// non-localized errors like defaulting). +func (v *ValidationFailureExpectation) WithFieldError(errType, field string) *FieldErrorExpectation { + f := &FieldErrorExpectation{ + parent: v, + errType: errType, + field: field, + } + v.errors = append(v.errors, f) + + return f +} + +// AndValidationFailure returns to the parent ErrorExpectation to chain +// another WithValidationFailure. +func (v *ValidationFailureExpectation) AndValidationFailure() *ErrorExpectation { + return v.parent +} + +// WithMessageContaining adds a substring assertion on this field +// error's Message. Optional. +func (f *FieldErrorExpectation) WithMessageContaining(s string) *FieldErrorExpectation { + f.messageContains = s + return f +} + +// WithValue pins the bad value on this field error. Pass the value as +// it would arrive after JSON decoding (e.g. float64 for numbers, +// string for strings); for "any number is fine" omit this method. +func (f *FieldErrorExpectation) WithValue(v any) *FieldErrorExpectation { + f.hasValue = true + f.value = v + + return f +} + +// AndFieldError returns to the parent ValidationFailureExpectation to +// chain another WithFieldError. +func (f *FieldErrorExpectation) AndFieldError() *ValidationFailureExpectation { + return f.parent +} + // WithRemovedResource adds an expectation for a removed resource. func (e *ExpectedDiff) WithRemovedResource(kind, name, namespace string) *ResourceExpectation { r := &ResourceExpectation{ @@ -311,6 +451,146 @@ func AssertStructuredDiff(t *testing.T, jsonOutput string, e DiffExpectation) { if len(expected.resources) > 0 && len(output.Changes) != len(expected.resources) { t.Errorf("Expected %d changes, got %d", len(expected.resources), len(output.Changes)) } + + // Check each error expectation against output.Errors. + for _, want := range expected.errors { + got := findMatchingError(output.Errors, want.resourceID) + if got == nil { + actualIDs := make([]string, 0, len(output.Errors)) + for _, e := range output.Errors { + actualIDs = append(actualIDs, e.ResourceID) + } + + t.Errorf("Expected error for resourceID %q not found in output. Actual error IDs: %v", + want.resourceID, actualIDs) + + continue + } + + assertErrorMatch(t, *got, want) + } + + // If the test pinned a specific number of error expectations, verify + // no extras were emitted. Zero expectations = no opinion on errors. + if len(expected.errors) > 0 && len(output.Errors) != len(expected.errors) { + t.Errorf("Expected %d errors in structured output, got %d", len(expected.errors), len(output.Errors)) + } +} + +// assertErrorMatch validates a single OutputError against its +// expectation. Split out from AssertStructuredDiff so the per-error +// branching doesn't push that function further over the gocognit +// threshold its existing exemption already silences. +func assertErrorMatch(t *testing.T, got OutputError, want *ErrorExpectation) { + t.Helper() + + if want.messageContains != "" && !strings.Contains(got.Message, want.messageContains) { + t.Errorf("error %q: message %q does not contain %q", + want.resourceID, got.Message, want.messageContains) + } + + for _, wantVF := range want.validationFailures { + gotVF := findMatchingValidationFailure(got.ValidationFailures, wantVF) + if gotVF == nil { + actual := make([]string, 0, len(got.ValidationFailures)) + for _, vf := range got.ValidationFailures { + actual = append(actual, fmt.Sprintf("%s/%s %s/%s", vf.APIVersion, vf.Kind, vf.Namespace, vf.Name)) + } + + t.Errorf("error %q: expected validationFailure %s/%s %s/%s not found. Actual: %v", + want.resourceID, wantVF.apiVersion, wantVF.kind, wantVF.namespace, wantVF.name, actual) + + continue + } + + assertValidationFailureMatch(t, *gotVF, wantVF) + } + + if len(want.validationFailures) > 0 && len(got.ValidationFailures) != len(want.validationFailures) { + t.Errorf("error %q: expected %d validationFailures, got %d", + want.resourceID, len(want.validationFailures), len(got.ValidationFailures)) + } +} + +// assertValidationFailureMatch validates a single +// ResourceValidationFailure against its expectation. +func assertValidationFailureMatch(t *testing.T, got ResourceValidationFailure, want *ValidationFailureExpectation) { + t.Helper() + + if want.status != "" && got.Status != want.status { + t.Errorf("validationFailure %s/%s %s: status: expected %q, got %q", + want.apiVersion, want.kind, want.name, want.status, got.Status) + } + + for _, wantFE := range want.errors { + gotFE := findMatchingFieldError(got.Errors, wantFE) + if gotFE == nil { + actual := make([]string, 0, len(got.Errors)) + for _, fe := range got.Errors { + actual = append(actual, fmt.Sprintf("[%s] %s", fe.Type, fe.Field)) + } + + t.Errorf("validationFailure %s/%s %s: expected fieldError [%s] %s not found. Actual: %v", + want.apiVersion, want.kind, want.name, wantFE.errType, wantFE.field, actual) + + continue + } + + if wantFE.messageContains != "" && !strings.Contains(gotFE.Message, wantFE.messageContains) { + t.Errorf("validationFailure %s/%s %s: fieldError [%s] %s: message %q does not contain %q", + want.apiVersion, want.kind, want.name, wantFE.errType, wantFE.field, + gotFE.Message, wantFE.messageContains) + } + + if wantFE.hasValue && !reflect.DeepEqual(gotFE.Value, wantFE.value) { + t.Errorf("validationFailure %s/%s %s: fieldError [%s] %s: value: expected %v (%T), got %v (%T)", + want.apiVersion, want.kind, want.name, wantFE.errType, wantFE.field, + wantFE.value, wantFE.value, gotFE.Value, gotFE.Value) + } + } + + if len(want.errors) > 0 && len(got.Errors) != len(want.errors) { + t.Errorf("validationFailure %s/%s %s: expected %d field errors, got %d", + want.apiVersion, want.kind, want.name, len(want.errors), len(got.Errors)) + } +} + +func findMatchingError(errs []OutputError, resourceID string) *OutputError { + for i := range errs { + if errs[i].ResourceID == resourceID { + return &errs[i] + } + } + + return nil +} + +func findMatchingValidationFailure(vfs []ResourceValidationFailure, want *ValidationFailureExpectation) *ResourceValidationFailure { + for i := range vfs { + v := &vfs[i] + if v.APIVersion != want.apiVersion || v.Kind != want.kind || v.Name != want.name { + continue + } + + if want.namespace != "" && v.Namespace != want.namespace { + continue + } + + return v + } + + return nil +} + +func findMatchingFieldError(fes []FieldValidationError, want *FieldErrorExpectation) *FieldValidationError { + for i := range fes { + fe := &fes[i] + if fe.Type == want.errType && fe.Field == want.field { + return fe + } + } + + return nil } // findMatchingChange finds a change that matches the resource expectation. @@ -449,8 +729,27 @@ type StructuredCompDiffOutput struct { // OutputError mirrors dt.OutputError. type OutputError struct { - ResourceID string `json:"resourceID,omitempty"` - Message string `json:"message"` + ResourceID string `json:"resourceID,omitempty"` + Message string `json:"message"` + ValidationFailures []ResourceValidationFailure `json:"validationFailures,omitempty"` +} + +// ResourceValidationFailure mirrors dt.ResourceValidationFailure. +type ResourceValidationFailure struct { + APIVersion string `json:"apiVersion"` + Kind string `json:"kind"` + Name string `json:"name,omitempty"` + Namespace string `json:"namespace,omitempty"` + Status string `json:"status"` + Errors []FieldValidationError `json:"errors,omitempty"` +} + +// FieldValidationError mirrors dt.FieldValidationError. +type FieldValidationError struct { + Type string `json:"type"` + Field string `json:"field,omitempty"` + Message string `json:"message"` + Value any `json:"value,omitempty"` } // CompositionDiffJSON mirrors compositionDiffJSON from the renderer. From 1a82016b8141a34658cca568937ab6d28cdf16c3 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 16 Jun 2026 11:09:08 -0400 Subject: [PATCH 5/7] test(validate): adopt project's reason-field convention; address review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes, all in response to review feedback on PR #347: - Adopt the established reason-field test convention (used in diff_integration_test and the client/crossplane test suites) for the new TestFormatValidationErrors, TestNewOutputError, and TestValidationFailuresFromResult cases. Inline `// comment` blocks move into a `reason string` table-struct field that surfaces in failure messages, so a failing case prints both what went wrong and why the case exists. - Match the bad-value duplication check to k8s emit conventions: string values get %q (quoted) for both display and the Contains-search, so an unquoted incidental substring (e.g. Value="k" against message "spec.kind: Required value") no longer suppresses the "(got )" tail. Numbers / bools / structs keep %v unchanged. New regression cases: BadValueNotSuppressedByIncidentalSubstring and NumericBadValueRendersUnquoted. - Drop the misleading preloadedCRDs entry in the TestDefaultSchemaValidator_ValidateResources ValidationError case. The field is dead state — `tt.preloadedCRDs` is never read in any test body in this file; CRDs flow through the mock SchemaClient set up by setupClients. Removing the wider unused field is out-of-scope for this PR (touches many unrelated tests), so just empty this entry to stop implying it influences validation behavior. Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/errors_test.go | 39 ++++------ cmd/diff/diffprocessor/schema_validator.go | 41 +++++++--- .../diffprocessor/schema_validator_test.go | 77 ++++++++++++------- 3 files changed, 95 insertions(+), 62 deletions(-) diff --git a/cmd/diff/diffprocessor/errors_test.go b/cmd/diff/diffprocessor/errors_test.go index 60a3e7f4..5ac964a4 100644 --- a/cmd/diff/diffprocessor/errors_test.go +++ b/cmd/diff/diffprocessor/errors_test.go @@ -161,13 +161,13 @@ func TestSchemaValidationError_WithResult(t *testing.T) { func TestNewOutputError(t *testing.T) { tests := map[string]struct { + reason string resourceID string err error want dt.OutputError }{ "NonValidationError_NoFailuresAttached": { - // A plain error has no structured slot to populate, so - // ValidationFailures stays nil. + reason: "A plain error has no structured slot to populate; ValidationFailures stays nil and only Message is set.", resourceID: "XR/my-xr", err: errors.New("kube unreachable"), want: dt.OutputError{ @@ -176,8 +176,7 @@ func TestNewOutputError(t *testing.T) { }, }, "SchemaValidationError_WithoutResult_NoFailuresAttached": { - // A SchemaValidationError that wasn't given a Result (e.g., - // scope-validation failure path) only contributes Message. + reason: "A SchemaValidationError without a Result (e.g. scope-validation path) contributes only Message; the typed slot stays nil so consumers can distinguish 'no failures' from 'no structured detail available'.", resourceID: "XR/my-xr", err: NewSchemaValidationError("", "scope failure", errors.New("inner")), want: dt.OutputError{ @@ -186,6 +185,7 @@ func TestNewOutputError(t *testing.T) { }, }, "SchemaValidationError_WithResult_ExposesTypedFailures": { + reason: "A SchemaValidationError carrying a Result populates ValidationFailures with the converted per-resource breakdown.", resourceID: "XR/my-xr", err: NewSchemaValidationError("", "msg", errors.New("inner")).WithResult( &pkgvalidate.ValidationResult{ @@ -218,8 +218,7 @@ func TestNewOutputError(t *testing.T) { }, }, "WrappedSchemaValidationError_StillSurfacesFailures": { - // errors.As walks the chain, so a SchemaValidationError - // hidden behind errors.Wrap should still surface its Result. + reason: "errors.As walks the chain, so a SchemaValidationError behind errors.Wrap still has its Result extracted into ValidationFailures.", resourceID: "XR/my-xr", err: xperrors.Wrap( NewSchemaValidationError("", "msg", errors.New("inner")).WithResult( @@ -249,7 +248,7 @@ func TestNewOutputError(t *testing.T) { t.Run(name, func(t *testing.T) { got := NewOutputError(tc.resourceID, tc.err) if diff := gcmp.Diff(tc.want, got); diff != "" { - t.Errorf("NewOutputError() mismatch (-want +got):\n%s", diff) + t.Errorf("\n%s\nNewOutputError() mismatch (-want +got):\n%s", tc.reason, diff) } }) } @@ -257,16 +256,17 @@ func TestNewOutputError(t *testing.T) { func TestValidationFailuresFromResult(t *testing.T) { tests := map[string]struct { + reason string result *pkgvalidate.ValidationResult want []dt.ResourceValidationFailure }{ "NilResultReturnsNil": { + reason: "A nil ValidationResult yields nil; the converter never invents structure.", result: nil, want: nil, }, "OnlyValidResourcesReturnsNil": { - // Valid status entries are filtered: ValidationFailures is - // "what failed", not the full audit log. + reason: "ValidationFailures lists 'what failed', not the full audit log; valid resources are filtered out.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -278,9 +278,7 @@ func TestValidationFailuresFromResult(t *testing.T) { want: nil, }, "DefaultingFailedFiltered": { - // pkgvalidate.ResultError treats DefaultingFailed as - // success, so a SchemaValidationError that reaches us - // shouldn't advertise these resources as failures. + reason: "pkgvalidate.ResultError treats DefaultingFailed as success, so a SchemaValidationError reaching us shouldn't advertise these resources as failures via ValidationFailures.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -296,9 +294,7 @@ func TestValidationFailuresFromResult(t *testing.T) { want: nil, }, "InvalidWithMixedErrorsSuppressesDefaulting": { - // Mirror the human formatter: when actionable errors are - // present, defaulting entries are dropped so the typed and - // rendered views agree on the failure detail. + reason: "Mirrors formatValidationErrors: when actionable errors are present, defaulting entries are dropped so the typed and rendered views agree on the failure detail.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -326,10 +322,7 @@ func TestValidationFailuresFromResult(t *testing.T) { }}, }, "InvalidWithOnlyDefaultingErrorsKeepsThem": { - // Defensive: if a future upstream change ever produced an - // Invalid resource whose Errors are all defaulting, we'd - // rather emit the entries than produce an invalid resource - // row with no errors. + reason: "Defensive: upstream's statusFromErrors won't produce Invalid+only-defaulting today, but if it ever did we surface the defaulting entries rather than emit an Invalid row with no errors.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -354,6 +347,7 @@ func TestValidationFailuresFromResult(t *testing.T) { }}, }, "MissingSchemaSurfacedWithoutErrors": { + reason: "Missing-schema resources surface with their GVK / name / namespace and Status, but no errors[] (the validator never ran).", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "other.org/v1", @@ -370,9 +364,7 @@ func TestValidationFailuresFromResult(t *testing.T) { }}, }, "BadValuePropagated": { - // The structured Value should travel through unchanged, - // preserving its Go type (here: a string), so JSON output - // renders it without forcing callers to parse it back. + reason: "FieldValidationError.Value travels through unchanged, preserving its Go type, so JSON output renders it without forcing callers to parse it back.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -401,6 +393,7 @@ func TestValidationFailuresFromResult(t *testing.T) { }}, }, "MultipleResourcesPreserveOrder": { + reason: "The output preserves input order across resources and skips valid ones, so consumers can rely on the position correspondence between input resources and emitted failures.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{ { @@ -452,7 +445,7 @@ func TestValidationFailuresFromResult(t *testing.T) { t.Run(name, func(t *testing.T) { got := validationFailuresFromResult(tc.result) if diff := gcmp.Diff(tc.want, got); diff != "" { - t.Errorf("validationFailuresFromResult() mismatch (-want +got):\n%s", diff) + t.Errorf("\n%s\nvalidationFailuresFromResult() mismatch (-want +got):\n%s", tc.reason, diff) } }) } diff --git a/cmd/diff/diffprocessor/schema_validator.go b/cmd/diff/diffprocessor/schema_validator.go index 4dfdd571..2bd75b7a 100644 --- a/cmd/diff/diffprocessor/schema_validator.go +++ b/cmd/diff/diffprocessor/schema_validator.go @@ -405,26 +405,45 @@ func formatInvalidBlock(r pkgvalidate.ResourceValidationResult) string { // formatErrorLine renders one FieldValidationError as // "[ (got )] []". The bad value tail is omitted -// when Value is nil or already substring-present in the message, so -// k8s-derived errors (which embed the value in their text) don't -// produce duplicate output. +// when Value is nil or already present in the message in the form +// k8s validators emit it (quoted for strings, unquoted for numbers / +// bools / structs). Matching the emit shape avoids both duplication +// and the false-positive that an unquoted substring search would +// produce — e.g. Value="k" against message "spec.kind: Required" must +// not suppress the tail just because "k" appears inside "kind". func formatErrorLine(e pkgvalidate.FieldValidationError) string { msg := e.Message - if rendered := renderBadValue(e.Value); rendered != "" && !strings.Contains(msg, rendered) { + if rendered, found := renderBadValue(e.Value); rendered != "" && !strings.Contains(msg, found) { msg = fmt.Sprintf("%s (got %s)", msg, rendered) } return fmt.Sprintf("%s [%s]", msg, e.Type) } -// renderBadValue formats a FieldValidationError.Value for display. -// Returns the empty string for nil so callers can use it as a presence -// check; non-empty results are suitable for direct concatenation into -// an error message. -func renderBadValue(value any) string { +// renderBadValue formats a FieldValidationError.Value for display and +// returns the substring to look for in the message when deciding +// whether the value is already embedded. +// +// Strings get %q (quoted) for both purposes: k8s validation messages +// embed string bad values as `Invalid value: "five"`, so quoting is +// what the user expects to read AND is the form to search for. Other +// types use %v unchanged: numbers and booleans appear unquoted in +// k8s messages and read naturally in our display. +// +// Returns the empty rendered string for a nil Value so callers can +// use it as a presence check; the returned `found` mirrors `rendered` +// in that case. +func renderBadValue(value any) (rendered, found string) { if value == nil { - return "" + return "", "" } - return fmt.Sprintf("%v", value) + if s, ok := value.(string); ok { + quoted := fmt.Sprintf("%q", s) + return quoted, quoted + } + + rendered = fmt.Sprintf("%v", value) + + return rendered, rendered } diff --git a/cmd/diff/diffprocessor/schema_validator_test.go b/cmd/diff/diffprocessor/schema_validator_test.go index 1a2984f9..4f2979f5 100644 --- a/cmd/diff/diffprocessor/schema_validator_test.go +++ b/cmd/diff/diffprocessor/schema_validator_test.go @@ -161,7 +161,7 @@ func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { WithSpecField("field", int64(123)). Build(), composed: []cpd.Unstructured{*composedResource1, *composedResource2}, - preloadedCRDs: []*extv1.CustomResourceDefinition{createCRDWithStringField(xrCRD)}, + preloadedCRDs: []*extv1.CustomResourceDefinition{}, expectedErr: true, expectedErrMsg: "missing schema", }, @@ -537,10 +537,12 @@ func TestDefaultSchemaValidator_ValidateResources_AppliesDefaults(t *testing.T) func TestFormatValidationErrors(t *testing.T) { tests := map[string]struct { + reason string result *pkgvalidate.ValidationResult expected string }{ "SingleSchemaError": { + reason: "A single field-level schema error renders as ' :' header plus an indented ' [schema]' line.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -557,6 +559,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "example.org/v1/XR my-xr:\n spec.region: Required value [schema]", }, "SingleMissingSchema": { + reason: "A missing-schema resource collapses to a single ' : missing schema' line (no errors[] to expand).", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "other.org/v1", @@ -568,9 +571,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "other.org/v1/SomeResource thing: missing schema", }, "MissingSchemaWithoutName": { - // We may not always have a name (e.g. when the user feeds a - // resource without metadata.name). Header collapses to just - // the GVK rather than producing a trailing space. + reason: "A resource without metadata.name collapses to just the GVK in the header rather than producing a trailing space.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "other.org/v1", @@ -581,6 +582,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "other.org/v1/SomeResource: missing schema", }, "NamespacedResource": { + reason: "Namespaced resources render '/' in the header so cluster-vs-namespaced scope is visible.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -597,8 +599,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "example.org/v1/Thing production/my-thing:\n spec.size: Required value [schema]", }, "MultipleErrorsGroupedUnderResource": { - // Two errors on the same resource should be grouped under a - // single header, not produce a header per error. + reason: "Multiple errors on the same resource share a single header rather than producing a header per error.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -614,9 +615,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "example.org/v1/XR my-xr:\n spec.region: Required value [schema]\n spec.replicas: must be > 0 [cel]", }, "BadValueAppendedWhenAbsentFromMessage": { - // CEL-style errors don't always embed the bad value into - // the message; the structured Value should be rendered as - // "(got )" so users can see what was rejected. + reason: "When Value is set and the message doesn't already include it (typical for CEL errors), '(got )' is appended so users see what was rejected.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -633,9 +632,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "example.org/v1/XR my-xr:\n spec.replicas: must be > 0 (got -1) [cel]", }, "BadValueOmittedWhenAlreadyInMessage": { - // k8s field errors typically embed the bad value as a - // quoted string in the message text. Avoid duplicating it - // when the message already contains the rendered form. + reason: "k8s field errors typically embed the bad value as a quoted string in the message text; the '(got )' tail is omitted to avoid duplication.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -651,7 +648,42 @@ func TestFormatValidationErrors(t *testing.T) { }, expected: `example.org/v1/XR my-xr:` + "\n" + ` spec.replicas: Invalid value: "five": must be integer [schema]`, }, + "BadValueNotSuppressedByIncidentalSubstring": { + reason: "Regression: the duplication check matches the quoted form for strings (\"k\") so an unquoted incidental substring (\"k\" inside \"kind\") doesn't suppress the tail.", + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeCEL, + Message: "spec.kind: Required value", + Value: "k", + }}, + }}, + }, + expected: `example.org/v1/XR my-xr:` + "\n" + ` spec.kind: Required value (got "k") [cel]`, + }, + "NumericBadValueRendersUnquoted": { + reason: "Numbers / bools / structs aren't quoted in k8s messages and shouldn't be quoted in our display either.", + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeCEL, + Message: "spec.replicas: must be > 0", + Value: 42, + }}, + }}, + }, + expected: "example.org/v1/XR my-xr:\n spec.replicas: must be > 0 (got 42) [cel]", + }, "MultipleResourcesEachOnTheirOwnBlock": { + reason: "Failures from distinct resources produce distinct blocks joined by newlines, in input order.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{ { @@ -675,8 +707,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "example.org/v1/XR my-xr:\n spec.region: Required value [schema]\nother.org/v1/SomeResource thing: missing schema", }, "ValidResourcesProduceNoBlock": { - // A resource that passed validation shouldn't appear in the - // output at all; only the failing resource does. + reason: "Only failing resources contribute blocks; valid resources are filtered out so the output is the failure summary, not an audit log.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{ { @@ -700,10 +731,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "example.org/v1/XR bad-xr:\n spec.field: Invalid value [schema]", }, "NoApplicableEntriesReturnsEmpty": { - // formatValidationErrors is only invoked after ResultError - // has flagged a failure, so an empty/all-valid input is - // unreachable in production. We pin the contract anyway: - // no entries → empty string, not a misleading fallback. + reason: "formatValidationErrors is only invoked after ResultError has flagged a failure; an all-valid input is unreachable in production. The contract is empty string (not a generic fallback) so the wrapping ResultError carries the message.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -715,11 +743,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "", }, "DefaultingErrorAccompanyingSchemaError": { - // A defaulting failure that co-occurs with a schema-class - // error must not produce a separate entry: the schema entry - // already conveys the failure, and dropping the defaulting - // line keeps the error block focused on the actionable - // problem. + reason: "A defaulting entry that co-occurs with a schema-class error is suppressed: the schema entry already conveys the failure, and the defaulting line would just be noise.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -735,10 +759,7 @@ func TestFormatValidationErrors(t *testing.T) { expected: "example.org/v1/XR my-xr:\n spec.field: Required value [schema]", }, "InvalidWithOnlyDefaultingErrorsStillEmitted": { - // Defensive: upstream's statusFromErrors today won't return - // Invalid for a resource whose Errors are all defaulting, - // but if that ever changed we'd rather surface the - // defaulting message than emit an empty block. + reason: "Defensive: upstream's statusFromErrors won't produce Invalid+only-defaulting today, but if it ever did we surface the defaulting message rather than emit an empty block.", result: &pkgvalidate.ValidationResult{ Resources: []pkgvalidate.ResourceValidationResult{{ APIVersion: "example.org/v1", @@ -758,7 +779,7 @@ func TestFormatValidationErrors(t *testing.T) { t.Run(name, func(t *testing.T) { got := formatValidationErrors(tt.result) if got != tt.expected { - t.Errorf("formatValidationErrors() = %q, want %q", got, tt.expected) + t.Errorf("\n%s\nformatValidationErrors() = %q, want %q", tt.reason, got, tt.expected) } }) } From cb43f61269c16a9b5d14f8b23c8ded9d2df993ae Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 16 Jun 2026 11:27:06 -0400 Subject: [PATCH 6/7] test(validate): drop dead preloadedCRDs state and orphaned helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preloadedCRDs field was historical residue: a previous refactor moved CRD plumbing through a mock SchemaClient (set up by each test case's setupClients / setupClient closure) but left the original preloadedCRDs slice on three test struct types. The field was never read; every assignment went straight to the GC. Comments like "// No longer needed" at several call sites confirmed the previous author knew the data was unused but stopped short of removing the field. Sweep the field from all three table-driven test functions in cmd/diff/diffprocessor/schema_validator_test.go: - TestDefaultSchemaValidator_ValidateResources (4 cases) - TestDefaultSchemaValidator_LoadCRDs (1 case) - TestDefaultSchemaValidator_ValidateScopeConstraints (8 cases) Removing the field exposed two more pieces of dead state in the same file: - The ValidationError closure in TestDefaultSchemaValidator_ValidateResources had a composedCRDUn block that built an unstructured via runtime.DefaultUnstructuredConverter and discarded the result. Drop the block; with no remaining caller, MustToUnstructured and the runtime import go with it. - The const testComposedResource = "ComposedResource" was defined but never referenced — every call site uses the literal string "testComposedResource" (matching the const's name, not its value). Drop the const. No behavior change; all unit tests still pass under earthly -P +reviewable. Signed-off-by: Jonathan Ogilvie --- .../diffprocessor/schema_validator_test.go | 55 ++++--------------- 1 file changed, 10 insertions(+), 45 deletions(-) diff --git a/cmd/diff/diffprocessor/schema_validator_test.go b/cmd/diff/diffprocessor/schema_validator_test.go index 4f2979f5..e06d5526 100644 --- a/cmd/diff/diffprocessor/schema_validator_test.go +++ b/cmd/diff/diffprocessor/schema_validator_test.go @@ -11,7 +11,6 @@ import ( "github.com/google/go-cmp/cmp" extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" @@ -21,9 +20,8 @@ import ( var _ SchemaValidator = (*tu.MockSchemaValidator)(nil) const ( - testExampleOrg = "example.org" - testComposedResource = "ComposedResource" - testCpdOrg = "cpd.org" + testExampleOrg = "example.org" + testCpdOrg = "cpd.org" ) func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { @@ -57,7 +55,6 @@ func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { setupClients func() (*tu.MockSchemaClient, *tu.MockDefinitionClient) xr *un.Unstructured composed []cpd.Unstructured - preloadedCRDs []*extv1.CustomResourceDefinition expectedErr bool expectedErrMsg string }{ @@ -74,10 +71,9 @@ func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { return sch, tu.NewMockDefinitionClient().Build() }, - xr: xr, - composed: []cpd.Unstructured{*composedResource1, *composedResource2}, - preloadedCRDs: []*extv1.CustomResourceDefinition{}, // No longer needed - expectedErr: false, + xr: xr, + composed: []cpd.Unstructured{*composedResource1, *composedResource2}, + expectedErr: false, }, "SuccessfulValidationWithFetchedCRDs": { setupClients: func() (*tu.MockSchemaClient, *tu.MockDefinitionClient) { @@ -97,10 +93,9 @@ func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { return sch, def }, - xr: xr, - composed: []cpd.Unstructured{*composedResource1, *composedResource2}, - preloadedCRDs: []*extv1.CustomResourceDefinition{}, - expectedErr: false, + xr: xr, + composed: []cpd.Unstructured{*composedResource1, *composedResource2}, + expectedErr: false, }, "MissingCRD": { setupClients: func() (*tu.MockSchemaClient, *tu.MockDefinitionClient) { @@ -119,22 +114,14 @@ func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { return sch, def }, - xr: xr, - composed: []cpd.Unstructured{*composedResource1, *composedResource2}, - preloadedCRDs: []*extv1.CustomResourceDefinition{}, + xr: xr, + composed: []cpd.Unstructured{*composedResource1, *composedResource2}, // Now we expect an error because we've configured it to require a CRD but can't find it expectedErr: true, expectedErrMsg: "unable to find CRDs for", }, "ValidationError": { setupClients: func() (*tu.MockSchemaClient, *tu.MockDefinitionClient) { - // Convert CRDs to un for the mock - composedCRDUn := &un.Unstructured{} - _ = runtime.DefaultUnstructuredConverter.FromUnstructured( - MustToUnstructured(createCRDWithStringField(composedCRD)), - composedCRDUn, - ) - sch := tu.NewMockSchemaClient(). // Add GetCRD implementation for typed CRDs WithGetCRD(func(_ context.Context, gvk schema.GroupVersionKind) (*extv1.CustomResourceDefinition, error) { @@ -161,7 +148,6 @@ func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { WithSpecField("field", int64(123)). Build(), composed: []cpd.Unstructured{*composedResource1, *composedResource2}, - preloadedCRDs: []*extv1.CustomResourceDefinition{}, expectedErr: true, expectedErrMsg: "missing schema", }, @@ -297,7 +283,6 @@ func TestDefaultSchemaValidator_LoadCRDs(t *testing.T) { tests := map[string]struct { setupClient func() xp.DefinitionClient - preloadedCRDs []*extv1.CustomResourceDefinition expectedErr bool expectedErrMsg string // for caching tests @@ -329,7 +314,6 @@ func TestDefaultSchemaValidator_LoadCRDs(t *testing.T) { Build(), } }, - preloadedCRDs: nil, // No preloaded CRDs expectedErr: false, callTwice: true, // Make two calls to LoadCRDs expectXRDCalls: 1, // GetXRDs should only be called once due to caching @@ -392,16 +376,6 @@ func createCRDWithStringField(baseCRD *extv1.CustomResourceDefinition) *extv1.Cu return crd } -// Helper function to convert to un. -func MustToUnstructured(obj any) map[string]any { - u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) - if err != nil { - panic(err) - } - - return u -} - // Helper type to track GetXRDs calls. type xrdCountingClient struct { tu.MockDefinitionClient @@ -797,7 +771,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { tests := map[string]struct { setupClient func() *tu.MockSchemaClient - preloadedCRDs []*extv1.CustomResourceDefinition resource *un.Unstructured expectedNamespace string isClaimRoot bool @@ -810,7 +783,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { WithFoundCRD(testExampleOrg, "NamespacedResource", namespacedCRD). Build() }, - preloadedCRDs: []*extv1.CustomResourceDefinition{}, // No longer needed resource: tu.NewResource(testExampleOrg+"/v1", "NamespacedResource", "test-resource"). InNamespace("default"). Build(), @@ -824,7 +796,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { WithFoundCRD(testExampleOrg, "NamespacedResource", namespacedCRD). Build() }, - preloadedCRDs: []*extv1.CustomResourceDefinition{namespacedCRD}, resource: tu.NewResource(testExampleOrg+"/v1", "NamespacedResource", "test-resource"). Build(), // No namespace expectedNamespace: "default", @@ -838,7 +809,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { WithFoundCRD(testExampleOrg, "NamespacedResource", namespacedCRD). Build() }, - preloadedCRDs: []*extv1.CustomResourceDefinition{namespacedCRD}, resource: tu.NewResource(testExampleOrg+"/v1", "NamespacedResource", "test-resource"). InNamespace("wrong"). Build(), @@ -853,7 +823,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { WithFoundCRD(testExampleOrg, "ClusterResource", clusterCRD). Build() }, - preloadedCRDs: []*extv1.CustomResourceDefinition{clusterCRD}, resource: tu.NewResource(testExampleOrg+"/v1", "ClusterResource", "test-resource"). Build(), // No namespace - correct for cluster-scoped expectedNamespace: "", @@ -866,7 +835,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { WithFoundCRD(testExampleOrg, "ClusterResource", clusterCRD). Build() }, - preloadedCRDs: []*extv1.CustomResourceDefinition{clusterCRD}, resource: tu.NewResource(testExampleOrg+"/v1", "ClusterResource", "test-resource"). InNamespace("default"). Build(), @@ -881,7 +849,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { WithFoundCRD(testExampleOrg, "ClusterResource", clusterCRD). Build() }, - preloadedCRDs: []*extv1.CustomResourceDefinition{clusterCRD}, resource: tu.NewResource(testExampleOrg+"/v1", "ClusterResource", "test-resource"). Build(), expectedNamespace: "default", // XR is namespaced @@ -895,7 +862,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { WithFoundCRD(testExampleOrg, "ClusterResource", clusterCRD). Build() }, - preloadedCRDs: []*extv1.CustomResourceDefinition{clusterCRD}, resource: tu.NewResource(testExampleOrg+"/v1", "ClusterResource", "test-resource"). Build(), expectedNamespace: "default", // Claim is namespaced @@ -910,7 +876,6 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { }). Build() }, - preloadedCRDs: []*extv1.CustomResourceDefinition{}, resource: tu.NewResource(testExampleOrg+"/v1", "UnknownResource", "test-resource"). Build(), expectedNamespace: "default", From 3b7936b4fddd0af6f4f9a9fee609d52fa5dc9903 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 16 Jun 2026 18:02:24 -0400 Subject: [PATCH 7/7] fix(validate): address final round of review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues caught by Copilot on the rebased branch: 1. SchemaValidate setup-error path returned a generic errors.Wrap rather than a SchemaValidationError, so IsSchemaValidationError / DetermineExitCode would classify it as ExitCodeToolError (1) instead of ExitCodeSchemaValidation (2). Pre-refactor, the old validate.SchemaValidation entry point produced a SchemaValidationError on every error path; the structured-API split inadvertently broke that contract. Restore it: setup errors are wrapped via NewSchemaValidationError(...) with no Result attached (the validator never produced one). 2. resourceHeader produced " /" with a stray trailing slash when a resource had metadata.namespace set but no metadata.name. The empty-name guard checked the *computed* name string ("ns/") rather than r.Name. Gate on r.Name directly so an empty name collapses to GVK-only regardless of namespace, matching the intent already documented for the empty-namespace case. Regression case: NamespaceWithoutNameCollapsesToGVK. 3. Non-string Value duplicate check used a raw strings.Contains, so Value=42 against message containing "420" would suppress the "(got 42)" tail incorrectly — same class of false-positive Copilot caught earlier for unquoted strings, just on the numeric path. Split the duplicate check out as valueAlreadyInMessage: strings still use the quoted-form substring search (delimiter-safe via the surrounding %q quotes), non-strings use a word-boundary regex so 42 doesn't match inside 420. Two new regression cases: NumericBadValueNotSuppressedByDigitPrefix (Value=42 vs "420" — tail correctly rendered) and NumericBadValueSuppressedAsToken (Value=42 vs "Invalid value: 42:" — tail correctly suppressed because 42 appears as a standalone token). Signed-off-by: Jonathan Ogilvie --- cmd/diff/diffprocessor/schema_validator.go | 93 ++++++++++++------- .../diffprocessor/schema_validator_test.go | 46 +++++++++ 2 files changed, 108 insertions(+), 31 deletions(-) diff --git a/cmd/diff/diffprocessor/schema_validator.go b/cmd/diff/diffprocessor/schema_validator.go index 2bd75b7a..4825e053 100644 --- a/cmd/diff/diffprocessor/schema_validator.go +++ b/cmd/diff/diffprocessor/schema_validator.go @@ -3,6 +3,7 @@ package diffprocessor import ( "context" "fmt" + "regexp" "strings" xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" @@ -125,7 +126,15 @@ func (v *DefaultSchemaValidator) ValidateResources(ctx context.Context, xr *un.U result, err := pkgvalidate.SchemaValidate(ctx, resources, v.schemaClient.GetAllCRDs()) if err != nil { - return errors.Wrap(err, "schema validation failed") + // SchemaValidate's error return is reserved for setup + // failures (e.g. a CRD that can't be compiled into a + // validator). Wrap it as a SchemaValidationError so + // IsSchemaValidationError / DetermineExitCode classify it the + // same way the pre-refactor SchemaValidation entry point + // did — exit code 2 (schema validation), not 1 (tool error). + // No Result is attached because the validator never produced + // one. + return NewSchemaValidationError("", "schema validation setup failed", err) } v.logResultDetails(result) @@ -349,17 +358,21 @@ func formatValidationErrors(result *pkgvalidate.ValidationResult) string { // resourceHeader formats the per-resource header used by both the // missing-schema and invalid blocks. Cluster-scoped resources omit the -// namespace prefix; namespaced resources produce "/". +// namespace prefix; namespaced resources produce "/". A +// resource without metadata.name collapses to just the GVK regardless +// of namespace — the namespace alone isn't a useful identifier for a +// nameless resource and emitting "/" would leave a stray trailing +// slash. func resourceHeader(r pkgvalidate.ResourceValidationResult) string { + if r.Name == "" { + return fmt.Sprintf("%s/%s", r.APIVersion, r.Kind) + } + name := r.Name if r.Namespace != "" { name = r.Namespace + "/" + r.Name } - if name == "" { - return fmt.Sprintf("%s/%s", r.APIVersion, r.Kind) - } - return fmt.Sprintf("%s/%s %s", r.APIVersion, r.Kind, name) } @@ -405,45 +418,63 @@ func formatInvalidBlock(r pkgvalidate.ResourceValidationResult) string { // formatErrorLine renders one FieldValidationError as // "[ (got )] []". The bad value tail is omitted -// when Value is nil or already present in the message in the form -// k8s validators emit it (quoted for strings, unquoted for numbers / -// bools / structs). Matching the emit shape avoids both duplication -// and the false-positive that an unquoted substring search would -// produce — e.g. Value="k" against message "spec.kind: Required" must -// not suppress the tail just because "k" appears inside "kind". +// when Value is nil or already present in the message. Duplication +// detection is type-aware to avoid false positives: +// +// - String values are checked in their %q (quoted) form, matching +// how k8s validators emit them (`Invalid value: "five"`); the +// surrounding quotes give the search natural delimiters so +// Value="k" against message "spec.kind: Required" doesn't +// suppress the tail just because "k" sits inside "kind". +// +// - Non-string values use a word-boundary regex so Value=42 against +// message "...Invalid value: 420..." doesn't suppress the tail +// just because "42" is a prefix of "420". func formatErrorLine(e pkgvalidate.FieldValidationError) string { msg := e.Message - if rendered, found := renderBadValue(e.Value); rendered != "" && !strings.Contains(msg, found) { + if rendered := renderBadValue(e.Value); rendered != "" && !valueAlreadyInMessage(msg, e.Value, rendered) { msg = fmt.Sprintf("%s (got %s)", msg, rendered) } return fmt.Sprintf("%s [%s]", msg, e.Type) } -// renderBadValue formats a FieldValidationError.Value for display and -// returns the substring to look for in the message when deciding -// whether the value is already embedded. +// valueAlreadyInMessage reports whether the rendered form of a bad +// value is already present in the validator's message text. The check +// shape depends on the value's Go type — see formatErrorLine's +// comment for the rationale. +func valueAlreadyInMessage(msg string, value any, rendered string) bool { + if _, ok := value.(string); ok { + // rendered already includes surrounding %q quotes; substring + // search is delimiter-safe. + return strings.Contains(msg, rendered) + } + + // For non-strings rendered is unquoted, so anchor the search at + // word boundaries to avoid 42 matching inside 420. + return regexp.MustCompile(`\b` + regexp.QuoteMeta(rendered) + `\b`).MatchString(msg) +} + +// renderBadValue formats a FieldValidationError.Value for display. +// Returns the empty string for a nil Value so callers can use it as a +// presence check. // -// Strings get %q (quoted) for both purposes: k8s validation messages -// embed string bad values as `Invalid value: "five"`, so quoting is -// what the user expects to read AND is the form to search for. Other -// types use %v unchanged: numbers and booleans appear unquoted in -// k8s messages and read naturally in our display. +// Strings get %q (quoted), matching how k8s validation messages embed +// them (`Invalid value: "five"`). Other types use %v: numbers and +// booleans read naturally unquoted, and structs render via their +// default Go representation. // -// Returns the empty rendered string for a nil Value so callers can -// use it as a presence check; the returned `found` mirrors `rendered` -// in that case. -func renderBadValue(value any) (rendered, found string) { +// Duplication detection against the validator's message text is done +// separately by valueAlreadyInMessage, which knows how to anchor the +// search at the right boundaries for each value type. +func renderBadValue(value any) string { if value == nil { - return "", "" + return "" } if s, ok := value.(string); ok { - quoted := fmt.Sprintf("%q", s) - return quoted, quoted + return fmt.Sprintf("%q", s) } - rendered = fmt.Sprintf("%v", value) - - return rendered, rendered + return fmt.Sprintf("%v", value) } diff --git a/cmd/diff/diffprocessor/schema_validator_test.go b/cmd/diff/diffprocessor/schema_validator_test.go index e06d5526..f2381e10 100644 --- a/cmd/diff/diffprocessor/schema_validator_test.go +++ b/cmd/diff/diffprocessor/schema_validator_test.go @@ -572,6 +572,18 @@ func TestFormatValidationErrors(t *testing.T) { }, expected: "example.org/v1/Thing production/my-thing:\n spec.size: Required value [schema]", }, + "NamespaceWithoutNameCollapsesToGVK": { + reason: "When namespace is set but name is empty, the header collapses to just the GVK rather than rendering '/' with a stray trailing slash.", + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "Thing", + Namespace: "production", + Status: pkgvalidate.ValidationStatusMissingSchema, + }}, + }, + expected: "example.org/v1/Thing: missing schema", + }, "MultipleErrorsGroupedUnderResource": { reason: "Multiple errors on the same resource share a single header rather than producing a header per error.", result: &pkgvalidate.ValidationResult{ @@ -656,6 +668,40 @@ func TestFormatValidationErrors(t *testing.T) { }, expected: "example.org/v1/XR my-xr:\n spec.replicas: must be > 0 (got 42) [cel]", }, + "NumericBadValueNotSuppressedByDigitPrefix": { + reason: "Regression: a raw substring search would suppress the tail when the rendered number is a digit-prefix of another token (Value=42 vs message containing '420'). Word-boundary matching keeps the tail rendered so the user sees the actual rejected value.", + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeCEL, + Message: "spec.size: must be at most 420", + Value: 42, + }}, + }}, + }, + expected: "example.org/v1/XR my-xr:\n spec.size: must be at most 420 (got 42) [cel]", + }, + "NumericBadValueSuppressedAsToken": { + reason: "When the rendered number does appear as a standalone token in the message (delimited by non-word chars), the tail is correctly suppressed to avoid duplication.", + result: &pkgvalidate.ValidationResult{ + Resources: []pkgvalidate.ResourceValidationResult{{ + APIVersion: "example.org/v1", + Kind: "XR", + Name: "my-xr", + Status: pkgvalidate.ValidationStatusInvalid, + Errors: []pkgvalidate.FieldValidationError{{ + Type: pkgvalidate.FieldErrorTypeSchema, + Message: "spec.replicas: Invalid value: 42: must be at most 10", + Value: 42, + }}, + }}, + }, + expected: "example.org/v1/XR my-xr:\n spec.replicas: Invalid value: 42: must be at most 10 [schema]", + }, "MultipleResourcesEachOnTheirOwnBlock": { reason: "Failures from distinct resources produce distinct blocks joined by newlines, in input order.", result: &pkgvalidate.ValidationResult{