From c05c0cd9416b53e8c2554495d98c33acc7689347 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 7 Mar 2024 14:56:50 +0100 Subject: [PATCH] Include `dyn.Path` as argument to the visit callback function (#1260) ## Changes This change means the callback supplied to `dyn.Foreach` can introspect the path of the value it is being called for. It also prepares for allowing visiting path patterns where the exact path is not known upfront. ## Tests Unit tests. --- bundle/config/mutator/merge_job_clusters.go | 2 +- bundle/config/mutator/merge_job_tasks.go | 2 +- .../config/mutator/merge_pipeline_clusters.go | 2 +- bundle/config/mutator/rewrite_sync_paths.go | 4 +- bundle/config/root.go | 4 +- bundle/deploy/terraform/tfdyn/convert_job.go | 4 +- libs/dyn/merge/elements_by_key.go | 2 +- libs/dyn/visit.go | 4 +- libs/dyn/visit_get.go | 2 +- libs/dyn/visit_map.go | 8 +-- libs/dyn/visit_map_test.go | 50 +++++++++++++------ libs/dyn/visit_set.go | 2 +- 12 files changed, 52 insertions(+), 34 deletions(-) diff --git a/bundle/config/mutator/merge_job_clusters.go b/bundle/config/mutator/merge_job_clusters.go index e8378f48..9c99cfaa 100644 --- a/bundle/config/mutator/merge_job_clusters.go +++ b/bundle/config/mutator/merge_job_clusters.go @@ -35,7 +35,7 @@ func (m *mergeJobClusters) Apply(ctx context.Context, b *bundle.Bundle) error { return v, nil } - return dyn.Map(v, "resources.jobs", dyn.Foreach(func(job dyn.Value) (dyn.Value, error) { + return dyn.Map(v, "resources.jobs", dyn.Foreach(func(_ dyn.Path, job dyn.Value) (dyn.Value, error) { return dyn.Map(job, "job_clusters", merge.ElementsByKey("job_cluster_key", m.jobClusterKey)) })) }) diff --git a/bundle/config/mutator/merge_job_tasks.go b/bundle/config/mutator/merge_job_tasks.go index 7394368a..91aee3a0 100644 --- a/bundle/config/mutator/merge_job_tasks.go +++ b/bundle/config/mutator/merge_job_tasks.go @@ -35,7 +35,7 @@ func (m *mergeJobTasks) Apply(ctx context.Context, b *bundle.Bundle) error { return v, nil } - return dyn.Map(v, "resources.jobs", dyn.Foreach(func(job dyn.Value) (dyn.Value, error) { + return dyn.Map(v, "resources.jobs", dyn.Foreach(func(_ dyn.Path, job dyn.Value) (dyn.Value, error) { return dyn.Map(job, "tasks", merge.ElementsByKey("task_key", m.taskKeyString)) })) }) diff --git a/bundle/config/mutator/merge_pipeline_clusters.go b/bundle/config/mutator/merge_pipeline_clusters.go index 777ce611..552d997b 100644 --- a/bundle/config/mutator/merge_pipeline_clusters.go +++ b/bundle/config/mutator/merge_pipeline_clusters.go @@ -38,7 +38,7 @@ func (m *mergePipelineClusters) Apply(ctx context.Context, b *bundle.Bundle) err return v, nil } - return dyn.Map(v, "resources.pipelines", dyn.Foreach(func(pipeline dyn.Value) (dyn.Value, error) { + return dyn.Map(v, "resources.pipelines", dyn.Foreach(func(_ dyn.Path, pipeline dyn.Value) (dyn.Value, error) { return dyn.Map(pipeline, "clusters", merge.ElementsByKey("label", m.clusterLabel)) })) }) diff --git a/bundle/config/mutator/rewrite_sync_paths.go b/bundle/config/mutator/rewrite_sync_paths.go index c1761690..5e17b1b5 100644 --- a/bundle/config/mutator/rewrite_sync_paths.go +++ b/bundle/config/mutator/rewrite_sync_paths.go @@ -30,7 +30,7 @@ func (m *rewriteSyncPaths) Name() string { // // Then the resulting value will be "bar/somefile.*". func (m *rewriteSyncPaths) makeRelativeTo(root string) dyn.MapFunc { - return func(v dyn.Value) (dyn.Value, error) { + return func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { dir := filepath.Dir(v.Location().File) rel, err := filepath.Rel(root, dir) if err != nil { @@ -43,7 +43,7 @@ func (m *rewriteSyncPaths) makeRelativeTo(root string) dyn.MapFunc { func (m *rewriteSyncPaths) Apply(ctx context.Context, b *bundle.Bundle) error { return b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { - return dyn.Map(v, "sync", func(v dyn.Value) (nv dyn.Value, err error) { + return dyn.Map(v, "sync", func(_ dyn.Path, v dyn.Value) (nv dyn.Value, err error) { v, err = dyn.Map(v, "include", dyn.Foreach(m.makeRelativeTo(b.Config.Path))) if err != nil { return dyn.NilValue, err diff --git a/bundle/config/root.go b/bundle/config/root.go index c8b6c599..8e1ff650 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -409,14 +409,14 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) { } // For each target, rewrite the variables block. - return dyn.Map(v, "targets", dyn.Foreach(func(target dyn.Value) (dyn.Value, error) { + return dyn.Map(v, "targets", dyn.Foreach(func(_ dyn.Path, target dyn.Value) (dyn.Value, error) { // Confirm it has a variables block. if target.Get("variables") == dyn.NilValue { return target, nil } // For each variable, normalize its contents if it is a single string. - return dyn.Map(target, "variables", dyn.Foreach(func(variable dyn.Value) (dyn.Value, error) { + return dyn.Map(target, "variables", dyn.Foreach(func(_ dyn.Path, variable dyn.Value) (dyn.Value, error) { if variable.Kind() != dyn.KindString { return variable, nil } diff --git a/bundle/deploy/terraform/tfdyn/convert_job.go b/bundle/deploy/terraform/tfdyn/convert_job.go index b488df15..778af1ad 100644 --- a/bundle/deploy/terraform/tfdyn/convert_job.go +++ b/bundle/deploy/terraform/tfdyn/convert_job.go @@ -30,7 +30,7 @@ func convertJobResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { } // Modify keys in the "git_source" block - vout, err = dyn.Map(vout, "git_source", func(v dyn.Value) (dyn.Value, error) { + vout, err = dyn.Map(vout, "git_source", func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return renameKeys(v, map[string]string{ "git_branch": "branch", "git_commit": "commit", @@ -44,7 +44,7 @@ func convertJobResource(ctx context.Context, vin dyn.Value) (dyn.Value, error) { } // Modify keys in the "task" blocks - vout, err = dyn.Map(vout, "task", dyn.Foreach(func(v dyn.Value) (dyn.Value, error) { + vout, err = dyn.Map(vout, "task", dyn.Foreach(func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return renameKeys(v, map[string]string{ "libraries": "library", }) diff --git a/libs/dyn/merge/elements_by_key.go b/libs/dyn/merge/elements_by_key.go index 3ce571bf..da20ee84 100644 --- a/libs/dyn/merge/elements_by_key.go +++ b/libs/dyn/merge/elements_by_key.go @@ -7,7 +7,7 @@ type elementsByKey struct { keyFunc func(dyn.Value) string } -func (e elementsByKey) Map(v dyn.Value) (dyn.Value, error) { +func (e elementsByKey) Map(_ dyn.Path, v dyn.Value) (dyn.Value, error) { // We know the type of this value is a sequence. // For additional defence, return self if it is not. elements, ok := v.AsSequence() diff --git a/libs/dyn/visit.go b/libs/dyn/visit.go index 077fd51c..d1a8d73b 100644 --- a/libs/dyn/visit.go +++ b/libs/dyn/visit.go @@ -44,7 +44,7 @@ type visitOptions struct { // // If this function returns an error, the original visit function call // returns this error and the value is left unmodified. - fn func(Value) (Value, error) + fn func(Path, Value) (Value, error) // If set, tolerate the absence of the last component in the path. // This option is needed to set a key in a map that is not yet present. @@ -53,7 +53,7 @@ type visitOptions struct { func visit(v Value, prefix, suffix Path, opts visitOptions) (Value, error) { if len(suffix) == 0 { - return opts.fn(v) + return opts.fn(prefix, v) } // Initialize prefix if it is empty. diff --git a/libs/dyn/visit_get.go b/libs/dyn/visit_get.go index a0f848cd..8b083fc6 100644 --- a/libs/dyn/visit_get.go +++ b/libs/dyn/visit_get.go @@ -15,7 +15,7 @@ func Get(v Value, path string) (Value, error) { func GetByPath(v Value, p Path) (Value, error) { out := InvalidValue _, err := visit(v, EmptyPath, p, visitOptions{ - fn: func(ev Value) (Value, error) { + fn: func(_ Path, ev Value) (Value, error) { // Capture the value argument to return it. out = ev return ev, nil diff --git a/libs/dyn/visit_map.go b/libs/dyn/visit_map.go index ed89baa4..e6053d9d 100644 --- a/libs/dyn/visit_map.go +++ b/libs/dyn/visit_map.go @@ -7,18 +7,18 @@ import ( ) // MapFunc is a function that maps a value to another value. -type MapFunc func(Value) (Value, error) +type MapFunc func(Path, Value) (Value, error) // Foreach returns a [MapFunc] that applies the specified [MapFunc] to each // value in a map or sequence and returns the new map or sequence. func Foreach(fn MapFunc) MapFunc { - return func(v Value) (Value, error) { + return func(p Path, v Value) (Value, error) { switch v.Kind() { case KindMap: m := maps.Clone(v.MustMap()) for key, value := range m { var err error - m[key], err = fn(value) + m[key], err = fn(p.Append(Key(key)), value) if err != nil { return InvalidValue, err } @@ -28,7 +28,7 @@ func Foreach(fn MapFunc) MapFunc { s := slices.Clone(v.MustSequence()) for i, value := range s { var err error - s[i], err = fn(value) + s[i], err = fn(p.Append(Index(i)), value) if err != nil { return InvalidValue, err } diff --git a/libs/dyn/visit_map_test.go b/libs/dyn/visit_map_test.go index 117d03f0..2659b71f 100644 --- a/libs/dyn/visit_map_test.go +++ b/libs/dyn/visit_map_test.go @@ -12,7 +12,7 @@ import ( func TestMapWithEmptyPath(t *testing.T) { // An empty path means to return the value itself. vin := dyn.V(42) - vout, err := dyn.MapByPath(dyn.InvalidValue, dyn.EmptyPath, func(v dyn.Value) (dyn.Value, error) { + vout, err := dyn.MapByPath(dyn.InvalidValue, dyn.EmptyPath, func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return vin, nil }) assert.NoError(t, err) @@ -45,7 +45,7 @@ func TestMapFuncOnMap(t *testing.T) { // Note: in the test cases below we implicitly test that the original // value is not modified as we repeatedly set values on it. - vfoo, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("foo")), func(v dyn.Value) (dyn.Value, error) { + vfoo, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("foo")), func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { assert.Equal(t, dyn.V(42), v) return dyn.V(44), nil }) @@ -55,7 +55,7 @@ func TestMapFuncOnMap(t *testing.T) { "bar": 43, }, vfoo.AsAny()) - vbar, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("bar")), func(v dyn.Value) (dyn.Value, error) { + vbar, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("bar")), func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { assert.Equal(t, dyn.V(43), v) return dyn.V(45), nil }) @@ -67,7 +67,7 @@ func TestMapFuncOnMap(t *testing.T) { // Return error from map function. var ref = fmt.Errorf("error") - verr, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("foo")), func(v dyn.Value) (dyn.Value, error) { + verr, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("foo")), func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return dyn.InvalidValue, ref }) assert.Equal(t, dyn.InvalidValue, verr) @@ -88,7 +88,7 @@ func TestMapFuncOnMapWithEmptySequence(t *testing.T) { }) for j := 0; j < len(variants); j++ { - vout, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("key")), func(v dyn.Value) (dyn.Value, error) { + vout, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Key("key")), func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return variants[j], nil }) assert.NoError(t, err) @@ -115,14 +115,14 @@ func TestMapFuncOnSequence(t *testing.T) { // Note: in the test cases below we implicitly test that the original // value is not modified as we repeatedly set values on it. - v0, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(0)), func(v dyn.Value) (dyn.Value, error) { + v0, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(0)), func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { assert.Equal(t, dyn.V(42), v) return dyn.V(44), nil }) assert.NoError(t, err) assert.Equal(t, []any{44, 43}, v0.AsAny()) - v1, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(1)), func(v dyn.Value) (dyn.Value, error) { + v1, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(1)), func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { assert.Equal(t, dyn.V(43), v) return dyn.V(45), nil }) @@ -131,7 +131,7 @@ func TestMapFuncOnSequence(t *testing.T) { // Return error from map function. var ref = fmt.Errorf("error") - verr, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(0)), func(v dyn.Value) (dyn.Value, error) { + verr, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(0)), func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return dyn.InvalidValue, ref }) assert.Equal(t, dyn.InvalidValue, verr) @@ -152,7 +152,7 @@ func TestMapFuncOnSequenceWithEmptySequence(t *testing.T) { }) for j := 0; j < len(variants); j++ { - vout, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(0)), func(v dyn.Value) (dyn.Value, error) { + vout, err := dyn.MapByPath(vin, dyn.NewPath(dyn.Index(0)), func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return variants[j], nil }) assert.NoError(t, err) @@ -170,10 +170,19 @@ func TestMapForeachOnMap(t *testing.T) { var err error // Run foreach, adding 1 to each of the elements. - vout, err := dyn.Map(vin, ".", dyn.Foreach(func(v dyn.Value) (dyn.Value, error) { + vout, err := dyn.Map(vin, ".", dyn.Foreach(func(p dyn.Path, v dyn.Value) (dyn.Value, error) { i, ok := v.AsInt() require.True(t, ok, "expected an integer") - return dyn.V(int(i) + 1), nil + switch p[0].Key() { + case "foo": + assert.EqualValues(t, 42, i) + return dyn.V(43), nil + case "bar": + assert.EqualValues(t, 43, i) + return dyn.V(44), nil + default: + return dyn.InvalidValue, fmt.Errorf("unexpected key %q", p[0].Key()) + } })) assert.NoError(t, err) assert.Equal(t, map[string]any{ @@ -196,7 +205,7 @@ func TestMapForeachOnMapError(t *testing.T) { // Check that an error from the map function propagates. var ref = fmt.Errorf("error") - _, err := dyn.Map(vin, ".", dyn.Foreach(func(v dyn.Value) (dyn.Value, error) { + _, err := dyn.Map(vin, ".", dyn.Foreach(func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return dyn.InvalidValue, ref })) assert.ErrorIs(t, err, ref) @@ -211,10 +220,19 @@ func TestMapForeachOnSequence(t *testing.T) { var err error // Run foreach, adding 1 to each of the elements. - vout, err := dyn.Map(vin, ".", dyn.Foreach(func(v dyn.Value) (dyn.Value, error) { + vout, err := dyn.Map(vin, ".", dyn.Foreach(func(p dyn.Path, v dyn.Value) (dyn.Value, error) { i, ok := v.AsInt() require.True(t, ok, "expected an integer") - return dyn.V(int(i) + 1), nil + switch p[0].Index() { + case 0: + assert.EqualValues(t, 42, i) + return dyn.V(43), nil + case 1: + assert.EqualValues(t, 43, i) + return dyn.V(44), nil + default: + return dyn.InvalidValue, fmt.Errorf("unexpected index %d", p[0].Index()) + } })) assert.NoError(t, err) assert.Equal(t, []any{43, 44}, vout.AsAny()) @@ -231,7 +249,7 @@ func TestMapForeachOnSequenceError(t *testing.T) { // Check that an error from the map function propagates. var ref = fmt.Errorf("error") - _, err := dyn.Map(vin, ".", dyn.Foreach(func(v dyn.Value) (dyn.Value, error) { + _, err := dyn.Map(vin, ".", dyn.Foreach(func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return dyn.InvalidValue, ref })) assert.ErrorIs(t, err, ref) @@ -241,7 +259,7 @@ func TestMapForeachOnOtherError(t *testing.T) { vin := dyn.V(42) // Check that if foreach is applied to something other than a map or a sequence, it returns an error. - _, err := dyn.Map(vin, ".", dyn.Foreach(func(v dyn.Value) (dyn.Value, error) { + _, err := dyn.Map(vin, ".", dyn.Foreach(func(_ dyn.Path, v dyn.Value) (dyn.Value, error) { return dyn.InvalidValue, nil })) assert.ErrorContains(t, err, "expected a map or sequence, found int") diff --git a/libs/dyn/visit_set.go b/libs/dyn/visit_set.go index fdbf41c2..3ad770f0 100644 --- a/libs/dyn/visit_set.go +++ b/libs/dyn/visit_set.go @@ -15,7 +15,7 @@ func Set(v Value, path string, nv Value) (Value, error) { // If the path doesn't exist, it returns InvalidValue and an error. func SetByPath(v Value, p Path, nv Value) (Value, error) { return visit(v, EmptyPath, p, visitOptions{ - fn: func(_ Value) (Value, error) { + fn: func(_ Path, _ Value) (Value, error) { // Return the incoming value to set it. return nv, nil },