Fix glob expansion after running a generic build command (#1662)

## Changes

This didn't work as expected because the generic build mutator called
into the type-specific build mutator in the middle of the function. This
invalidated the `config.Artifact` pointer that was being mutated later
on, effectively hiding these mutations from its caller.

To fix this, I turned glob expansion into its own mutator. It now works
as expected, _and_ produces better errors if the glob patterns are
invalid or do not match files.

## Tests

Unit tests.

Manual verification:
```
% databricks bundle deploy
Building sbt_example...

Error: target/scala-2.12/sbt-e[xam22ple*.jar: syntax error in pattern
  at artifacts.sbt_example.files[1].source
  in databricks.yml:15:17
```

bundle/artifacts/build.go

@@ -3,10 +3,8 @@ package artifacts
 import (
 	"context"
 	"fmt"
-	"path/filepath"
 
 	"github.com/databricks/cli/bundle"
-	"github.com/databricks/cli/bundle/config"
 	"github.com/databricks/cli/libs/diag"
 )
 
@@ -35,6 +33,8 @@ func (m *build) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
 		return diag.Errorf("artifact doesn't exist: %s", m.name)
 	}
 
+	var mutators []bundle.Mutator
+
 	// Skip building if build command is not specified or infered
 	if artifact.BuildCommand == "" {
 		// If no build command was specified or infered and there is no
@@ -45,44 +45,13 @@ func (m *build) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
 
 		// We can skip calling build mutator if there is no build command
 		// But we still need to expand glob references in files source path.
-		diags := expandGlobReference(artifact)
-		return diags
-	}
-
-	diags := bundle.Apply(ctx, b, getBuildMutator(artifact.Type, m.name))
-	if diags.HasError() {
-		return diags
+	} else {
+		mutators = append(mutators, getBuildMutator(artifact.Type, m.name))
 	}
 
 	// We need to expand glob reference after build mutator is applied because
 	// if we do it before, any files that are generated by build command will
 	// not be included into artifact.Files and thus will not be uploaded.
-	d := expandGlobReference(artifact)
-	return diags.Extend(d)
-}
-
-func expandGlobReference(artifact *config.Artifact) diag.Diagnostics {
-	var diags diag.Diagnostics
-
-	// Expand any glob reference in files source path
-	files := make([]config.ArtifactFile, 0, len(artifact.Files))
-	for _, f := range artifact.Files {
-		matches, err := filepath.Glob(f.Source)
-		if err != nil {
-			return diags.Extend(diag.Errorf("unable to find files for %s: %v", f.Source, err))
-		}
-
-		if len(matches) == 0 {
-			return diags.Extend(diag.Errorf("no files found for %s", f.Source))
-		}
-
-		for _, match := range matches {
-			files = append(files, config.ArtifactFile{
-				Source: match,
-			})
-		}
-	}
-
-	artifact.Files = files
-	return diags
+	mutators = append(mutators, &expandGlobs{name: m.name})
+	return bundle.Apply(ctx, b, bundle.Seq(mutators...))
 }

bundle/artifacts/expand_globs.go

