Merge df0a98066a into 72dde793d8

## Changes
The workspace extensions filer should not read or stat a notebook called
`foo` if the user calls `.Stat(ctx, "foo")`.

Instead, the filer should return a file not found error. This is because
the contract for the workspace extensions filer is to only work for
notebooks when the file path / name includes the extension (example:
`foo.ipynb` or `foo.sql` instead of just `foo`)

## Tests
Integration tests.

bundle/config/mutator/prepend_workspace_prefix.go

@@ -44,6 +44,11 @@ func (m *prependWorkspacePrefix) Apply(ctx context.Context, b *bundle.Bundle) di
 					return dyn.InvalidValue, fmt.Errorf("expected string, got %s", v.Kind())
 				}
 
+				// Skip prefixing if the path does not start with /, it might be variable reference or smth else.
+				if !strings.HasPrefix(path, "/") {
+					return pv, nil
+				}
+
 				for _, prefix := range skipPrefixes {
 					if strings.HasPrefix(path, prefix) {
 						return pv, nil

bundle/config/mutator/prepend_workspace_prefix_test.go

@@ -31,6 +31,14 @@ func TestPrependWorkspacePrefix(t *testing.T) {
 			path:     "/Volumes/Users/test",
 			expected: "/Volumes/Users/test",
 		},
+		{
+			path:     "~/test",
+			expected: "~/test",
+		},
+		{
+			path:     "${workspace.file_path}/test",
+			expected: "${workspace.file_path}/test",
+		},
 	}
 
 	for _, tc := range testCases {

cmd/fs/helpers_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/databricks/cli/cmd/root"
+	"github.com/databricks/cli/libs/fakefs"
 	"github.com/databricks/cli/libs/filer"
 	"github.com/databricks/databricks-sdk-go/experimental/mocks"
 	"github.com/spf13/cobra"
@@ -84,7 +85,7 @@ func setupTest(t *testing.T) (*validArgs, *cobra.Command, *mocks.MockWorkspaceCl
 	cmd, m := setupCommand(t)
 
 	fakeFilerForPath := func(ctx context.Context, fullPath string) (filer.Filer, string, error) {
-		fakeFiler := filer.NewFakeFiler(map[string]filer.FakeFileInfo{
+		fakeFiler := filer.NewFakeFiler(map[string]fakefs.FileInfo{
 			"dir":       {FakeName: "root", FakeDir: true},
 			"dir/dirA":  {FakeDir: true},
 			"dir/dirB":  {FakeDir: true},

internal/filer_test.go

@@ -723,6 +723,63 @@ func TestAccWorkspaceFilesExtensionsDirectoriesAreNotNotebooks(t *testing.T) {
 	assert.ErrorIs(t, err, fs.ErrNotExist)
 }
 
+func TestAccWorkspaceFilesExtensionsNotebooksAreNotReadAsFiles(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	wf, _ := setupWsfsExtensionsFiler(t)
+
+	// Create a notebook
+	err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb")))
+	require.NoError(t, err)
+
+	// Reading foo should fail. Even though the WSFS name for the notebook is foo
+	// reading the notebook should only work with the .ipynb extension.
+	_, err = wf.Read(ctx, "foo")
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+
+	_, err = wf.Read(ctx, "foo.ipynb")
+	assert.NoError(t, err)
+}
+
+func TestAccWorkspaceFilesExtensionsNotebooksAreNotStatAsFiles(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	wf, _ := setupWsfsExtensionsFiler(t)
+
+	// Create a notebook
+	err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb")))
+	require.NoError(t, err)
+
+	// Stating foo should fail. Even though the WSFS name for the notebook is foo
+	// stating the notebook should only work with the .ipynb extension.
+	_, err = wf.Stat(ctx, "foo")
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+
+	_, err = wf.Stat(ctx, "foo.ipynb")
+	assert.NoError(t, err)
+}
+
+func TestAccWorkspaceFilesExtensionsNotebooksAreNotDeletedAsFiles(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	wf, _ := setupWsfsExtensionsFiler(t)
+
+	// Create a notebook
+	err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb")))
+	require.NoError(t, err)
+
+	// Deleting foo should fail. Even though the WSFS name for the notebook is foo
+	// deleting the notebook should only work with the .ipynb extension.
+	err = wf.Delete(ctx, "foo")
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+
+	err = wf.Delete(ctx, "foo.ipynb")
+	assert.NoError(t, err)
+}
+
 func TestAccWorkspaceFilesExtensions_ExportFormatIsPreserved(t *testing.T) {
 	t.Parallel()
 

libs/fakefs/fakefs.go

@@ -1,18 +1,21 @@
 package fakefs
 
 import (
+	"fmt"
 	"io/fs"
 	"time"
 )
 
+var ErrNotImplemented = fmt.Errorf("not implemented")
+
 // DirEntry is a fake implementation of [fs.DirEntry].
 type DirEntry struct {
-	FileInfo
+	fs.FileInfo
 }
 
 func (entry DirEntry) Type() fs.FileMode {
 	typ := fs.ModePerm
-	if entry.FakeDir {
+	if entry.IsDir() {
 		typ |= fs.ModeDir
 	}
 	return typ
@@ -53,3 +56,32 @@ func (info FileInfo) IsDir() bool {
 func (info FileInfo) Sys() any {
 	return nil
 }
+
+// File is a fake implementation of [fs.File].
+type File struct {
+	FileInfo fs.FileInfo
+}
+
+func (f File) Close() error {
+	return nil
+}
+
+func (f File) Read(p []byte) (n int, err error) {
+	return 0, ErrNotImplemented
+}
+
+func (f File) Stat() (fs.FileInfo, error) {
+	return f.FileInfo, nil
+}
+
+// FS is a fake implementation of [fs.FS].
+type FS map[string]fs.File
+
+func (f FS) Open(name string) (fs.File, error) {
+	e, ok := f[name]
+	if !ok {
+		return nil, fs.ErrNotExist
+	}
+
+	return e, nil
+}

libs/fakefs/fakefs_test.go

@@ -0,0 +1,38 @@
+package fakefs
+
+import (
+	"io/fs"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestFile(t *testing.T) {
+	var fakefile fs.File = File{
+		FileInfo: FileInfo{
+			FakeName: "file",
+		},
+	}
+
+	_, err := fakefile.Read([]byte{})
+	assert.ErrorIs(t, err, ErrNotImplemented)
+
+	fi, err := fakefile.Stat()
+	assert.NoError(t, err)
+	assert.Equal(t, "file", fi.Name())
+
+	err = fakefile.Close()
+	assert.NoError(t, err)
+}
+
+func TestFS(t *testing.T) {
+	var fakefs fs.FS = FS{
+		"file": File{},
+	}
+
+	_, err := fakefs.Open("doesntexist")
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+
+	_, err = fakefs.Open("file")
+	assert.NoError(t, err)
+}

libs/filer/completer/completer_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/databricks/cli/cmd/root"
+	"github.com/databricks/cli/libs/fakefs"
 	"github.com/databricks/cli/libs/filer"
 	"github.com/databricks/databricks-sdk-go/experimental/mocks"
 	"github.com/spf13/cobra"
@@ -17,7 +18,7 @@ func setupCompleter(t *testing.T, onlyDirs bool) *completer {
 	// Needed to make type context.valueCtx for mockFilerForPath
 	ctx = root.SetWorkspaceClient(ctx, mocks.NewMockWorkspaceClient(t).WorkspaceClient)
 
-	fakeFiler := filer.NewFakeFiler(map[string]filer.FakeFileInfo{
+	fakeFiler := filer.NewFakeFiler(map[string]fakefs.FileInfo{
 		"dir":       {FakeName: "root", FakeDir: true},
 		"dir/dirA":  {FakeDir: true},
 		"dir/dirB":  {FakeDir: true},

libs/filer/fake_filer.go

@@ -8,58 +8,12 @@ import (
 	"path"
 	"sort"
 	"strings"
-	"time"
+
+	"github.com/databricks/cli/libs/fakefs"
 )
 
-type FakeDirEntry struct {
-	FakeFileInfo
-}
-
-func (entry FakeDirEntry) Type() fs.FileMode {
-	typ := fs.ModePerm
-	if entry.FakeDir {
-		typ |= fs.ModeDir
-	}
-	return typ
-}
-
-func (entry FakeDirEntry) Info() (fs.FileInfo, error) {
-	return entry.FakeFileInfo, nil
-}
-
-type FakeFileInfo struct {
-	FakeName string
-	FakeSize int64
-	FakeDir  bool
-	FakeMode fs.FileMode
-}
-
-func (info FakeFileInfo) Name() string {
-	return info.FakeName
-}
-
-func (info FakeFileInfo) Size() int64 {
-	return info.FakeSize
-}
-
-func (info FakeFileInfo) Mode() fs.FileMode {
-	return info.FakeMode
-}
-
-func (info FakeFileInfo) ModTime() time.Time {
-	return time.Now()
-}
-
-func (info FakeFileInfo) IsDir() bool {
-	return info.FakeDir
-}
-
-func (info FakeFileInfo) Sys() any {
-	return nil
-}
-
 type FakeFiler struct {
-	entries map[string]FakeFileInfo
+	entries map[string]fakefs.FileInfo
 }
 
 func (f *FakeFiler) Write(ctx context.Context, p string, reader io.Reader, mode ...WriteMode) error {
@@ -97,7 +51,7 @@ func (f *FakeFiler) ReadDir(ctx context.Context, p string) ([]fs.DirEntry, error
 			continue
 		}
 
-		out = append(out, FakeDirEntry{v})
+		out = append(out, fakefs.DirEntry{FileInfo: v})
 	}
 
 	sort.Slice(out, func(i, j int) bool { return out[i].Name() < out[j].Name() })
@@ -117,7 +71,11 @@ func (f *FakeFiler) Stat(ctx context.Context, path string) (fs.FileInfo, error)
 	return entry, nil
 }
 
-func NewFakeFiler(entries map[string]FakeFileInfo) *FakeFiler {
+// NewFakeFiler creates a new fake [Filer] instance with the given entries.
+// It sets the [Name] field of each entry to the base name of the path.
+//
+// This is meant to be used in tests.
+func NewFakeFiler(entries map[string]fakefs.FileInfo) *FakeFiler {
 	fakeFiler := &FakeFiler{
 		entries: entries,
 	}

libs/filer/fake_filer_test.go

@@ -0,0 +1,98 @@
+package filer
+
+import (
+	"context"
+	"io"
+	"io/fs"
+	"testing"
+
+	"github.com/databricks/cli/libs/fakefs"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestFakeFiler_Read(t *testing.T) {
+	f := NewFakeFiler(map[string]fakefs.FileInfo{
+		"file": {},
+	})
+
+	ctx := context.Background()
+	r, err := f.Read(ctx, "file")
+	require.NoError(t, err)
+	contents, err := io.ReadAll(r)
+	require.NoError(t, err)
+
+	// Contents of every file is "foo".
+	assert.Equal(t, "foo", string(contents))
+}
+
+func TestFakeFiler_Read_NotFound(t *testing.T) {
+	f := NewFakeFiler(map[string]fakefs.FileInfo{
+		"foo": {},
+	})
+
+	ctx := context.Background()
+	_, err := f.Read(ctx, "bar")
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+}
+
+func TestFakeFiler_ReadDir_NotFound(t *testing.T) {
+	f := NewFakeFiler(map[string]fakefs.FileInfo{
+		"dir1": {FakeDir: true},
+	})
+
+	ctx := context.Background()
+	_, err := f.ReadDir(ctx, "dir2")
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+}
+
+func TestFakeFiler_ReadDir_NotADirectory(t *testing.T) {
+	f := NewFakeFiler(map[string]fakefs.FileInfo{
+		"file": {},
+	})
+
+	ctx := context.Background()
+	_, err := f.ReadDir(ctx, "file")
+	assert.ErrorIs(t, err, fs.ErrInvalid)
+}
+
+func TestFakeFiler_ReadDir(t *testing.T) {
+	f := NewFakeFiler(map[string]fakefs.FileInfo{
+		"dir1":       {FakeDir: true},
+		"dir1/file2": {},
+		"dir1/dir2":  {FakeDir: true},
+	})
+
+	ctx := context.Background()
+	entries, err := f.ReadDir(ctx, "dir1/")
+	require.NoError(t, err)
+	require.Len(t, entries, 2)
+
+	// The entries are sorted by name.
+	assert.Equal(t, "dir2", entries[0].Name())
+	assert.True(t, entries[0].IsDir())
+	assert.Equal(t, "file2", entries[1].Name())
+	assert.False(t, entries[1].IsDir())
+}
+
+func TestFakeFiler_Stat(t *testing.T) {
+	f := NewFakeFiler(map[string]fakefs.FileInfo{
+		"file": {},
+	})
+
+	ctx := context.Background()
+	info, err := f.Stat(ctx, "file")
+	require.NoError(t, err)
+
+	assert.Equal(t, "file", info.Name())
+}
+
+func TestFakeFiler_Stat_NotFound(t *testing.T) {
+	f := NewFakeFiler(map[string]fakefs.FileInfo{
+		"foo": {},
+	})
+
+	ctx := context.Background()
+	_, err := f.Stat(ctx, "bar")
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+}

libs/filer/fs_test.go

@@ -6,6 +6,7 @@ import (
 	"io/fs"
 	"testing"
 
+	"github.com/databricks/cli/libs/fakefs"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -35,7 +36,7 @@ func TestFsDirImplementsFsReadDirFile(t *testing.T) {
 }
 
 func fakeFS() fs.FS {
-	fakeFiler := NewFakeFiler(map[string]FakeFileInfo{
+	fakeFiler := NewFakeFiler(map[string]fakefs.FileInfo{
 		".":     {FakeName: "root", FakeDir: true},
 		"dirA":  {FakeDir: true},
 		"dirB":  {FakeDir: true},

libs/filer/workspace_files_extensions_client.go

@@ -244,6 +244,17 @@ func (w *workspaceFilesExtensionsClient) Write(ctx context.Context, name string,
 
 // Try to read the file as a regular file. If the file is not found, try to read it as a notebook.
 func (w *workspaceFilesExtensionsClient) Read(ctx context.Context, name string) (io.ReadCloser, error) {
+	// Ensure that the file / notebook exists. We do this check here to avoid reading
+	// the content of a notebook called `foo` when the user actually wanted
+	// to read the content of a file called `foo`.
+	//
+	// To read the content of a notebook called `foo` in the workspace the user
+	// should use the name with the extension included like `foo.ipynb` or `foo.sql`.
+	_, err := w.Stat(ctx, name)
+	if err != nil {
+		return nil, err
+	}
+
 	r, err := w.wsfs.Read(ctx, name)
 
 	// If the file is not found, it might be a notebook.
@@ -276,7 +287,18 @@ func (w *workspaceFilesExtensionsClient) Delete(ctx context.Context, name string
 		return ReadOnlyError{"delete"}
 	}
 
-	err := w.wsfs.Delete(ctx, name, mode...)
+	// Ensure that the file / notebook exists. We do this check here to avoid
+	// deleting the a notebook called `foo` when the user actually wanted to
+	// delete a file called `foo`.
+	//
+	// To delete a notebook called `foo` in the workspace the user should use the
+	// name with the extension included like `foo.ipynb` or `foo.sql`.
+	_, err := w.Stat(ctx, name)
+	if err != nil {
+		return err
+	}
+
+	err = w.wsfs.Delete(ctx, name, mode...)
 
 	// If the file is not found, it might be a notebook.
 	if errors.As(err, &FileDoesNotExistError{}) {
@@ -315,7 +337,24 @@ func (w *workspaceFilesExtensionsClient) Stat(ctx context.Context, name string)
 		return wsfsFileInfo{ObjectInfo: stat.ObjectInfo}, nil
 	}
 
-	return info, err
+	if err != nil {
+		return nil, err
+	}
+
+	// If an object is found and it is a notebook, return a FileDoesNotExistError.
+	// If a notebook is found by the workspace files client, without having stripped
+	// the extension, this implies that no file with the same name exists.
+	//
+	// This check is done to avoid returning the stat for a notebook called `foo`
+	// when the user actually wanted to stat a file called `foo`.
+	//
+	// To stat the metadata of a notebook called `foo` in the workspace the user
+	// should use the name with the extension included like `foo.ipynb` or `foo.sql`.
+	if info.Sys().(workspace.ObjectInfo).ObjectType == workspace.ObjectTypeNotebook {
+		return nil, FileDoesNotExistError{name}
+	}
+
+	return info, nil
 }
 
 // Note: The import API returns opaque internal errors for namespace clashes

libs/notebook/detect_test.go

@@ -7,6 +7,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/databricks/cli/libs/fakefs"
 	"github.com/databricks/databricks-sdk-go/service/workspace"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -100,11 +101,21 @@ func TestDetectFileWithLongHeader(t *testing.T) {
 	assert.False(t, nb)
 }
 
+type fileInfoWithWorkspaceInfo struct {
+	fakefs.FileInfo
+
+	oi workspace.ObjectInfo
+}
+
+func (f fileInfoWithWorkspaceInfo) WorkspaceObjectInfo() workspace.ObjectInfo {
+	return f.oi
+}
+
 func TestDetectWithObjectInfo(t *testing.T) {
-	fakeFS := &fakeFS{
-		fakeFile{
-			fakeFileInfo{
-				workspace.ObjectInfo{
+	fakefs := fakefs.FS{
+		"file.py": fakefs.File{
+			FileInfo: fileInfoWithWorkspaceInfo{
+				oi: workspace.ObjectInfo{
 					ObjectType: workspace.ObjectTypeNotebook,
 					Language:   workspace.LanguagePython,
 				},
@@ -112,7 +123,7 @@ func TestDetectWithObjectInfo(t *testing.T) {
 		},
 	}
 
-	nb, lang, err := DetectWithFS(fakeFS, "doesntmatter")
+	nb, lang, err := DetectWithFS(fakefs, "file.py")
 	require.NoError(t, err)
 	assert.True(t, nb)
 	assert.Equal(t, workspace.LanguagePython, lang)

libs/notebook/fakefs_test.go

@@ -1,77 +0,0 @@
-package notebook
-
-import (
-	"fmt"
-	"io/fs"
-	"time"
-
-	"github.com/databricks/databricks-sdk-go/service/workspace"
-)
-
-type fakeFS struct {
-	fakeFile
-}
-
-type fakeFile struct {
-	fakeFileInfo
-}
-
-func (f fakeFile) Close() error {
-	return nil
-}
-
-func (f fakeFile) Read(p []byte) (n int, err error) {
-	return 0, fmt.Errorf("not implemented")
-}
-
-func (f fakeFile) Stat() (fs.FileInfo, error) {
-	return f.fakeFileInfo, nil
-}
-
-type fakeFileInfo struct {
-	oi workspace.ObjectInfo
-}
-
-func (f fakeFileInfo) WorkspaceObjectInfo() workspace.ObjectInfo {
-	return f.oi
-}
-
-func (f fakeFileInfo) Name() string {
-	return ""
-}
-
-func (f fakeFileInfo) Size() int64 {
-	return 0
-}
-
-func (f fakeFileInfo) Mode() fs.FileMode {
-	return 0
-}
-
-func (f fakeFileInfo) ModTime() time.Time {
-	return time.Time{}
-}
-
-func (f fakeFileInfo) IsDir() bool {
-	return false
-}
-
-func (f fakeFileInfo) Sys() any {
-	return nil
-}
-
-func (f fakeFS) Open(name string) (fs.File, error) {
-	return f.fakeFile, nil
-}
-
-func (f fakeFS) Stat(name string) (fs.FileInfo, error) {
-	panic("not implemented")
-}
-
-func (f fakeFS) ReadDir(name string) ([]fs.DirEntry, error) {
-	panic("not implemented")
-}
-
-func (f fakeFS) ReadFile(name string) ([]byte, error) {
-	panic("not implemented")
-}