Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass copy of dyn.Path to callback function #1747

Merged
merged 3 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
pietern marked this conversation as resolved.
Show resolved Hide resolved
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but was unable to create a failing test case for this. The dyn.Foreach function is called as "inner" function for dyn.Map, so it is already provided with a cloned path.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

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",
})
}
Loading