@@ -0,0 +1,110 @@
+package artifacts
+
+import (
+	"context"
+	"fmt"
+	"path/filepath"
+
+	"github.com/databricks/cli/bundle"
+	"github.com/databricks/cli/libs/diag"
+	"github.com/databricks/cli/libs/dyn"
+)
+
+type expandGlobs struct {
+	name string
+}
+
+func (m *expandGlobs) Name() string {
+	return fmt.Sprintf("artifacts.ExpandGlobs(%s)", m.name)
+}
+
+func createGlobError(v dyn.Value, p dyn.Path, message string) diag.Diagnostic {
+	// The pattern contained in v is an absolute path.
+	// Make it relative to the value's location to make it more readable.
+	source := v.MustString()
+	if l := v.Location(); l.File != "" {
+		rel, err := filepath.Rel(filepath.Dir(l.File), source)
+		if err == nil {
+			source = rel
+		}
+	}
+
+	return 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(),
+		},
+	}
+}
+
+func (m *expandGlobs) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
+	// Base path for this mutator.
+	// This path is set with the list of expanded globs when done.
+	base := dyn.NewPath(
+		dyn.Key("artifacts"),
+		dyn.Key(m.name),
+		dyn.Key("files"),
+	)
+
+	// Pattern to match the source key in the files sequence.
+	pattern := dyn.NewPatternFromPath(base).Append(
+		dyn.AnyIndex(),
+		dyn.Key("source"),
+	)
+
+	var diags diag.Diagnostics
+	err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
+		var output []dyn.Value
+		_, err := dyn.MapByPattern(v, pattern, func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
+			if v.Kind() != dyn.KindString {
+				return v, nil
+			}
+
+			source := v.MustString()
+
+			// Expand any glob reference in files source path
+			matches, err := filepath.Glob(source)
+			if err != nil {
+				diags = diags.Append(createGlobError(v, p, err.Error()))
+
+				// Continue processing and leave this value unchanged.
+				return v, nil
+			}
+
+			if len(matches) == 0 {
+				diags = diags.Append(createGlobError(v, p, "no matching files"))
+
+				// Continue processing and leave this value unchanged.
+				return v, nil
+			}
+
+			for _, match := range matches {
+				output = append(output, dyn.V(
+					map[string]dyn.Value{
+						"source": dyn.NewValue(match, v.Locations()),
+					},
+				))
+			}
+
+			return v, nil
+		})
+
+		if err != nil || diags.HasError() {
+			return v, err
+		}
+
+		// Set the expanded globs back into the configuration.
+		return dyn.SetByPath(v, base, dyn.V(output))
+	})
+
+	if err != nil {
+		return diag.FromErr(err)
+	}
+
+	return diags
+}

bundle/artifacts/expand_globs_test.go

