From 2c48383bba1e21d22e782c57d33219890704863f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 17:50:40 +0200 Subject: [PATCH 01/12] structstest: check the bundle's resource types against encoding/json encoding/json decides which fields exist on the wire, under which names, at which paths. Every libs/structs package claims to speak that vocabulary -- structwalk enumerates it, structaccess reads and writes it, structdiff reports changes in it -- so a disagreement is a bug in one of them, or in the type. The new package fills every field of a type with a non-zero value, marshals it, and compares the result against structwalk's leaves and structaccess.Get/ValidatePath. The test drives it off config.Resources by reflection, so a newly added resource is covered without touching the test. It finds two things today, both enumerated rather than fixed here: - Eleven resource types embed a struct that declares MarshalJSON and declare none of their own, so the embedded marshaler takes over and id, url, lifecycle and permissions never reach the wire. Nothing marshals a config type today (bundle validate -o json marshals the dyn tree), so this is latent. - Free-form any fields and types that marshal themselves as a scalar (duration.Duration, the SDK time wrapper) are never visited by structwalk, so structdiff cannot report drift on them. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 97 +++++ bundle/config/structstest/structstest.go | 431 ++++++++++++++++++++ 2 files changed, 528 insertions(+) create mode 100644 bundle/config/structstest/resources_test.go create mode 100644 bundle/config/structstest/structstest.go diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go new file mode 100644 index 00000000000..95098bb0d61 --- /dev/null +++ b/bundle/config/structstest/resources_test.go @@ -0,0 +1,97 @@ +package structstest_test + +import ( + "reflect" + "testing" + + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/structstest" + "github.com/databricks/cli/libs/structs/structtag" + "github.com/stretchr/testify/require" +) + +// knownDivergences lists the disagreements the bundle's resource types have with +// encoding/json today. Each entry is a bug somewhere other than this test; the test +// enumerates them so that a *new* disagreement fails while these are worked through. +// +// Nothing in the CLI marshals a resource config type with encoding/json today -- bundle +// validate -o json marshals the dyn tree -- so none of these is user-visible yet. They +// are one json.Marshal away from being so. +var knownDivergences = map[string][]string{ + // The resource type embeds another struct that declares MarshalJSON and does not + // declare its own, so the embedded marshaler takes over and every field the outer + // struct adds -- id, url, lifecycle, permissions -- never reaches the wire. Fixed by + // giving the resource type the marshaler pair resources.Job has. + "dashboards": baseResourceFields("file_path", "permissions"), + "genie_spaces": baseResourceFields("file_path", "permissions"), + "database_instances": baseResourceFields("permissions"), + "database_catalogs": baseResourceFields(), + "synced_database_tables": baseResourceFields(), + "postgres_projects": baseResourceFields("permissions"), + "postgres_branches": baseResourceFields(), + "postgres_endpoints": baseResourceFields(), + "postgres_catalogs": baseResourceFields(), + "postgres_databases": baseResourceFields(), + "postgres_roles": baseResourceFields(), + "postgres_synced_tables": baseResourceFields(), +} + +// interfaceFieldPaths are free-form any fields. structwalk documents that it does not +// traverse an interface, so these never reach a visit callback and structdiff never +// reports a change to one. Intentional, but it means drift in a serialized dashboard or a +// cluster policy definition is invisible to the packages. +var interfaceFieldPaths = map[string][]string{ + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, + "cluster_policies": {"definition", "policy_family_definition_overrides"}, +} + +// baseResourceFields returns the paths a resource gains from BaseResource, plus any extra +// fields the resource declares alongside it. They are lost together, by one cause. +func baseResourceFields(extra ...string) []string { + return append([]string{"id", "url", "modified_status", "lifecycle.prevent_destroy"}, extra...) +} + +// TestResourceTypesAgreeWithJSON feeds every resource type in config.Resources through +// structstest.Check. Driving it off the struct by reflection means a newly added resource +// is covered without touching this test. +func TestResourceTypesAgreeWithJSON(t *testing.T) { + rt := reflect.TypeOf(config.Resources{}) + + var checked int + for i := range rt.NumField() { + field := rt.Field(i) + if field.Type.Kind() != reflect.Map { + continue + } + elem := field.Type.Elem() + if elem.Kind() != reflect.Pointer || elem.Elem().Kind() != reflect.Struct { + continue + } + group := structtag.JSONTag(field.Tag.Get("json")).Name() + + t.Run(group, func(t *testing.T) { + report, err := structstest.Check(elem) + require.NoError(t, err) + + var known []string + known = append(known, knownDivergences[group]...) + known = append(known, interfaceFieldPaths[group]...) + report = report.Filter(known) + if len(report.SelfMarshalingScalars) > 0 { + // A known structwalk limitation, tracked as one item rather than one entry per + // timestamp field, because every new SDK time field joins it. + t.Logf("%d self-marshaling scalar field(s) structwalk does not visit: %v", + len(report.SelfMarshalingScalars), report.SelfMarshalingScalars) + report.SelfMarshalingScalars = nil + } + require.True(t, report.Empty(), + "%s (%s) disagrees with encoding/json:%s", group, elem, report) + }) + checked++ + } + + // A guard against the loop silently matching nothing, which would make the whole + // test vacuous. + require.Greater(t, checked, 20, "expected every resource group to be checked") +} diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go new file mode 100644 index 00000000000..9867361eeb1 --- /dev/null +++ b/bundle/config/structstest/structstest.go @@ -0,0 +1,431 @@ +// Package structstest checks that the libs/structs packages agree with encoding/json +// about a type. +// +// encoding/json is the oracle: it decides which fields exist on the wire, under which +// names, and at which paths. Every libs/structs package claims to speak that same +// vocabulary -- structwalk enumerates it, structaccess reads and writes it, structdiff +// reports changes in it -- so any disagreement is a bug in one of them, or in the type. +// +// The package is consumed by tests (bundle/config and bundle/direct/dresources feed it +// the bundle's own resource, state and remote types) but is not itself a _test package, +// because those two callers live in different trees. +package structstest + +import ( + "encoding/json" + "fmt" + "reflect" + "slices" + "sort" + "strconv" + "strings" + + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/databricks/cli/libs/structs/structpath" + "github.com/databricks/cli/libs/structs/structtag" + "github.com/databricks/cli/libs/structs/structwalk" +) + +// Report lists the disagreements found for one type. A field is identified by the JSON +// path encoding/json puts it at, which is the only name all the packages share. +type Report struct { + // WalkMissing are paths encoding/json emits that structwalk never visits, so + // structdiff cannot see a change to them either. + WalkMissing []string + + // WalkExtra are paths structwalk visits that encoding/json does not emit. Usually a + // type that embeds another with its own MarshalJSON and does not define one itself: + // the embedded marshaler takes over and the outer fields never reach the wire. + WalkExtra []string + + // GetFailed are paths encoding/json emits that structaccess.Get cannot resolve. + GetFailed []string + + // ValidateFailed are paths encoding/json emits that structaccess.ValidatePath + // rejects against the type, even though Get resolves them on the value. + ValidateFailed []string + + // ValueMismatch are paths where structaccess.Get and encoding/json disagree about + // the value stored at the path. + ValueMismatch []string + + // SelfMarshalingScalars are paths whose Go type is a struct that marshals itself as a + // scalar through its own MarshalJSON -- duration.Duration and the SDK's time wrapper. + // structwalk looks for scalar *fields* and finds none inside them, so it never visits + // them and structdiff never reports drift on them. One known limitation rather than a + // per-field list, because every new timestamp field in the SDK joins it. + SelfMarshalingScalars []string +} + +// Empty reports whether the type and the libs/structs packages agree completely. +func (r Report) Empty() bool { + return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && + len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 +} + +// String renders the report as one indented line per category, for a test failure message. +func (r Report) String() string { + var b strings.Builder + for _, s := range []struct { + label string + paths []string + }{ + {"structwalk does not visit, encoding/json emits", r.WalkMissing}, + {"structwalk visits, encoding/json does not emit", r.WalkExtra}, + {"structaccess.Get cannot resolve", r.GetFailed}, + {"structaccess.ValidatePath rejects", r.ValidateFailed}, + {"structaccess.Get and encoding/json disagree on the value at", r.ValueMismatch}, + {"marshals itself as a scalar, so structwalk never visits", r.SelfMarshalingScalars}, + } { + if len(s.paths) == 0 { + continue + } + fmt.Fprintf(&b, "\n %s (%d):", s.label, len(s.paths)) + for _, p := range s.paths { + fmt.Fprintf(&b, "\n %s", p) + } + } + return b.String() +} + +// Check fills every field of a fresh value of type t with a non-zero value, marshals it +// with encoding/json, and compares the result against what structwalk and structaccess +// make of the same value. t must be a struct or a pointer to one. +func Check(t reflect.Type) (Report, error) { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return Report{}, fmt.Errorf("structstest: %s is not a struct", t) + } + + ptr := reflect.New(t) + FillNonZero(ptr.Elem()) + v := ptr.Interface() + + jsonLeaves, selfMarshaling, err := jsonLeaves(v) + if err != nil { + return Report{}, err + } + + walkLeaves := map[string]string{} + err = structwalk.Walk(v, func(path *structpath.PathNode, val any, _ *reflect.StructField) { + walkLeaves[path.String()] = render(val) + }) + if err != nil { + return Report{}, fmt.Errorf("structstest: walk %s: %w", t, err) + } + + var report Report + for path, want := range jsonLeaves { + if selfMarshaling[path] { + report.SelfMarshalingScalars = append(report.SelfMarshalingScalars, path) + continue + } + if _, ok := walkLeaves[path]; !ok { + report.WalkMissing = append(report.WalkMissing, path) + } + + node, err := structpath.ParsePath(path) + if err != nil { + report.GetFailed = append(report.GetFailed, path+": "+err.Error()) + continue + } + if skippedByTag(reflect.TypeOf(v), node) { + // structaccess refuses bundle:"internal" and bundle:"readonly" fields by + // design; encoding/json still emits them. + continue + } + if err := structaccess.ValidatePath(reflect.TypeOf(v), node); err != nil { + report.ValidateFailed = append(report.ValidateFailed, path+": "+err.Error()) + } + got, err := structaccess.Get(v, node) + if err != nil { + report.GetFailed = append(report.GetFailed, path+": "+err.Error()) + continue + } + if render(got) != want { + report.ValueMismatch = append(report.ValueMismatch, + fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, render(got), want)) + } + } + for path := range walkLeaves { + if _, ok := jsonLeaves[path]; !ok { + report.WalkExtra = append(report.WalkExtra, path) + } + } + + sort.Strings(report.WalkMissing) + sort.Strings(report.WalkExtra) + sort.Strings(report.GetFailed) + sort.Strings(report.ValidateFailed) + sort.Strings(report.ValueMismatch) + sort.Strings(report.SelfMarshalingScalars) + return report, nil +} + +// JSONLeaves marshals v and returns its scalar leaves keyed by the structpath rendering +// of their location, which is the dialect every libs/structs package speaks. +func JSONLeaves(v any) (map[string]string, error) { + leaves, _, err := jsonLeaves(v) + return leaves, err +} + +func jsonLeaves(v any) (map[string]string, map[string]bool, error) { + blob, err := json.Marshal(v) + if err != nil { + return nil, nil, fmt.Errorf("structstest: marshal %T: %w", v, err) + } + var generic any + if err := json.Unmarshal(blob, &generic); err != nil { + return nil, nil, fmt.Errorf("structstest: unmarshal %T: %w", v, err) + } + out := map[string]string{} + selfMarshaling := map[string]bool{} + flatten(nil, reflect.TypeOf(v), generic, out, selfMarshaling) + return out, selfMarshaling, nil +} + +// flatten walks the decoded JSON alongside the Go type, because the path syntax for an +// object member depends on which one it is: a struct field is .name, a map entry is +// ['name'], and only the type knows the difference. +func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string]string, selfMarshaling map[string]bool) { + for typ != nil && typ.Kind() == reflect.Pointer { + typ = typ.Elem() + } + + switch value := v.(type) { + case map[string]any: + isMap := typ != nil && typ.Kind() == reflect.Map + for key, member := range value { + var next *structpath.PathNode + var memberType reflect.Type + if isMap { + next = structpath.NewBracketString(path, key) + memberType = typ.Elem() + } else { + next = structpath.NewStringKey(path, key) + if typ != nil && typ.Kind() == reflect.Struct { + if sf, ok := embeddedSliceField(typ, key); ok { + // An EmbeddedSlice field is transparent by design: the walkers put + // its elements at the parent path, while the wire format keeps the + // __embed__ key. Follow the walkers, or every such type reads as a + // disagreement when it is really the convention working. + flatten(path, sf.Type, member, out, selfMarshaling) + continue + } + if sf, _, ok := structaccess.FindStructFieldByKeyType(typ, key); ok { + memberType = sf.Type + } + } + } + flatten(next, memberType, member, out, selfMarshaling) + } + case []any: + var elemType reflect.Type + if typ != nil && (typ.Kind() == reflect.Slice || typ.Kind() == reflect.Array) { + elemType = typ.Elem() + } + for i, member := range value { + flatten(structpath.NewIndex(path, i), elemType, member, out, selfMarshaling) + } + case nil: + // A JSON null carries no scalar leaf. + default: + out[path.String()] = render(value) + // A scalar on the wire whose Go type is a struct marshalled itself: the walkers + // cannot see inside it. + if typ != nil && typ.Kind() == reflect.Struct { + selfMarshaling[path.String()] = true + } + } +} + +// embeddedSliceField reports whether key names the struct's EmbeddedSlice field. +func embeddedSliceField(typ reflect.Type, key string) (reflect.StructField, bool) { + for i := range typ.NumField() { + sf := typ.Field(i) + if sf.Name != structaccess.EmbeddedSliceFieldName { + continue + } + if structtag.JSONTag(sf.Tag.Get("json")).Name() == key { + return sf, true + } + } + return reflect.StructField{}, false +} + +// render normalises a scalar so a value decoded from JSON and the same value read out of +// the struct compare equal: JSON numbers decode to float64, the struct holds int64 and +// friends, and a nil pointer reads back as nil. +func render(v any) string { + if v == nil { + return "" + } + rv := reflect.ValueOf(v) + for rv.Kind() == reflect.Pointer { + if rv.IsNil() { + return "" + } + rv = rv.Elem() + } + switch rv.Kind() { + case reflect.Float32, reflect.Float64: + return strconv.FormatFloat(rv.Float(), 'g', -1, 64) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return strconv.FormatFloat(float64(rv.Int()), 'g', -1, 64) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return strconv.FormatFloat(float64(rv.Uint()), 'g', -1, 64) + default: + return fmt.Sprintf("%v", rv.Interface()) + } +} + +// skippedByTag reports whether the last segment of path names a field structaccess +// refuses on purpose. The lookup goes through embedded structs, so a field promoted from +// BaseResource is recognised too. +func skippedByTag(typ reflect.Type, path *structpath.PathNode) bool { + nodes := path.AsSlice() + cur := typ + for i, node := range nodes { + for cur.Kind() == reflect.Pointer { + cur = cur.Elem() + } + key, ok := node.StringKey() + if !ok || cur.Kind() != reflect.Struct { + return false + } + sf, _, found := structaccess.FindStructFieldByKeyType(cur, key) + if !found { + // structaccess drops internal and readonly fields from the type-level + // lookup as well, so a miss on the last segment is the tag talking. + return i == len(nodes)-1 && taggedInternal(cur, key) + } + cur = sf.Type + } + return false +} + +// taggedInternal reports whether the struct, or a struct it embeds, declares key with +// bundle:"internal" or bundle:"readonly". +func taggedInternal(typ reflect.Type, key string) bool { + for i := range typ.NumField() { + sf := typ.Field(i) + if !sf.IsExported() { + continue + } + if structtag.JSONTag(sf.Tag.Get("json")).Name() == key { + bt := structtag.BundleTag(sf.Tag.Get("bundle")) + if bt.Internal() || bt.ReadOnly() { + return true + } + } + if !sf.Anonymous { + continue + } + ft := sf.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + if ft.Kind() == reflect.Struct && taggedInternal(ft, key) { + return true + } + } + return false +} + +// FillNonZero populates every serializable field of v with a non-zero value, so that a +// field the packages disagree about is always observable rather than indistinguishable +// from an omitted zero. Recursion is bounded because SDK types are self-referential. +func FillNonZero(v reflect.Value) { fillNonZero(v, 0) } + +func fillNonZero(v reflect.Value, depth int) { + if depth > 5 { + return + } + switch v.Kind() { + case reflect.Bool: + v.SetBool(true) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + v.SetInt(1) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + v.SetUint(1) + case reflect.Float32, reflect.Float64: + v.SetFloat(1) + case reflect.String: + v.SetString("x") + case reflect.Pointer: + v.Set(reflect.New(v.Type().Elem())) + fillNonZero(v.Elem(), depth+1) + case reflect.Slice: + elem := reflect.New(v.Type().Elem()).Elem() + fillNonZero(elem, depth+1) + v.Set(reflect.Append(v, elem)) + case reflect.Map: + v.Set(reflect.MakeMap(v.Type())) + val := reflect.New(v.Type().Elem()).Elem() + fillNonZero(val, depth+1) + v.SetMapIndex(reflect.ValueOf("k").Convert(v.Type().Key()), val) + case reflect.Interface: + // A free-form any field gets a scalar, not a map: structwalk deliberately does not + // traverse into an interface, so a composite here would show up as a path mismatch + // that says nothing about the type under test. + v.Set(reflect.ValueOf("x")) + case reflect.Struct: + for i := range v.Type().NumField() { + sf := v.Type().Field(i) + if !sf.IsExported() || sf.Name == "ForceSendFields" { + continue + } + if structtag.JSONTag(sf.Tag.Get("json")).Name() == "-" { + continue + } + fillNonZero(v.Field(i), depth+1) + } + default: + // Kinds that do not appear in bundle or SDK types (chan, func, complex) stay zero. + } +} + +// coveredBy reports whether path equals one of the entries or sits underneath it. +func coveredBy(known []string, path string) bool { + if slices.Contains(known, path) { + return true + } + for _, k := range known { + if strings.HasPrefix(path, k) && strings.ContainsAny(path[len(k):len(k)+1], ".[") { + return true + } + } + return false +} + +// KnownDivergence records a disagreement that exists today and is not this test's to fix. +// Every entry must say why it is here and what removes it. +type KnownDivergence struct { + Type string + Paths []string + Reason string +} + +// Filter removes the known divergences from a report, so the test fails only on new ones. +// An entry covers the path it names and everything under it, so listing a field that is +// lost wholesale does not mean enumerating each of its leaves. +func (r Report) Filter(known []string) Report { + drop := func(paths []string) []string { + var out []string + for _, p := range paths { + if !coveredBy(known, strings.SplitN(p, ":", 2)[0]) { + out = append(out, p) + } + } + return out + } + return Report{ + WalkMissing: drop(r.WalkMissing), + WalkExtra: drop(r.WalkExtra), + GetFailed: drop(r.GetFailed), + ValidateFailed: drop(r.ValidateFailed), + ValueMismatch: drop(r.ValueMismatch), + } +} From fd5f0db7dbaa5169b6fbc33087e77447f8874b8c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 17:50:55 +0200 Subject: [PATCH 02/12] dresources: check StateType and RemoteType against encoding/json These are the types the direct engine actually reads fields out of: the plan resolves a ${resources...} reference by walking them, the state file is the JSON encoding of StateType, and a refresh decodes into RemoteType. A disagreement here is not latent the way it is for the config types -- it is a field the plan cannot see or the state file cannot carry. assertJSONRoundTrip already covers whether a wrapper loses fields across Marshal -> Unmarshal. This covers whether the packages and encoding/json name and reach the same fields at all. Both flavours pass for every registered resource once the EmbeddedSlice convention is accounted for: __embed__ is transparent to the walkers by design, so the check follows them rather than the literal wire key. Co-authored-by: Isaac --- bundle/direct/dresources/structs_test.go | 72 ++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 bundle/direct/dresources/structs_test.go diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go new file mode 100644 index 00000000000..0bd697a80b8 --- /dev/null +++ b/bundle/direct/dresources/structs_test.go @@ -0,0 +1,72 @@ +package dresources + +import ( + "reflect" + "testing" + + "github.com/databricks/cli/bundle/config/structstest" + "github.com/stretchr/testify/require" +) + +// StateType and RemoteType are the types the direct engine actually reads fields out of: +// the plan resolves a ${resources...} reference by walking them, the state file is the +// JSON encoding of StateType, and RemoteType is what a refresh decodes into. So a +// disagreement between them and encoding/json is not latent the way it is for the config +// types -- it is a field the plan cannot see or the state file cannot carry. +// +// assertJSONRoundTrip in serialize_test.go covers the neighbouring question, whether a +// wrapper loses fields across Marshal -> Unmarshal. This covers whether the libs/structs +// packages and encoding/json name and reach the same fields in the first place. + +// knownStateDivergences and knownRemoteDivergences enumerate what disagrees today, so a +// new disagreement fails the test while these are worked through. +var ( + knownStateDivergences = map[string][]string{ + // Free-form any fields: structwalk documents that it does not traverse an interface, + // so drift inside a serialized dashboard or a cluster policy definition is invisible + // to it and to structdiff. + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, + "cluster_policies": {"definition", "policy_family_definition_overrides"}, + } + + knownRemoteDivergences = map[string][]string{ + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, + "cluster_policies": {"definition", "policy_family_definition_overrides"}, + } +) + +func TestStateTypeAgreesWithJSON(t *testing.T) { + testAgreesWithJSON(t, (*Adapter).StateType, knownStateDivergences) +} + +func TestRemoteTypeAgreesWithJSON(t *testing.T) { + testAgreesWithJSON(t, (*Adapter).RemoteType, knownRemoteDivergences) +} + +// testAgreesWithJSON runs the check for every registered resource, so a newly supported +// resource type is covered without touching this test. +func testAgreesWithJSON(t *testing.T, typeOf func(*Adapter) reflect.Type, known map[string][]string) { + for resourceType, resource := range SupportedResources { + adapter, err := NewAdapter(resource, resourceType, nil) + require.NoError(t, err) + + t.Run(resourceType, func(t *testing.T) { + typ := typeOf(adapter) + report, err := structstest.Check(typ) + require.NoError(t, err) + + report = report.Filter(known[resourceType]) + if len(report.SelfMarshalingScalars) > 0 { + // A known structwalk limitation, tracked as one item rather than one entry per + // timestamp field, because every new SDK time field joins it. + t.Logf("%d self-marshaling scalar field(s) structwalk does not visit: %v", + len(report.SelfMarshalingScalars), report.SelfMarshalingScalars) + report.SelfMarshalingScalars = nil + } + require.True(t, report.Empty(), + "%s (%s) disagrees with encoding/json:%s", resourceType, typ, report) + }) + } +} From c5cf71bbb20c4892d1982c00d4a64d3c1b52db9a Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:02:07 +0200 Subject: [PATCH 03/12] structstest: use slices.Sort The repo forbids sort.Strings in favour of the standard library's generic version. --- bundle/config/structstest/structstest.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 9867361eeb1..6deccf010fa 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -16,7 +16,6 @@ import ( "fmt" "reflect" "slices" - "sort" "strconv" "strings" @@ -155,12 +154,12 @@ func Check(t reflect.Type) (Report, error) { } } - sort.Strings(report.WalkMissing) - sort.Strings(report.WalkExtra) - sort.Strings(report.GetFailed) - sort.Strings(report.ValidateFailed) - sort.Strings(report.ValueMismatch) - sort.Strings(report.SelfMarshalingScalars) + slices.Sort(report.WalkMissing) + slices.Sort(report.WalkExtra) + slices.Sort(report.GetFailed) + slices.Sort(report.ValidateFailed) + slices.Sort(report.ValueMismatch) + slices.Sort(report.SelfMarshalingScalars) return report, nil } @@ -243,8 +242,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] // embeddedSliceField reports whether key names the struct's EmbeddedSlice field. func embeddedSliceField(typ reflect.Type, key string) (reflect.StructField, bool) { - for i := range typ.NumField() { - sf := typ.Field(i) + for sf := range typ.Fields() { if sf.Name != structaccess.EmbeddedSliceFieldName { continue } @@ -309,8 +307,7 @@ func skippedByTag(typ reflect.Type, path *structpath.PathNode) bool { // taggedInternal reports whether the struct, or a struct it embeds, declares key with // bundle:"internal" or bundle:"readonly". func taggedInternal(typ reflect.Type, key string) bool { - for i := range typ.NumField() { - sf := typ.Field(i) + for sf := range typ.Fields() { if !sf.IsExported() { continue } From 1a53e1bf80ae1d135ed8379896c6c5cb0079e767 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:02:07 +0200 Subject: [PATCH 04/12] libs/structs: check each package against encoding/json on a shared shape corpus The corpus in libs/structs/internal/jsonshapes pairs a struct shape whose JSON behaviour is easy to get wrong -- two levels of embedding, a shadowed name, a same-depth collision, a diamond, a cyclic embed, an embed behind a nil pointer -- with the fields encoding/json actually serializes for it. Its own test asserts those expectations against json.Marshal, so the corpus cannot teach every consumer the same wrong answer. Each package is then checked against it on its own terms: structaccess must read, write and validate exactly what the wire carries, and the write assertion goes through json.Marshal so a Set into a field the wire format ignores fails; structwalk must visit exactly those paths; structdiff must report a change to each of them and none to a field encoding/json drops. structpath and structtag have no corpus to check against, so they are pinned directly: a rendered path must survive being parsed again (a map key with dots is the case that matters), and a json tag must resolve to the name encoding/json chose for it. Three disagreements are recorded rather than fixed, each as a ratchet that asserts the disagreement is still present, so fixing one breaks the test and forces the entry out: - structwalk visits every declaration of a shadowed embedded field, so it reports the field twice while the wire format carries one value. - structwalk and structdiff both expose an ambiguous embedded field that encoding/json refuses to serialize, so the engine can plan an update that can never be sent. - structaccess.Set will not descend through a nil embedded pointer, so a field json.Unmarshal reaches by allocating it cannot be written. Co-authored-by: Isaac --- .../structs/internal/jsonshapes/jsonshapes.go | 240 ++++++++++++++++++ .../internal/jsonshapes/jsonshapes_test.go | 40 +++ .../structaccess/jsonagreement_test.go | 84 ++++++ libs/structs/structdiff/jsonagreement_test.go | 93 +++++++ libs/structs/structpath/jsonagreement_test.go | 80 ++++++ libs/structs/structtag/jsonagreement_test.go | 102 ++++++++ libs/structs/structwalk/jsonagreement_test.go | 100 ++++++++ 7 files changed, 739 insertions(+) create mode 100644 libs/structs/internal/jsonshapes/jsonshapes.go create mode 100644 libs/structs/internal/jsonshapes/jsonshapes_test.go create mode 100644 libs/structs/structaccess/jsonagreement_test.go create mode 100644 libs/structs/structdiff/jsonagreement_test.go create mode 100644 libs/structs/structpath/jsonagreement_test.go create mode 100644 libs/structs/structtag/jsonagreement_test.go create mode 100644 libs/structs/structwalk/jsonagreement_test.go diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go new file mode 100644 index 00000000000..ab91a31f33d --- /dev/null +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -0,0 +1,240 @@ +// Package jsonshapes is a corpus of struct shapes whose JSON behaviour is easy to get +// wrong, shared by the tests of the libs/structs packages. +// +// Each shape pairs a value with the fields encoding/json actually serializes for it, so a +// package can be checked against the wire format without restating the reasoning. The +// interesting shapes are all about embedding: how deep a promoted field may sit, which of +// two same-named fields wins, and when encoding/json gives up and serializes neither. +package jsonshapes + +import "slices" + +// Shape is one struct whose JSON behaviour a libs/structs package must match. +type Shape struct { + // Name identifies the shape in test output. + Name string + + // Value is a pointer to a populated struct. + Value any + + // JSONFields are the json names encoding/json emits for Value, in any order. They are + // asserted against json.Marshal by TestCorpusMatchesEncodingJSON, so a shape whose + // expectation drifts from reality fails in the corpus itself rather than silently + // teaching every consumer the wrong thing. + JSONFields []string + + // TypeFields are the json names a type-level walk should yield, which differs from + // JSONFields only where a value cannot reach what its type declares: an embedded nil + // pointer contributes its fields to the type but nothing to the wire. Empty means + // "same as JSONFields". + TypeFields []string + + // KnownGaps names the entry points that disagree with encoding/json about this shape + // today: "structwalk.Walk", "structwalk.WalkType", "structdiff". The package's own test asserts the disagreement is still there, so fixing + // it breaks the test and the entry has to be removed: a ratchet, not an exemption. + KnownGaps []string + + // KnownSetGap are json names encoding/json can unmarshal into but structaccess.Set + // cannot reach today. Consumers assert that Set still fails for them, so fixing the gap + // breaks the test and the entry has to go -- a ratchet rather than a silent exemption. + KnownSetGap []string + + // Unreachable are json names that look available on the Go type -- some field declares + // them -- but that encoding/json does not serialize, so no package may expose them + // either. Ambiguous embedded fields land here. + Unreachable []string +} + +type Leaf struct { + Value string `json:"value,omitempty"` +} + +type MiddleWithLeaf struct { + Leaf +} + +// DoublyEmbedded reaches a field through two levels of embedding, the shape every postgres +// resource has (PostgresProject -> PostgresProjectConfig -> postgres.ProjectSpec). +type DoublyEmbedded struct { + MiddleWithLeaf + + Own string `json:"own,omitempty"` +} + +type ShallowValue struct { + Value string `json:"value,omitempty"` +} + +type DeepHolder struct { + Leaf +} + +// ShallowWins declares value at two depths. encoding/json takes the shallower one, so Get +// and Set must resolve to the same field the wire format uses. +type ShallowWins struct { + ShallowValue + DeepHolder +} + +type SideA struct { + Value string `json:"value,omitempty"` +} + +type SideB struct { + Value string `json:"value,omitempty"` +} + +// SameDepthConflict declares value twice at one depth. encoding/json calls that ambiguous +// and emits neither, so the field is not readable or writable either. +type SameDepthConflict struct { + SideA + SideB +} + +type DiamondLeft struct { + Leaf +} + +type DiamondRight struct { + Leaf +} + +// Diamond reaches one Leaf by two routes of equal length: ambiguous, like SameDepthConflict. +type Diamond struct { + DiamondLeft + DiamondRight +} + +type NilSide struct { + Value string `json:"value,omitempty"` +} + +// AmbiguousViaNilPointer declares value twice at one depth, with one side behind a nil +// pointer. encoding/json resolves fields from the type, so it is ambiguous either way and +// neither is serialized -- a value-level search that skips the nil side sees only one +// declaration and wrongly concludes the field is reachable. +type AmbiguousViaNilPointer struct { + *NilSide + SideB +} + +// Cyclic embeds a pointer to itself, so a type-level search that does not remember where it +// has been never terminates. The corpus leaves the pointer nil: encoding/json flattens an +// embedded type once and stops, but a value walk following a self-referential pointer is +// genuinely unbounded, so a populated cycle is not a shape the packages can be compared on. +// structaccess exercises the populated case in its own test. +type Cyclic struct { + *Cyclic + + Name string `json:"name,omitempty"` +} + +// NilPointerEmbed embeds a pointer left nil. Whether a path resolves is a property of the +// type, so it must not depend on the pointer being set. +type NilPointerEmbed struct { + *Leaf + + Own string `json:"own,omitempty"` +} + +// SetPointerEmbed is NilPointerEmbed with the pointer populated. +type SetPointerEmbed struct { + *Leaf + + Own string `json:"own,omitempty"` +} + +// SkippedField declares a field encoding/json never serializes. +type SkippedField struct { + Kept string `json:"kept,omitempty"` + Skipped string `json:"-"` + + unexported string //nolint:unused // present to prove it is ignored +} + +// Gap reports whether the named entry point is recorded as disagreeing about this shape. +func (s Shape) Gap(entryPoint string) bool { + return slices.Contains(s.KnownGaps, entryPoint) +} + +// Fields returns the names a type-level walk should yield for the shape. +func (s Shape) Fields() []string { + if len(s.TypeFields) > 0 { + return s.TypeFields + } + return s.JSONFields +} + +// Shapes returns the corpus. Each call builds fresh values so a test may mutate them. +func Shapes() []Shape { + return []Shape{ + { + Name: "doubly embedded", + Value: &DoublyEmbedded{MiddleWithLeaf: MiddleWithLeaf{Leaf: Leaf{Value: "v"}}, Own: "o"}, + JSONFields: []string{"value", "own"}, + }, + { + Name: "shallower embed wins", + Value: &ShallowWins{ShallowValue: ShallowValue{Value: "shallow"}, DeepHolder: DeepHolder{Leaf: Leaf{Value: "deep"}}}, + JSONFields: []string{"value"}, + // Both walks visit each declaration, so a shadowed field is reported twice while the + // wire format carries one value. + KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType"}, + }, + { + Name: "same depth conflict", + Value: &SameDepthConflict{SideA: SideA{Value: "a"}, SideB: SideB{Value: "b"}}, + JSONFields: nil, + Unreachable: []string{"value"}, + // structaccess reports the name as not found, as encoding/json does. The walks still + // visit both declarations and structdiff still reports a change at the path, so the + // engine can plan an update for a field that cannot be serialized. + KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + }, + { + Name: "diamond", + Value: &Diamond{DiamondLeft: DiamondLeft{Leaf: Leaf{Value: "l"}}, DiamondRight: DiamondRight{Leaf: Leaf{Value: "r"}}}, + JSONFields: nil, + Unreachable: []string{"value"}, + KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + }, + { + Name: "ambiguous via nil pointer", + Value: &AmbiguousViaNilPointer{SideB: SideB{Value: "b"}}, //exhaustruct:ignore + JSONFields: nil, + Unreachable: []string{"value"}, + KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + }, + { + Name: "cyclic embed", + Value: &Cyclic{Name: "n"}, //exhaustruct:ignore + JSONFields: []string{"name"}, + // The embedded *Cyclic promotes name a level down, where encoding/json shadows it + // with the outer one. The type walk reports both; the value walk agrees, because the + // corpus leaves the pointer nil. + KnownGaps: []string{"structwalk.WalkType"}, + }, + { + Name: "nil pointer embed", + Value: &NilPointerEmbed{Own: "o"}, //exhaustruct:ignore + JSONFields: []string{"own"}, + TypeFields: []string{"value", "own"}, + }, + { + Name: "set pointer embed", + Value: &SetPointerEmbed{Leaf: &Leaf{Value: "v"}, Own: "o"}, + JSONFields: []string{"value", "own"}, + // json.Unmarshal allocates the embedded pointer to reach value; Set refuses to + // descend through a nil one, so a fresh value cannot be written through. Fixing it + // means allocating only once the write is known to succeed, or a failed Set leaves + // an allocated embed behind and changes what the type marshals to. + KnownSetGap: []string{"value"}, + }, + { + Name: "skipped field", + Value: &SkippedField{Kept: "k", Skipped: "s"}, //exhaustruct:ignore + JSONFields: []string{"kept"}, + Unreachable: []string{"-"}, + }, + } +} diff --git a/libs/structs/internal/jsonshapes/jsonshapes_test.go b/libs/structs/internal/jsonshapes/jsonshapes_test.go new file mode 100644 index 00000000000..9ebebae226f --- /dev/null +++ b/libs/structs/internal/jsonshapes/jsonshapes_test.go @@ -0,0 +1,40 @@ +package jsonshapes_test + +import ( + "encoding/json" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/internal/jsonshapes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCorpusMatchesEncodingJSON keeps the corpus honest: every shape's declared JSONFields +// are what encoding/json actually emits, and nothing it declares unreachable shows up. +// Without this the corpus could teach every consumer the same wrong answer. +func TestCorpusMatchesEncodingJSON(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + blob, err := json.Marshal(shape.Value) + require.NoError(t, err) + + var got map[string]any + require.NoError(t, json.Unmarshal(blob, &got)) + + var names []string + for name := range got { + names = append(names, name) + } + slices.Sort(names) + want := append([]string(nil), shape.JSONFields...) + slices.Sort(want) + assert.Equal(t, want, names, "encoding/json emitted %s", blob) + + for _, name := range shape.Unreachable { + assert.NotContains(t, got, name, + "%q is declared unreachable but encoding/json emitted it", name) + } + }) + } +} diff --git a/libs/structs/structaccess/jsonagreement_test.go b/libs/structs/structaccess/jsonagreement_test.go new file mode 100644 index 00000000000..2a68de4944f --- /dev/null +++ b/libs/structs/structaccess/jsonagreement_test.go @@ -0,0 +1,84 @@ +package structaccess_test + +import ( + "encoding/json" + "reflect" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/internal/jsonshapes" + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAgreesWithEncodingJSON checks structaccess against encoding/json over the shape +// corpus: a name the wire format carries must be readable, writable and valid, and a name +// it does not carry must be none of those. +// +// The write assertion goes through json.Marshal rather than the Go field, so it fails if +// Set stores into a field the wire format ignores -- which is the failure mode a shadowed +// or ambiguous embed produces, and the one a Go-field assertion cannot see. +func TestAgreesWithEncodingJSON(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + for _, name := range shape.JSONFields { + if slices.Contains(shape.KnownSetGap, name) { + // Read side still has to agree; the write side is a recorded gap, asserted as + // failing so that fixing it forces the entry out of the corpus. + require.NoError(t, structaccess.ValidateByString(reflect.TypeOf(shape.Value), name)) + _, err := structaccess.GetByString(shape.Value, name) + require.NoError(t, err) + require.Error(t, structaccess.SetByString(freshLike(shape.Value), name, "written"), + "KnownSetGap %q now works -- remove it from the corpus", name) + continue + } + + require.NoError(t, structaccess.ValidateByString(reflect.TypeOf(shape.Value), name), + "%q is on the wire, so the type must validate it", name) + + _, err := structaccess.GetByString(shape.Value, name) + require.NoError(t, err, "%q is on the wire, so Get must resolve it", name) + + fresh := freshLike(shape.Value) + require.NoError(t, structaccess.SetByString(fresh, name, "written"), + "%q is on the wire, so Set must reach it", name) + + blob, err := json.Marshal(fresh) + require.NoError(t, err) + var got map[string]any + require.NoError(t, json.Unmarshal(blob, &got)) + assert.Equal(t, "written", got[name], + "Set wrote somewhere encoding/json does not serialize: %s", blob) + } + + for _, name := range shape.Unreachable { + assert.Error(t, structaccess.ValidateByString(reflect.TypeOf(shape.Value), name), + "%q never reaches the wire, so the type must not validate it", name) + + _, err := structaccess.GetByString(shape.Value, name) + assert.Error(t, err, "%q never reaches the wire, so Get must not resolve it", name) + + assert.Error(t, structaccess.SetByString(freshLike(shape.Value), name, "written"), + "%q never reaches the wire, so Set must not claim to write it", name) + } + }) + } +} + +// TestValidateIsAPropertyOfTheType checks that whether a path validates does not depend on +// the value: an embedded pointer left nil declares the same fields as one that is set. +func TestValidateIsAPropertyOfTheType(t *testing.T) { + nilEmbed := reflect.TypeFor[*jsonshapes.NilPointerEmbed]() //exhaustruct:ignore + setEmbed := reflect.TypeFor[*jsonshapes.SetPointerEmbed]() //exhaustruct:ignore + + for _, name := range []string{"value", "own"} { + assert.NoError(t, structaccess.ValidateByString(nilEmbed, name)) + assert.NoError(t, structaccess.ValidateByString(setEmbed, name)) + } +} + +// freshLike returns a new zero value of the same type as v, which is a pointer to a struct. +func freshLike(v any) any { + return reflect.New(reflect.TypeOf(v).Elem()).Interface() +} diff --git a/libs/structs/structdiff/jsonagreement_test.go b/libs/structs/structdiff/jsonagreement_test.go new file mode 100644 index 00000000000..eebdfbf79ab --- /dev/null +++ b/libs/structs/structdiff/jsonagreement_test.go @@ -0,0 +1,93 @@ +package structdiff_test + +import ( + "reflect" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/internal/jsonshapes" + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/databricks/cli/libs/structs/structdiff" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestDiffReportsWhatJSONCarries checks that a change to a field the wire format carries +// shows up in the diff, at the path encoding/json puts it at. A field structdiff cannot see +// is a field the direct engine never sends an update for. +func TestDiffReportsWhatJSONCarries(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + for _, name := range shape.JSONFields { + before := shape.Value + after := cloneWith(t, shape, name, "changed") + if after == nil { + continue // recorded write gap; the read side is covered in structaccess + } + + changes, err := structdiff.GetStructDiff(before, after, nil) + require.NoError(t, err) + + var paths []string + for _, change := range changes { + paths = append(paths, change.Path.String()) + } + assert.Contains(t, paths, name, + "%q changed but structdiff did not report it", name) + } + }) + } +} + +// TestDiffNeverReportsAnUnreachableField checks the other direction: a name encoding/json +// refuses to serialize must never appear in a diff, or the engine would try to send a field +// that cannot exist on the wire. +func TestDiffNeverReportsAnUnreachableField(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + if len(shape.Unreachable) == 0 { + continue + } + t.Run(shape.Name, func(t *testing.T) { + zero := freshLike(shape.Value) + changes, err := structdiff.GetStructDiff(zero, shape.Value, nil) + require.NoError(t, err) + + var reported []string + for _, change := range changes { + if slices.Contains(shape.Unreachable, change.Path.String()) { + reported = append(reported, change.Path.String()) + } + } + if shape.Gap("structdiff") { + assert.NotEmpty(t, reported, + "structdiff no longer reports an unreachable field -- remove the recorded gap") + t.Logf("recorded gap: structdiff reports %v, which encoding/json does not serialize", reported) + return + } + assert.Empty(t, reported, + "structdiff reported %v, which encoding/json does not serialize", reported) + }) + } +} + +// cloneWith returns a copy of the shape's value with one field set, or nil when the shape +// records that structaccess cannot write that field yet. +func cloneWith(t *testing.T, shape jsonshapes.Shape, name, value string) any { + t.Helper() + + clone := freshLike(shape.Value) + if err := structaccess.SetByString(clone, name, value); err != nil { + for _, gap := range shape.KnownSetGap { + if gap == name { + return nil + } + } + t.Fatalf("cannot set %q: %v", name, err) + } + return clone +} + +// freshLike returns a new zero value of the same type as v, a pointer to a struct. +func freshLike(v any) any { + return reflect.New(reflect.TypeOf(v).Elem()).Interface() +} diff --git a/libs/structs/structpath/jsonagreement_test.go b/libs/structs/structpath/jsonagreement_test.go new file mode 100644 index 00000000000..185e322f74f --- /dev/null +++ b/libs/structs/structpath/jsonagreement_test.go @@ -0,0 +1,80 @@ +package structpath_test + +import ( + "encoding/json" + "testing" + + "github.com/databricks/cli/libs/structs/structpath" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMapKeyRoundTripsThroughRendering checks that a path built from a JSON object key +// survives being rendered and parsed again. Every libs/structs package identifies a field +// by the rendered string, and the corpus tests compare those strings across packages, so a +// key that renders to something ParsePath reads back differently silently makes two +// packages "disagree" about a field they both found. +// +// The keys here are the ones a Databricks API can really return: a Spark conf key has dots, +// a tag value can contain almost anything. +func TestMapKeyRoundTripsThroughRendering(t *testing.T) { + keys := []string{ + "simple", + "spark.databricks.delta.retentionDurationCheck.enabled", + "with space", + "with'quote", + "with\"doublequote", + "with[bracket]", + "with.dot", + "", + "ünïcode", + "123", + } + + for _, key := range keys { + t.Run(key, func(t *testing.T) { + node := structpath.NewBracketString(structpath.NewStringKey(nil, "conf"), key) + rendered := node.String() + + parsed, err := structpath.ParsePath(rendered) + require.NoError(t, err, "rendered as %q", rendered) + assert.Equal(t, rendered, parsed.String(), + "%q does not survive render -> parse -> render", key) + }) + } +} + +// TestIndexRoundTripsThroughRendering does the same for a slice index, which is how every +// package refers to an element of a JSON array. +func TestIndexRoundTripsThroughRendering(t *testing.T) { + for _, index := range []int{0, 1, 9, 10, 12345} { + node := structpath.NewIndex(structpath.NewStringKey(nil, "tasks"), index) + rendered := node.String() + + parsed, err := structpath.ParsePath(rendered) + require.NoError(t, err, "rendered as %q", rendered) + assert.Equal(t, rendered, parsed.String()) + } +} + +// TestRenderedPathAddressesTheSameJSONMember pins the dialect against encoding/json: the +// rendering of a struct field is the object key itself, and the rendering of a map entry is +// the key in brackets. The two are different syntax for the same kind of JSON member, which +// is exactly the distinction a flattener has to get right to compare paths at all. +func TestRenderedPathAddressesTheSameJSONMember(t *testing.T) { + type inner struct { + Conf map[string]string `json:"conf,omitempty"` + } + value := &inner{Conf: map[string]string{"a.b": "v"}} + + blob, err := json.Marshal(value) + require.NoError(t, err) + assert.JSONEq(t, `{"conf":{"a.b":"v"}}`, string(blob)) + + field := structpath.NewStringKey(nil, "conf") + assert.Equal(t, "conf", field.String()) + + entry := structpath.NewBracketString(field, "a.b") + assert.Equal(t, `conf['a.b']`, entry.String(), + "a map entry must not render as a dotted field, or a path with a dotted key reads as two fields") +} diff --git a/libs/structs/structtag/jsonagreement_test.go b/libs/structs/structtag/jsonagreement_test.go new file mode 100644 index 00000000000..7897de63760 --- /dev/null +++ b/libs/structs/structtag/jsonagreement_test.go @@ -0,0 +1,102 @@ +package structtag_test + +import ( + "encoding/json" + "reflect" + "testing" + + "github.com/databricks/cli/libs/structs/structtag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestJSONTagNameMatchesEncodingJSON checks structtag's reading of a json tag against what +// encoding/json does with the same tag. Every other package asks structtag for a field's +// name, so a tag it reads differently renames or hides the field for all of them at once. +// +// The oracle is a real marshal: the emitted object key is, by definition, the name +// encoding/json chose. +func TestJSONTagNameMatchesEncodingJSON(t *testing.T) { + tests := []struct { + tag string + // key is the object key encoding/json emits, or "" when it omits the field. + key string + }{ + {tag: `json:"name"`, key: "name"}, + {tag: `json:"name,omitempty"`, key: "name"}, + {tag: `json:"name,string"`, key: "name"}, + {tag: `json:"-"`, key: ""}, + // A lone dash means "skip"; a dash with a comma means a field literally named "-". + {tag: `json:"-,"`, key: "-"}, + {tag: `json:",omitempty"`, key: "Field"}, + {tag: `json:""`, key: "Field"}, + } + + for _, tc := range tests { + t.Run(tc.tag, func(t *testing.T) { + typ := reflect.StructOf([]reflect.StructField{{ + Name: "Field", + Type: reflect.TypeOf(""), + Tag: reflect.StructTag(tc.tag), + }}) + value := reflect.New(typ) + value.Elem().Field(0).SetString("v") + + blob, err := json.Marshal(value.Interface()) + require.NoError(t, err) + var emitted map[string]any + require.NoError(t, json.Unmarshal(blob, &emitted)) + + if tc.key == "" { + require.Empty(t, emitted, "expected the field to be skipped, got %s", blob) + } else { + require.Contains(t, emitted, tc.key, "encoding/json emitted %s", blob) + } + + // What structtag reports has to lead every package to the same conclusion: the + // emitted key, or "-" for a field encoding/json skips. + name := structtag.JSONTag(typ.Field(0).Tag.Get("json")).Name() + switch { + case tc.key == "": + assert.Equal(t, "-", name, "a skipped field must read as %q", "-") + case name == "": + // An empty tag name means "fall back to the Go field name", which is what + // encoding/json did. + assert.Equal(t, "Field", tc.key) + default: + assert.Equal(t, tc.key, name) + } + }) + } +} + +// TestOmitEmptyMatchesEncodingJSON checks the other half of the tag: whether a zero value is +// dropped. structaccess decides ForceSendFields from this, so reading it wrongly means a +// field is sent when it should be absent, or absent when it should be sent. +func TestOmitEmptyMatchesEncodingJSON(t *testing.T) { + for _, tc := range []struct { + tag string + omitEmpty bool + }{ + {tag: `json:"name"`, omitEmpty: false}, + {tag: `json:"name,omitempty"`, omitEmpty: true}, + {tag: `json:",omitempty"`, omitEmpty: true}, + {tag: `json:"name,string,omitempty"`, omitEmpty: true}, + } { + t.Run(tc.tag, func(t *testing.T) { + typ := reflect.StructOf([]reflect.StructField{{ + Name: "Field", + Type: reflect.TypeOf(""), + Tag: reflect.StructTag(tc.tag), + }}) + + blob, err := json.Marshal(reflect.New(typ).Interface()) + require.NoError(t, err) + + dropped := string(blob) == "{}" + assert.Equal(t, tc.omitEmpty, dropped, + "encoding/json emitted %s for a zero value", blob) + assert.Equal(t, tc.omitEmpty, structtag.JSONTag(typ.Field(0).Tag.Get("json")).OmitEmpty()) + }) + } +} diff --git a/libs/structs/structwalk/jsonagreement_test.go b/libs/structs/structwalk/jsonagreement_test.go new file mode 100644 index 00000000000..22fbca1075f --- /dev/null +++ b/libs/structs/structwalk/jsonagreement_test.go @@ -0,0 +1,100 @@ +package structwalk_test + +import ( + "encoding/json" + "reflect" + "slices" + "testing" + + "github.com/databricks/cli/libs/structs/internal/jsonshapes" + "github.com/databricks/cli/libs/structs/structpath" + "github.com/databricks/cli/libs/structs/structwalk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestWalkVisitsExactlyWhatJSONEmits checks the value walk against encoding/json over the +// shape corpus. A path structwalk does not visit is a path structdiff cannot report drift +// on, and a path it visits but the wire format drops is a change that can never be sent. +func TestWalkVisitsExactlyWhatJSONEmits(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + var visited []string + require.NoError(t, structwalk.Walk(shape.Value, func(path *structpath.PathNode, _ any, _ *reflect.StructField) { + visited = append(visited, path.String()) + })) + slices.Sort(visited) + + blob, err := json.Marshal(shape.Value) + require.NoError(t, err) + var emitted map[string]any + require.NoError(t, json.Unmarshal(blob, &emitted)) + var want []string + for name := range emitted { + want = append(want, name) + } + slices.Sort(want) + + if shape.Gap("structwalk.Walk") { + assert.NotEqual(t, want, visited, + "structwalk.Walk now agrees with encoding/json here -- remove the recorded gap") + t.Logf("recorded gap: encoding/json emits %v, Walk visits %v", want, visited) + return + } + + assert.Equal(t, want, visited, "encoding/json emitted %s", blob) + + for _, name := range shape.Unreachable { + assert.NotContains(t, visited, name, + "%q never reaches the wire, so the walk must not offer it", name) + } + }) + } +} + +// isScalarKind mirrors what the value walk treats as a leaf, so the two walks are compared +// on the same footing. +func isScalarKind(k reflect.Kind) bool { + switch k { + case reflect.Bool, reflect.String, + reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return true + default: + return false + } +} + +// TestWalkTypeCoversTheType checks the type walk, which differs from the value walk only +// where a value cannot reach what its type declares -- an embedded nil pointer contributes +// its fields to the type and nothing to the wire. +func TestWalkTypeCoversTheType(t *testing.T) { + for _, shape := range jsonshapes.Shapes() { + t.Run(shape.Name, func(t *testing.T) { + var visited []string + require.NoError(t, structwalk.WalkType(reflect.TypeOf(shape.Value), func(path *structpath.PatternNode, typ reflect.Type, _ *reflect.StructField) bool { + if isScalarKind(typ.Kind()) { + visited = append(visited, path.String()) + } + return true + })) + slices.Sort(visited) + + want := append([]string(nil), shape.Fields()...) + slices.Sort(want) + if shape.Gap("structwalk.WalkType") { + assert.NotEqual(t, want, visited, + "structwalk.WalkType now agrees with encoding/json here -- remove the recorded gap") + t.Logf("recorded gap: the type declares %v, WalkType visits %v", want, visited) + return + } + assert.Equal(t, want, visited) + + for _, name := range shape.Unreachable { + assert.NotContains(t, visited, name, + "%q never reaches the wire, so the type walk must not offer it", name) + } + }) + } +} From 7b97a98c6da252a69aee6ccb92cf6bec25af03b2 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:04:11 +0200 Subject: [PATCH 05/12] structstest: drop an unused export, modernise the field loop JSONLeaves had no caller once Check took the self-marshaling set from the internal helper, and KnownDivergence was never used. task fmt rewrote the reflection loop to reflect.TypeFor and Type.Fields. --- bundle/config/structstest/resources_test.go | 5 ++--- bundle/config/structstest/structstest.go | 18 +++--------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index 95098bb0d61..58d5bcc33c3 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -56,11 +56,10 @@ func baseResourceFields(extra ...string) []string { // structstest.Check. Driving it off the struct by reflection means a newly added resource // is covered without touching this test. func TestResourceTypesAgreeWithJSON(t *testing.T) { - rt := reflect.TypeOf(config.Resources{}) + rt := reflect.TypeFor[config.Resources]() var checked int - for i := range rt.NumField() { - field := rt.Field(i) + for field := range rt.Fields() { if field.Type.Kind() != reflect.Map { continue } diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 6deccf010fa..b8cfae0f4d1 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -163,13 +163,9 @@ func Check(t reflect.Type) (Report, error) { return report, nil } -// JSONLeaves marshals v and returns its scalar leaves keyed by the structpath rendering -// of their location, which is the dialect every libs/structs package speaks. -func JSONLeaves(v any) (map[string]string, error) { - leaves, _, err := jsonLeaves(v) - return leaves, err -} - +// jsonLeaves marshals v and returns its scalar leaves keyed by the structpath rendering of +// their location, which is the dialect every libs/structs package speaks, plus the subset of +// those leaves whose Go type marshalled itself as a scalar. func jsonLeaves(v any) (map[string]string, map[string]bool, error) { blob, err := json.Marshal(v) if err != nil { @@ -397,14 +393,6 @@ func coveredBy(known []string, path string) bool { return false } -// KnownDivergence records a disagreement that exists today and is not this test's to fix. -// Every entry must say why it is here and what removes it. -type KnownDivergence struct { - Type string - Paths []string - Reason string -} - // Filter removes the known divergences from a report, so the test fails only on new ones. // An entry covers the path it names and everything under it, so listing a field that is // lost wholesale does not mean enumerating each of its leaves. From 8cd48a2fe9fc1d8c3a4dfa98fdfc74eb1401de47 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 18:32:13 +0200 Subject: [PATCH 06/12] structstest: make the recorded divergences ratchets, not exemptions From the adversarial review. Four ways the checks were weaker than they read: Filter dropped SelfMarshalingScalars from the report it returned, so the callers' check on that category could never fire and the category was silently ignored rather than reported. A known-divergence entry only filtered; nothing noticed when the underlying bug was fixed and the entry went stale. Filter now returns the entries that matched nothing and both tests fail on them. That immediately found two stale entries. Prefix coverage applied to every category, so an entry naming a field could absorb an unrelated Get or value failure at a path beneath it. It now applies only to the two walk categories, where a field lost wholesale really does take its leaves with it. The per-shape gaps asserted merely that *some* disagreement remained, which a different regression would satisfy. They now hold the exact current output, so any change in behaviour fails and the entry has to be revisited. Paths inside a free-form any field are a category of their own now, like self-marshaling scalars: structwalk does not traverse an interface and structaccess cannot validate a path through one, so the whole subtree is opaque and listing individual paths would only pin the filler's choice of map key. With that, the state and remote types need no recorded divergences at all. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 22 ++- bundle/config/structstest/structstest.go | 165 +++++++++++++----- bundle/direct/dresources/structs_test.go | 39 ++--- .../structs/internal/jsonshapes/jsonshapes.go | 46 +++-- libs/structs/structdiff/jsonagreement_test.go | 18 +- libs/structs/structtag/jsonagreement_test.go | 4 +- libs/structs/structwalk/jsonagreement_test.go | 20 ++- 7 files changed, 202 insertions(+), 112 deletions(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index 58d5bcc33c3..880f9ba1967 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -36,16 +36,6 @@ var knownDivergences = map[string][]string{ "postgres_synced_tables": baseResourceFields(), } -// interfaceFieldPaths are free-form any fields. structwalk documents that it does not -// traverse an interface, so these never reach a visit callback and structdiff never -// reports a change to one. Intentional, but it means drift in a serialized dashboard or a -// cluster policy definition is invisible to the packages. -var interfaceFieldPaths = map[string][]string{ - "dashboards": {"serialized_dashboard"}, - "genie_spaces": {"serialized_space"}, - "cluster_policies": {"definition", "policy_family_definition_overrides"}, -} - // baseResourceFields returns the paths a resource gains from BaseResource, plus any extra // fields the resource declares alongside it. They are lost together, by one cause. func baseResourceFields(extra ...string) []string { @@ -75,8 +65,16 @@ func TestResourceTypesAgreeWithJSON(t *testing.T) { var known []string known = append(known, knownDivergences[group]...) - known = append(known, interfaceFieldPaths[group]...) - report = report.Filter(known) + report, stale := report.Filter(known) + require.Empty(t, stale, + "these recorded divergences no longer occur -- remove them from the list: %v", stale) + if len(report.InsideFreeFormField) > 0 { + // A known limitation: structwalk does not traverse an interface and structaccess + // cannot validate a path through one, so a free-form field is opaque to both. + t.Logf("%d path(s) inside a free-form any field: %v", + len(report.InsideFreeFormField), report.InsideFreeFormField) + report.InsideFreeFormField = nil + } if len(report.SelfMarshalingScalars) > 0 { // A known structwalk limitation, tracked as one item rather than one entry per // timestamp field, because every new SDK time field joins it. diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index b8cfae0f4d1..297e19fa657 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -48,6 +48,13 @@ type Report struct { // the value stored at the path. ValueMismatch []string + // InsideFreeFormField are paths that sit inside a free-form any field. structwalk does not + // traverse an interface and structaccess cannot validate a path through one, so nothing + // below such a field is visible to the packages -- a serialized dashboard or a cluster + // policy definition authored as inline YAML is opaque to them. One known limitation rather + // than a list of paths, which would depend on the filler's choice of key. + InsideFreeFormField []string + // SelfMarshalingScalars are paths whose Go type is a struct that marshals itself as a // scalar through its own MarshalJSON -- duration.Duration and the SDK's time wrapper. // structwalk looks for scalar *fields* and finds none inside them, so it never visits @@ -59,7 +66,8 @@ type Report struct { // Empty reports whether the type and the libs/structs packages agree completely. func (r Report) Empty() bool { return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && - len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 + len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 && + len(r.InsideFreeFormField) == 0 } // String renders the report as one indented line per category, for a test failure message. @@ -75,6 +83,7 @@ func (r Report) String() string { {"structaccess.ValidatePath rejects", r.ValidateFailed}, {"structaccess.Get and encoding/json disagree on the value at", r.ValueMismatch}, {"marshals itself as a scalar, so structwalk never visits", r.SelfMarshalingScalars}, + {"sits inside a free-form any field, which the packages do not look into", r.InsideFreeFormField}, } { if len(s.paths) == 0 { continue @@ -102,7 +111,7 @@ func Check(t reflect.Type) (Report, error) { FillNonZero(ptr.Elem()) v := ptr.Interface() - jsonLeaves, selfMarshaling, err := jsonLeaves(v) + jsonLeaves, marks, err := jsonLeaves(v) if err != nil { return Report{}, err } @@ -117,7 +126,11 @@ func Check(t reflect.Type) (Report, error) { var report Report for path, want := range jsonLeaves { - if selfMarshaling[path] { + if marks.freeForm[path] { + report.InsideFreeFormField = append(report.InsideFreeFormField, path) + continue + } + if marks.selfMarshaling[path] { report.SelfMarshalingScalars = append(report.SelfMarshalingScalars, path) continue } @@ -150,6 +163,12 @@ func Check(t reflect.Type) (Report, error) { } for path := range walkLeaves { if _, ok := jsonLeaves[path]; !ok { + if marks.freeFormField[path] { + // The any field itself: the walk offers it as a scalar leaf while the wire format + // carries whatever it holds, which is the same limitation seen from the other side. + report.InsideFreeFormField = append(report.InsideFreeFormField, path) + continue + } report.WalkExtra = append(report.WalkExtra, path) } } @@ -166,28 +185,48 @@ func Check(t reflect.Type) (Report, error) { // jsonLeaves marshals v and returns its scalar leaves keyed by the structpath rendering of // their location, which is the dialect every libs/structs package speaks, plus the subset of // those leaves whose Go type marshalled itself as a scalar. -func jsonLeaves(v any) (map[string]string, map[string]bool, error) { +// leafMarks records leaves that need a category of their own rather than a path-by-path +// comparison. +type leafMarks struct { + // selfMarshaling are leaves whose Go type is a struct that marshalled itself as a scalar. + selfMarshaling map[string]bool + // freeForm are leaves below an any field; freeFormField holds the any fields themselves. + freeForm map[string]bool + freeFormField map[string]bool +} + +func jsonLeaves(v any) (map[string]string, leafMarks, error) { + marks := leafMarks{ + selfMarshaling: map[string]bool{}, + freeForm: map[string]bool{}, + freeFormField: map[string]bool{}, + } + blob, err := json.Marshal(v) if err != nil { - return nil, nil, fmt.Errorf("structstest: marshal %T: %w", v, err) + return nil, marks, fmt.Errorf("structstest: marshal %T: %w", v, err) } var generic any if err := json.Unmarshal(blob, &generic); err != nil { - return nil, nil, fmt.Errorf("structstest: unmarshal %T: %w", v, err) + return nil, marks, fmt.Errorf("structstest: unmarshal %T: %w", v, err) } out := map[string]string{} - selfMarshaling := map[string]bool{} - flatten(nil, reflect.TypeOf(v), generic, out, selfMarshaling) - return out, selfMarshaling, nil + flatten(nil, reflect.TypeOf(v), generic, out, marks, false) + return out, marks, nil } // flatten walks the decoded JSON alongside the Go type, because the path syntax for an // object member depends on which one it is: a struct field is .name, a map entry is // ['name'], and only the type knows the difference. -func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string]string, selfMarshaling map[string]bool) { +func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string]string, marks leafMarks, freeForm bool) { for typ != nil && typ.Kind() == reflect.Pointer { typ = typ.Elem() } + if typ != nil && typ.Kind() == reflect.Interface { + // Everything below an any field is opaque to the packages. + marks.freeFormField[path.String()] = true + freeForm = true + } switch value := v.(type) { case map[string]any: @@ -206,7 +245,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] // its elements at the parent path, while the wire format keeps the // __embed__ key. Follow the walkers, or every such type reads as a // disagreement when it is really the convention working. - flatten(path, sf.Type, member, out, selfMarshaling) + flatten(path, sf.Type, member, out, marks, freeForm) continue } if sf, _, ok := structaccess.FindStructFieldByKeyType(typ, key); ok { @@ -214,7 +253,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] } } } - flatten(next, memberType, member, out, selfMarshaling) + flatten(next, memberType, member, out, marks, freeForm) } case []any: var elemType reflect.Type @@ -222,16 +261,19 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] elemType = typ.Elem() } for i, member := range value { - flatten(structpath.NewIndex(path, i), elemType, member, out, selfMarshaling) + flatten(structpath.NewIndex(path, i), elemType, member, out, marks, freeForm) } case nil: // A JSON null carries no scalar leaf. default: out[path.String()] = render(value) + if freeForm { + marks.freeForm[path.String()] = true + } // A scalar on the wire whose Go type is a struct marshalled itself: the walkers // cannot see inside it. if typ != nil && typ.Kind() == reflect.Struct { - selfMarshaling[path.String()] = true + marks.selfMarshaling[path.String()] = true } } } @@ -360,10 +402,10 @@ func fillNonZero(v reflect.Value, depth int) { fillNonZero(val, depth+1) v.SetMapIndex(reflect.ValueOf("k").Convert(v.Type().Key()), val) case reflect.Interface: - // A free-form any field gets a scalar, not a map: structwalk deliberately does not - // traverse into an interface, so a composite here would show up as a path mismatch - // that says nothing about the type under test. - v.Set(reflect.ValueOf("x")) + // A free-form any field holds a composite in practice -- a cluster policy definition or + // a serialized dashboard authored as inline YAML -- and that is the case worth covering, + // because structwalk does not traverse into an interface and so cannot see any of it. + v.Set(reflect.ValueOf(map[string]any{"k": "v"})) case reflect.Struct: for i := range v.Type().NumField() { sf := v.Type().Field(i) @@ -380,37 +422,76 @@ func fillNonZero(v reflect.Value, depth int) { } } -// coveredBy reports whether path equals one of the entries or sits underneath it. -func coveredBy(known []string, path string) bool { - if slices.Contains(known, path) { - return true - } - for _, k := range known { - if strings.HasPrefix(path, k) && strings.ContainsAny(path[len(k):len(k)+1], ".[") { - return true +// Filter removes the known divergences from a report and returns the entries that matched +// nothing, so a caller can fail when a recorded divergence has been fixed and the entry is +// stale. Without that, a known-divergence list is an exemption rather than a ratchet. +// +// Prefix coverage applies only to the two walk categories, where a field lost wholesale takes +// all of its leaves with it. The categories that name a specific failure -- Get, ValidatePath, +// a value mismatch -- match exactly, so an entry cannot quietly absorb an unrelated failure at +// a path beneath it. +func (r Report) Filter(known []string) (Report, []string) { + used := map[string]bool{} + + dropPrefix := func(paths []string) []string { + var out []string + for _, p := range paths { + if match, ok := coveredBy(known, pathOf(p)); ok { + used[match] = true + continue + } + out = append(out, p) } + return out } - return false -} - -// Filter removes the known divergences from a report, so the test fails only on new ones. -// An entry covers the path it names and everything under it, so listing a field that is -// lost wholesale does not mean enumerating each of its leaves. -func (r Report) Filter(known []string) Report { - drop := func(paths []string) []string { + dropExact := func(paths []string) []string { var out []string for _, p := range paths { - if !coveredBy(known, strings.SplitN(p, ":", 2)[0]) { - out = append(out, p) + if slices.Contains(known, pathOf(p)) { + used[pathOf(p)] = true + continue } + out = append(out, p) } return out } - return Report{ - WalkMissing: drop(r.WalkMissing), - WalkExtra: drop(r.WalkExtra), - GetFailed: drop(r.GetFailed), - ValidateFailed: drop(r.ValidateFailed), - ValueMismatch: drop(r.ValueMismatch), + + filtered := Report{ + WalkMissing: dropPrefix(r.WalkMissing), + WalkExtra: dropPrefix(r.WalkExtra), + GetFailed: dropExact(r.GetFailed), + ValidateFailed: dropExact(r.ValidateFailed), + ValueMismatch: dropExact(r.ValueMismatch), + // These two are categories, not per-path lists, so the caller decides what to do with + // them. Dropping them here would make that decision unreachable. + SelfMarshalingScalars: r.SelfMarshalingScalars, + InsideFreeFormField: r.InsideFreeFormField, + } + + var stale []string + for _, k := range known { + if !used[k] { + stale = append(stale, k) + } + } + return filtered, stale +} + +// pathOf strips the explanatory suffix some categories append after a colon. +func pathOf(reported string) string { + return strings.SplitN(reported, ":", 2)[0] +} + +// coveredBy reports which entry covers path: the one that equals it, or names a field it sits +// underneath. +func coveredBy(known []string, path string) (string, bool) { + for _, k := range known { + if path == k { + return k, true + } + if strings.HasPrefix(path, k) && strings.ContainsAny(path[len(k):len(k)+1], ".[") { + return k, true + } } + return "", false } diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go index 0bd697a80b8..e8e33380c65 100644 --- a/bundle/direct/dresources/structs_test.go +++ b/bundle/direct/dresources/structs_test.go @@ -18,31 +18,19 @@ import ( // wrapper loses fields across Marshal -> Unmarshal. This covers whether the libs/structs // packages and encoding/json name and reach the same fields in the first place. -// knownStateDivergences and knownRemoteDivergences enumerate what disagrees today, so a -// new disagreement fails the test while these are worked through. -var ( - knownStateDivergences = map[string][]string{ - // Free-form any fields: structwalk documents that it does not traverse an interface, - // so drift inside a serialized dashboard or a cluster policy definition is invisible - // to it and to structdiff. - "dashboards": {"serialized_dashboard"}, - "genie_spaces": {"serialized_space"}, - "cluster_policies": {"definition", "policy_family_definition_overrides"}, - } - - knownRemoteDivergences = map[string][]string{ - "dashboards": {"serialized_dashboard"}, - "genie_spaces": {"serialized_space"}, - "cluster_policies": {"definition", "policy_family_definition_overrides"}, - } -) +// knownDivergences is where a per-path disagreement would be recorded. It is empty: the state +// and remote types agree with encoding/json on every path today, and the two limitations that +// remain -- free-form any fields and types that marshal themselves as a scalar -- are reported +// as categories rather than paths. A new disagreement fails the test rather than landing here +// silently. +var knownDivergences = map[string][]string{} func TestStateTypeAgreesWithJSON(t *testing.T) { - testAgreesWithJSON(t, (*Adapter).StateType, knownStateDivergences) + testAgreesWithJSON(t, (*Adapter).StateType, knownDivergences) } func TestRemoteTypeAgreesWithJSON(t *testing.T) { - testAgreesWithJSON(t, (*Adapter).RemoteType, knownRemoteDivergences) + testAgreesWithJSON(t, (*Adapter).RemoteType, knownDivergences) } // testAgreesWithJSON runs the check for every registered resource, so a newly supported @@ -57,7 +45,16 @@ func testAgreesWithJSON(t *testing.T, typeOf func(*Adapter) reflect.Type, known report, err := structstest.Check(typ) require.NoError(t, err) - report = report.Filter(known[resourceType]) + report, stale := report.Filter(known[resourceType]) + require.Empty(t, stale, + "these recorded divergences no longer occur -- remove them from the list: %v", stale) + if len(report.InsideFreeFormField) > 0 { + // A known limitation: structwalk does not traverse an interface and structaccess + // cannot validate a path through one, so a free-form field is opaque to both. + t.Logf("%d path(s) inside a free-form any field: %v", + len(report.InsideFreeFormField), report.InsideFreeFormField) + report.InsideFreeFormField = nil + } if len(report.SelfMarshalingScalars) > 0 { // A known structwalk limitation, tracked as one item rather than one entry per // timestamp field, because every new SDK time field joins it. diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go index ab91a31f33d..126899879a8 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes.go +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -7,8 +7,6 @@ // two same-named fields wins, and when encoding/json gives up and serializes neither. package jsonshapes -import "slices" - // Shape is one struct whose JSON behaviour a libs/structs package must match. type Shape struct { // Name identifies the shape in test output. @@ -29,10 +27,13 @@ type Shape struct { // "same as JSONFields". TypeFields []string - // KnownGaps names the entry points that disagree with encoding/json about this shape - // today: "structwalk.Walk", "structwalk.WalkType", "structdiff". The package's own test asserts the disagreement is still there, so fixing - // it breaks the test and the entry has to be removed: a ratchet, not an exemption. - KnownGaps []string + // WalkGap, WalkTypeGap and DiffGap record what a package does today where it disagrees + // with encoding/json. They hold the exact current output, not merely a "this is broken" + // flag, so any change to the behaviour -- including a different wrong answer -- fails the + // package's test and forces the entry to be revisited. Nil means "must agree". + WalkGap []string + WalkTypeGap []string + DiffGap []string // KnownSetGap are json names encoding/json can unmarshal into but structaccess.Set // cannot reach today. Consumers assert that Set still fails for them, so fixing the gap @@ -88,7 +89,7 @@ type SideB struct { // and emits neither, so the field is not readable or writable either. type SameDepthConflict struct { SideA - SideB + SideB //nolint:govet // the repeated json tag is the point: both embeds declare "value" } type DiamondLeft struct { @@ -102,7 +103,7 @@ type DiamondRight struct { // Diamond reaches one Leaf by two routes of equal length: ambiguous, like SameDepthConflict. type Diamond struct { DiamondLeft - DiamondRight + DiamondRight //nolint:govet // the repeated json tag is the point: both routes reach Leaf } type NilSide struct { @@ -115,7 +116,7 @@ type NilSide struct { // declaration and wrongly concludes the field is reachable. type AmbiguousViaNilPointer struct { *NilSide - SideB + SideB //nolint:govet // the repeated json tag is the point: both embeds declare "value" } // Cyclic embeds a pointer to itself, so a type-level search that does not remember where it @@ -152,11 +153,6 @@ type SkippedField struct { unexported string //nolint:unused // present to prove it is ignored } -// Gap reports whether the named entry point is recorded as disagreeing about this shape. -func (s Shape) Gap(entryPoint string) bool { - return slices.Contains(s.KnownGaps, entryPoint) -} - // Fields returns the names a type-level walk should yield for the shape. func (s Shape) Fields() []string { if len(s.TypeFields) > 0 { @@ -179,7 +175,8 @@ func Shapes() []Shape { JSONFields: []string{"value"}, // Both walks visit each declaration, so a shadowed field is reported twice while the // wire format carries one value. - KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType"}, + WalkGap: []string{"value", "value"}, + WalkTypeGap: []string{"value", "value"}, }, { Name: "same depth conflict", @@ -189,21 +186,32 @@ func Shapes() []Shape { // structaccess reports the name as not found, as encoding/json does. The walks still // visit both declarations and structdiff still reports a change at the path, so the // engine can plan an update for a field that cannot be serialized. - KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + WalkGap: []string{"value", "value"}, + WalkTypeGap: []string{"value", "value"}, + // Once per declaration, since structdiff walks both. + DiffGap: []string{"value", "value"}, }, { Name: "diamond", Value: &Diamond{DiamondLeft: DiamondLeft{Leaf: Leaf{Value: "l"}}, DiamondRight: DiamondRight{Leaf: Leaf{Value: "r"}}}, JSONFields: nil, Unreachable: []string{"value"}, - KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + WalkGap: []string{"value", "value"}, + WalkTypeGap: []string{"value", "value"}, + // Once per declaration, since structdiff walks both. + DiffGap: []string{"value", "value"}, }, { Name: "ambiguous via nil pointer", Value: &AmbiguousViaNilPointer{SideB: SideB{Value: "b"}}, //exhaustruct:ignore JSONFields: nil, Unreachable: []string{"value"}, - KnownGaps: []string{"structwalk.Walk", "structwalk.WalkType", "structdiff"}, + // The value walk reaches only the non-nil declaration; the type walk sees both. + WalkGap: []string{"value"}, + WalkTypeGap: []string{"value", "value"}, + // Once per declaration, since structdiff walks both. + // Once: the nil side is not reachable in the value, so only one declaration is walked. + DiffGap: []string{"value"}, }, { Name: "cyclic embed", @@ -212,7 +220,7 @@ func Shapes() []Shape { // The embedded *Cyclic promotes name a level down, where encoding/json shadows it // with the outer one. The type walk reports both; the value walk agrees, because the // corpus leaves the pointer nil. - KnownGaps: []string{"structwalk.WalkType"}, + WalkTypeGap: []string{"name", "name"}, }, { Name: "nil pointer embed", diff --git a/libs/structs/structdiff/jsonagreement_test.go b/libs/structs/structdiff/jsonagreement_test.go index eebdfbf79ab..ca82c72c529 100644 --- a/libs/structs/structdiff/jsonagreement_test.go +++ b/libs/structs/structdiff/jsonagreement_test.go @@ -58,10 +58,14 @@ func TestDiffNeverReportsAnUnreachableField(t *testing.T) { reported = append(reported, change.Path.String()) } } - if shape.Gap("structdiff") { - assert.NotEmpty(t, reported, - "structdiff no longer reports an unreachable field -- remove the recorded gap") - t.Logf("recorded gap: structdiff reports %v, which encoding/json does not serialize", reported) + if shape.DiffGap != nil { + // The recorded gap is the exact set structdiff reports today, so a change in + // either direction fails here. + slices.Sort(reported) + gap := slices.Clone(shape.DiffGap) + slices.Sort(gap) + assert.Equal(t, gap, reported, + "structdiff changed here -- update or remove the recorded gap") return } assert.Empty(t, reported, @@ -77,10 +81,8 @@ func cloneWith(t *testing.T, shape jsonshapes.Shape, name, value string) any { clone := freshLike(shape.Value) if err := structaccess.SetByString(clone, name, value); err != nil { - for _, gap := range shape.KnownSetGap { - if gap == name { - return nil - } + if slices.Contains(shape.KnownSetGap, name) { + return nil } t.Fatalf("cannot set %q: %v", name, err) } diff --git a/libs/structs/structtag/jsonagreement_test.go b/libs/structs/structtag/jsonagreement_test.go index 7897de63760..7631aea3e12 100644 --- a/libs/structs/structtag/jsonagreement_test.go +++ b/libs/structs/structtag/jsonagreement_test.go @@ -36,7 +36,7 @@ func TestJSONTagNameMatchesEncodingJSON(t *testing.T) { t.Run(tc.tag, func(t *testing.T) { typ := reflect.StructOf([]reflect.StructField{{ Name: "Field", - Type: reflect.TypeOf(""), + Type: reflect.TypeFor[string](), Tag: reflect.StructTag(tc.tag), }}) value := reflect.New(typ) @@ -86,7 +86,7 @@ func TestOmitEmptyMatchesEncodingJSON(t *testing.T) { t.Run(tc.tag, func(t *testing.T) { typ := reflect.StructOf([]reflect.StructField{{ Name: "Field", - Type: reflect.TypeOf(""), + Type: reflect.TypeFor[string](), Tag: reflect.StructTag(tc.tag), }}) diff --git a/libs/structs/structwalk/jsonagreement_test.go b/libs/structs/structwalk/jsonagreement_test.go index 22fbca1075f..69afd4ed657 100644 --- a/libs/structs/structwalk/jsonagreement_test.go +++ b/libs/structs/structwalk/jsonagreement_test.go @@ -35,10 +35,13 @@ func TestWalkVisitsExactlyWhatJSONEmits(t *testing.T) { } slices.Sort(want) - if shape.Gap("structwalk.Walk") { - assert.NotEqual(t, want, visited, - "structwalk.Walk now agrees with encoding/json here -- remove the recorded gap") - t.Logf("recorded gap: encoding/json emits %v, Walk visits %v", want, visited) + if shape.WalkGap != nil { + // A recorded gap holds the exact current output, so a different wrong answer + // fails here too rather than passing as "still broken". + gap := slices.Clone(shape.WalkGap) + slices.Sort(gap) + assert.Equal(t, gap, visited, + "structwalk.Walk changed here; encoding/json emits %v -- update or remove the recorded gap", want) return } @@ -83,10 +86,11 @@ func TestWalkTypeCoversTheType(t *testing.T) { want := append([]string(nil), shape.Fields()...) slices.Sort(want) - if shape.Gap("structwalk.WalkType") { - assert.NotEqual(t, want, visited, - "structwalk.WalkType now agrees with encoding/json here -- remove the recorded gap") - t.Logf("recorded gap: the type declares %v, WalkType visits %v", want, visited) + if shape.WalkTypeGap != nil { + gap := slices.Clone(shape.WalkTypeGap) + slices.Sort(gap) + assert.Equal(t, gap, visited, + "structwalk.WalkType changed here; the type declares %v -- update or remove the recorded gap", want) return } assert.Equal(t, want, visited) From 1b8b5951fd3f7b439585379fa92a049a3bd03d39 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:18:54 +0200 Subject: [PATCH 07/12] jsonshapes: cover a tagged embed and an option-only one A json name on an anonymous field makes it a named field to encoding/json; a tag that sets only an option leaves it flattened. Both shapes are in the corpus now, and Leaves flattens a shape's JSON to leaf paths so the corpus's expectations stay comparable once a shape nests. Also: coveredBy takes the longest matching entry rather than the first, so overlapping known-divergence entries are each credited with what they alone cover instead of one absorbing everything and leaving the other looking stale. --- bundle/config/structstest/structstest.go | 16 ++-- .../structs/internal/jsonshapes/jsonshapes.go | 75 +++++++++++++++++++ .../internal/jsonshapes/jsonshapes_test.go | 4 +- .../structaccess/jsonagreement_test.go | 9 +-- libs/structs/structwalk/jsonagreement_test.go | 4 +- 5 files changed, 93 insertions(+), 15 deletions(-) diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 297e19fa657..b06b552885b 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -482,16 +482,22 @@ func pathOf(reported string) string { return strings.SplitN(reported, ":", 2)[0] } -// coveredBy reports which entry covers path: the one that equals it, or names a field it sits -// underneath. +// coveredBy reports which entry covers path: the one that equals it, or the longest one that +// names a field it sits underneath. Longest wins so that overlapping entries -- "a" and "a.b" +// -- are each credited with what they alone cover, rather than the first one absorbing +// everything and leaving the other looking stale. func coveredBy(known []string, path string) (string, bool) { + best := "" for _, k := range known { if path == k { return k, true } - if strings.HasPrefix(path, k) && strings.ContainsAny(path[len(k):len(k)+1], ".[") { - return k, true + if !strings.HasPrefix(path, k) || !strings.ContainsAny(path[len(k):len(k)+1], ".[") { + continue + } + if len(k) > len(best) { + best = k } } - return "", false + return best, best != "" } diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go index 126899879a8..b995b727f3a 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes.go +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -7,6 +7,12 @@ // two same-named fields wins, and when encoding/json gives up and serializes neither. package jsonshapes +import ( + "encoding/json" + "fmt" + "strconv" +) + // Shape is one struct whose JSON behaviour a libs/structs package must match. type Shape struct { // Name identifies the shape in test output. @@ -145,6 +151,22 @@ type SetPointerEmbed struct { Own string `json:"own,omitempty"` } +// TaggedEmbed carries a json name on an anonymous field, which makes it an ordinary field to +// encoding/json: it serializes as a nested object under that name rather than being flattened. +type TaggedEmbed struct { + Leaf `json:"leaf"` + + Own string `json:"own,omitempty"` +} + +// OptionOnlyEmbed sets an option on the embed's tag without giving it a name, which leaves it +// flattened -- the presence of a tag is not what decides. +type OptionOnlyEmbed struct { + Leaf `json:",omitempty"` + + Own string `json:"own,omitempty"` +} + // SkippedField declares a field encoding/json never serializes. type SkippedField struct { Kept string `json:"kept,omitempty"` @@ -238,6 +260,18 @@ func Shapes() []Shape { // an allocated embed behind and changes what the type marshals to. KnownSetGap: []string{"value"}, }, + { + Name: "tagged embed is a named field", + Value: &TaggedEmbed{Leaf: Leaf{Value: "v"}, Own: "o"}, + JSONFields: []string{"leaf.value", "own"}, + // Not flattened, so the outer object has no "value" member. + Unreachable: []string{"value"}, + }, + { + Name: "option-only embed stays flattened", + Value: &OptionOnlyEmbed{Leaf: Leaf{Value: "v"}, Own: "o"}, + JSONFields: []string{"value", "own"}, + }, { Name: "skipped field", Value: &SkippedField{Kept: "k", Skipped: "s"}, //exhaustruct:ignore @@ -246,3 +280,44 @@ func Shapes() []Shape { }, } } + +// Leaves marshals v and returns its scalar leaves keyed by path, so a shape's JSONFields can +// be compared against what encoding/json actually produces even when a shape nests. +// +// Object members are joined with a dot, which is the struct-field rendering. That is enough +// here because no shape in the corpus contains a map; the type-aware version, which has to +// tell a map entry's ['key'] from a field's .name, lives in bundle/config/structstest. +func Leaves(v any) (map[string]string, error) { + blob, err := json.Marshal(v) + if err != nil { + return nil, err + } + var generic any + if err := json.Unmarshal(blob, &generic); err != nil { + return nil, err + } + out := map[string]string{} + flattenLeaves("", generic, out) + return out, nil +} + +func flattenLeaves(prefix string, v any, out map[string]string) { + switch value := v.(type) { + case map[string]any: + for key, member := range value { + path := key + if prefix != "" { + path = prefix + "." + key + } + flattenLeaves(path, member, out) + } + case []any: + for i, member := range value { + flattenLeaves(prefix+"["+strconv.Itoa(i)+"]", member, out) + } + case nil: + // A JSON null carries no scalar leaf. + default: + out[prefix] = fmt.Sprintf("%v", value) + } +} diff --git a/libs/structs/internal/jsonshapes/jsonshapes_test.go b/libs/structs/internal/jsonshapes/jsonshapes_test.go index 9ebebae226f..012228e0636 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes_test.go +++ b/libs/structs/internal/jsonshapes/jsonshapes_test.go @@ -19,8 +19,8 @@ func TestCorpusMatchesEncodingJSON(t *testing.T) { blob, err := json.Marshal(shape.Value) require.NoError(t, err) - var got map[string]any - require.NoError(t, json.Unmarshal(blob, &got)) + got, err := jsonshapes.Leaves(shape.Value) + require.NoError(t, err) var names []string for name := range got { diff --git a/libs/structs/structaccess/jsonagreement_test.go b/libs/structs/structaccess/jsonagreement_test.go index 2a68de4944f..786f9bbd0d2 100644 --- a/libs/structs/structaccess/jsonagreement_test.go +++ b/libs/structs/structaccess/jsonagreement_test.go @@ -1,7 +1,6 @@ package structaccess_test import ( - "encoding/json" "reflect" "slices" "testing" @@ -44,12 +43,10 @@ func TestAgreesWithEncodingJSON(t *testing.T) { require.NoError(t, structaccess.SetByString(fresh, name, "written"), "%q is on the wire, so Set must reach it", name) - blob, err := json.Marshal(fresh) + leaves, err := jsonshapes.Leaves(fresh) require.NoError(t, err) - var got map[string]any - require.NoError(t, json.Unmarshal(blob, &got)) - assert.Equal(t, "written", got[name], - "Set wrote somewhere encoding/json does not serialize: %s", blob) + assert.Equal(t, "written", leaves[name], + "Set wrote somewhere encoding/json does not serialize: %v", leaves) } for _, name := range shape.Unreachable { diff --git a/libs/structs/structwalk/jsonagreement_test.go b/libs/structs/structwalk/jsonagreement_test.go index 69afd4ed657..cc94d3b5e28 100644 --- a/libs/structs/structwalk/jsonagreement_test.go +++ b/libs/structs/structwalk/jsonagreement_test.go @@ -27,8 +27,8 @@ func TestWalkVisitsExactlyWhatJSONEmits(t *testing.T) { blob, err := json.Marshal(shape.Value) require.NoError(t, err) - var emitted map[string]any - require.NoError(t, json.Unmarshal(blob, &emitted)) + emitted, err := jsonshapes.Leaves(shape.Value) + require.NoError(t, err) var want []string for name := range emitted { want = append(want, name) From 7f20d9e78a222fe476b66704bc5434b413d8a21c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:21:01 +0200 Subject: [PATCH 08/12] jsonshapes: drop a redundant loop-variable copy --- libs/structs/internal/jsonshapes/jsonshapes.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go index b995b727f3a..541b022a9f1 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes.go +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -305,11 +305,10 @@ func flattenLeaves(prefix string, v any, out map[string]string) { switch value := v.(type) { case map[string]any: for key, member := range value { - path := key if prefix != "" { - path = prefix + "." + key + key = prefix + "." + key } - flattenLeaves(path, member, out) + flattenLeaves(key, member, out) } case []any: for i, member := range value { From 62a659ad9b571ee921a0c135079fe776fe4e09a6 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 21:52:55 +0200 Subject: [PATCH 09/12] structstest: inventory containers, and ratchet the two tolerated categories Three coverage holes from the final review round. Check inventoried scalar leaves only, so a field encoding/json emits as {} or [] contributed nothing to compare and a field the packages could not reach at all would have passed unnoticed. Container paths are now collected too and each one has to resolve through ValidatePath and Get. Every resource, state and remote type already satisfies that. The free-form and self-marshaling categories were logged and cleared, so a newly introduced blind spot passed silently. Both are ratcheted now, each at the level where the ratchet says something: - Free-form any fields are listed by name per resource. Which resources have one is stable, so a new one has to be added here deliberately. That immediately caught a real difference: cluster policies have two in the config type and none in the state type, because the definition is normalized to the string the API takes before deploy. - Self-marshaling scalars are ratcheted on the Go *types* that behave this way, not the paths. A new timestamp field of duration.Duration or the SDK time wrapper says nothing; a new type that hides itself from the walkers is a finding, and fails. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 48 +++++++++--- bundle/config/structstest/structstest.go | 83 +++++++++++++++++---- bundle/direct/dresources/structs_test.go | 46 +++++++++--- 3 files changed, 144 insertions(+), 33 deletions(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index 880f9ba1967..edeaee28fd5 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -2,11 +2,14 @@ package structstest_test import ( "reflect" + "slices" + "strings" "testing" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/structstest" "github.com/databricks/cli/libs/structs/structtag" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -36,6 +39,27 @@ var knownDivergences = map[string][]string{ "postgres_synced_tables": baseResourceFields(), } +// freeFormFields lists the any-typed fields of each resource. Everything at or below one is +// invisible to the packages. +var freeFormFields = map[string][]string{ + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, + "cluster_policies": {"definition", "policy_family_definition_overrides"}, +} + +// freeFormFieldNames reduces the reported paths to the distinct top-level field each sits under, +// so the expectation does not depend on the filler's choice of map key. +func freeFormFieldNames(paths []string) []string { + var out []string + for _, path := range paths { + name, _, _ := strings.Cut(strings.SplitN(path, ":", 2)[0], ".") + if !slices.Contains(out, name) { + out = append(out, name) + } + } + return out +} + // baseResourceFields returns the paths a resource gains from BaseResource, plus any extra // fields the resource declares alongside it. They are lost together, by one cause. func baseResourceFields(extra ...string) []string { @@ -68,19 +92,23 @@ func TestResourceTypesAgreeWithJSON(t *testing.T) { report, stale := report.Filter(known) require.Empty(t, stale, "these recorded divergences no longer occur -- remove them from the list: %v", stale) - if len(report.InsideFreeFormField) > 0 { - // A known limitation: structwalk does not traverse an interface and structaccess - // cannot validate a path through one, so a free-form field is opaque to both. - t.Logf("%d path(s) inside a free-form any field: %v", - len(report.InsideFreeFormField), report.InsideFreeFormField) - report.InsideFreeFormField = nil - } + // A known limitation: structwalk does not traverse an interface and structaccess cannot + // validate a path through one, so a free-form field is opaque to both. Which resources + // have one is stable, so it is ratcheted by name: a new free-form field is a new blind + // spot and has to be added here deliberately. + assert.ElementsMatch(t, freeFormFields[group], freeFormFieldNames(report.InsideFreeFormField), + "free-form any fields changed for %s", group) + report.InsideFreeFormField = nil if len(report.SelfMarshalingScalars) > 0 { - // A known structwalk limitation, tracked as one item rather than one entry per - // timestamp field, because every new SDK time field joins it. - t.Logf("%d self-marshaling scalar field(s) structwalk does not visit: %v", + // A known structwalk limitation. The ratchet is on the *types* that behave this + // way, not the paths: a new field of a type already known to hide itself tells us + // nothing, while a new such type is a finding. + assert.Subset(t, structstest.KnownSelfMarshalingTypes, report.SelfMarshalingTypes, + "a Go type that marshals itself as a scalar and so is invisible to structwalk") + t.Logf("%d self-marshaling scalar field(s): %v", len(report.SelfMarshalingScalars), report.SelfMarshalingScalars) report.SelfMarshalingScalars = nil + report.SelfMarshalingTypes = nil } require.True(t, report.Empty(), "%s (%s) disagrees with encoding/json:%s", group, elem, report) diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index b06b552885b..234ff0fc8fa 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -25,6 +25,16 @@ import ( "github.com/databricks/cli/libs/structs/structwalk" ) +// KnownSelfMarshalingTypes are the Go types that serialize themselves as a scalar through +// their own MarshalJSON, which is why structwalk never visits a field of one: it looks for +// scalar fields and finds none inside. A new *type* here is a new way for a field to hide from +// the packages, so callers assert the set stays within this list rather than listing every +// field of these types. +var KnownSelfMarshalingTypes = []string{ + "duration.Duration", + "time.Time", +} + // Report lists the disagreements found for one type. A field is identified by the JSON // path encoding/json puts it at, which is the only name all the packages share. type Report struct { @@ -55,19 +65,30 @@ type Report struct { // than a list of paths, which would depend on the filler's choice of key. InsideFreeFormField []string + // ContainerUnreachable are paths at which encoding/json emits an object or an array that + // structaccess cannot resolve. Scalar leaves alone would miss them: a field serialized as + // {} or [] contributes no leaf, so a field the packages cannot reach at all would otherwise + // go unnoticed. + ContainerUnreachable []string + // SelfMarshalingScalars are paths whose Go type is a struct that marshals itself as a // scalar through its own MarshalJSON -- duration.Duration and the SDK's time wrapper. // structwalk looks for scalar *fields* and finds none inside them, so it never visits - // them and structdiff never reports drift on them. One known limitation rather than a - // per-field list, because every new timestamp field in the SDK joins it. + // them and structdiff never reports drift on them. SelfMarshalingScalars []string + + // SelfMarshalingTypes are the distinct Go types behind SelfMarshalingScalars. Callers + // ratchet on these rather than on the paths: a new timestamp field of a type already known + // to behave this way tells them nothing, while a new *type* that hides itself from the + // walkers is a finding. + SelfMarshalingTypes []string } // Empty reports whether the type and the libs/structs packages agree completely. func (r Report) Empty() bool { return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 && - len(r.InsideFreeFormField) == 0 + len(r.InsideFreeFormField) == 0 && len(r.ContainerUnreachable) == 0 } // String renders the report as one indented line per category, for a test failure message. @@ -84,6 +105,7 @@ func (r Report) String() string { {"structaccess.Get and encoding/json disagree on the value at", r.ValueMismatch}, {"marshals itself as a scalar, so structwalk never visits", r.SelfMarshalingScalars}, {"sits inside a free-form any field, which the packages do not look into", r.InsideFreeFormField}, + {"is emitted as an object or array structaccess cannot resolve", r.ContainerUnreachable}, } { if len(s.paths) == 0 { continue @@ -130,8 +152,11 @@ func Check(t reflect.Type) (Report, error) { report.InsideFreeFormField = append(report.InsideFreeFormField, path) continue } - if marks.selfMarshaling[path] { + if typeName, ok := marks.selfMarshaling[path]; ok { report.SelfMarshalingScalars = append(report.SelfMarshalingScalars, path) + if !slices.Contains(report.SelfMarshalingTypes, typeName) { + report.SelfMarshalingTypes = append(report.SelfMarshalingTypes, typeName) + } continue } if _, ok := walkLeaves[path]; !ok { @@ -161,6 +186,27 @@ func Check(t reflect.Type) (Report, error) { fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, render(got), want)) } } + for path := range marks.containers { + if marks.freeForm[path] || marks.freeFormField[path] { + continue + } + node, err := structpath.ParsePath(path) + if err != nil { + report.ContainerUnreachable = append(report.ContainerUnreachable, path+": "+err.Error()) + continue + } + if skippedByTag(reflect.TypeOf(v), node) { + continue + } + if err := structaccess.ValidatePath(reflect.TypeOf(v), node); err != nil { + report.ContainerUnreachable = append(report.ContainerUnreachable, path+": "+err.Error()) + continue + } + if _, err := structaccess.Get(v, node); err != nil { + report.ContainerUnreachable = append(report.ContainerUnreachable, path+": "+err.Error()) + } + } + for path := range walkLeaves { if _, ok := jsonLeaves[path]; !ok { if marks.freeFormField[path] { @@ -188,18 +234,21 @@ func Check(t reflect.Type) (Report, error) { // leafMarks records leaves that need a category of their own rather than a path-by-path // comparison. type leafMarks struct { - // selfMarshaling are leaves whose Go type is a struct that marshalled itself as a scalar. - selfMarshaling map[string]bool + // selfMarshaling maps such a leaf to the Go type that marshalled itself. + selfMarshaling map[string]string // freeForm are leaves below an any field; freeFormField holds the any fields themselves. freeForm map[string]bool freeFormField map[string]bool + // containers are paths at which an object or array is emitted. + containers map[string]bool } func jsonLeaves(v any) (map[string]string, leafMarks, error) { marks := leafMarks{ - selfMarshaling: map[string]bool{}, + selfMarshaling: map[string]string{}, freeForm: map[string]bool{}, freeFormField: map[string]bool{}, + containers: map[string]bool{}, } blob, err := json.Marshal(v) @@ -230,6 +279,9 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] switch value := v.(type) { case map[string]any: + if path != nil { + marks.containers[path.String()] = true + } isMap := typ != nil && typ.Kind() == reflect.Map for key, member := range value { var next *structpath.PathNode @@ -256,6 +308,9 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] flatten(next, memberType, member, out, marks, freeForm) } case []any: + if path != nil { + marks.containers[path.String()] = true + } var elemType reflect.Type if typ != nil && (typ.Kind() == reflect.Slice || typ.Kind() == reflect.Array) { elemType = typ.Elem() @@ -273,7 +328,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] // A scalar on the wire whose Go type is a struct marshalled itself: the walkers // cannot see inside it. if typ != nil && typ.Kind() == reflect.Struct { - marks.selfMarshaling[path.String()] = true + marks.selfMarshaling[path.String()] = typ.String() } } } @@ -457,14 +512,16 @@ func (r Report) Filter(known []string) (Report, []string) { } filtered := Report{ - WalkMissing: dropPrefix(r.WalkMissing), - WalkExtra: dropPrefix(r.WalkExtra), - GetFailed: dropExact(r.GetFailed), - ValidateFailed: dropExact(r.ValidateFailed), - ValueMismatch: dropExact(r.ValueMismatch), + WalkMissing: dropPrefix(r.WalkMissing), + WalkExtra: dropPrefix(r.WalkExtra), + GetFailed: dropExact(r.GetFailed), + ContainerUnreachable: dropExact(r.ContainerUnreachable), + ValidateFailed: dropExact(r.ValidateFailed), + ValueMismatch: dropExact(r.ValueMismatch), // These two are categories, not per-path lists, so the caller decides what to do with // them. Dropping them here would make that decision unreachable. SelfMarshalingScalars: r.SelfMarshalingScalars, + SelfMarshalingTypes: r.SelfMarshalingTypes, InsideFreeFormField: r.InsideFreeFormField, } diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go index e8e33380c65..650828ede2a 100644 --- a/bundle/direct/dresources/structs_test.go +++ b/bundle/direct/dresources/structs_test.go @@ -2,9 +2,12 @@ package dresources import ( "reflect" + "slices" + "strings" "testing" "github.com/databricks/cli/bundle/config/structstest" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -25,6 +28,27 @@ import ( // silently. var knownDivergences = map[string][]string{} +// freeFormFields lists the any-typed fields of each resource's state and remote types. Unlike +// the config types, cluster policies have none: the state type carries definition as the string +// the API takes, which ConfigureClusterPolicyDefinition has already normalized. +var freeFormFields = map[string][]string{ + "dashboards": {"serialized_dashboard"}, + "genie_spaces": {"serialized_space"}, +} + +// topLevelNames reduces reported paths to the distinct top-level field each sits under, so the +// expectation does not depend on the filler's choice of map key. +func topLevelNames(paths []string) []string { + var out []string + for _, path := range paths { + name, _, _ := strings.Cut(strings.SplitN(path, ":", 2)[0], ".") + if !slices.Contains(out, name) { + out = append(out, name) + } + } + return out +} + func TestStateTypeAgreesWithJSON(t *testing.T) { testAgreesWithJSON(t, (*Adapter).StateType, knownDivergences) } @@ -48,19 +72,21 @@ func testAgreesWithJSON(t *testing.T, typeOf func(*Adapter) reflect.Type, known report, stale := report.Filter(known[resourceType]) require.Empty(t, stale, "these recorded divergences no longer occur -- remove them from the list: %v", stale) - if len(report.InsideFreeFormField) > 0 { - // A known limitation: structwalk does not traverse an interface and structaccess - // cannot validate a path through one, so a free-form field is opaque to both. - t.Logf("%d path(s) inside a free-form any field: %v", - len(report.InsideFreeFormField), report.InsideFreeFormField) - report.InsideFreeFormField = nil - } + // Free-form any fields are opaque to the packages; which resources have one is stable, + // so it is ratcheted by name rather than logged away. + assert.ElementsMatch(t, freeFormFields[resourceType], topLevelNames(report.InsideFreeFormField), + "free-form any fields changed for %s", resourceType) + report.InsideFreeFormField = nil if len(report.SelfMarshalingScalars) > 0 { - // A known structwalk limitation, tracked as one item rather than one entry per - // timestamp field, because every new SDK time field joins it. - t.Logf("%d self-marshaling scalar field(s) structwalk does not visit: %v", + // A known structwalk limitation. The ratchet is on the *types* that behave this + // way, not the paths: a new field of a type already known to hide itself tells us + // nothing, while a new such type is a finding. + assert.Subset(t, structstest.KnownSelfMarshalingTypes, report.SelfMarshalingTypes, + "a Go type that marshals itself as a scalar and so is invisible to structwalk") + t.Logf("%d self-marshaling scalar field(s): %v", len(report.SelfMarshalingScalars), report.SelfMarshalingScalars) report.SelfMarshalingScalars = nil + report.SelfMarshalingTypes = nil } require.True(t, report.Empty(), "%s (%s) disagrees with encoding/json:%s", resourceType, typ, report) From d9909a2eb0c221d6b3d77c62c7c1c7b72b2ca92e Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:09:15 +0200 Subject: [PATCH 10/12] structstest: count structwalk's visits instead of collapsing them Check recorded the walk's leaves in a map, so two visits to one path became one entry and a shadowed embedded field looked like agreement. That is the worst failure mode for a test whose whole job is to notice disagreement, and it was hiding real cases. Visits are counted now, and six resource types turn out to have one: apps (id, url, lifecycle.prevent_destroy), pipelines and alerts (id), and job_runs, clusters and sql_warehouses (lifecycle.prevent_destroy). Each embeds BaseResource alongside an SDK type that declares the same json name, or two structs that each carry a Lifecycle. encoding/json serializes the shallower field and nothing else, so the second visit is a field that cannot reach the wire under that name -- and structdiff reports a change at that path twice. Recorded per resource rather than fixed: making structwalk resolve a name the way encoding/json does is a change to the walk itself, and it would move the refschema golden. The state and remote types have none of these, so the duplication is confined to the config types. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 19 ++++++++++++++++++ bundle/config/structstest/structstest.go | 22 ++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index edeaee28fd5..8cd9a179cc3 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -39,6 +39,21 @@ var knownDivergences = map[string][]string{ "postgres_synced_tables": baseResourceFields(), } +// walkDuplicates lists the paths structwalk visits twice for a resource, because the resource +// embeds BaseResource alongside an SDK type that declares the same json name, or two structs +// that each carry a Lifecycle. encoding/json serializes the shallower one and nothing else, so +// the second visit is a field that cannot reach the wire under that name -- and structdiff +// reports a change at the path twice. Ratcheted by name: fixing structwalk to resolve a name +// the way encoding/json does empties these, and a new shadowed field has to be added here. +var walkDuplicates = map[string][]string{ + "job_runs": {"lifecycle.prevent_destroy"}, + "pipelines": {"id"}, + "clusters": {"lifecycle.prevent_destroy"}, + "apps": {"id", "url", "lifecycle.prevent_destroy"}, + "alerts": {"id"}, + "sql_warehouses": {"lifecycle.prevent_destroy"}, +} + // freeFormFields lists the any-typed fields of each resource. Everything at or below one is // invisible to the packages. var freeFormFields = map[string][]string{ @@ -96,6 +111,10 @@ func TestResourceTypesAgreeWithJSON(t *testing.T) { // validate a path through one, so a free-form field is opaque to both. Which resources // have one is stable, so it is ratcheted by name: a new free-form field is a new blind // spot and has to be added here deliberately. + assert.ElementsMatch(t, walkDuplicates[group], report.WalkDuplicated, + "paths structwalk visits twice changed for %s", group) + report.WalkDuplicated = nil + assert.ElementsMatch(t, freeFormFields[group], freeFormFieldNames(report.InsideFreeFormField), "free-form any fields changed for %s", group) report.InsideFreeFormField = nil diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 234ff0fc8fa..3bc80fed292 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -65,6 +65,13 @@ type Report struct { // than a list of paths, which would depend on the filler's choice of key. InsideFreeFormField []string + // WalkDuplicated are paths structwalk visits more than once. encoding/json serializes a + // name once, so a second visit is a second field under one name -- a shadowed embed -- and + // structdiff would report a change at that path twice. Carried through Filter untouched: + // callers ratchet on it by name, like the other categories that name a limitation rather + // than a path-by-path failure. + WalkDuplicated []string + // ContainerUnreachable are paths at which encoding/json emits an object or an array that // structaccess cannot resolve. Scalar leaves alone would miss them: a field serialized as // {} or [] contributes no leaf, so a field the packages cannot reach at all would otherwise @@ -88,7 +95,8 @@ type Report struct { func (r Report) Empty() bool { return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 && - len(r.InsideFreeFormField) == 0 && len(r.ContainerUnreachable) == 0 + len(r.InsideFreeFormField) == 0 && len(r.ContainerUnreachable) == 0 && + len(r.WalkDuplicated) == 0 } // String renders the report as one indented line per category, for a test failure message. @@ -106,6 +114,7 @@ func (r Report) String() string { {"marshals itself as a scalar, so structwalk never visits", r.SelfMarshalingScalars}, {"sits inside a free-form any field, which the packages do not look into", r.InsideFreeFormField}, {"is emitted as an object or array structaccess cannot resolve", r.ContainerUnreachable}, + {"structwalk visits more than once", r.WalkDuplicated}, } { if len(s.paths) == 0 { continue @@ -139,7 +148,11 @@ func Check(t reflect.Type) (Report, error) { } walkLeaves := map[string]string{} + walkVisits := map[string]int{} err = structwalk.Walk(v, func(path *structpath.PathNode, val any, _ *reflect.StructField) { + // Counted, not just recorded: a map would collapse two visits to one path and hide a + // shadowed embedded field, which is agreement reported where there is none. + walkVisits[path.String()]++ walkLeaves[path.String()] = render(val) }) if err != nil { @@ -186,6 +199,12 @@ func Check(t reflect.Type) (Report, error) { fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, render(got), want)) } } + for path, visits := range walkVisits { + if visits > 1 && !marks.freeForm[path] && !marks.freeFormField[path] { + report.WalkDuplicated = append(report.WalkDuplicated, path) + } + } + for path := range marks.containers { if marks.freeForm[path] || marks.freeFormField[path] { continue @@ -516,6 +535,7 @@ func (r Report) Filter(known []string) (Report, []string) { WalkExtra: dropPrefix(r.WalkExtra), GetFailed: dropExact(r.GetFailed), ContainerUnreachable: dropExact(r.ContainerUnreachable), + WalkDuplicated: r.WalkDuplicated, ValidateFailed: dropExact(r.ValidateFailed), ValueMismatch: dropExact(r.ValueMismatch), // These two are categories, not per-path lists, so the caller decides what to do with From cb4a88596a42fedf7194f216cc0ccae24a712c46 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:30:35 +0200 Subject: [PATCH 11/12] structstest: stop treating a dash-named field as skipped FillNonZero skipped any field whose parsed json name was "-", which includes json:"-,omitempty" -- a field encoding/json does serialize, under the name "-". Left at its zero value it was then omitted from the marshal output, so the harness saw no disagreement and passed: a false pass on exactly the shape structaccess could not resolve. It uses structaccess.IsSkippedField now, and the corpus carries a dash-named field alongside the genuinely skipped one so all four packages are held to the distinction. --- bundle/config/structstest/structstest.go | 2 +- libs/structs/internal/jsonshapes/jsonshapes.go | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 3bc80fed292..5a564ad852e 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -486,7 +486,7 @@ func fillNonZero(v reflect.Value, depth int) { if !sf.IsExported() || sf.Name == "ForceSendFields" { continue } - if structtag.JSONTag(sf.Tag.Get("json")).Name() == "-" { + if structaccess.IsSkippedField(sf) { continue } fillNonZero(v.Field(i), depth+1) diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go index 541b022a9f1..6c5f07d95c8 100644 --- a/libs/structs/internal/jsonshapes/jsonshapes.go +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -167,10 +167,12 @@ type OptionOnlyEmbed struct { Own string `json:"own,omitempty"` } -// SkippedField declares a field encoding/json never serializes. +// SkippedField declares a field encoding/json never serializes, alongside one whose tag names +// it "-": only the exact tag json:"-" is a skip. type SkippedField struct { - Kept string `json:"kept,omitempty"` - Skipped string `json:"-"` + Kept string `json:"kept,omitempty"` + Skipped string `json:"-"` + DashNamed string `json:"-,omitempty"` //nolint:staticcheck // the odd tag is the point unexported string //nolint:unused // present to prove it is ignored } @@ -273,10 +275,9 @@ func Shapes() []Shape { JSONFields: []string{"value", "own"}, }, { - Name: "skipped field", - Value: &SkippedField{Kept: "k", Skipped: "s"}, //exhaustruct:ignore - JSONFields: []string{"kept"}, - Unreachable: []string{"-"}, + Name: "skipped and dash-named fields", + Value: &SkippedField{Kept: "k", Skipped: "s", DashNamed: "d"}, //exhaustruct:ignore + JSONFields: []string{"kept", "-"}, }, } } From 8855935c784efd0de1725885d5fed1186d9fd1e1 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 22:36:33 +0200 Subject: [PATCH 12/12] structstest: compare wire types, and report the __embed__ rename Two more false-pass paths in the harness itself. render normalised every scalar to its text, so a JSON string and a JSON number of the same digits compared equal. A field tagged json:",string" puts a number on the wire as "1" while the packages expose the int behind it -- a real difference in what is at the path, reported as agreement. The JSON side is decoded with UseNumber now and both sides render with their type, so number:1 and string:1 no longer match while 1, 1.0 and 1e0 still do. flatten silently rewrote the EmbeddedSlice convention: __embed__ carries the slice on the wire while the walkers put its elements at the parent path. Following the walkers is right -- it is how the state file and the engine's paths relate -- but doing it silently meant the harness could not notice a change to the convention. The rename is reported now, and both tests assert that __embed__ is the only key it ever applies to. Co-authored-by: Isaac --- bundle/config/structstest/resources_test.go | 8 ++ bundle/config/structstest/structstest.go | 91 ++++++++++++++++----- bundle/direct/dresources/structs_test.go | 8 ++ 3 files changed, 88 insertions(+), 19 deletions(-) diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go index 8cd9a179cc3..9a49c297712 100644 --- a/bundle/config/structstest/resources_test.go +++ b/bundle/config/structstest/resources_test.go @@ -111,6 +111,14 @@ func TestResourceTypesAgreeWithJSON(t *testing.T) { // validate a path through one, so a free-form field is opaque to both. Which resources // have one is stable, so it is ratcheted by name: a new free-form field is a new blind // spot and has to be added here deliberately. + // The EmbeddedSlice convention renames exactly one key: __embed__ carries the slice + // while the walkers put its elements at the parent path. Anything else renamed would + // be a change to the convention. + for _, path := range report.RenamedByConvention { + assert.Equal(t, "__embed__", path, "only __embed__ is renamed by convention") + } + report.RenamedByConvention = nil + assert.ElementsMatch(t, walkDuplicates[group], report.WalkDuplicated, "paths structwalk visits twice changed for %s", group) report.WalkDuplicated = nil diff --git a/bundle/config/structstest/structstest.go b/bundle/config/structstest/structstest.go index 5a564ad852e..62634e13b91 100644 --- a/bundle/config/structstest/structstest.go +++ b/bundle/config/structstest/structstest.go @@ -12,6 +12,7 @@ package structstest import ( + "bytes" "encoding/json" "fmt" "reflect" @@ -72,6 +73,12 @@ type Report struct { // than a path-by-path failure. WalkDuplicated []string + // RenamedByConvention are paths where the packages deliberately use a different name than + // the wire: an EmbeddedSlice field is tagged __embed__ but the walkers put its elements at + // the parent path. Reported rather than silently rewritten, so the convention is visible and + // a change to it is noticed. + RenamedByConvention []string + // ContainerUnreachable are paths at which encoding/json emits an object or an array that // structaccess cannot resolve. Scalar leaves alone would miss them: a field serialized as // {} or [] contributes no leaf, so a field the packages cannot reach at all would otherwise @@ -96,7 +103,7 @@ func (r Report) Empty() bool { return len(r.WalkMissing) == 0 && len(r.WalkExtra) == 0 && len(r.GetFailed) == 0 && len(r.ValidateFailed) == 0 && len(r.ValueMismatch) == 0 && len(r.SelfMarshalingScalars) == 0 && len(r.InsideFreeFormField) == 0 && len(r.ContainerUnreachable) == 0 && - len(r.WalkDuplicated) == 0 + len(r.WalkDuplicated) == 0 && len(r.RenamedByConvention) == 0 } // String renders the report as one indented line per category, for a test failure message. @@ -115,6 +122,7 @@ func (r Report) String() string { {"sits inside a free-form any field, which the packages do not look into", r.InsideFreeFormField}, {"is emitted as an object or array structaccess cannot resolve", r.ContainerUnreachable}, {"structwalk visits more than once", r.WalkDuplicated}, + {"is at a different path on the wire, by the EmbeddedSlice convention", r.RenamedByConvention}, } { if len(s.paths) == 0 { continue @@ -153,7 +161,7 @@ func Check(t reflect.Type) (Report, error) { // Counted, not just recorded: a map would collapse two visits to one path and hide a // shadowed embedded field, which is agreement reported where there is none. walkVisits[path.String()]++ - walkLeaves[path.String()] = render(val) + walkLeaves[path.String()] = renderGo(val) }) if err != nil { return Report{}, fmt.Errorf("structstest: walk %s: %w", t, err) @@ -194,11 +202,15 @@ func Check(t reflect.Type) (Report, error) { report.GetFailed = append(report.GetFailed, path+": "+err.Error()) continue } - if render(got) != want { + if renderGo(got) != want { report.ValueMismatch = append(report.ValueMismatch, - fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, render(got), want)) + fmt.Sprintf("%s: structaccess=%s encoding/json=%s", path, renderGo(got), want)) } } + for path := range marks.renamed { + report.RenamedByConvention = append(report.RenamedByConvention, path) + } + for path, visits := range walkVisits { if visits > 1 && !marks.freeForm[path] && !marks.freeFormField[path] { report.WalkDuplicated = append(report.WalkDuplicated, path) @@ -260,6 +272,8 @@ type leafMarks struct { freeFormField map[string]bool // containers are paths at which an object or array is emitted. containers map[string]bool + // renamed are wire paths the packages present under a different name by convention. + renamed map[string]bool } func jsonLeaves(v any) (map[string]string, leafMarks, error) { @@ -268,14 +282,20 @@ func jsonLeaves(v any) (map[string]string, leafMarks, error) { freeForm: map[string]bool{}, freeFormField: map[string]bool{}, containers: map[string]bool{}, + renamed: map[string]bool{}, } blob, err := json.Marshal(v) if err != nil { return nil, marks, fmt.Errorf("structstest: marshal %T: %w", v, err) } + // UseNumber keeps a JSON number distinct from a JSON string, so a field tagged + // json:",string" -- which puts a number on the wire as "1" -- is not mistaken for agreement + // with the Go int behind it. + decoder := json.NewDecoder(bytes.NewReader(blob)) + decoder.UseNumber() var generic any - if err := json.Unmarshal(blob, &generic); err != nil { + if err := decoder.Decode(&generic); err != nil { return nil, marks, fmt.Errorf("structstest: unmarshal %T: %w", v, err) } out := map[string]string{} @@ -312,10 +332,11 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] next = structpath.NewStringKey(path, key) if typ != nil && typ.Kind() == reflect.Struct { if sf, ok := embeddedSliceField(typ, key); ok { - // An EmbeddedSlice field is transparent by design: the walkers put - // its elements at the parent path, while the wire format keeps the - // __embed__ key. Follow the walkers, or every such type reads as a - // disagreement when it is really the convention working. + // An EmbeddedSlice field is transparent by design: the walkers put its + // elements at the parent path, while the wire format keeps the __embed__ + // key. Follow the walkers so the rest of the type can be compared, and + // record the rename so it is visible rather than assumed. + marks.renamed[next.String()] = true flatten(path, sf.Type, member, out, marks, freeForm) continue } @@ -340,7 +361,7 @@ func flatten(path *structpath.PathNode, typ reflect.Type, v any, out map[string] case nil: // A JSON null carries no scalar leaf. default: - out[path.String()] = render(value) + out[path.String()] = renderWire(value) if freeForm { marks.freeForm[path.String()] = true } @@ -365,15 +386,32 @@ func embeddedSliceField(typ reflect.Type, key string) (reflect.StructField, bool return reflect.StructField{}, false } -// render normalises a scalar so a value decoded from JSON and the same value read out of -// the struct compare equal: JSON numbers decode to float64, the struct holds int64 and -// friends, and a nil pointer reads back as nil. -func render(v any) string { +// renderWire describes a scalar as it appears on the wire, keeping the JSON type: a number and +// the string of the same digits are different values, which is the difference json:",string" +// makes and the reason the two renderings are not one function. +func renderWire(v any) string { + switch value := v.(type) { + case nil: + return "" + case json.Number: + return "number:" + normalizeNumber(value.String()) + case string: + return "string:" + value + case bool: + return fmt.Sprintf("bool:%v", value) + default: + return fmt.Sprintf("other:%v", value) + } +} + +// renderGo describes a Go value in the same vocabulary, so the two can be compared. A pointer +// is followed, since the wire carries what it points at. +func renderGo(v any) string { if v == nil { return "" } rv := reflect.ValueOf(v) - for rv.Kind() == reflect.Pointer { + for rv.Kind() == reflect.Pointer || rv.Kind() == reflect.Interface { if rv.IsNil() { return "" } @@ -381,14 +419,28 @@ func render(v any) string { } switch rv.Kind() { case reflect.Float32, reflect.Float64: - return strconv.FormatFloat(rv.Float(), 'g', -1, 64) + return "number:" + normalizeNumber(strconv.FormatFloat(rv.Float(), 'f', -1, 64)) case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - return strconv.FormatFloat(float64(rv.Int()), 'g', -1, 64) + return "number:" + normalizeNumber(strconv.FormatInt(rv.Int(), 10)) case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - return strconv.FormatFloat(float64(rv.Uint()), 'g', -1, 64) + return "number:" + normalizeNumber(strconv.FormatUint(rv.Uint(), 10)) + case reflect.String: + return "string:" + rv.String() + case reflect.Bool: + return fmt.Sprintf("bool:%v", rv.Bool()) default: - return fmt.Sprintf("%v", rv.Interface()) + return fmt.Sprintf("other:%v", rv.Interface()) + } +} + +// normalizeNumber renders a number the same way whichever side it came from, so 1 and 1.0 and +// 1e0 compare equal without letting a string masquerade as a number. +func normalizeNumber(text string) string { + f, err := strconv.ParseFloat(text, 64) + if err != nil { + return text } + return strconv.FormatFloat(f, 'g', -1, 64) } // skippedByTag reports whether the last segment of path names a field structaccess @@ -536,6 +588,7 @@ func (r Report) Filter(known []string) (Report, []string) { GetFailed: dropExact(r.GetFailed), ContainerUnreachable: dropExact(r.ContainerUnreachable), WalkDuplicated: r.WalkDuplicated, + RenamedByConvention: r.RenamedByConvention, ValidateFailed: dropExact(r.ValidateFailed), ValueMismatch: dropExact(r.ValueMismatch), // These two are categories, not per-path lists, so the caller decides what to do with diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go index 650828ede2a..c054c0d2a0f 100644 --- a/bundle/direct/dresources/structs_test.go +++ b/bundle/direct/dresources/structs_test.go @@ -72,6 +72,14 @@ func testAgreesWithJSON(t *testing.T, typeOf func(*Adapter) reflect.Type, known report, stale := report.Filter(known[resourceType]) require.Empty(t, stale, "these recorded divergences no longer occur -- remove them from the list: %v", stale) + // The EmbeddedSlice convention renames exactly one key: __embed__ carries the slice + // while the walkers put its elements at the parent path. Anything else renamed would + // be a change to the convention. + for _, path := range report.RenamedByConvention { + assert.Equal(t, "__embed__", path, "only __embed__ is renamed by convention") + } + report.RenamedByConvention = nil + // Free-form any fields are opaque to the packages; which resources have one is stable, // so it is ratcheted by name rather than logged away. assert.ElementsMatch(t, freeFormFields[resourceType], topLevelNames(report.InsideFreeFormField),