From f0e8b7ea8f10226c8103844d6ccfb70ea186071f Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 21 Jan 2025 16:45:38 +0000 Subject: [PATCH] Raise an error when double underscore variable reference is used --- .../double_underscore/databricks.yml | 14 +++++++++++ .../variables/double_underscore/output.txt | 13 ++++++++++ .../bundle/variables/double_underscore/script | 1 + libs/dyn/dynvar/ref.go | 24 ++++++++++++++----- libs/dyn/dynvar/ref_test.go | 19 ++++++++++++--- libs/dyn/dynvar/resolve.go | 8 +++++-- 6 files changed, 68 insertions(+), 11 deletions(-) create mode 100644 acceptance/bundle/variables/double_underscore/databricks.yml create mode 100644 acceptance/bundle/variables/double_underscore/output.txt create mode 100644 acceptance/bundle/variables/double_underscore/script diff --git a/acceptance/bundle/variables/double_underscore/databricks.yml b/acceptance/bundle/variables/double_underscore/databricks.yml new file mode 100644 index 000000000..3bb15d42d --- /dev/null +++ b/acceptance/bundle/variables/double_underscore/databricks.yml @@ -0,0 +1,14 @@ +bundle: + name: double_underscore + +variables: + double__underscore: + description: "This is a variable with a double underscore" + default: "default" + +resources: + jobs: + test_job: + name: "test" + tasks: + - task_key: "test ${var.double__underscore}" diff --git a/acceptance/bundle/variables/double_underscore/output.txt b/acceptance/bundle/variables/double_underscore/output.txt new file mode 100644 index 000000000..e87ce5345 --- /dev/null +++ b/acceptance/bundle/variables/double_underscore/output.txt @@ -0,0 +1,13 @@ + +>>> $CLI bundle validate +Error: incorrect variable name: [${var.double__underscore}] + +Name: double_underscore +Target: default +Workspace: + User: $USERNAME + Path: /Workspace/Users/$USERNAME/.bundle/double_underscore/default + +Found 1 error + +Exit code: 1 diff --git a/acceptance/bundle/variables/double_underscore/script b/acceptance/bundle/variables/double_underscore/script new file mode 100644 index 000000000..535087615 --- /dev/null +++ b/acceptance/bundle/variables/double_underscore/script @@ -0,0 +1 @@ +trace $CLI bundle validate diff --git a/libs/dyn/dynvar/ref.go b/libs/dyn/dynvar/ref.go index a28938823..697dca87a 100644 --- a/libs/dyn/dynvar/ref.go +++ b/libs/dyn/dynvar/ref.go @@ -6,7 +6,10 @@ import ( "github.com/databricks/cli/libs/dyn" ) -var re = regexp.MustCompile(`\$\{([a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\.[a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\[[0-9]+\])*)*(\[[0-9]+\])*)\}`) +var ( + re = regexp.MustCompile(`\$\{([a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\.[a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\[[0-9]+\])*)*(\[[0-9]+\])*)\}`) + potentialVarRef = regexp.MustCompile(`\$\{[a-zA-Z0-9\.-_]+}`) +) // ref represents a variable reference. // It is a string [dyn.Value] contained in a larger [dyn.Value]. @@ -30,23 +33,32 @@ type ref struct { // - "${a.b}" // - "${a.b.c}" // - "${a} ${b} ${c}" -func newRef(v dyn.Value) (ref, bool) { +func newRef(v dyn.Value) (ref, ref, bool) { s, ok := v.AsString() if !ok { - return ref{}, false + return ref{}, ref{}, false } // Check if the string contains any variable references. m := re.FindAllStringSubmatch(s, -1) if len(m) == 0 { - return ref{}, false + // Check if the string contains any potential variable references but they are not valid. + pm := potentialVarRef.FindAllStringSubmatch(s, -1) + if len(pm) > 0 { + return ref{}, ref{ + value: v, + str: s, + matches: pm, + }, false + } + return ref{}, ref{}, false } return ref{ value: v, str: s, matches: m, - }, true + }, ref{}, true } // isPure returns true if the variable reference contains a single @@ -75,7 +87,7 @@ func IsPureVariableReference(s string) bool { // If s is a pure variable reference, this function returns the corresponding // dyn.Path. Otherwise, it returns false. func PureReferenceToPath(s string) (dyn.Path, bool) { - ref, ok := newRef(dyn.V(s)) + ref, _, ok := newRef(dyn.V(s)) if !ok { return nil, false } diff --git a/libs/dyn/dynvar/ref_test.go b/libs/dyn/dynvar/ref_test.go index 4110732f8..07d168e2e 100644 --- a/libs/dyn/dynvar/ref_test.go +++ b/libs/dyn/dynvar/ref_test.go @@ -9,7 +9,7 @@ import ( ) func TestNewRefNoString(t *testing.T) { - _, ok := newRef(dyn.V(1)) + _, _, ok := newRef(dyn.V(1)) require.False(t, ok, "should not match non-string") } @@ -19,7 +19,7 @@ func TestNewRefValidPattern(t *testing.T) { "${helloworld.world-world}": {"helloworld.world-world"}, "${hello-world.world-world}": {"hello-world.world-world"}, } { - ref, ok := newRef(dyn.V(in)) + ref, _, ok := newRef(dyn.V(in)) require.True(t, ok, "should match valid pattern: %s", in) assert.Equal(t, refs, ref.references()) } @@ -40,8 +40,21 @@ func TestNewRefInvalidPattern(t *testing.T) { "${a-a.a--a-a.id}", // fails because of -- in the second segment } for _, v := range invalid { - _, ok := newRef(dyn.V(v)) + _, pr, ok := newRef(dyn.V(v)) require.False(t, ok, "should not match invalid pattern: %s", v) + require.Empty(t, pr.matches) + } +} + +func TestNewRefInvalidPatternWithDoubleUnderscore(t *testing.T) { + invalid := []string{ + "${hello__world.world_world}", + "${hello_world.world__world}", + } + for _, v := range invalid { + _, pr, ok := newRef(dyn.V(v)) + require.False(t, ok, "should not match invalid pattern: %s", v) + require.NotEmpty(t, pr.matches) } } diff --git a/libs/dyn/dynvar/resolve.go b/libs/dyn/dynvar/resolve.go index 111da25c8..148a259b0 100644 --- a/libs/dyn/dynvar/resolve.go +++ b/libs/dyn/dynvar/resolve.go @@ -78,8 +78,12 @@ func (r *resolver) collectVariableReferences() (err error) { // First walk the input to gather all values with a variable reference. _, err = dyn.Walk(r.in, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - ref, ok := newRef(v) + ref, potentialRef, ok := newRef(v) if !ok { + if len(potentialRef.matches) > 0 { + // If the value contains a potential variable reference, we should skip it. + return dyn.InvalidValue, fmt.Errorf("incorrect variable name: %s", potentialRef.matches[0]) + } // Skip values without variable references. return v, nil } @@ -206,7 +210,7 @@ func (r *resolver) resolveKey(key string, seen []string) (dyn.Value, error) { } // If the returned value is a valid variable reference, resolve it. - ref, ok := newRef(v) + ref, _, ok := newRef(v) if ok { v, err = r.resolveRef(ref, seen) }