@@ -0,0 +1,156 @@
+package artifacts
+
+import (
+	"context"
+	"fmt"
+	"path/filepath"
+	"testing"
+
+	"github.com/databricks/cli/bundle"
+	"github.com/databricks/cli/bundle/config"
+	"github.com/databricks/cli/bundle/internal/bundletest"
+	"github.com/databricks/cli/internal/testutil"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestExpandGlobs_Nominal(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	testutil.Touch(t, tmpDir, "aa1.txt")
+	testutil.Touch(t, tmpDir, "aa2.txt")
+	testutil.Touch(t, tmpDir, "bb.txt")
+	testutil.Touch(t, tmpDir, "bc.txt")
+
+	b := &bundle.Bundle{
+		RootPath: tmpDir,
+		Config: config.Root{
+			Artifacts: config.Artifacts{
+				"test": {
+					Files: []config.ArtifactFile{
+						{Source: "./aa*.txt"},
+						{Source: "./b[bc].txt"},
+					},
+				},
+			},
+		},
+	}
+
+	bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml"))
+
+	ctx := context.Background()
+	diags := bundle.Apply(ctx, b, bundle.Seq(
+		// Run prepare first to make paths absolute.
+		&prepare{"test"},
+		&expandGlobs{"test"},
+	))
+	require.NoError(t, diags.Error())
+
+	// Assert that the expanded paths are correct.
+	a, ok := b.Config.Artifacts["test"]
+	if !assert.True(t, ok) {
+		return
+	}
+	assert.Len(t, a.Files, 4)
+	assert.Equal(t, filepath.Join(tmpDir, "aa1.txt"), a.Files[0].Source)
+	assert.Equal(t, filepath.Join(tmpDir, "aa2.txt"), a.Files[1].Source)
+	assert.Equal(t, filepath.Join(tmpDir, "bb.txt"), a.Files[2].Source)
+	assert.Equal(t, filepath.Join(tmpDir, "bc.txt"), a.Files[3].Source)
+}
+
+func TestExpandGlobs_InvalidPattern(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	b := &bundle.Bundle{
+		RootPath: tmpDir,
+		Config: config.Root{
+			Artifacts: config.Artifacts{
+				"test": {
+					Files: []config.ArtifactFile{
+						{Source: "a[.txt"},
+						{Source: "./a[.txt"},
+						{Source: "../a[.txt"},
+						{Source: "subdir/a[.txt"},
+					},
+				},
+			},
+		},
+	}
+
+	bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml"))
+
+	ctx := context.Background()
+	diags := bundle.Apply(ctx, b, bundle.Seq(
+		// Run prepare first to make paths absolute.
+		&prepare{"test"},
+		&expandGlobs{"test"},
+	))
+
+	assert.Len(t, diags, 4)
+	assert.Equal(t, fmt.Sprintf("%s: syntax error in pattern", filepath.Clean("a[.txt")), diags[0].Summary)
+	assert.Equal(t, filepath.Join(tmpDir, "databricks.yml"), diags[0].Locations[0].File)
+	assert.Equal(t, "artifacts.test.files[0].source", diags[0].Paths[0].String())
+	assert.Equal(t, fmt.Sprintf("%s: syntax error in pattern", filepath.Clean("a[.txt")), diags[1].Summary)
+	assert.Equal(t, filepath.Join(tmpDir, "databricks.yml"), diags[1].Locations[0].File)
+	assert.Equal(t, "artifacts.test.files[1].source", diags[1].Paths[0].String())
+	assert.Equal(t, fmt.Sprintf("%s: syntax error in pattern", filepath.Clean("../a[.txt")), diags[2].Summary)
+	assert.Equal(t, filepath.Join(tmpDir, "databricks.yml"), diags[2].Locations[0].File)
+	assert.Equal(t, "artifacts.test.files[2].source", diags[2].Paths[0].String())
+	assert.Equal(t, fmt.Sprintf("%s: syntax error in pattern", filepath.Clean("subdir/a[.txt")), diags[3].Summary)
+	assert.Equal(t, filepath.Join(tmpDir, "databricks.yml"), diags[3].Locations[0].File)
+	assert.Equal(t, "artifacts.test.files[3].source", diags[3].Paths[0].String())
+}
+
+func TestExpandGlobs_NoMatches(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	testutil.Touch(t, tmpDir, "a1.txt")
+	testutil.Touch(t, tmpDir, "a2.txt")
+	testutil.Touch(t, tmpDir, "b1.txt")
+	testutil.Touch(t, tmpDir, "b2.txt")
+
+	b := &bundle.Bundle{
+		RootPath: tmpDir,
+		Config: config.Root{
+			Artifacts: config.Artifacts{
+				"test": {
+					Files: []config.ArtifactFile{
+						{Source: "a*.txt"},
+						{Source: "b*.txt"},
+						{Source: "c*.txt"},
+						{Source: "d*.txt"},
+					},
+				},
+			},
+		},
+	}
+
+	bundletest.SetLocation(b, "artifacts", filepath.Join(tmpDir, "databricks.yml"))
+
+	ctx := context.Background()
+	diags := bundle.Apply(ctx, b, bundle.Seq(
+		// Run prepare first to make paths absolute.
+		&prepare{"test"},
+		&expandGlobs{"test"},
+	))
+
+	assert.Len(t, diags, 2)
+	assert.Equal(t, "c*.txt: no matching files", diags[0].Summary)
+	assert.Equal(t, filepath.Join(tmpDir, "databricks.yml"), diags[0].Locations[0].File)
+	assert.Equal(t, "artifacts.test.files[2].source", diags[0].Paths[0].String())
+	assert.Equal(t, "d*.txt: no matching files", diags[1].Summary)
+	assert.Equal(t, filepath.Join(tmpDir, "databricks.yml"), diags[1].Locations[0].File)
+	assert.Equal(t, "artifacts.test.files[3].source", diags[1].Paths[0].String())
+
+	// Assert that the original paths are unchanged.
+	a, ok := b.Config.Artifacts["test"]
+	if !assert.True(t, ok) {
+		return
+	}
+
+	assert.Len(t, a.Files, 4)
+	assert.Equal(t, "a*.txt", filepath.Base(a.Files[0].Source))
+	assert.Equal(t, "b*.txt", filepath.Base(a.Files[1].Source))
+	assert.Equal(t, "c*.txt", filepath.Base(a.Files[2].Source))
+	assert.Equal(t, "d*.txt", filepath.Base(a.Files[3].Source))
+}

bundle/artifacts/upload_test.go

@@ -110,5 +110,5 @@ func TestExpandGlobFilesSourceWithNoMatches(t *testing.T) {
 	}
 
 	diags := bundle.Apply(context.Background(), b, bundle.Seq(bm, u))
-	require.ErrorContains(t, diags.Error(), "no files found for")
+	require.ErrorContains(t, diags.Error(), "no matching files")
 }