Skip to content

Commit

Permalink
Pass copy of dyn.Path to callback function (#1747)
Browse files Browse the repository at this point in the history
## Changes

Some call sites hold on to the `dyn.Path` provided to them by the
callback. It must therefore never be mutated after the callback returns,
or these mutations leak out into unknown scope.

This change means it is no longer possible for this failure mode to
happen.

## Tests

Unit test.
  • Loading branch information
pietern authored Sep 5, 2024
1 parent f71d9e7 commit ceefa80
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 20 deletions.
7 changes: 1 addition & 6 deletions bundle/artifacts/expand_globs.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,7 @@ func createGlobError(v dyn.Value, p dyn.Path, message string) diag.Diagnostic {
Severity: diag.Error,
Summary: fmt.Sprintf("%s: %s", source, message),
Locations: []dyn.Location{v.Location()},

Paths: []dyn.Path{
// Hack to clone the path. This path copy is mutable.
// To be addressed in a later PR.
p.Append(),
},
Paths: []dyn.Path{p},
}
}

Expand Down
6 changes: 1 addition & 5 deletions bundle/config/validate/unique_resource_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package validate
import (
"context"
"fmt"
"slices"
"sort"

"github.com/databricks/cli/bundle"
Expand Down Expand Up @@ -66,10 +65,7 @@ func (m *uniqueResourceKeys) Apply(ctx context.Context, b *bundle.Bundle) diag.D
}
}

// dyn.Path under the hood is a slice. The code that walks the configuration
// tree uses the same underlying slice to track the path as it walks
// the tree. So, we need to clone it here.
m.paths = append(m.paths, slices.Clone(p))
m.paths = append(m.paths, p)
m.locations = append(m.locations, v.Locations()...)

resourceMetadata[k] = m
Expand Down
8 changes: 3 additions & 5 deletions bundle/libraries/expand_glob_references.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,10 @@ type expand struct {

func matchError(p dyn.Path, l []dyn.Location, message string) diag.Diagnostic {
return diag.Diagnostic{
Severity: diag.Error,
Summary: message,
Paths: []dyn.Path{
p.Append(),
},
Severity: diag.Error,
Summary: message,
Locations: l,
Paths: []dyn.Path{p},
}
}

Expand Down
2 changes: 1 addition & 1 deletion bundle/libraries/upload.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func collectLocalLibraries(b *bundle.Bundle) (map[string][]configLocation, error

source = filepath.Join(b.RootPath, source)
libs[source] = append(libs[source], configLocation{
configPath: p.Append(), // Hack to get the copy of path
configPath: p,
location: v.Location(),
})

Expand Down
2 changes: 1 addition & 1 deletion libs/dyn/visit.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ type visitOptions struct {

func visit(v Value, prefix Path, suffix Pattern, opts visitOptions) (Value, error) {
if len(suffix) == 0 {
return opts.fn(prefix, v)
return opts.fn(slices.Clone(prefix), v)
}

// Initialize prefix if it is empty.
Expand Down
4 changes: 2 additions & 2 deletions libs/dyn/visit_map.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func Foreach(fn MapFunc) MapFunc {
for _, pair := range m.Pairs() {
pk := pair.Key
pv := pair.Value
nv, err := fn(append(p, Key(pk.MustString())), pv)
nv, err := fn(p.Append(Key(pk.MustString())), pv)
if err != nil {
return InvalidValue, err
}
Expand All @@ -32,7 +32,7 @@ func Foreach(fn MapFunc) MapFunc {
s := slices.Clone(v.MustSequence())
for i, value := range s {
var err error
s[i], err = fn(append(p, Index(i)), value)
s[i], err = fn(p.Append(Index(i)), value)
if err != nil {
return InvalidValue, err
}
Expand Down
36 changes: 36 additions & 0 deletions libs/dyn/visit_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package dyn_test

import (
"testing"

"github.com/databricks/cli/libs/dyn"
assert "github.com/databricks/cli/libs/dyn/dynassert"
)

func TestVisitCallbackPathCopy(t *testing.T) {
vin := dyn.V(map[string]dyn.Value{
"foo": dyn.V(42),
"bar": dyn.V(43),
})

var paths []dyn.Path

// The callback should receive a copy of the path.
// If the same underlying value is used, all collected paths will be the same.
// This test uses `MapByPattern` to collect all paths in the map.
// Visit itself doesn't have public functions and we exclusively use black-box testing for this package.
_, _ = dyn.MapByPattern(vin, dyn.NewPattern(dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
paths = append(paths, p)
return v, nil
})

// Verify that the paths retained their original values.
var strings []string
for _, p := range paths {
strings = append(strings, p.String())
}
assert.ElementsMatch(t, strings, []string{
"foo",
"bar",
})
}

0 comments on commit ceefa80

Please sign in to comment.