diff --git a/bundle/config/structstest/resources_test.go b/bundle/config/structstest/resources_test.go new file mode 100644 index 00000000000..9a49c297712 --- /dev/null +++ b/bundle/config/structstest/resources_test.go @@ -0,0 +1,149 @@ +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" +) + +// 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(), +} + +// 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{ + "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 { + 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.TypeFor[config.Resources]() + + var checked int + for field := range rt.Fields() { + 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]...) + report, stale := report.Filter(known) + require.Empty(t, stale, + "these recorded divergences no longer occur -- remove them from the list: %v", stale) + // 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. + // 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 + + 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. 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) + }) + 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..62634e13b91 --- /dev/null +++ b/bundle/config/structstest/structstest.go @@ -0,0 +1,633 @@ +// 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 ( + "bytes" + "encoding/json" + "fmt" + "reflect" + "slices" + "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" +) + +// 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 { + // 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 + + // 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 + + // 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 + + // 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 + // 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. + 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.ContainerUnreachable) == 0 && + len(r.WalkDuplicated) == 0 && len(r.RenamedByConvention) == 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}, + {"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 + } + 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, marks, err := jsonLeaves(v) + if err != nil { + return Report{}, err + } + + 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()] = renderGo(val) + }) + if err != nil { + return Report{}, fmt.Errorf("structstest: walk %s: %w", t, err) + } + + var report Report + for path, want := range jsonLeaves { + if marks.freeForm[path] { + report.InsideFreeFormField = append(report.InsideFreeFormField, path) + continue + } + 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 { + 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 renderGo(got) != want { + report.ValueMismatch = append(report.ValueMismatch, + 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) + } + } + + 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] { + // 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) + } + } + + 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 +} + +// 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. +// leafMarks records leaves that need a category of their own rather than a path-by-path +// comparison. +type leafMarks struct { + // 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 + // 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) { + marks := leafMarks{ + selfMarshaling: map[string]string{}, + 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 := decoder.Decode(&generic); err != nil { + return nil, marks, fmt.Errorf("structstest: unmarshal %T: %w", v, err) + } + out := map[string]string{} + 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, 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: + if path != nil { + marks.containers[path.String()] = true + } + 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 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 + } + if sf, _, ok := structaccess.FindStructFieldByKeyType(typ, key); ok { + memberType = sf.Type + } + } + } + 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() + } + for i, member := range value { + flatten(structpath.NewIndex(path, i), elemType, member, out, marks, freeForm) + } + case nil: + // A JSON null carries no scalar leaf. + default: + out[path.String()] = renderWire(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 { + marks.selfMarshaling[path.String()] = typ.String() + } + } +} + +// embeddedSliceField reports whether key names the struct's EmbeddedSlice field. +func embeddedSliceField(typ reflect.Type, key string) (reflect.StructField, bool) { + for sf := range typ.Fields() { + if sf.Name != structaccess.EmbeddedSliceFieldName { + continue + } + if structtag.JSONTag(sf.Tag.Get("json")).Name() == key { + return sf, true + } + } + return reflect.StructField{}, false +} + +// 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 || rv.Kind() == reflect.Interface { + if rv.IsNil() { + return "" + } + rv = rv.Elem() + } + switch rv.Kind() { + case reflect.Float32, reflect.Float64: + return "number:" + normalizeNumber(strconv.FormatFloat(rv.Float(), 'f', -1, 64)) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return "number:" + normalizeNumber(strconv.FormatInt(rv.Int(), 10)) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + 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("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 +// 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 sf := range typ.Fields() { + 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 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) + if !sf.IsExported() || sf.Name == "ForceSendFields" { + continue + } + if structaccess.IsSkippedField(sf) { + continue + } + fillNonZero(v.Field(i), depth+1) + } + default: + // Kinds that do not appear in bundle or SDK types (chan, func, complex) stay zero. + } +} + +// 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 + } + dropExact := func(paths []string) []string { + var out []string + for _, p := range paths { + if slices.Contains(known, pathOf(p)) { + used[pathOf(p)] = true + continue + } + out = append(out, p) + } + return out + } + + filtered := Report{ + WalkMissing: dropPrefix(r.WalkMissing), + WalkExtra: dropPrefix(r.WalkExtra), + 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 + // them. Dropping them here would make that decision unreachable. + SelfMarshalingScalars: r.SelfMarshalingScalars, + SelfMarshalingTypes: r.SelfMarshalingTypes, + 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 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], ".[") { + continue + } + if len(k) > len(best) { + best = k + } + } + return best, best != "" +} diff --git a/bundle/direct/dresources/structs_test.go b/bundle/direct/dresources/structs_test.go new file mode 100644 index 00000000000..c054c0d2a0f --- /dev/null +++ b/bundle/direct/dresources/structs_test.go @@ -0,0 +1,103 @@ +package dresources + +import ( + "reflect" + "slices" + "strings" + "testing" + + "github.com/databricks/cli/bundle/config/structstest" + "github.com/stretchr/testify/assert" + "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. + +// 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{} + +// 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) +} + +func TestRemoteTypeAgreesWithJSON(t *testing.T) { + testAgreesWithJSON(t, (*Adapter).RemoteType, knownDivergences) +} + +// 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, 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), + "free-form any fields changed for %s", resourceType) + report.InsideFreeFormField = nil + if len(report.SelfMarshalingScalars) > 0 { + // 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) + }) + } +} diff --git a/libs/structs/internal/jsonshapes/jsonshapes.go b/libs/structs/internal/jsonshapes/jsonshapes.go new file mode 100644 index 00000000000..6c5f07d95c8 --- /dev/null +++ b/libs/structs/internal/jsonshapes/jsonshapes.go @@ -0,0 +1,323 @@ +// 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 ( + "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. + 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 + + // 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 + // 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 //nolint:govet // the repeated json tag is the point: both embeds declare "value" +} + +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 //nolint:govet // the repeated json tag is the point: both routes reach Leaf +} + +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 //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 +// 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"` +} + +// 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, 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:"-"` + DashNamed string `json:"-,omitempty"` //nolint:staticcheck // the odd tag is the point + + unexported string //nolint:unused // present to prove it is ignored +} + +// 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. + WalkGap: []string{"value", "value"}, + WalkTypeGap: []string{"value", "value"}, + }, + { + 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. + 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"}, + 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"}, + // 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", + 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. + WalkTypeGap: []string{"name", "name"}, + }, + { + 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: "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 and dash-named fields", + Value: &SkippedField{Kept: "k", Skipped: "s", DashNamed: "d"}, //exhaustruct:ignore + JSONFields: []string{"kept", "-"}, + }, + } +} + +// 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 { + if prefix != "" { + key = prefix + "." + key + } + flattenLeaves(key, 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 new file mode 100644 index 00000000000..012228e0636 --- /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) + + got, err := jsonshapes.Leaves(shape.Value) + require.NoError(t, err) + + 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..786f9bbd0d2 --- /dev/null +++ b/libs/structs/structaccess/jsonagreement_test.go @@ -0,0 +1,81 @@ +package structaccess_test + +import ( + "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) + + leaves, err := jsonshapes.Leaves(fresh) + require.NoError(t, err) + assert.Equal(t, "written", leaves[name], + "Set wrote somewhere encoding/json does not serialize: %v", leaves) + } + + 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..ca82c72c529 --- /dev/null +++ b/libs/structs/structdiff/jsonagreement_test.go @@ -0,0 +1,95 @@ +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.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, + "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 { + if slices.Contains(shape.KnownSetGap, 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..7631aea3e12 --- /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.TypeFor[string](), + 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.TypeFor[string](), + 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..cc94d3b5e28 --- /dev/null +++ b/libs/structs/structwalk/jsonagreement_test.go @@ -0,0 +1,104 @@ +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) + emitted, err := jsonshapes.Leaves(shape.Value) + require.NoError(t, err) + var want []string + for name := range emitted { + want = append(want, name) + } + slices.Sort(want) + + 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 + } + + 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.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) + + for _, name := range shape.Unreachable { + assert.NotContains(t, visited, name, + "%q never reaches the wire, so the type walk must not offer it", name) + } + }) + } +}