From d25423f84178ac27e44ea63fb4d79de786f470f6 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Sat, 28 Mar 2026 20:37:10 +0100 Subject: [PATCH 1/3] Add "did you mean?" suggestions for variable reference typos When a ${...} variable reference fails to resolve, the error now suggests the closest valid path using Levenshtein distance. The algorithm walks the entire path greedily, correcting multiple segments in one suggestion. Co-authored-by: Isaac --- .../variables/did-you-mean/databricks.yml | 11 + .../variables/did-you-mean/out.test.toml | 5 + .../bundle/variables/did-you-mean/output.txt | 11 + .../bundle/variables/did-you-mean/script | 1 + .../mutator/resolve_variable_references.go | 47 ++- bundle/config/mutator/suggest.go | 367 ++++++++++++++++++ bundle/config/mutator/suggest_test.go | 239 ++++++++++++ cmd/root/flag_suggestions.go | 38 +- cmd/root/flag_suggestions_test.go | 24 +- libs/dyn/dynvar/lookup.go | 17 + libs/dyn/dynvar/resolve.go | 20 +- libs/dyn/dynvar/resolve_test.go | 31 ++ libs/textutil/levenshtein.go | 37 ++ libs/textutil/levenshtein_test.go | 35 ++ 14 files changed, 820 insertions(+), 63 deletions(-) create mode 100644 acceptance/bundle/variables/did-you-mean/databricks.yml create mode 100644 acceptance/bundle/variables/did-you-mean/out.test.toml create mode 100644 acceptance/bundle/variables/did-you-mean/output.txt create mode 100644 acceptance/bundle/variables/did-you-mean/script create mode 100644 bundle/config/mutator/suggest.go create mode 100644 bundle/config/mutator/suggest_test.go create mode 100644 libs/textutil/levenshtein.go create mode 100644 libs/textutil/levenshtein_test.go diff --git a/acceptance/bundle/variables/did-you-mean/databricks.yml b/acceptance/bundle/variables/did-you-mean/databricks.yml new file mode 100644 index 0000000000..4b8bb34816 --- /dev/null +++ b/acceptance/bundle/variables/did-you-mean/databricks.yml @@ -0,0 +1,11 @@ +bundle: + name: did-you-mean + +variables: + my_cluster_id: + default: "abc-123" + +resources: + jobs: + my_job: + name: "test-${var.my_clster_id}" diff --git a/acceptance/bundle/variables/did-you-mean/out.test.toml b/acceptance/bundle/variables/did-you-mean/out.test.toml new file mode 100644 index 0000000000..d560f1de04 --- /dev/null +++ b/acceptance/bundle/variables/did-you-mean/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/variables/did-you-mean/output.txt b/acceptance/bundle/variables/did-you-mean/output.txt new file mode 100644 index 0000000000..27dce749cc --- /dev/null +++ b/acceptance/bundle/variables/did-you-mean/output.txt @@ -0,0 +1,11 @@ +Error: reference does not exist: ${var.my_clster_id}. Did you mean ${var.my_cluster_id}? + +Name: did-you-mean +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/did-you-mean/default + +Found 1 error + +Exit code: 1 diff --git a/acceptance/bundle/variables/did-you-mean/script b/acceptance/bundle/variables/did-you-mean/script new file mode 100644 index 0000000000..72555b332a --- /dev/null +++ b/acceptance/bundle/variables/did-you-mean/script @@ -0,0 +1 @@ +$CLI bundle validate diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index 9868b4fb95..b7a53ee2ae 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -44,6 +44,7 @@ var defaultPrefixes = []string{ } var artifactPath = dyn.MustPathFromString("artifacts") +var resourcesPath = dyn.MustPathFromString("resources") type resolveVariableReferences struct { prefixes []string @@ -202,6 +203,8 @@ func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn // normalized, _ := convert.Normalize(b.Config, root, convert.IncludeMissingFields) + suggestFn := m.makeSuggestFn(normalized, prefixes, varPath) + // If the pattern is nil, we resolve references in the entire configuration. root, err := dyn.MapByPattern(root, m.pattern, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { // Resolve variable references in all values. @@ -235,8 +238,50 @@ func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn } } + // For references starting with "resources" that are not in + // the resolution prefixes: validate the path against the + // normalized tree. If invalid, emit a warning with a + // suggestion. Either way, skip resolution (resources are + // resolved later by terraform). + if path.HasPrefix(resourcesPath) { + _, lookupErr := m.lookupFn(normalized, path, b) + if lookupErr != nil && dyn.IsNoSuchKeyError(lookupErr) { + key := rewriteToVarShorthand(path.String()) + msg := fmt.Sprintf("reference does not exist: ${%s}", key) + if suggestion := suggestFn(key); suggestion != "" { + msg += fmt.Sprintf(". Did you mean ${%s}?", suggestion) + } + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: msg, + }) + } + return dyn.InvalidValue, dynvar.ErrSkipResolution + } + + // Check for prefix typos before skipping. If the first + // component is close to a valid prefix, emit a warning + // with a suggestion. The reference is left unresolved to + // avoid breaking existing behavior. + if len(path) > 0 { + firstKey := path[0].Key() + prefixNames := m.suggestPrefixNames(prefixes) + best, dist := closestMatch(firstKey, prefixNames) + if best != "" && dist > 0 { + corrected := make(dyn.Path, len(path)) + copy(corrected, path) + corrected[0] = dyn.Key(best) + suggestion := rewriteToVarShorthand(corrected.String()) + key := rewriteToVarShorthand(path.String()) + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("reference does not exist: ${%s}. Did you mean ${%s}?", key, suggestion), + }) + } + } + return dyn.InvalidValue, dynvar.ErrSkipResolution - }) + }, dynvar.WithSuggestFn(suggestFn)) }) if err != nil { return dyn.InvalidValue, err diff --git a/bundle/config/mutator/suggest.go b/bundle/config/mutator/suggest.go new file mode 100644 index 0000000000..07cb3aaa0d --- /dev/null +++ b/bundle/config/mutator/suggest.go @@ -0,0 +1,367 @@ +package mutator + +import ( + "math" + "reflect" + "strings" + + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/dynvar" + "github.com/databricks/cli/libs/structs/structaccess" + "github.com/databricks/cli/libs/structs/structtag" + "github.com/databricks/cli/libs/textutil" +) + +// suggestionThreshold returns the maximum edit distance for a given key length. +func suggestionThreshold(keyLen int) int { + return min(3, max(1, keyLen/2)) +} + +// closestMatch finds the candidate with the smallest Levenshtein distance to key, +// within the suggestion threshold. Returns ("", math.MaxInt) if no match is close enough. +func closestMatch(key string, candidates []string) (string, int) { + best := "" + bestDist := math.MaxInt + threshold := suggestionThreshold(len(key)) + for _, c := range candidates { + d := textutil.LevenshteinDistance(key, c) + if d < bestDist { + bestDist = d + best = c + } + } + if bestDist > threshold { + return "", math.MaxInt + } + return best, bestDist +} + +// structFieldNames enumerates all valid JSON field names for a struct type, +// including embedded/flattened fields and bundle:"readonly" fields. +// It excludes bundle:"internal", json:"-", unexported, and EmbeddedSlice fields. +func structFieldNames(t reflect.Type) []string { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return nil + } + + var names []string + for i := range t.NumField() { + sf := t.Field(i) + if sf.PkgPath != "" { + continue // unexported + } + if sf.Name == structaccess.EmbeddedSliceFieldName { + continue + } + + // For anonymous (embedded) structs without json tags, flatten their fields. + jsonTag := sf.Tag.Get("json") + if sf.Anonymous && jsonTag == "" { + ft := sf.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + names = append(names, structFieldNames(ft)...) + continue + } + + name := structtag.JSONTag(jsonTag).Name() + if name == "-" || name == "" { + continue + } + + // Exclude internal fields but include readonly fields (like resource IDs). + btag := structtag.BundleTag(sf.Tag.Get("bundle")) + if btag.Internal() { + continue + } + + names = append(names, name) + } + return names +} + +// mapKeysFromDyn extracts string keys from a dyn.Value map. +func mapKeysFromDyn(v dyn.Value) []string { + m, ok := v.AsMap() + if !ok { + return nil + } + var keys []string + for _, k := range m.Keys() { + if s, ok := k.AsString(); ok { + keys = append(keys, s) + } + } + return keys +} + +// rewriteToVarShorthand converts "variables.X.value" back to "var.X". +func rewriteToVarShorthand(key string) string { + if strings.HasPrefix(key, "variables.") && strings.HasSuffix(key, ".value") { + middle := key[len("variables."):] + middle = middle[:len(middle)-len(".value")] + if !strings.Contains(middle, ".") { + return "var." + middle + } + } + return key +} + +// suggestPrefixNames returns the list of prefix names used for suggestion +// matching. This includes the resolution prefixes plus additional well-known +// prefixes like "var" and "resources" that users commonly reference. +func (m *resolveVariableReferences) suggestPrefixNames(prefixes []dyn.Path) []string { + seen := map[string]bool{} + var names []string + for _, p := range prefixes { + key := p[0].Key() + if !seen[key] { + seen[key] = true + names = append(names, key) + } + } + // Add well-known prefixes that may not be in the resolution set. + for _, extra := range []string{"var", "resources"} { + if !seen[extra] { + seen[extra] = true + names = append(names, extra) + } + } + return names +} + +// makeSuggestFn builds a SuggestFn that uses Go type information and runtime +// dyn values to generate suggestions for unresolved variable references. +// +// The algorithm walks the entire path segment by segment, greedily correcting +// each wrong segment. This means multiple segments can be corrected in a single +// suggestion (e.g., ${bundl.nme} → ${bundle.name}). +// +// At each segment: +// - Struct types: candidates come from the Go type via reflection (works even +// for fields not in the config tree, like resource IDs). +// - Map types: candidates come from the actual runtime dyn.Value keys. +func (m *resolveVariableReferences) makeSuggestFn( + normalized dyn.Value, + prefixes []dyn.Path, + varPath dyn.Path, +) dynvar.SuggestFn { + return func(key string) string { + return m.suggest(key, normalized, prefixes, varPath) + } +} + +func (m *resolveVariableReferences) suggest( + key string, + normalized dyn.Value, + prefixes []dyn.Path, + varPath dyn.Path, +) string { + // Parse the key into path segments. + path, err := dyn.NewPathFromString(key) + if err != nil || len(path) == 0 { + return "" + } + + // Handle var.X → variables.X.value rewriting for internal lookup. + isVar := path.HasPrefix(varPath) + if isVar { + newPath := dyn.NewPath(dyn.Key("variables"), path[1], dyn.Key("value")) + if len(path) > 2 { + newPath = newPath.Append(path[2:]...) + } + path = newPath + } + + // Extract segment strings from the path. Only support simple key segments. + segments := make([]string, len(path)) + for i, c := range path { + segments[i] = c.Key() + } + + suggestion := suggestPath(segments, normalized) + if suggestion == "" { + return "" + } + + if isVar { + suggestion = rewriteToVarShorthand(suggestion) + } + return suggestion +} + +// suggestPath walks the config type tree and dyn value tree segment by segment, +// greedily correcting each wrong segment. Returns the corrected dot-separated +// path if any corrections were made, or "" if no suggestion can be produced. +func suggestPath(segments []string, normalized dyn.Value) string { + currentType := reflect.TypeOf(config.Root{}) + currentDyn := normalized + corrected := make([]string, len(segments)) + hasFix := false + + for i, seg := range segments { + // Dereference pointer types. + for currentType != nil && currentType.Kind() == reflect.Pointer { + currentType = currentType.Elem() + } + + if currentType == nil { + return "" + } + + var candidates []string + var nextType reflect.Type + + switch currentType.Kind() { + case reflect.Struct: + candidates = structFieldNames(currentType) + + // Try exact match first. + sf, _, ok := structaccess.FindStructFieldByKeyType(currentType, seg) + if ok { + corrected[i] = seg + nextType = sf.Type + } else { + // Exact match failed. Also check readonly fields that + // FindStructFieldByKeyType skips. + if hasFieldName(currentType, seg) { + corrected[i] = seg + nextType = fieldTypeByName(currentType, seg) + } + } + + case reflect.Map: + if currentType.Key().Kind() != reflect.String { + return "" + } + candidates = mapKeysFromDyn(currentDyn) + nextType = currentType.Elem() + + // Check if the key exists in the map. + if currentDyn.IsValid() { + if _, err := dyn.GetByPath(currentDyn, dyn.NewPath(dyn.Key(seg))); err == nil { + corrected[i] = seg + } + } + + default: + return "" + } + + // If not matched, try fuzzy correction. + if corrected[i] == "" { + best, _ := closestMatch(seg, candidates) + if best == "" { + return "" + } + corrected[i] = best + hasFix = true + + // Advance type using the corrected segment. + if currentType.Kind() == reflect.Struct { + sf, _, ok := structaccess.FindStructFieldByKeyType(currentType, best) + if ok { + nextType = sf.Type + } else { + nextType = fieldTypeByName(currentType, best) + } + } + // For maps, nextType is already set to the map element type. + } + + // Advance dyn value for the next segment. + if currentDyn.IsValid() { + next, err := dyn.GetByPath(currentDyn, dyn.NewPath(dyn.Key(corrected[i]))) + if err == nil { + currentDyn = next + } else { + currentDyn = dyn.InvalidValue + } + } + + currentType = nextType + } + + if !hasFix { + return "" + } + + return strings.Join(corrected, ".") +} + +// hasFieldName checks if a struct type has a field with the given JSON name, +// including readonly fields (which FindStructFieldByKeyType skips). +func hasFieldName(t reflect.Type, name string) bool { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return false + } + for i := range t.NumField() { + sf := t.Field(i) + if sf.PkgPath != "" { + continue + } + if sf.Anonymous && sf.Tag.Get("json") == "" { + ft := sf.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + if hasFieldName(ft, name) { + return true + } + continue + } + jsonName := structtag.JSONTag(sf.Tag.Get("json")).Name() + if jsonName == name { + btag := structtag.BundleTag(sf.Tag.Get("bundle")) + if btag.Internal() { + continue + } + return true + } + } + return false +} + +// fieldTypeByName returns the reflect.Type of a struct field identified by its +// JSON name, searching embedded structs. Includes readonly fields. +func fieldTypeByName(t reflect.Type, name string) reflect.Type { + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return nil + } + for i := range t.NumField() { + sf := t.Field(i) + if sf.PkgPath != "" { + continue + } + if sf.Anonymous && sf.Tag.Get("json") == "" { + ft := sf.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + if result := fieldTypeByName(ft, name); result != nil { + return result + } + continue + } + jsonName := structtag.JSONTag(sf.Tag.Get("json")).Name() + if jsonName == name { + btag := structtag.BundleTag(sf.Tag.Get("bundle")) + if btag.Internal() { + continue + } + return sf.Type + } + } + return nil +} diff --git a/bundle/config/mutator/suggest_test.go b/bundle/config/mutator/suggest_test.go new file mode 100644 index 0000000000..e69b2cfb6f --- /dev/null +++ b/bundle/config/mutator/suggest_test.go @@ -0,0 +1,239 @@ +package mutator + +import ( + "math" + "reflect" + "testing" + + "github.com/databricks/cli/libs/dyn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSuggestionThreshold(t *testing.T) { + tests := []struct { + keyLen int + want int + }{ + {0, 1}, // max(1, 0) = 1 + {1, 1}, // max(1, 0) = 1 + {2, 1}, // max(1, 1) = 1 + {3, 1}, // max(1, 1) = 1 + {4, 2}, + {5, 2}, + {6, 3}, + {7, 3}, + {100, 3}, // capped at 3 + } + for _, tt := range tests { + assert.Equal(t, tt.want, suggestionThreshold(tt.keyLen), "keyLen=%d", tt.keyLen) + } +} + +func TestClosestMatch(t *testing.T) { + candidates := []string{"bundle", "workspace", "variables", "resources"} + + tests := []struct { + key string + wantName string + wantDist int + }{ + {"bundle", "bundle", 0}, + {"bundl", "bundle", 1}, + {"bunlde", "bundle", 2}, + {"xxxxxxx", "", math.MaxInt}, // too far from any candidate + {"var", "", math.MaxInt}, // distance 7+ from all candidates, too far + } + + for _, tt := range tests { + name, dist := closestMatch(tt.key, candidates) + assert.Equal(t, tt.wantName, name, "key=%q", tt.key) + assert.Equal(t, tt.wantDist, dist, "key=%q", tt.key) + } +} + +func TestClosestMatchEmptyCandidates(t *testing.T) { + name, dist := closestMatch("foo", nil) + assert.Equal(t, "", name) + assert.Equal(t, math.MaxInt, dist) +} + +func TestStructFieldNames(t *testing.T) { + type Embedded struct { + EmbeddedField string `json:"embedded_field"` + } + + type TestStruct struct { + Embedded + Name string `json:"name"` + ID string `json:"id,omitempty" bundle:"readonly"` + Internal string `json:"internal_field" bundle:"internal"` + Skipped string `json:"-"` + NoTag string + } + + names := structFieldNames(reflect.TypeOf(TestStruct{})) + + assert.Contains(t, names, "embedded_field") + assert.Contains(t, names, "name") + assert.Contains(t, names, "id") // readonly should be included + assert.NotContains(t, names, "internal_field") // internal should be excluded + assert.NotContains(t, names, "-") + assert.NotContains(t, names, "NoTag") // No json tag → excluded +} + +func TestStructFieldNamesNonStruct(t *testing.T) { + assert.Nil(t, structFieldNames(reflect.TypeOf(""))) + assert.Nil(t, structFieldNames(reflect.TypeOf(42))) +} + +func TestMapKeysFromDyn(t *testing.T) { + v := dyn.V(map[string]dyn.Value{ + "alpha": dyn.V("a"), + "beta": dyn.V("b"), + "gamma": dyn.V("c"), + }) + keys := mapKeysFromDyn(v) + assert.ElementsMatch(t, []string{"alpha", "beta", "gamma"}, keys) +} + +func TestMapKeysFromDynInvalid(t *testing.T) { + assert.Nil(t, mapKeysFromDyn(dyn.InvalidValue)) + assert.Nil(t, mapKeysFromDyn(dyn.V("not a map"))) +} + +func TestRewriteToVarShorthand(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"variables.my_cluster.value", "var.my_cluster"}, + {"variables.x.value", "var.x"}, + {"bundle.name", "bundle.name"}, // not a variables path + {"variables.foo.bar.value", "variables.foo.bar.value"}, // nested - don't rewrite + {"variables.foo.default", "variables.foo.default"}, // not .value suffix + } + for _, tt := range tests { + assert.Equal(t, tt.want, rewriteToVarShorthand(tt.in), "in=%q", tt.in) + } +} + +func TestSuggestPathSingleSegmentFix(t *testing.T) { + // Simulate: ${bundle.nme} where "name" is the correct field. + normalized := dyn.V(map[string]dyn.Value{ + "bundle": dyn.V(map[string]dyn.Value{ + "name": dyn.V("test"), + }), + }) + + result := suggestPath([]string{"bundle", "nme"}, normalized) + assert.Equal(t, "bundle.name", result) +} + +func TestSuggestPathMultiSegmentFix(t *testing.T) { + // Simulate: ${bundl.nme} where both segments are wrong. + normalized := dyn.V(map[string]dyn.Value{ + "bundle": dyn.V(map[string]dyn.Value{ + "name": dyn.V("test"), + }), + }) + + result := suggestPath([]string{"bundl", "nme"}, normalized) + assert.Equal(t, "bundle.name", result) +} + +func TestSuggestPathAllCorrect(t *testing.T) { + normalized := dyn.V(map[string]dyn.Value{ + "bundle": dyn.V(map[string]dyn.Value{ + "name": dyn.V("test"), + }), + }) + + // All segments match exactly → no suggestion needed. + result := suggestPath([]string{"bundle", "name"}, normalized) + assert.Equal(t, "", result) +} + +func TestSuggestPathNoMatch(t *testing.T) { + normalized := dyn.V(map[string]dyn.Value{ + "bundle": dyn.V(map[string]dyn.Value{ + "name": dyn.V("test"), + }), + }) + + // "zzzzz" is too far from any candidate. + result := suggestPath([]string{"bundle", "zzzzz"}, normalized) + assert.Equal(t, "", result) +} + +func TestSuggestPathMapKey(t *testing.T) { + // Simulate: ${variables.my_clster.value} where "my_cluster" is the correct key. + normalized := dyn.V(map[string]dyn.Value{ + "variables": dyn.V(map[string]dyn.Value{ + "my_cluster": dyn.V(map[string]dyn.Value{ + "value": dyn.V("abc"), + }), + }), + }) + + result := suggestPath([]string{"variables", "my_clster", "value"}, normalized) + assert.Equal(t, "variables.my_cluster.value", result) +} + +func TestSuggestPathResourceField(t *testing.T) { + // The suggestion should work based on Go type information, even if the + // dyn value doesn't have the field. We test that structFieldNames includes + // readonly fields like "id". + normalized := dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "jobs": dyn.V(map[string]dyn.Value{ + "my_job": dyn.V(map[string]dyn.Value{ + "name": dyn.V("test-job"), + }), + }), + }), + }) + + result := suggestPath([]string{"resources", "jobs", "my_job", "nme"}, normalized) + assert.Equal(t, "resources.jobs.my_job.name", result) +} + +func TestSuggestVarRewriting(t *testing.T) { + normalized := dyn.V(map[string]dyn.Value{ + "variables": dyn.V(map[string]dyn.Value{ + "my_cluster_id": dyn.V(map[string]dyn.Value{ + "value": dyn.V("abc-123"), + }), + }), + }) + + m := &resolveVariableReferences{ + prefixes: defaultPrefixes, + } + + prefixes := []dyn.Path{dyn.MustPathFromString("variables")} + varPath := dyn.NewPath(dyn.Key("var")) + + suggestion := m.suggest("var.my_clster_id", normalized, prefixes, varPath) + require.Equal(t, "var.my_cluster_id", suggestion) +} + +func TestSuggestNoSuggestionForCorrectPath(t *testing.T) { + normalized := dyn.V(map[string]dyn.Value{ + "variables": dyn.V(map[string]dyn.Value{ + "my_cluster_id": dyn.V(map[string]dyn.Value{ + "value": dyn.V("abc-123"), + }), + }), + }) + + m := &resolveVariableReferences{ + prefixes: defaultPrefixes, + } + + prefixes := []dyn.Path{dyn.MustPathFromString("variables")} + varPath := dyn.NewPath(dyn.Key("var")) + + suggestion := m.suggest("var.my_cluster_id", normalized, prefixes, varPath) + assert.Equal(t, "", suggestion) +} diff --git a/cmd/root/flag_suggestions.go b/cmd/root/flag_suggestions.go index effef1fcca..982ede3e3c 100644 --- a/cmd/root/flag_suggestions.go +++ b/cmd/root/flag_suggestions.go @@ -5,47 +5,13 @@ import ( "fmt" "strings" + "github.com/databricks/cli/libs/textutil" "github.com/spf13/cobra" "github.com/spf13/pflag" ) const maxSuggestionDistance = 2 -// levenshteinDistance computes the edit distance between two strings. -func levenshteinDistance(a, b string) int { - if len(a) == 0 { - return len(b) - } - if len(b) == 0 { - return len(a) - } - - // Use a single row for the DP table. - prev := make([]int, len(b)+1) - for j := range len(b) + 1 { - prev[j] = j - } - - for i := range len(a) { - curr := make([]int, len(b)+1) - curr[0] = i + 1 - for j := range len(b) { - cost := 1 - if a[i] == b[j] { - cost = 0 - } - curr[j+1] = min( - curr[j]+1, // insertion - prev[j+1]+1, // deletion - prev[j]+cost, // substitution - ) - } - prev = curr - } - - return prev[len(b)] -} - // suggestFlagFromError inspects the error from Cobra for unknown-flag errors. // If a close match is found among the command's flags, it returns an enhanced error // with a "Did you mean" suggestion appended. Otherwise it returns the original error. @@ -110,7 +76,7 @@ func findClosestFlag(cmd *cobra.Command, name string) (string, int) { } seen[f.Name] = true - d := levenshteinDistance(name, f.Name) + d := textutil.LevenshteinDistance(name, f.Name) if d < bestDist { bestDist = d best = f.Name diff --git a/cmd/root/flag_suggestions_test.go b/cmd/root/flag_suggestions_test.go index 6403e6dd30..60fc39b0c7 100644 --- a/cmd/root/flag_suggestions_test.go +++ b/cmd/root/flag_suggestions_test.go @@ -2,7 +2,6 @@ package root import ( "errors" - "fmt" "testing" "github.com/spf13/cobra" @@ -20,28 +19,7 @@ func parseUnknownFlag(cmd *cobra.Command, args []string) error { return cmd.Execute() } -func TestLevenshteinDistance(t *testing.T) { - tests := []struct { - a, b string - want int - }{ - {"", "", 0}, - {"abc", "abc", 0}, - {"", "abc", 3}, - {"abc", "", 3}, - {"kitten", "sitting", 3}, - {"output", "outpu", 1}, // deletion - {"output", "ouptut", 2}, // transposition = 2 edits - {"output", "outpux", 1}, // substitution - {"output", "outputx", 1}, // insertion - } - - for _, tt := range tests { - t.Run(fmt.Sprintf("%s_%s", tt.a, tt.b), func(t *testing.T) { - assert.Equal(t, tt.want, levenshteinDistance(tt.a, tt.b)) - }) - } -} +// Levenshtein tests are in libs/textutil/levenshtein_test.go. func TestSuggestFlagFromError_LongFlagCloseMatch(t *testing.T) { cmd := &cobra.Command{Use: "test"} diff --git a/libs/dyn/dynvar/lookup.go b/libs/dyn/dynvar/lookup.go index 2bc08f47d4..5e20292c2e 100644 --- a/libs/dyn/dynvar/lookup.go +++ b/libs/dyn/dynvar/lookup.go @@ -9,6 +9,23 @@ import ( // Lookup is the type of lookup functions that can be used with [Resolve]. type Lookup func(path dyn.Path) (dyn.Value, error) +// SuggestFn is called when a variable reference cannot be resolved. +// It receives the original reference key (e.g., "bundle.nme") and returns +// a suggested correction (e.g., "bundle.name"), or "" if no suggestion is found. +type SuggestFn func(key string) string + +// ResolveOption configures the [Resolve] function. +type ResolveOption func(*resolver) + +// WithSuggestFn sets a suggestion function that is called when a reference +// cannot be resolved due to a missing key. The suggestion is appended to +// the error message as "Did you mean ${...}?". +func WithSuggestFn(fn SuggestFn) ResolveOption { + return func(r *resolver) { + r.suggestFn = fn + } +} + // ErrSkipResolution is returned by a lookup function to indicate that the // resolution of a variable reference should be skipped. var ErrSkipResolution = errors.New("skip resolution") diff --git a/libs/dyn/dynvar/resolve.go b/libs/dyn/dynvar/resolve.go index b1366d93bb..0123bd09f3 100644 --- a/libs/dyn/dynvar/resolve.go +++ b/libs/dyn/dynvar/resolve.go @@ -33,8 +33,12 @@ import ( // If a cycle is detected in the variable references, an error is returned. // If for some path the resolution function returns [ErrSkipResolution], the variable reference is left in place. // This is useful when some variable references are not yet ready to be interpolated. -func Resolve(in dyn.Value, fn Lookup) (out dyn.Value, err error) { - return resolver{in: in, fn: fn}.run() +func Resolve(in dyn.Value, fn Lookup, opts ...ResolveOption) (out dyn.Value, err error) { + r := resolver{in: in, fn: fn} + for _, opt := range opts { + opt(&r) + } + return r.run() } type lookupResult struct { @@ -46,6 +50,10 @@ type resolver struct { in dyn.Value fn Lookup + // suggestFn is called when a reference cannot be resolved to generate + // a "Did you mean?" suggestion. + suggestFn SuggestFn + refs map[string]Ref resolved map[string]dyn.Value @@ -198,7 +206,13 @@ func (r *resolver) resolveKey(key string, seen []string) (dyn.Value, error) { v, err := r.fn(p) if err != nil { if dyn.IsNoSuchKeyError(err) { - err = fmt.Errorf("reference does not exist: ${%s}", key) + msg := fmt.Sprintf("reference does not exist: ${%s}", key) + if r.suggestFn != nil { + if suggestion := r.suggestFn(key); suggestion != "" { + msg += fmt.Sprintf(". Did you mean ${%s}?", suggestion) + } + } + err = errors.New(msg) } // Cache the return value and return to the caller. diff --git a/libs/dyn/dynvar/resolve_test.go b/libs/dyn/dynvar/resolve_test.go index 5b64029bae..53cb566fcd 100644 --- a/libs/dyn/dynvar/resolve_test.go +++ b/libs/dyn/dynvar/resolve_test.go @@ -39,6 +39,37 @@ func TestResolveNotFound(t *testing.T) { require.ErrorContains(t, err, `reference does not exist: ${a}`) } +func TestResolveNotFoundWithSuggestFn(t *testing.T) { + in := dyn.V(map[string]dyn.Value{ + "alpha": dyn.V("hello"), + "b": dyn.V("${alph}"), + }) + + suggest := func(key string) string { + if key == "alph" { + return "alpha" + } + return "" + } + + _, err := dynvar.Resolve(in, dynvar.DefaultLookup(in), dynvar.WithSuggestFn(suggest)) + require.ErrorContains(t, err, `reference does not exist: ${alph}. Did you mean ${alpha}?`) +} + +func TestResolveNotFoundSuggestFnReturnsEmpty(t *testing.T) { + in := dyn.V(map[string]dyn.Value{ + "b": dyn.V("${zzzzz}"), + }) + + suggest := func(key string) string { + return "" + } + + _, err := dynvar.Resolve(in, dynvar.DefaultLookup(in), dynvar.WithSuggestFn(suggest)) + require.ErrorContains(t, err, `reference does not exist: ${zzzzz}`) + assert.NotContains(t, err.Error(), "Did you mean") +} + func TestResolveWithNesting(t *testing.T) { in := dyn.V(map[string]dyn.Value{ "a": dyn.V("${f.a}"), diff --git a/libs/textutil/levenshtein.go b/libs/textutil/levenshtein.go new file mode 100644 index 0000000000..9703e28d7e --- /dev/null +++ b/libs/textutil/levenshtein.go @@ -0,0 +1,37 @@ +package textutil + +// LevenshteinDistance computes the edit distance between two strings using +// a space-optimized dynamic programming approach. +func LevenshteinDistance(a, b string) int { + if len(a) == 0 { + return len(b) + } + if len(b) == 0 { + return len(a) + } + + // Use a single row for the DP table. + prev := make([]int, len(b)+1) + for j := range len(b) + 1 { + prev[j] = j + } + + for i := range len(a) { + curr := make([]int, len(b)+1) + curr[0] = i + 1 + for j := range len(b) { + cost := 1 + if a[i] == b[j] { + cost = 0 + } + curr[j+1] = min( + curr[j]+1, // insertion + prev[j+1]+1, // deletion + prev[j]+cost, // substitution + ) + } + prev = curr + } + + return prev[len(b)] +} diff --git a/libs/textutil/levenshtein_test.go b/libs/textutil/levenshtein_test.go new file mode 100644 index 0000000000..4768526164 --- /dev/null +++ b/libs/textutil/levenshtein_test.go @@ -0,0 +1,35 @@ +package textutil + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestLevenshteinDistance(t *testing.T) { + tests := []struct { + a, b string + want int + }{ + {"", "", 0}, + {"abc", "abc", 0}, + {"", "abc", 3}, + {"abc", "", 3}, + {"kitten", "sitting", 3}, + {"output", "outpu", 1}, // deletion + {"output", "ouptut", 2}, // transposition = 2 edits + {"output", "outpux", 1}, // substitution + {"output", "outputx", 1}, // insertion + {"a", "b", 1}, + {"ab", "ba", 2}, + } + + for _, tt := range tests { + t.Run(fmt.Sprintf("%s_%s", tt.a, tt.b), func(t *testing.T) { + assert.Equal(t, tt.want, LevenshteinDistance(tt.a, tt.b)) + // Verify symmetry. + assert.Equal(t, tt.want, LevenshteinDistance(tt.b, tt.a)) + }) + } +} From 1fd0df4841ebc9e47fcad859f72515c44db567fe Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Mar 2026 17:05:00 +0200 Subject: [PATCH 2/3] Simplify prefix typo detection to use full suggestFn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of only correcting the first path segment for prefix typos, use the full multi-segment suggestFn. This produces better suggestions (e.g., ${bundl.nme} → ${bundle.name}) and removes dead code. Co-authored-by: Isaac --- .../mutator/resolve_variable_references.go | 35 ++++++++----------- bundle/config/mutator/suggest.go | 23 ------------ bundle/config/mutator/suggest_test.go | 8 ++--- 3 files changed, 18 insertions(+), 48 deletions(-) diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index b7a53ee2ae..39c5ed33a2 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -43,8 +43,10 @@ var defaultPrefixes = []string{ "variables", } -var artifactPath = dyn.MustPathFromString("artifacts") -var resourcesPath = dyn.MustPathFromString("resources") +var ( + artifactPath = dyn.MustPathFromString("artifacts") + resourcesPath = dyn.MustPathFromString("resources") +) type resolveVariableReferences struct { prefixes []string @@ -259,25 +261,16 @@ func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn return dyn.InvalidValue, dynvar.ErrSkipResolution } - // Check for prefix typos before skipping. If the first - // component is close to a valid prefix, emit a warning - // with a suggestion. The reference is left unresolved to - // avoid breaking existing behavior. - if len(path) > 0 { - firstKey := path[0].Key() - prefixNames := m.suggestPrefixNames(prefixes) - best, dist := closestMatch(firstKey, prefixNames) - if best != "" && dist > 0 { - corrected := make(dyn.Path, len(path)) - copy(corrected, path) - corrected[0] = dyn.Key(best) - suggestion := rewriteToVarShorthand(corrected.String()) - key := rewriteToVarShorthand(path.String()) - diags = diags.Append(diag.Diagnostic{ - Severity: diag.Warning, - Summary: fmt.Sprintf("reference does not exist: ${%s}. Did you mean ${%s}?", key, suggestion), - }) - } + // Check for prefix typos before skipping. Use the full + // suggestFn to correct all segments (not just the prefix). + // The reference is left unresolved to avoid breaking + // existing behavior. + key := rewriteToVarShorthand(path.String()) + if suggestion := suggestFn(key); suggestion != "" { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("reference does not exist: ${%s}. Did you mean ${%s}?", key, suggestion), + }) } return dyn.InvalidValue, dynvar.ErrSkipResolution diff --git a/bundle/config/mutator/suggest.go b/bundle/config/mutator/suggest.go index 07cb3aaa0d..7075b97b30 100644 --- a/bundle/config/mutator/suggest.go +++ b/bundle/config/mutator/suggest.go @@ -112,29 +112,6 @@ func rewriteToVarShorthand(key string) string { return key } -// suggestPrefixNames returns the list of prefix names used for suggestion -// matching. This includes the resolution prefixes plus additional well-known -// prefixes like "var" and "resources" that users commonly reference. -func (m *resolveVariableReferences) suggestPrefixNames(prefixes []dyn.Path) []string { - seen := map[string]bool{} - var names []string - for _, p := range prefixes { - key := p[0].Key() - if !seen[key] { - seen[key] = true - names = append(names, key) - } - } - // Add well-known prefixes that may not be in the resolution set. - for _, extra := range []string{"var", "resources"} { - if !seen[extra] { - seen[extra] = true - names = append(names, extra) - } - } - return names -} - // makeSuggestFn builds a SuggestFn that uses Go type information and runtime // dyn values to generate suggestions for unresolved variable references. // diff --git a/bundle/config/mutator/suggest_test.go b/bundle/config/mutator/suggest_test.go index e69b2cfb6f..cfea021985 100644 --- a/bundle/config/mutator/suggest_test.go +++ b/bundle/config/mutator/suggest_test.go @@ -76,7 +76,7 @@ func TestStructFieldNames(t *testing.T) { assert.Contains(t, names, "embedded_field") assert.Contains(t, names, "name") - assert.Contains(t, names, "id") // readonly should be included + assert.Contains(t, names, "id") // readonly should be included assert.NotContains(t, names, "internal_field") // internal should be excluded assert.NotContains(t, names, "-") assert.NotContains(t, names, "NoTag") // No json tag → excluded @@ -109,9 +109,9 @@ func TestRewriteToVarShorthand(t *testing.T) { }{ {"variables.my_cluster.value", "var.my_cluster"}, {"variables.x.value", "var.x"}, - {"bundle.name", "bundle.name"}, // not a variables path - {"variables.foo.bar.value", "variables.foo.bar.value"}, // nested - don't rewrite - {"variables.foo.default", "variables.foo.default"}, // not .value suffix + {"bundle.name", "bundle.name"}, // not a variables path + {"variables.foo.bar.value", "variables.foo.bar.value"}, // nested - don't rewrite + {"variables.foo.default", "variables.foo.default"}, // not .value suffix } for _, tt := range tests { assert.Equal(t, tt.want, rewriteToVarShorthand(tt.in), "in=%q", tt.in) From 316b897c5592137c479a14cde83de02126d4afce Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 30 Mar 2026 17:14:54 +0200 Subject: [PATCH 3/3] Handle typos in the synthetic "var" prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the first segment is close to "var" (e.g., "vr", "va"), rewrite to variables.X.value and suggest through the normal path. This handles cases like ${vr.my_cluster_id} → ${var.my_cluster_id}. Co-authored-by: Isaac --- bundle/config/mutator/suggest.go | 18 ++++++++++++++++++ bundle/config/mutator/suggest_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/bundle/config/mutator/suggest.go b/bundle/config/mutator/suggest.go index 7075b97b30..52b8ae1f35 100644 --- a/bundle/config/mutator/suggest.go +++ b/bundle/config/mutator/suggest.go @@ -146,7 +146,16 @@ func (m *resolveVariableReferences) suggest( } // Handle var.X → variables.X.value rewriting for internal lookup. + // Also detect typos in the "var" prefix itself (e.g., "vr", "va"). isVar := path.HasPrefix(varPath) + varPrefixCorrected := false + if !isVar && len(path) >= 2 { + if c, _ := closestMatch(path[0].Key(), []string{"var"}); c != "" { + isVar = true + varPrefixCorrected = true + } + } + if isVar { newPath := dyn.NewPath(dyn.Key("variables"), path[1], dyn.Key("value")) if len(path) > 2 { @@ -162,6 +171,15 @@ func (m *resolveVariableReferences) suggest( } suggestion := suggestPath(segments, normalized) + + // If suggestPath found no deeper fixes but the var prefix itself was + // corrected, verify the rewritten path is valid and use it. + if suggestion == "" && varPrefixCorrected { + if _, err := dyn.GetByPath(normalized, path); err == nil { + suggestion = path.String() + } + } + if suggestion == "" { return "" } diff --git a/bundle/config/mutator/suggest_test.go b/bundle/config/mutator/suggest_test.go index cfea021985..b480c6e3ba 100644 --- a/bundle/config/mutator/suggest_test.go +++ b/bundle/config/mutator/suggest_test.go @@ -218,6 +218,32 @@ func TestSuggestVarRewriting(t *testing.T) { require.Equal(t, "var.my_cluster_id", suggestion) } +func TestSuggestVarPrefixTypo(t *testing.T) { + normalized := dyn.V(map[string]dyn.Value{ + "variables": dyn.V(map[string]dyn.Value{ + "my_cluster_id": dyn.V(map[string]dyn.Value{ + "value": dyn.V("abc-123"), + }), + }), + }) + + m := &resolveVariableReferences{ + prefixes: defaultPrefixes, + } + + prefixes := []dyn.Path{dyn.MustPathFromString("variables")} + varPath := dyn.NewPath(dyn.Key("var")) + + // Typo in var prefix only, variable name is correct. + assert.Equal(t, "var.my_cluster_id", m.suggest("vr.my_cluster_id", normalized, prefixes, varPath)) + + // Typo in var prefix AND variable name. + assert.Equal(t, "var.my_cluster_id", m.suggest("vr.my_clster_id", normalized, prefixes, varPath)) + + // Var prefix typo but no matching variable. + assert.Equal(t, "", m.suggest("vr.nonexistent", normalized, prefixes, varPath)) +} + func TestSuggestNoSuggestionForCorrectPath(t *testing.T) { normalized := dyn.V(map[string]dyn.Value{ "variables": dyn.V(map[string]dyn.Value{