Skip to content

Commit

Permalink
Raise an error when double underscore variable reference is used
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewnester committed Jan 21, 2025
1 parent 34a37cf commit f0e8b7e
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 11 deletions.
14 changes: 14 additions & 0 deletions acceptance/bundle/variables/double_underscore/databricks.yml
Original file line number Diff line number Diff line change
@@ -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}"
13 changes: 13 additions & 0 deletions acceptance/bundle/variables/double_underscore/output.txt
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions acceptance/bundle/variables/double_underscore/script
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
trace $CLI bundle validate
24 changes: 18 additions & 6 deletions libs/dyn/dynvar/ref.go
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
19 changes: 16 additions & 3 deletions libs/dyn/dynvar/ref_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand All @@ -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())
}
Expand All @@ -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)
}
}

Expand Down
8 changes: 6 additions & 2 deletions libs/dyn/dynvar/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit f0e8b7e

Please sign in to comment.