diff --git a/.nextchanges/cli/did-you-mean-variables.md b/.nextchanges/cli/did-you-mean-variables.md new file mode 100644 index 00000000000..623ccd8f196 --- /dev/null +++ b/.nextchanges/cli/did-you-mean-variables.md @@ -0,0 +1 @@ +Error messages for failed key lookups and variable references now suggest the closest matching key if one is found. ([#6208](https://github.com/databricks/cli/pull/6208)) diff --git a/acceptance/bundle/variables/reference-typo/double-nested-typo-multiple/databricks.yml b/acceptance/bundle/variables/reference-typo/double-nested-typo-multiple/databricks.yml new file mode 100644 index 00000000000..284414fb427 --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/double-nested-typo-multiple/databricks.yml @@ -0,0 +1,21 @@ +bundle: + name: reference-typo-double-nested-typo-multiple + +variables: + cluster: + type: complex + default: + spark_version: "13.2.x" + clusters: + type: complex + default: + spark_version: "13.2.x" + +resources: + jobs: + one: + # Two typos in a single reference across nesting layers, where the outermost + # key "clustr" is within distance 2 of both "cluster" and "clusters". The + # lookup fails at the outermost key, so both are suggested and the inner typo + # ("spark_versio") is never reached. + name: ${var.clustr.spark_versio} diff --git a/acceptance/bundle/variables/reference-typo/double-nested-typo/databricks.yml b/acceptance/bundle/variables/reference-typo/double-nested-typo/databricks.yml new file mode 100644 index 00000000000..627adcff302 --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/double-nested-typo/databricks.yml @@ -0,0 +1,16 @@ +bundle: + name: reference-typo-double-nested-typo + +variables: + cluster: + type: complex + default: + spark_version: "13.2.x" + +resources: + jobs: + one: + # Two typos in a single reference across nesting layers: "clustr" (cluster) + # and "spark_versio" (spark_version). The lookup fails at the outermost key, + # so only "cluster" is suggested; the inner typo is never reached. + name: ${var.clustr.spark_versio} diff --git a/acceptance/bundle/variables/reference-typo/multiple-typos/databricks.yml b/acceptance/bundle/variables/reference-typo/multiple-typos/databricks.yml new file mode 100644 index 00000000000..53b7d7ffeba --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/multiple-typos/databricks.yml @@ -0,0 +1,21 @@ +bundle: + name: reference-typo-multiple-typos + +variables: + cluster: + type: complex + default: + spark_version: "13.2.x" + node_type_id: Standard_DS3_v2 + +resources: + jobs: + # Two typos in references to a complex (nested) variable. References are + # resolved in sorted order of their config path, and resolution stops at the + # first unresolved one, so only job "a" is reported even though "b" is wrong too. + a: + # Deeply nested typo: ${var.cluster.spark_versio} should be spark_version. + name: ${var.cluster.spark_versio} + b: + # Typo at the variable name: ${var.clustr.node_type_id} should be cluster. + name: ${var.clustr.node_type_id} diff --git a/acceptance/bundle/variables/reference-typo/multiple/databricks.yml b/acceptance/bundle/variables/reference-typo/multiple/databricks.yml new file mode 100644 index 00000000000..2071ff28c32 --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/multiple/databricks.yml @@ -0,0 +1,14 @@ +bundle: + name: reference-typo-multiple + +variables: + host: + default: https://example.test + hosts: + default: https://example.test + +resources: + jobs: + one: + # "hst" is within edit distance 2 of both "host" and "hosts" + name: ${var.hst} diff --git a/acceptance/bundle/variables/reference-typo/non-var-multiple/databricks.yml b/acceptance/bundle/variables/reference-typo/non-var-multiple/databricks.yml new file mode 100644 index 00000000000..0f43e908687 --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/non-var-multiple/databricks.yml @@ -0,0 +1,9 @@ +bundle: + name: reference-typo-non-var-multiple + +resources: + jobs: + one: + # A non-"var" reference. "stot_path" is within edit distance 2 of both + # workspace.root_path and workspace.state_path + name: ${workspace.stot_path} diff --git a/acceptance/bundle/variables/reference-typo/out.test.toml b/acceptance/bundle/variables/reference-typo/out.test.toml new file mode 100644 index 00000000000..98ea5040486 --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/variables/reference-typo/output.txt b/acceptance/bundle/variables/reference-typo/output.txt new file mode 100644 index 00000000000..72de565c011 --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/output.txt @@ -0,0 +1,99 @@ + +=== Typo of a defined variable: single suggestion + +>>> [CLI] bundle validate +Error: reference does not exist: ${var.hst} + +did you mean: + ${var.host} + +Name: reference-typo +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/reference-typo/default + +Found 1 error + +=== Typo close to two defined variables: multiple suggestions + +>>> [CLI] bundle validate +Error: reference does not exist: ${var.hst} + +did you mean one of: + ${var.host} + ${var.hosts} + +Name: reference-typo-multiple +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/reference-typo-multiple/default + +Found 1 error + +=== Typo in a non-variable reference (workspace field): multiple suggestions + +>>> [CLI] bundle validate +Error: reference does not exist: ${workspace.stot_path} + +did you mean one of: + ${workspace.root_path} + ${workspace.state_path} + +Name: reference-typo-non-var-multiple +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/reference-typo-non-var-multiple/default + +Found 1 error + +=== Nested complex variable with multiple typos: only the first is reported + +>>> [CLI] bundle validate +Error: reference does not exist: ${var.cluster.spark_versio} + +did you mean: + ${var.cluster.spark_version} + +Name: reference-typo-multiple-typos +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/reference-typo-multiple-typos/default + +Found 1 error + +=== Two typos in one reference: lookup fails at the outermost key + +>>> [CLI] bundle validate +Error: reference does not exist: ${var.clustr.spark_versio} + +did you mean: + ${var.cluster.spark_versio} + +Name: reference-typo-double-nested-typo +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/reference-typo-double-nested-typo/default + +Found 1 error + +=== Two typos in one reference, outermost close to two variables: multiple suggestions + +>>> [CLI] bundle validate +Error: reference does not exist: ${var.clustr.spark_versio} + +did you mean one of: + ${var.cluster.spark_versio} + ${var.clusters.spark_versio} + +Name: reference-typo-double-nested-typo-multiple +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/reference-typo-double-nested-typo-multiple/default + +Found 1 error diff --git a/acceptance/bundle/variables/reference-typo/script b/acceptance/bundle/variables/reference-typo/script new file mode 100644 index 00000000000..5fb854524c5 --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/script @@ -0,0 +1,17 @@ +title "Typo of a defined variable: single suggestion\n" +withdir single musterr trace $CLI bundle validate + +title "Typo close to two defined variables: multiple suggestions\n" +withdir multiple musterr trace $CLI bundle validate + +title "Typo in a non-variable reference (workspace field): multiple suggestions\n" +withdir non-var-multiple musterr trace $CLI bundle validate + +title "Nested complex variable with multiple typos: only the first is reported\n" +withdir multiple-typos musterr trace $CLI bundle validate + +title "Two typos in one reference: lookup fails at the outermost key\n" +withdir double-nested-typo musterr trace $CLI bundle validate + +title "Two typos in one reference, outermost close to two variables: multiple suggestions\n" +withdir double-nested-typo-multiple musterr trace $CLI bundle validate diff --git a/acceptance/bundle/variables/reference-typo/single/databricks.yml b/acceptance/bundle/variables/reference-typo/single/databricks.yml new file mode 100644 index 00000000000..6e4a3b438d1 --- /dev/null +++ b/acceptance/bundle/variables/reference-typo/single/databricks.yml @@ -0,0 +1,12 @@ +bundle: + name: reference-typo + +variables: + host: + default: https://example.test + +resources: + jobs: + one: + # "hst" is a typo of the "host" variable defined above + name: ${var.hst} diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index 9a03c85d362..0b1a02680d4 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "slices" + "strings" "github.com/databricks/cli/libs/dyn/merge" @@ -289,12 +290,37 @@ func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn return root, nil }) if err != nil { - diags = diags.Extend(diag.FromErr(err)) + diags = diags.Extend(resolveErrorDiags(err)) } return hasUpdates, diags } +// resolveErrorDiags renders "did you mean" suggestions as a diagnostic Detail so +// libs/diag owns the multi-line formatting. +func resolveErrorDiags(err error) diag.Diagnostics { + refErr, ok := errors.AsType[*dynvar.ReferenceError](err) + if !ok || len(refErr.Suggestions) == 0 { + return diag.FromErr(err) + } + + header := "did you mean:" + if len(refErr.Suggestions) > 1 { + header = "did you mean one of:" + } + var detail strings.Builder + detail.WriteString(header) + for _, ref := range refErr.Suggestions { + detail.WriteString("\n ${" + ref + "}") + } + + return diag.Diagnostics{{ + Severity: diag.Error, + Summary: refErr.Error(), + Detail: detail.String(), + }} +} + // selectivelyMutate applies a function to a subset of the configuration func (m *resolveVariableReferences) selectivelyMutate(b *bundle.Bundle, fn func(value dyn.Value) (dyn.Value, error)) error { return b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) { diff --git a/libs/dyn/dynvar/resolve.go b/libs/dyn/dynvar/resolve.go index 612f9c26fad..b7edf5efd7a 100644 --- a/libs/dyn/dynvar/resolve.go +++ b/libs/dyn/dynvar/resolve.go @@ -37,6 +37,17 @@ func Resolve(in dyn.Value, fn Lookup) (out dyn.Value, err error) { return resolver{in: in, fn: fn}.run() } +// ReferenceError is returned for an unresolved variable reference. Suggestions +// are carried as data so callers (which can import libs/diag) format them. +type ReferenceError struct { + Reference string // original reference text, e.g. "var.hst" + Suggestions []string // corrected references, e.g. ["var.host", "var.hosts"] +} + +func (e *ReferenceError) Error() string { + return fmt.Sprintf("reference does not exist: ${%s}", e.Reference) +} + type lookupResult struct { v dyn.Value err error @@ -215,7 +226,8 @@ 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) + // Carry suggestions as data; the caller formats them via libs/diag. + err = &ReferenceError{Reference: key, Suggestions: dyn.SuggestedReferences(err, key)} } // 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 3399f2057a6..f519272df85 100644 --- a/libs/dyn/dynvar/resolve_test.go +++ b/libs/dyn/dynvar/resolve_test.go @@ -39,6 +39,20 @@ func TestResolveNotFound(t *testing.T) { require.ErrorContains(t, err, `reference does not exist: ${a}`) } +func TestResolveNotFoundSuggestsCloseKey(t *testing.T) { + in := dyn.V(map[string]dyn.Value{ + "host": dyn.V("example.com"), + "b": dyn.V("${hst}"), + }) + + _, err := dynvar.Resolve(in, dynvar.DefaultLookup(in)) + require.ErrorContains(t, err, "reference does not exist: ${hst}") + + var refErr *dynvar.ReferenceError + require.ErrorAs(t, err, &refErr) + assert.Equal(t, []string{"host"}, refErr.Suggestions) +} + func TestResolveWithNesting(t *testing.T) { in := dyn.V(map[string]dyn.Value{ "a": dyn.V("${f.a}"), diff --git a/libs/dyn/suggest.go b/libs/dyn/suggest.go new file mode 100644 index 00000000000..ef8008aae6f --- /dev/null +++ b/libs/dyn/suggest.go @@ -0,0 +1,108 @@ +package dyn + +import ( + "fmt" + "slices" + "strings" +) + +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)] +} + +// suggestKeys returns the keys in m whose edit distance from name is at most +// maxSuggestionDistance, ordered by increasing distance. It is used to build +// "did you mean" hints for a key that was not found in the map. +func suggestKeys(m Mapping, name string) []string { + type candidate struct { + key string + dist int + } + + var candidates []candidate + for _, kv := range m.Keys() { + key := kv.MustString() + d := levenshteinDistance(name, key) + if d <= maxSuggestionDistance { + candidates = append(candidates, candidate{key, d}) + } + } + + slices.SortStableFunc(candidates, func(a, b candidate) int { + return a.dist - b.dist + }) + + suggestions := make([]string, len(candidates)) + for i, c := range candidates { + suggestions[i] = c.key + } + return suggestions +} + +// didYouMean formats a suggestion clause like `, did you mean "x"?` (or, for +// multiple candidates, `, did you mean one of: "x", "y"?`). It returns an empty +// string when there are no suggestions. +func didYouMean(suggestions []string) string { + switch len(suggestions) { + case 0: + return "" + case 1: + return fmt.Sprintf(", did you mean %q?", suggestions[0]) + default: + quoted := make([]string, len(suggestions)) + for i, s := range suggestions { + quoted[i] = fmt.Sprintf("%q", s) + } + return fmt.Sprintf(", did you mean one of: %s?", strings.Join(quoted, ", ")) + } +} + +// replaceKey returns reference with the component matching failedKey swapped for +// replacement, or just replacement if reference can't be parsed or has no match. +func replaceKey(reference, failedKey, replacement string) string { + p, err := NewPathFromString(reference) + if err != nil { + return replacement + } + for i, c := range p { + if c.Key() == failedKey { + out := p.Append() + out[i] = Key(replacement) + return out.String() + } + } + return replacement +} diff --git a/libs/dyn/suggest_test.go b/libs/dyn/suggest_test.go new file mode 100644 index 00000000000..b0aa4a87168 --- /dev/null +++ b/libs/dyn/suggest_test.go @@ -0,0 +1,164 @@ +package dyn + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestLevenshteinDistance(t *testing.T) { + tests := []struct { + a string + b string + want int + }{ + {"", "", 0}, + {"", "abc", 3}, + {"abc", "", 3}, + {"abc", "abc", 0}, + {"abc", "abd", 1}, + {"kitten", "sitting", 3}, + {"host", "hosts", 1}, + } + for _, tt := range tests { + assert.Equal(t, tt.want, levenshteinDistance(tt.a, tt.b), "levenshteinDistance(%q, %q)", tt.a, tt.b) + } +} + +func newSuggestMapping(keys ...string) Mapping { + var m Mapping + for _, k := range keys { + m.SetLoc(k, nil, V(k)) + } + return m +} + +func TestSuggestKeys(t *testing.T) { + tests := []struct { + name string + keys []string + typo string + want []string + }{ + { + // Keys within distance 2 are returned ordered by increasing + // distance; ties keep the map's insertion order. + name: "ordered by distance", + keys: []string{"host", "hosts", "token", "auth_type"}, + typo: "host", + want: []string{"host", "hosts"}, + }, + { + name: "no key close enough", + keys: []string{"host", "hosts", "token", "auth_type"}, + typo: "completely_different", + want: []string{}, + }, + { + // Distance-2 substitutions and insertions are both included. + name: "distance two included", + keys: []string{"profile", "prfile", "prof"}, + typo: "prfil", + want: []string{"prfile", "profile"}, + }, + { + name: "empty map", + keys: nil, + typo: "anything", + want: []string{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, suggestKeys(newSuggestMapping(tt.keys...), tt.typo)) + }) + } +} + +func TestDidYouMean(t *testing.T) { + tests := []struct { + name string + suggestions []string + want string + }{ + {"nil", nil, ""}, + {"empty", []string{}, ""}, + {"single", []string{"host"}, `, did you mean "host"?`}, + {"multiple", []string{"host", "hosts"}, `, did you mean one of: "host", "hosts"?`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, didYouMean(tt.suggestions)) + }) + } +} + +func TestSuggestedReferences(t *testing.T) { + tests := []struct { + name string + err error + reference string + want []string + }{ + { + name: "single suggestion", + err: noSuchKeyError{p: NewPath(Key("variables"), Key("hst")), suggestions: []string{"host"}}, + reference: "var.hst", + want: []string{"var.host"}, + }, + { + name: "multiple suggestions", + err: noSuchKeyError{p: NewPath(Key("variables"), Key("hst")), suggestions: []string{"host", "hosts"}}, + reference: "var.hst", + want: []string{"var.host", "var.hosts"}, + }, + { + name: "nested outer-key typo keeps suffix", + err: noSuchKeyError{p: NewPath(Key("variables"), Key("clustr")), suggestions: []string{"cluster"}}, + reference: "var.clustr.spark_version", + want: []string{"var.cluster.spark_version"}, + }, + { + name: "deep leaf typo keeps prefix", + err: noSuchKeyError{p: NewPath(Key("variables"), Key("cluster"), Key("value"), Key("spark_versio")), suggestions: []string{"spark_version"}}, + reference: "var.cluster.spark_versio", + want: []string{"var.cluster.spark_version"}, + }, + { + name: "index component preserved", + err: noSuchKeyError{p: NewPath(Key("variables"), Key("librariez")), suggestions: []string{"libraries"}}, + reference: "var.librariez[0].jar", + want: []string{"var.libraries[0].jar"}, + }, + { + name: "non-var prefix", + err: noSuchKeyError{p: NewPath(Key("workspace"), Key("stot_path")), suggestions: []string{"root_path", "state_path"}}, + reference: "workspace.stot_path", + want: []string{"workspace.root_path", "workspace.state_path"}, + }, + { + name: "no suggestions", + err: noSuchKeyError{p: NewPath(Key("xyz"))}, + reference: "var.xyz", + want: nil, + }, + { + name: "other error type", + err: errors.New("some other error"), + reference: "var.xyz", + want: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, SuggestedReferences(tt.err, tt.reference)) + }) + } +} + +func TestReplaceKeyFallsBackWhenKeyAbsent(t *testing.T) { + // The failed key is not present in the reference, so only the replacement + // itself is returned rather than a spliced reference. + assert.Equal(t, "host", replaceKey("var.foo", "missing", "host")) +} diff --git a/libs/dyn/visit.go b/libs/dyn/visit.go index 7ae00fa8e08..58223e792b9 100644 --- a/libs/dyn/visit.go +++ b/libs/dyn/visit.go @@ -29,11 +29,12 @@ func IsCannotTraverseNilError(err error) bool { } type noSuchKeyError struct { - p Path + p Path + suggestions []string } func (e noSuchKeyError) Error() string { - return fmt.Sprintf("key not found at %q", e.p) + return fmt.Sprintf("key not found at %q%s", e.p, didYouMean(e.suggestions)) } func IsNoSuchKeyError(err error) bool { @@ -41,6 +42,23 @@ func IsNoSuchKeyError(err error) bool { return ok } +// SuggestedReferences returns drop-in replacement references for a noSuchKeyError +// (nil otherwise), rebuilt by swapping the failed segment of reference for each +// suggestion (e.g. "var.hst" -> ["var.host"]). +func SuggestedReferences(err error, reference string) []string { + e, ok := errors.AsType[noSuchKeyError](err) + if !ok || len(e.suggestions) == 0 { + return nil + } + // Last component of e.p is the failed key (same in original and rewritten space). + failedKey := e.p[len(e.p)-1].Key() + refs := make([]string, len(e.suggestions)) + for i, s := range e.suggestions { + refs[i] = replaceKey(reference, failedKey, s) + } + return refs +} + type indexOutOfBoundsError struct { p Path } @@ -124,7 +142,7 @@ func (c pathComponent) visit(v Value, prefix Path, suffix Pattern, opts visitOpt // Lookup current value in the map. ev, ok := m.GetByString(c.key) if !ok { - return InvalidValue, noSuchKeyError{path} + return InvalidValue, noSuchKeyError{p: path, suggestions: suggestKeys(m, c.key)} } // Recursively transform the value. diff --git a/libs/dyn/visit_get_test.go b/libs/dyn/visit_get_test.go index 22dce0858b3..524b4ed5cd9 100644 --- a/libs/dyn/visit_get_test.go +++ b/libs/dyn/visit_get_test.go @@ -36,7 +36,14 @@ func TestGetOnMap(t *testing.T) { _, err = dyn.GetByPath(vin, dyn.NewPath(dyn.Key("baz"))) assert.True(t, dyn.IsNoSuchKeyError(err)) - assert.ErrorContains(t, err, `key not found at "baz"`) + // "baz" is one edit away from "bar", so the error suggests it. + assert.ErrorContains(t, err, `key not found at "baz", did you mean "bar"?`) + + // A key that is close to no existing key gets no suggestion. + _, err = dyn.GetByPath(vin, dyn.NewPath(dyn.Key("completely_different"))) + assert.True(t, dyn.IsNoSuchKeyError(err)) + assert.ErrorContains(t, err, `key not found at "completely_different"`) + assert.NotContains(t, err.Error(), "did you mean") vfoo, err := dyn.GetByPath(vin, dyn.NewPath(dyn.Key("foo"))) assert.NoError(t, err)