From 47b46885b57b5be6e9cfa7166a27072bb55b5432 Mon Sep 17 00:00:00 2001 From: Akshay Singla Date: Thu, 5 Nov 2026 05:45:40 +0000 Subject: [PATCH 1/7] Add sandbox filesystem commands and filer --- .nextchanges/cli/sandbox-files.md | 1 + cmd/fs/cat.go | 4 +- cmd/fs/cp.go | 13 +- cmd/fs/fs.go | 21 ++- cmd/fs/helpers.go | 33 ++--- cmd/fs/helpers_test.go | 16 +++ cmd/fs/ls.go | 3 +- cmd/fs/mkdir.go | 3 +- cmd/fs/rm.go | 3 +- cmd/sandbox/fs.go | 80 +++++++++++ cmd/sandbox/fs_test.go | 148 +++++++++++++++++++++ cmd/sandbox/sandbox.go | 1 + libs/filer/sandbox_client.go | 212 ++++++++++++++++++++++++++++++ libs/filer/sandbox_client_test.go | 205 +++++++++++++++++++++++++++++ 14 files changed, 709 insertions(+), 34 deletions(-) create mode 100644 .nextchanges/cli/sandbox-files.md create mode 100644 cmd/sandbox/fs.go create mode 100644 cmd/sandbox/fs_test.go create mode 100644 libs/filer/sandbox_client.go create mode 100644 libs/filer/sandbox_client_test.go diff --git a/.nextchanges/cli/sandbox-files.md b/.nextchanges/cli/sandbox-files.md new file mode 100644 index 00000000000..633dd5a69ee --- /dev/null +++ b/.nextchanges/cli/sandbox-files.md @@ -0,0 +1 @@ +* Add `sandbox fs cp`, `ls`, `cat`, `mkdir`, and `rm` for sandbox file transfers. ([#7372](https://github.com/databricks/cli/pull/7372)) diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index c9d23c99d57..be0b578c665 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -6,7 +6,7 @@ import ( "github.com/spf13/cobra" ) -func newCatCommand() *cobra.Command { +func newCatCommand(filerForPath PathResolver) *cobra.Command { cmd := &cobra.Command{ Use: "cat FILE_PATH", Short: "Show file content.", @@ -27,10 +27,12 @@ func newCatCommand() *cobra.Command { if err != nil { return err } + defer r.Close() return cmdio.Render(ctx, r) } v := newValidArgs() + v.filerForPathFunc = filerForPath cmd.ValidArgsFunction = v.Validate return cmd diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index 734faaeb996..f1bd69ccf02 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -206,7 +206,7 @@ func trimTrailingDirSeparators(path string) string { return strings.TrimRight(path, string(os.PathSeparator)) } -func newCpCommand() *cobra.Command { +func newCpCommand(filerForPath PathResolver) *cobra.Command { cmd := &cobra.Command{ Use: "cp SOURCE_PATH TARGET_PATH", Short: "Copy files and directories.", @@ -253,14 +253,8 @@ func newCpCommand() *cobra.Command { return err } - c.sourceScheme = "" - if isDbfsPath(fullSourcePath) { - c.sourceScheme = "dbfs" - } - c.targetScheme = "" - if isDbfsPath(fullTargetPath) { - c.targetScheme = "dbfs" - } + c.sourceScheme, _ = SplitPath(fullSourcePath) + c.targetScheme, _ = SplitPath(fullTargetPath) c.sourceFiler = sourceFiler c.targetFiler = targetFiler @@ -296,6 +290,7 @@ func newCpCommand() *cobra.Command { } v := newValidArgs() + v.filerForPathFunc = filerForPath // The copy command has two paths that can be completed (SOURCE_PATH & TARGET_PATH) v.pathArgCount = 2 cmd.ValidArgsFunction = v.Validate diff --git a/cmd/fs/fs.go b/cmd/fs/fs.go index 074bd31a64c..1f320b19019 100644 --- a/cmd/fs/fs.go +++ b/cmd/fs/fs.go @@ -1,10 +1,21 @@ package fs import ( + "context" + + "github.com/databricks/cli/libs/filer" "github.com/spf13/cobra" ) +// PathResolver selects a filesystem and removes its prefix from a path. +type PathResolver func(context.Context, string) (filer.Filer, string, error) + func New() *cobra.Command { + return NewWithPathResolver(filerForPath) +} + +// NewWithPathResolver returns filesystem commands using the supplied path resolver. +func NewWithPathResolver(resolvePath PathResolver) *cobra.Command { cmd := &cobra.Command{ Use: "fs", Short: "Filesystem related commands", @@ -13,11 +24,11 @@ func New() *cobra.Command { } cmd.AddCommand( - newCatCommand(), - newCpCommand(), - newLsCommand(), - newMkdirCommand(), - newRmCommand(), + newCatCommand(resolvePath), + newCpCommand(resolvePath), + newLsCommand(resolvePath), + newMkdirCommand(resolvePath), + newRmCommand(resolvePath), ) return cmd diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index d2e98563314..911d59c82dc 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -13,29 +13,36 @@ import ( "github.com/spf13/cobra" ) -func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { +// SplitPath separates a remote prefix from a path, preserving local Windows drive paths. +func SplitPath(fullPath string) (scheme, name string) { // Split path at : to detect any file schemes parts := strings.SplitN(fullPath, ":", 2) // If no scheme is specified, then local path if len(parts) < 2 { - f, err := filer.NewLocalClient("") - return f, fullPath, err + return "", fullPath } // On windows systems, paths start with a drive letter. If the scheme // is a single letter and the OS is windows, then we conclude the path // is meant to be a local path. if runtime.GOOS == "windows" && len(parts[0]) == 1 { + return "", fullPath + } + return parts[0], parts[1] +} + +func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { + scheme, path := SplitPath(fullPath) + if scheme == "" && path == fullPath { f, err := filer.NewLocalClient("") - return f, fullPath, err + return f, path, err } - if parts[0] != "dbfs" { - return nil, "", fmt.Errorf("invalid scheme: %s", parts[0]) + if scheme != "dbfs" { + return nil, "", fmt.Errorf("invalid scheme: %s", scheme) } - path := parts[1] w := cmdctx.WorkspaceClient(ctx) // If the specified path has the "Volumes" or "Skills" prefix, use the Files API. @@ -49,12 +56,6 @@ func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, er return f, path, err } -const dbfsPrefix string = "dbfs:" - -func isDbfsPath(path string) bool { - return strings.HasPrefix(path, dbfsPrefix) -} - type validArgs struct { mustWorkspaceClientFunc func(cmd *cobra.Command, args []string) error filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error) @@ -91,9 +92,9 @@ func (v *validArgs) Validate(cmd *cobra.Command, args []string, toComplete strin completer := completer.New(cmd.Context(), filer, v.onlyDirs) // Dbfs should have a prefix and always use the "/" separator - isDbfsPath := isDbfsPath(toComplete) - if isDbfsPath { - completer.SetPrefix(dbfsPrefix) + scheme, _ := SplitPath(toComplete) + if scheme != "" { + completer.SetPrefix(scheme + ":") completer.SetIsLocalPath(false) } diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index ee1a88e23fb..862d2f59a70 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -176,3 +176,19 @@ func TestGetValidArgsFunctionNotCompletedArgument(t *testing.T) { assert.Nil(t, completions) assert.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive) } + +func TestSplitPath(t *testing.T) { + cases := []struct{ input, scheme, name string }{ + {"./local/file", "", "./local/file"}, + {"dbfs:/foo", "dbfs", "/foo"}, + {"my-sandbox:/foo:bar", "my-sandbox", "/foo:bar"}, + {":/foo", "", "/foo"}, + } + for _, tc := range cases { + t.Run(tc.input, func(t *testing.T) { + scheme, name := SplitPath(tc.input) + assert.Equal(t, tc.scheme, scheme) + assert.Equal(t, tc.name, name) + }) + } +} diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index c6151b8a27a..d9a7967d8d0 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -38,7 +38,7 @@ func toJsonDirEntry(f fs.DirEntry, baseDir string, isAbsolute bool) (*jsonDirEnt }, nil } -func newLsCommand() *cobra.Command { +func newLsCommand(filerForPath PathResolver) *cobra.Command { cmd := &cobra.Command{ Use: "ls DIR_PATH", Short: "Lists files.", @@ -91,6 +91,7 @@ func newLsCommand() *cobra.Command { } v := newValidArgs() + v.filerForPathFunc = filerForPath v.onlyDirs = true cmd.ValidArgsFunction = v.Validate diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index 15faaad1569..fdcb36ba62f 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -5,7 +5,7 @@ import ( "github.com/spf13/cobra" ) -func newMkdirCommand() *cobra.Command { +func newMkdirCommand(filerForPath PathResolver) *cobra.Command { cmd := &cobra.Command{ Use: "mkdir DIR_PATH", // Alias `mkdirs` for this command exists for legacy purposes. This command @@ -29,6 +29,7 @@ func newMkdirCommand() *cobra.Command { } v := newValidArgs() + v.filerForPathFunc = filerForPath v.onlyDirs = true cmd.ValidArgsFunction = v.Validate diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index 8855e528804..e0b468bcf95 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -6,7 +6,7 @@ import ( "github.com/spf13/cobra" ) -func newRmCommand() *cobra.Command { +func newRmCommand(filerForPath PathResolver) *cobra.Command { cmd := &cobra.Command{ Use: "rm PATH", Short: "Remove files and directories.", @@ -33,6 +33,7 @@ func newRmCommand() *cobra.Command { } v := newValidArgs() + v.filerForPathFunc = filerForPath cmd.ValidArgsFunction = v.Validate return cmd diff --git a/cmd/sandbox/fs.go b/cmd/sandbox/fs.go new file mode 100644 index 00000000000..17fdf3139a5 --- /dev/null +++ b/cmd/sandbox/fs.go @@ -0,0 +1,80 @@ +package sandbox + +import ( + "context" + "errors" + "fmt" + "path" + "slices" + + filesystem "github.com/databricks/cli/cmd/fs" + "github.com/databricks/cli/libs/cmdctx" + "github.com/databricks/cli/libs/filer" + "github.com/spf13/cobra" +) + +func newFsCommand() *cobra.Command { + cmd := filesystem.NewWithPathResolver(sandboxFilerForPath) + cmd.GroupID = "" + cmd.Long = `Transfer files between your local filesystem and sandboxes. + +Use SANDBOX:/absolute/path for sandbox paths. SANDBOX can be an ID or a +name from the local cache populated by sandbox list. Paths without a +sandbox prefix refer to the local filesystem.` + for _, sub := range cmd.Commands() { + sub.Long = sub.Short + " Use SANDBOX:/absolute/path for sandbox paths." + sub.ValidArgsFunction = completeSandboxPaths(sub.ValidArgsFunction) + } + cmd.Example = ` databricks sandbox fs cp -r my-sandbox:/foo/bar ./local/output + databricks sandbox fs cp ./file my-sandbox:/tmp/file --overwrite + databricks sandbox fs ls my-sandbox:/tmp` + return cmd +} + +func sandboxFilerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { + identifier, name := filesystem.SplitPath(fullPath) + if identifier == "" && name == fullPath { + f, err := filer.NewLocalClient("") + return f, name, err + } + if identifier == "" { + return nil, "", errors.New("empty sandbox identifier: use SANDBOX:/path") + } + if !path.IsAbs(name) { + return nil, "", fmt.Errorf("sandbox path must be absolute: use %s:/path", identifier) + } + w := cmdctx.WorkspaceClient(ctx) + profile := w.Config.Profile + if profile == "" { + profile = w.Config.Host + } + id, err := resolveLocalID(ctx, profile, identifier) + if err != nil { + return nil, "", err + } + f, err := filer.NewSandboxClient(w, id, "/") + return f, path.Clean(name), err +} + +func completeSandboxPaths(complete cobra.CompletionFunc) cobra.CompletionFunc { + return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + completions, directive := complete(cmd, args, toComplete) + identifier, _ := filesystem.SplitPath(toComplete) + if identifier != "" || directive != cobra.ShellCompDirectiveNoSpace { + return completions, directive + } + completions = slices.DeleteFunc(completions, func(p string) bool { return p == "dbfs:/" }) + w := cmdctx.WorkspaceClient(cmd.Context()) + profile := w.Config.Profile + if profile == "" { + profile = w.Config.Host + } + for _, s := range getSandboxes(cmd.Context(), profile) { + completions = append(completions, s.ID+":/") + if s.Name != "" && s.Name != s.ID { + completions = append(completions, s.Name+":/") + } + } + return completions, directive + } +} diff --git a/cmd/sandbox/fs_test.go b/cmd/sandbox/fs_test.go new file mode 100644 index 00000000000..32af4ec3098 --- /dev/null +++ b/cmd/sandbox/fs_test.go @@ -0,0 +1,148 @@ +package sandbox_test + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + sandboxcmd "github.com/databricks/cli/cmd/sandbox" + "github.com/databricks/cli/libs/cmdctx" + "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/testserver" + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/service/sandbox" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const filesRoute = "/api/2.0/sandbox-files/sandboxes/test-sandbox" + +func sandboxContext(t *testing.T, server *testserver.Server) context.Context { + t.Helper() + home := t.TempDir() + ctx := env.WithUserHomeDir(t.Context(), home) + w, err := databricks.NewWorkspaceClient(&databricks.Config{Host: server.URL, Token: "testtoken"}) + require.NoError(t, err) + state, err := json.Marshal(map[string]any{ + "sandboxes": map[string]any{server.URL: []map[string]string{{"id": "test-sandbox", "name": "my-sandbox"}}}, + }) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".databricks"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(home, ".databricks", "sandbox.json"), state, 0o600)) + return cmdctx.SetWorkspaceClient(ctx, w) +} + +func runFsCommand(t *testing.T, ctx context.Context, name string, args ...string) (string, error) { + t.Helper() + ctx, stdout := cmdio.NewTestContextWithStdout(ctx) + cmd, _, err := sandboxcmd.New().Find([]string{"fs", name}) + require.NoError(t, err) + cmd.SetContext(ctx) + require.NoError(t, cmd.ParseFlags(args)) + err = cmd.RunE(cmd, cmd.Flags().Args()) + return stdout.String(), err +} + +func TestSandboxFsCopySkipsAndOverwrites(t *testing.T) { + server := testserver.New(t) + ctx := sandboxContext(t, server) + contents := "original" + server.Handle("GET", filesRoute+"/paths:describe", func(req testserver.Request) any { + name := req.URL.Query().Get("path") + fileType := sandbox.SandboxFileTypeSandboxFileTypeFile + if name == "/remote" { + fileType = sandbox.SandboxFileTypeSandboxFileTypeDirectory + } + return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{Path: name, FileType: fileType}} + }) + var overwrites []string + server.Handle("PUT", filesRoute+"/content", func(req testserver.Request) any { + assert.Equal(t, "/remote/file", req.URL.Query().Get("path")) + overwrite := req.URL.Query().Get("overwrite") + overwrites = append(overwrites, overwrite) + if overwrite == "false" { + return testserver.Response{StatusCode: 412, Body: map[string]string{ + "error_code": "RESOURCE_ALREADY_EXISTS", "message": "already exists", + }} + } + contents = string(req.Body) + return testserver.Response{StatusCode: 204} + }) + local := filepath.Join(t.TempDir(), "file") + require.NoError(t, os.WriteFile(local, []byte("replacement"), 0o600)) + output, err := runFsCommand(t, ctx, "cp", local, "my-sandbox:/remote/file") + require.NoError(t, err) + assert.Contains(t, output, "my-sandbox:/remote/file (skipped; already exists)") + assert.Equal(t, "original", contents) + output, err = runFsCommand(t, ctx, "cp", local, "my-sandbox:/remote/file", "--overwrite") + require.NoError(t, err) + assert.NotContains(t, output, "skipped") + assert.Equal(t, "replacement", contents) + assert.Equal(t, []string{"false", "true"}, overwrites) +} + +func TestSandboxFsRecursiveDownload(t *testing.T) { + server := testserver.New(t) + ctx := sandboxContext(t, server) + server.Handle("GET", filesRoute+"/paths:describe", func(req testserver.Request) any { + return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{ + Path: "/remote", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory, + }} + }) + server.Handle("GET", filesRoute+"/directory-entries", func(req testserver.Request) any { + if req.URL.Query().Get("path") == "/remote" { + return sandbox.ListDirectoryContentsResponse{Contents: []sandbox.SandboxFileInfo{ + {Path: "/remote/file", FileType: sandbox.SandboxFileTypeSandboxFileTypeFile}, + {Path: "/remote/sub", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory}, + }} + } + return sandbox.ListDirectoryContentsResponse{Contents: []sandbox.SandboxFileInfo{ + {Path: "/remote/sub/nested", FileType: sandbox.SandboxFileTypeSandboxFileTypeFile}, + }} + }) + server.Handle("GET", filesRoute+"/content", func(req testserver.Request) any { + return []byte(req.URL.Query().Get("path")) + }) + target := filepath.Join(t.TempDir(), "output") + _, err := runFsCommand(t, ctx, "cp", "-r", "my-sandbox:/remote", target) + require.NoError(t, err) + for _, name := range []string{"file", "sub/nested"} { + contents, err := os.ReadFile(filepath.Join(target, filepath.FromSlash(name))) + require.NoError(t, err) + assert.Equal(t, "/remote/"+name, string(contents)) + } +} + +func TestSandboxFsCatAndLs(t *testing.T) { + server := testserver.New(t) + ctx := sandboxContext(t, server) + server.Handle("GET", filesRoute+"/content", func(req testserver.Request) any { + return []byte("contents") + }) + server.Handle("GET", filesRoute+"/directory-entries", func(req testserver.Request) any { + return sandbox.ListDirectoryContentsResponse{Contents: []sandbox.SandboxFileInfo{ + {Path: "/tmp/z", FileType: sandbox.SandboxFileTypeSandboxFileTypeFile}, + {Path: "/tmp/a", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory}, + }} + }) + output, err := runFsCommand(t, ctx, "cat", "my-sandbox:/tmp/z") + require.NoError(t, err) + assert.Equal(t, "contents", output) + output, err = runFsCommand(t, ctx, "ls", "my-sandbox:/tmp", "--absolute") + require.NoError(t, err) + assert.Equal(t, "my-sandbox:/tmp/a\nmy-sandbox:/tmp/z\n", output) +} + +func TestSandboxFsRejectsRelativeRemotePaths(t *testing.T) { + for _, input := range []string{"my-sandbox:relative", ":/tmp/file"} { + t.Run(input, func(t *testing.T) { + _, err := runFsCommand(t, t.Context(), "cat", input) + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "sandbox") + }) + } +} diff --git a/cmd/sandbox/sandbox.go b/cmd/sandbox/sandbox.go index 39ff208129b..590307a6508 100644 --- a/cmd/sandbox/sandbox.go +++ b/cmd/sandbox/sandbox.go @@ -44,6 +44,7 @@ Common workflows: cmd.AddCommand(newStartCommand()) cmd.AddCommand(newStatusCommand()) cmd.AddCommand(newConfigCommand()) + cmd.AddCommand(newFsCommand()) return cmd } diff --git a/libs/filer/sandbox_client.go b/libs/filer/sandbox_client.go new file mode 100644 index 00000000000..0aafe848136 --- /dev/null +++ b/libs/filer/sandbox_client.go @@ -0,0 +1,212 @@ +package filer + +import ( + "cmp" + "context" + "errors" + "fmt" + "io" + "io/fs" + "maps" + "net/http" + "net/url" + "path" + "slices" + "strings" + "time" + + "github.com/databricks/cli/libs/auth" + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/client" + "github.com/databricks/databricks-sdk-go/service/sandbox" +) + +// SandboxClient implements Filer using the sandbox Files APIs. +type SandboxClient struct { + workspaceClient *databricks.WorkspaceClient + apiClient *client.DatabricksClient + parent string + root WorkspaceRootPath +} + +// NewSandboxClient returns a filer rooted at an absolute path in a sandbox. +func NewSandboxClient(w *databricks.WorkspaceClient, sandboxID, root string) (Filer, error) { + if sandboxID == "" || sandboxID == "." || sandboxID == ".." || strings.Contains(sandboxID, "/") { + return nil, fmt.Errorf("invalid sandbox ID: %q", sandboxID) + } + if !path.IsAbs(root) { + return nil, fmt.Errorf("sandbox root must be absolute: %s", root) + } + apiClient, err := client.New(w.Config) + if err != nil { + return nil, err + } + return &SandboxClient{ + workspaceClient: w, + apiClient: apiClient, + parent: "sandboxes/" + url.PathEscape(sandboxID), + root: NewWorkspaceRootPath(root), + }, nil +} + +func (w *SandboxClient) Write(ctx context.Context, name string, reader io.Reader, mode ...WriteMode) error { + absPath, err := w.root.Join(name) + if err != nil { + return err + } + + if !slices.Contains(mode, CreateParentDirectories) { + dir := path.Dir(absPath) + info, err := w.Stat(ctx, path.Dir(name)) + if errors.Is(err, fs.ErrNotExist) { + return noSuchDirectoryError{dir} + } + if err != nil { + return err + } + if !info.IsDir() && info.Mode()&fs.ModeSymlink == 0 { + return notADirectory{dir} + } + } + + headers := map[string]string{"Content-Type": "application/octet-stream"} + maps.Copy(headers, auth.WorkspaceIDHeaders(w.workspaceClient.Config)) + // The pinned SDK predates the overwrite parameter. Keep the body streaming. + err = w.apiClient.Do(ctx, http.MethodPut, + "/api/2.0/sandbox-files/"+w.parent+"/content", headers, + map[string]any{"path": absPath, "overwrite": slices.Contains(mode, OverwriteIfExists)}, + io.NopCloser(reader), nil) + if aerr, ok := errors.AsType[*apierr.APIError](err); ok && aerr.StatusCode == http.StatusPreconditionFailed { + return fileAlreadyExistsError{absPath} + } + return sandboxPathError(err, absPath) +} + +func (w *SandboxClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { + absPath, err := w.root.Join(name) + if err != nil { + return nil, err + } + response, err := w.workspaceClient.Sandbox.DownloadFile(ctx, sandbox.DownloadFileRequest{ + Parent: w.parent, + Path: absPath, + }) + if err != nil { + return nil, sandboxPathError(err, absPath) + } + return response.Contents, nil +} + +func (w *SandboxClient) Delete(ctx context.Context, name string, mode ...DeleteMode) error { + absPath, err := w.root.Join(name) + if err != nil { + return err + } + if absPath == w.root.rootPath { + return cannotDeleteRootError{} + } + // DeletePath is idempotent; Filer reports a missing entry. + if _, err := w.Stat(ctx, name); err != nil { + return err + } + _, err = w.workspaceClient.Sandbox.DeletePath(ctx, sandbox.DeletePathRequest{ + Parent: w.parent, + Path: absPath, + Recursive: slices.Contains(mode, DeleteRecursively), + }) + return sandboxPathError(err, absPath) +} + +func (w *SandboxClient) ReadDir(ctx context.Context, name string) ([]fs.DirEntry, error) { + absPath, err := w.root.Join(name) + if err != nil { + return nil, err + } + files, err := w.workspaceClient.Sandbox.ListDirectoryContentsAll(ctx, sandbox.ListDirectoryContentsRequest{ + Parent: w.parent, + Path: absPath, + }) + if err != nil { + return nil, sandboxPathError(err, absPath) + } + entries := make([]fs.DirEntry, len(files)) + for i, file := range files { + entries[i] = fs.FileInfoToDirEntry(sandboxFileInfo{file}) + } + slices.SortFunc(entries, func(a, b fs.DirEntry) int { return cmp.Compare(a.Name(), b.Name()) }) + return entries, nil +} + +func (w *SandboxClient) Mkdir(ctx context.Context, name string) error { + absPath, err := w.root.Join(name) + if err != nil { + return err + } + _, err = w.workspaceClient.Sandbox.CreateDirectory(ctx, sandbox.CreateDirectoryRequest{ + Parent: w.parent, + Path: absPath, + Parents: true, + }) + return sandboxPathError(err, absPath) +} + +func (w *SandboxClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { + absPath, err := w.root.Join(name) + if err != nil { + return nil, err + } + response, err := w.workspaceClient.Sandbox.GetMetadata(ctx, sandbox.GetMetadataRequest{ + Parent: w.parent, + Path: absPath, + }) + if err != nil { + return nil, sandboxPathError(err, absPath) + } + return sandboxFileInfo{*response.File}, nil +} + +func sandboxPathError(err error, name string) error { + aerr, ok := errors.AsType[*apierr.APIError](err) + if !ok { + return err + } + switch aerr.StatusCode { + case http.StatusNotFound: + return fileDoesNotExistError{name} + case http.StatusForbidden: + return permissionError{name, err} + default: + return err + } +} + +type sandboxFileInfo struct { + fi sandbox.SandboxFileInfo +} + +func (info sandboxFileInfo) Name() string { return path.Base(info.fi.Path) } +func (info sandboxFileInfo) Size() int64 { return info.fi.Size } +func (info sandboxFileInfo) Sys() any { return info.fi } + +func (info sandboxFileInfo) IsDir() bool { + return info.fi.FileType == sandbox.SandboxFileTypeSandboxFileTypeDirectory +} + +func (info sandboxFileInfo) Mode() fs.FileMode { + mode := fs.FileMode(info.fi.Mode) & fs.ModePerm + switch info.fi.FileType { + case sandbox.SandboxFileTypeSandboxFileTypeDirectory: + mode |= fs.ModeDir + case sandbox.SandboxFileTypeSandboxFileTypeSymlink: + mode |= fs.ModeSymlink + case sandbox.SandboxFileTypeSandboxFileTypeFile: + default: + mode |= fs.ModeIrregular + } + return mode +} + +func (info sandboxFileInfo) ModTime() time.Time { + return info.fi.ModifyTime.AsTime() +} diff --git a/libs/filer/sandbox_client_test.go b/libs/filer/sandbox_client_test.go new file mode 100644 index 00000000000..6df6d22db54 --- /dev/null +++ b/libs/filer/sandbox_client_test.go @@ -0,0 +1,205 @@ +package filer_test + +import ( + "encoding/json" + "io" + "io/fs" + "net/http" + "strconv" + "strings" + "testing" + "time" + + "github.com/databricks/cli/libs/auth" + "github.com/databricks/cli/libs/filer" + "github.com/databricks/cli/libs/testserver" + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/apierr" + sdktime "github.com/databricks/databricks-sdk-go/common/types/time" + "github.com/databricks/databricks-sdk-go/service/sandbox" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const sandboxFilesRoute = "/api/2.0/sandbox-files/sandboxes/test-sandbox" + +func newSandboxFiler(t *testing.T, server *testserver.Server, root, workspaceID string) filer.Filer { + t.Helper() + w, err := databricks.NewWorkspaceClient(&databricks.Config{ + Host: server.URL, Token: "testtoken", WorkspaceID: workspaceID, + }) + require.NoError(t, err) + f, err := filer.NewSandboxClient(w, "test-sandbox", root) + require.NoError(t, err) + return f +} + +func TestSandboxWriteOverwrite(t *testing.T) { + for _, overwrite := range []bool{false, true} { + t.Run(strconv.FormatBool(overwrite), func(t *testing.T) { + server := testserver.New(t) + server.Handle("PUT", sandboxFilesRoute+"/content", func(req testserver.Request) any { + assert.Equal(t, "/root/a ?#雪.txt", req.URL.Query().Get("path")) + assert.Equal(t, []string{strconv.FormatBool(overwrite)}, req.URL.Query()["overwrite"]) + assert.Equal(t, "application/octet-stream", req.Headers.Get("Content-Type")) + assert.Equal(t, "123", req.Headers.Get(auth.WorkspaceIDHeader)) + assert.Equal(t, "contents", string(req.Body)) + if !overwrite { + return testserver.Response{StatusCode: 412, Body: map[string]string{ + "error_code": "RESOURCE_ALREADY_EXISTS", "message": "already exists", + }} + } + return testserver.Response{StatusCode: 204} + }) + f := newSandboxFiler(t, server, "/root", "123") + mode := []filer.WriteMode{filer.CreateParentDirectories} + if overwrite { + mode = append(mode, filer.OverwriteIfExists) + } + err := f.Write(t.Context(), "a ?#雪.txt", strings.NewReader("contents"), mode...) + if overwrite { + require.NoError(t, err) + } else { + require.ErrorIs(t, err, fs.ErrExist) + } + }) + } +} + +func TestSandboxWriteParent(t *testing.T) { + for _, fileType := range []sandbox.SandboxFileType{ + sandbox.SandboxFileTypeSandboxFileTypeDirectory, + sandbox.SandboxFileTypeSandboxFileTypeFile, + sandbox.SandboxFileTypeSandboxFileTypeSymlink, + } { + t.Run(string(fileType), func(t *testing.T) { + server := testserver.New(t) + server.Handle("GET", sandboxFilesRoute+"/paths:describe", func(req testserver.Request) any { + assert.Equal(t, "/root/sub", req.URL.Query().Get("path")) + return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{FileType: fileType}} + }) + uploaded := false + server.Handle("PUT", sandboxFilesRoute+"/content", func(req testserver.Request) any { + uploaded = true + assert.Empty(t, req.Headers.Get(auth.WorkspaceIDHeader)) + return testserver.Response{StatusCode: 204} + }) + f := newSandboxFiler(t, server, "/root", "none") + err := f.Write(t.Context(), "sub/file", strings.NewReader("content")) + if fileType != sandbox.SandboxFileTypeSandboxFileTypeFile { + require.NoError(t, err) + assert.True(t, uploaded) + } else { + require.ErrorIs(t, err, fs.ErrInvalid) + assert.False(t, uploaded) + } + }) + } +} + +func TestSandboxReadAndMetadata(t *testing.T) { + server := testserver.New(t) + modified := time.Date(2026, 11, 5, 1, 2, 3, 123456789, time.UTC) + server.Handle("GET", sandboxFilesRoute+"/content", func(req testserver.Request) any { + assert.Equal(t, "/tmp/file", req.URL.Query().Get("path")) + return testserver.Response{Body: []byte("contents"), Headers: http.Header{"Content-Type": {"application/octet-stream"}}} + }) + server.Handle("GET", sandboxFilesRoute+"/paths:describe", func(req testserver.Request) any { + return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{ + Path: "/tmp/file", Size: 8, Mode: 0o640, ModifyTime: sdktime.New(modified), + FileType: sandbox.SandboxFileTypeSandboxFileTypeFile, + }} + }) + f := newSandboxFiler(t, server, "/", "") + r, err := f.Read(t.Context(), "/tmp/file") + require.NoError(t, err) + defer r.Close() + contents, err := io.ReadAll(r) + require.NoError(t, err) + assert.Equal(t, "contents", string(contents)) + info, err := f.Stat(t.Context(), "/tmp/file") + require.NoError(t, err) + assert.Equal(t, "file", info.Name()) + assert.Equal(t, int64(8), info.Size()) + assert.Equal(t, fs.FileMode(0o640), info.Mode()) + assert.Equal(t, modified, info.ModTime()) + assert.False(t, info.IsDir()) +} + +func TestSandboxReadDirPagination(t *testing.T) { + server := testserver.New(t) + var tokens []string + server.Handle("GET", sandboxFilesRoute+"/directory-entries", func(req testserver.Request) any { + assert.Equal(t, "/tmp", req.URL.Query().Get("path")) + token := req.URL.Query().Get("page_token") + tokens = append(tokens, token) + if token == "" { + return sandbox.ListDirectoryContentsResponse{ + Contents: []sandbox.SandboxFileInfo{{Path: "/tmp/z", FileType: sandbox.SandboxFileTypeSandboxFileTypeSymlink}}, + NextPageToken: "page2", + } + } + return sandbox.ListDirectoryContentsResponse{Contents: []sandbox.SandboxFileInfo{ + {Path: "/tmp/a", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory}, + }} + }) + f := newSandboxFiler(t, server, "/", "") + entries, err := f.ReadDir(t.Context(), "/tmp") + require.NoError(t, err) + require.Len(t, entries, 2) + assert.Equal(t, []string{"", "page2"}, tokens) + assert.Equal(t, "a", entries[0].Name()) + assert.True(t, entries[0].IsDir()) + assert.Equal(t, "z", entries[1].Name()) + assert.Equal(t, fs.ModeSymlink, entries[1].Type()) +} + +func TestSandboxMkdirAndDelete(t *testing.T) { + server := testserver.New(t) + server.Handle("GET", sandboxFilesRoute+"/paths:describe", func(req testserver.Request) any { + return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{ + Path: "/root/sub", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory, + }} + }) + server.Handle("POST", sandboxFilesRoute+"/directories:make", func(req testserver.Request) any { + var body sandbox.CreateDirectoryRequest + require.NoError(t, json.Unmarshal(req.Body, &body)) + assert.Equal(t, "/root/sub", body.Path) + assert.True(t, body.Parents) + return sandbox.CreateDirectoryResponse{} + }) + server.Handle("POST", sandboxFilesRoute+"/paths:remove", func(req testserver.Request) any { + var body sandbox.DeletePathRequest + require.NoError(t, json.Unmarshal(req.Body, &body)) + assert.Equal(t, "/root/sub", body.Path) + assert.True(t, body.Recursive) + return sandbox.DeletePathResponse{} + }) + f := newSandboxFiler(t, server, "/root", "") + require.NoError(t, f.Mkdir(t.Context(), "sub")) + require.NoError(t, f.Delete(t.Context(), "sub", filer.DeleteRecursively)) + require.ErrorIs(t, f.Delete(t.Context(), ".", filer.DeleteRecursively), fs.ErrInvalid) + require.Error(t, f.Mkdir(t.Context(), "../escape")) +} + +func TestSandboxPathErrors(t *testing.T) { + for _, tc := range []struct { + status int + code string + want error + }{ + {404, "NOT_FOUND", fs.ErrNotExist}, + {403, "PERMISSION_DENIED", fs.ErrPermission}, + {400, "INVALID_PARAMETER_VALUE", apierr.ErrInvalidParameterValue}, + } { + t.Run(tc.code, func(t *testing.T) { + server := testserver.New(t) + server.Handle("GET", sandboxFilesRoute+"/paths:describe", func(req testserver.Request) any { + return testserver.Response{StatusCode: tc.status, Body: map[string]string{"error_code": tc.code, "message": "failed"}} + }) + f := newSandboxFiler(t, server, "/", "") + _, err := f.Stat(t.Context(), "missing") + require.ErrorIs(t, err, tc.want) + }) + } +} From e758af80290d5eaabb5c6d1b6080b3cfe363e11e Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 6 Nov 2026 16:54:50 +0100 Subject: [PATCH 2/7] Add a testserver fake for the sandbox file APIs Models the server's semantics as observed on a live sandbox: uploads always replace and create parents, deletes of absent paths succeed, rename refuses an existing destination atomically, and most failures share one generic error with only the status differing. Unknown upload parameters such as overwrite are rejected, as on the real server. Co-authored-by: Isaac --- libs/testserver/fake_workspace.go | 3 + libs/testserver/handlers.go | 24 ++ libs/testserver/sandbox_files.go | 407 ++++++++++++++++++++++++++ libs/testserver/sandbox_files_test.go | 115 ++++++++ 4 files changed, 549 insertions(+) create mode 100644 libs/testserver/sandbox_files.go create mode 100644 libs/testserver/sandbox_files_test.go diff --git a/libs/testserver/fake_workspace.go b/libs/testserver/fake_workspace.go index c48ebb04df2..d937dec159b 100644 --- a/libs/testserver/fake_workspace.go +++ b/libs/testserver/fake_workspace.go @@ -213,6 +213,9 @@ type FakeWorkspace struct { // dbfs is the DBFS namespace, created on first use (see dbfs.go). dbfs *fakeDbfs + // sandboxFiles holds the filesystem of each sandbox, created on first use (see sandbox_files.go). + sandboxFiles map[string]*fakeSandboxFs + // snapshots records immutable-snapshot content paths. Breaking the glass on a // recorded path flips it dirty for good, as in the real API. snapshots map[string]*snapshotRecord diff --git a/libs/testserver/handlers.go b/libs/testserver/handlers.go index e5f4c37a16b..b018daa95d5 100644 --- a/libs/testserver/handlers.go +++ b/libs/testserver/handlers.go @@ -465,6 +465,30 @@ func AddDefaultHandlers(server *Server) { return data }) + // Sandbox file APIs: + sandboxFiles := "/api/2.0/sandbox-files/sandboxes/{sandbox_id}" + server.Handle("PUT", sandboxFiles+"/content", func(req Request) any { + return req.Workspace.SandboxFilesUpload(req, req.Vars["sandbox_id"]) + }) + server.Handle("GET", sandboxFiles+"/content", func(req Request) any { + return req.Workspace.SandboxFilesDownload(req, req.Vars["sandbox_id"]) + }) + server.Handle("GET", sandboxFiles+"/paths:describe", func(req Request) any { + return req.Workspace.SandboxFilesGetMetadata(req, req.Vars["sandbox_id"]) + }) + server.Handle("GET", sandboxFiles+"/directory-entries", func(req Request) any { + return req.Workspace.SandboxFilesList(req, req.Vars["sandbox_id"]) + }) + server.Handle("POST", sandboxFiles+"/directories:make", func(req Request) any { + return req.Workspace.SandboxFilesMkdir(req, req.Vars["sandbox_id"]) + }) + server.Handle("POST", sandboxFiles+"/paths:remove", func(req Request) any { + return req.Workspace.SandboxFilesDelete(req, req.Vars["sandbox_id"]) + }) + server.Handle("POST", sandboxFiles+"/paths:rename", func(req Request) any { + return req.Workspace.SandboxFilesRename(req, req.Vars["sandbox_id"]) + }) + // DBFS (dbfs:/ paths): server.Handle("GET", "/api/2.0/dbfs/get-status", func(req Request) any { return req.Workspace.DbfsGetStatus(req) diff --git a/libs/testserver/sandbox_files.go b/libs/testserver/sandbox_files.go new file mode 100644 index 00000000000..a714e87c26c --- /dev/null +++ b/libs/testserver/sandbox_files.go @@ -0,0 +1,407 @@ +package testserver + +import ( + "encoding/json" + "maps" + "net/http" + "path" + "slices" + "strconv" + "strings" + "time" + + sdktime "github.com/databricks/databricks-sdk-go/common/types/time" + "github.com/databricks/databricks-sdk-go/service/sandbox" +) + +// TestDefaultSandboxId is the sandbox that acceptance tests address locally (TEST_SANDBOX_ID). +const TestDefaultSandboxId = "test-sandbox-1234" + +// The sandbox file APIs report most filesystem failures with this one reason and message, +// whatever the condition; only the HTTP status differs. The download and upload endpoints +// use their own messages and no reason. +const ( + sandboxFilesFsReason = "SANDBOX_FILES_FILESYSTEM_ERROR" + sandboxFilesFsMessage = "sandbox filesystem operation failed" + sandboxFilesNotFound = "The sandbox or file was not found." + sandboxFilesWrongState = "The sandbox or file is not in the required state." + sandboxFilesBadUpload = "Invalid file request. Use an absolute UTF-8 path and supported headers." + + sandboxFilesMaxPageSize = 1000 + sandboxFilesDirSize = 4096 + sandboxFilesOwner = "sandbox-agent" +) + +// fakeSandboxFs is the filesystem of one sandbox, keyed by absolute path. It models the +// server's semantics, which are looser than the Filer contract: uploads replace and create +// parents, and deletes of absent paths succeed. The filer must close those gaps itself, so +// the fake must not. +type fakeSandboxFs struct { + entries map[string]*sandboxFsEntry +} + +type sandboxFsEntry struct { + isDir bool + data []byte + mode int + permissions string + owner string + modTime time.Time +} + +// sandboxFs returns the filesystem of sandboxID, creating it on first use. The fake doesn't +// model sandbox lifecycle, so every sandbox ID exists; a real server returns 404 for unknown IDs. +func (s *FakeWorkspace) sandboxFs(sandboxID string) *fakeSandboxFs { + if s.sandboxFiles == nil { + s.sandboxFiles = map[string]*fakeSandboxFs{} + } + sfs, ok := s.sandboxFiles[sandboxID] + if !ok { + now := time.Now() + sfs = &fakeSandboxFs{entries: map[string]*sandboxFsEntry{ + "/": {isDir: true, mode: 0o755, permissions: "drwxr-xr-x", owner: "root", modTime: now}, + "/tmp": {isDir: true, mode: 0o1777, permissions: "drwxrwxrwt", owner: "root", modTime: now}, + }} + s.sandboxFiles[sandboxID] = sfs + } + return sfs +} + +func newSandboxFileEntry(data []byte) *sandboxFsEntry { + return &sandboxFsEntry{data: data, mode: 0o644, permissions: "-rw-r--r--", owner: sandboxFilesOwner, modTime: time.Now()} +} + +func newSandboxDirEntry() *sandboxFsEntry { + return &sandboxFsEntry{isDir: true, mode: 0o755, permissions: "drwxr-xr-x", owner: sandboxFilesOwner, modTime: time.Now()} +} + +func sandboxFilesError(statusCode int, errorCode, message string) Response { + return Response{ + StatusCode: statusCode, + Body: map[string]string{"error_code": errorCode, "message": message}, + } +} + +// sandboxFsError is the generic filesystem error, which carries an ErrorInfo reason. +func sandboxFsError(statusCode int, errorCode string) Response { + return Response{ + StatusCode: statusCode, + Body: map[string]any{ + "error_code": errorCode, + "message": sandboxFilesFsMessage, + "details": []map[string]any{{ + "@type": "type.googleapis.com/google.rpc.ErrorInfo", + "reason": sandboxFilesFsReason, + "domain": "databricks.sandbox.files", + }}, + }, + } +} + +// sandboxFilesPath validates and cleans an absolute path argument named field. +func sandboxFilesPath(field, p string) (string, *Response) { + if !strings.HasPrefix(p, "/") { + resp := sandboxFilesError(400, "INVALID_PARAMETER_VALUE", field+" must be an absolute UTF-8 path") + return "", &resp + } + // A ".." segment is rejected before the request reaches the service, with a bare 400. + if slices.Contains(strings.Split(p, "/"), "..") { + resp := Response{StatusCode: 400, Body: "Bad Request"} + return "", &resp + } + return path.Clean(p), nil +} + +// lookup returns the entry at p and, if p is absent, whether a parent component is a file. +func (sfs *fakeSandboxFs) lookup(p string) (entry *sandboxFsEntry, underFile bool) { + if e, ok := sfs.entries[p]; ok { + return e, false + } + for dir := path.Dir(p); dir != "/"; dir = path.Dir(dir) { + if e, ok := sfs.entries[dir]; ok { + return nil, !e.isDir + } + } + return nil, false +} + +func (sfs *fakeSandboxFs) isEmptyDir(p string) bool { + prefix := strings.TrimSuffix(p, "/") + "/" + for name := range sfs.entries { + if strings.HasPrefix(name, prefix) { + return false + } + } + return true +} + +// mkdirAll creates p and its missing parents. It reports false if p or a parent is a file. +func (sfs *fakeSandboxFs) mkdirAll(p string) bool { + var missing []string + for dir := p; ; dir = path.Dir(dir) { + if e, ok := sfs.entries[dir]; ok { + if !e.isDir { + return false + } + break + } + missing = append(missing, dir) + } + for _, dir := range missing { + sfs.entries[dir] = newSandboxDirEntry() + } + return true +} + +func (e *sandboxFsEntry) info(p string) sandbox.SandboxFileInfo { + info := sandbox.SandboxFileInfo{ + FileType: sandbox.SandboxFileTypeSandboxFileTypeFile, + Group: e.owner, + Mode: e.mode, + ModifyTime: sdktime.New(e.modTime), + Name: path.Base(p), + Owner: e.owner, + Path: p, + Permissions: e.permissions, + Size: int64(len(e.data)), + // The server sends these even when zero (an empty file has size 0). + ForceSendFields: []string{"FileType", "Group", "Mode", "Name", "Owner", "Path", "Permissions", "Size"}, + } + if e.isDir { + info.FileType = sandbox.SandboxFileTypeSandboxFileTypeDirectory + info.Size = sandboxFilesDirSize + } + return info +} + +// SandboxFilesUpload creates or replaces a file, creating missing parents. +func (s *FakeWorkspace) SandboxFilesUpload(req Request, sandboxID string) Response { + // The server rejects query parameters it doesn't know, including "overwrite". + for name := range req.URL.Query() { + if name != "path" { + return sandboxFilesError(400, "INVALID_PARAMETER_VALUE", sandboxFilesBadUpload) + } + } + p, errResp := sandboxFilesPath("path", req.URL.Query().Get("path")) + if errResp != nil { + return *errResp + } + + defer s.LockUnlock()() + sfs := s.sandboxFs(sandboxID) + existing, underFile := sfs.lookup(p) + if underFile || (existing != nil && existing.isDir) { + return sandboxFilesError(409, "BAD_REQUEST", sandboxFilesWrongState) + } + if !sfs.mkdirAll(path.Dir(p)) { + return sandboxFilesError(409, "BAD_REQUEST", sandboxFilesWrongState) + } + sfs.entries[p] = newSandboxFileEntry(req.Body) + if existing != nil { + return Response{StatusCode: http.StatusNoContent} + } + return Response{StatusCode: http.StatusCreated} +} + +// SandboxFilesDownload returns the contents of a regular file. +func (s *FakeWorkspace) SandboxFilesDownload(req Request, sandboxID string) Response { + p, errResp := sandboxFilesPath("path", req.URL.Query().Get("path")) + if errResp != nil { + return *errResp + } + + defer s.LockUnlock()() + entry, underFile := s.sandboxFs(sandboxID).lookup(p) + if underFile || (entry != nil && entry.isDir) { + return sandboxFilesError(409, "BAD_REQUEST", sandboxFilesWrongState) + } + if entry == nil { + return sandboxFilesError(404, "NOT_FOUND", sandboxFilesNotFound) + } + return Response{ + Headers: http.Header{"Content-Type": {"application/octet-stream"}}, + Body: slices.Clone(entry.data), + } +} + +// SandboxFilesGetMetadata describes a path. +func (s *FakeWorkspace) SandboxFilesGetMetadata(req Request, sandboxID string) Response { + p, errResp := sandboxFilesPath("path", req.URL.Query().Get("path")) + if errResp != nil { + return *errResp + } + + defer s.LockUnlock()() + entry, underFile := s.sandboxFs(sandboxID).lookup(p) + if underFile { + return sandboxFsError(400, "BAD_REQUEST") + } + if entry == nil { + return sandboxFsError(404, "NOT_FOUND") + } + info := entry.info(p) + return Response{Body: sandbox.GetMetadataResponse{File: &info}} +} + +// SandboxFilesList lists a directory's children in name order, one page at a time. +func (s *FakeWorkspace) SandboxFilesList(req Request, sandboxID string) Response { + query := req.URL.Query() + p, errResp := sandboxFilesPath("path", query.Get("path")) + if errResp != nil { + return *errResp + } + pageSize, _ := strconv.Atoi(query.Get("page_size")) + if pageSize <= 0 || pageSize > sandboxFilesMaxPageSize { + pageSize = sandboxFilesMaxPageSize + } + // The page token is an offset into the sorted listing; real tokens are opaque. + offset, _ := strconv.Atoi(query.Get("page_token")) + + defer s.LockUnlock()() + sfs := s.sandboxFs(sandboxID) + entry, underFile := sfs.lookup(p) + if underFile || (entry != nil && !entry.isDir) { + return sandboxFsError(400, "BAD_REQUEST") + } + if entry == nil { + return sandboxFsError(404, "NOT_FOUND") + } + + var names []string + for name := range sfs.entries { + if name != "/" && path.Dir(name) == p { + names = append(names, name) + } + } + // Byte order of the paths is UTF-8 name order, since all share the same parent. + slices.Sort(names) + + resp := sandbox.ListDirectoryContentsResponse{} + end := min(offset+pageSize, len(names)) + for _, name := range names[min(offset, end):end] { + resp.Contents = append(resp.Contents, sfs.entries[name].info(name)) + } + if end < len(names) { + resp.NextPageToken = strconv.Itoa(end) + } + return Response{Body: resp} +} + +// SandboxFilesMkdir creates a directory, with missing parents unless parents is false. +func (s *FakeWorkspace) SandboxFilesMkdir(req Request, sandboxID string) Response { + var body struct { + Path string `json:"path"` + Parents *bool `json:"parents"` + } + if err := json.Unmarshal(req.Body, &body); err != nil { + return sandboxFilesError(400, "INVALID_PARAMETER_VALUE", err.Error()) + } + p, errResp := sandboxFilesPath("path", body.Path) + if errResp != nil { + return *errResp + } + + defer s.LockUnlock()() + sfs := s.sandboxFs(sandboxID) + entry, underFile := sfs.lookup(p) + // Parents defaults to true, which also succeeds when the directory exists. + if body.Parents == nil || *body.Parents { + if !sfs.mkdirAll(p) { + return sandboxFsError(400, "BAD_REQUEST") + } + return Response{Body: sandbox.CreateDirectoryResponse{}} + } + + switch { + case entry != nil: + return sandboxFsError(409, "ALREADY_EXISTS") + case underFile: + return sandboxFsError(400, "BAD_REQUEST") + case sfs.entries[path.Dir(p)] == nil: + return sandboxFsError(404, "NOT_FOUND") + } + sfs.entries[p] = newSandboxDirEntry() + return Response{Body: sandbox.CreateDirectoryResponse{}} +} + +// SandboxFilesDelete deletes a path. It succeeds if the path is absent. +func (s *FakeWorkspace) SandboxFilesDelete(req Request, sandboxID string) Response { + var body sandbox.DeletePathRequest + if err := json.Unmarshal(req.Body, &body); err != nil { + return sandboxFilesError(400, "INVALID_PARAMETER_VALUE", err.Error()) + } + p, errResp := sandboxFilesPath("path", body.Path) + if errResp != nil { + return *errResp + } + + defer s.LockUnlock()() + sfs := s.sandboxFs(sandboxID) + entry, _ := sfs.lookup(p) + if entry == nil { + return Response{Body: sandbox.DeletePathResponse{}} + } + if entry.isDir && !body.Recursive && !sfs.isEmptyDir(p) { + return sandboxFsError(400, "BAD_REQUEST") + } + prefix := p + "/" + for name := range sfs.entries { + if name == p || strings.HasPrefix(name, prefix) { + delete(sfs.entries, name) + } + } + return Response{Body: sandbox.DeletePathResponse{}} +} + +// SandboxFilesRename moves a path. The destination must be absent: the check and the move +// happen under one lock, so of several concurrent renames onto the same destination exactly +// one succeeds, as on the real server. +func (s *FakeWorkspace) SandboxFilesRename(req Request, sandboxID string) Response { + var body sandbox.RenamePathRequest + if err := json.Unmarshal(req.Body, &body); err != nil { + return sandboxFilesError(400, "INVALID_PARAMETER_VALUE", err.Error()) + } + src, errResp := sandboxFilesPath("source", body.Source) + if errResp != nil { + return *errResp + } + // The server rejects a destination with a trailing slash instead of cleaning it. + if strings.HasSuffix(body.Destination, "/") && body.Destination != "/" { + return sandboxFsError(400, "BAD_REQUEST") + } + dst, errResp := sandboxFilesPath("destination", body.Destination) + if errResp != nil { + return *errResp + } + + defer s.LockUnlock()() + sfs := s.sandboxFs(sandboxID) + srcEntry, _ := sfs.lookup(src) + dstEntry, dstUnderFile := sfs.lookup(dst) + switch { + case srcEntry == nil: + return sandboxFsError(404, "NOT_FOUND") + case dstEntry != nil: + return sandboxFsError(409, "ALREADY_EXISTS") + case dstUnderFile, strings.HasPrefix(dst, src+"/"): + return sandboxFsError(400, "BAD_REQUEST") + case sfs.entries[path.Dir(dst)] == nil: + return sandboxFsError(404, "NOT_FOUND") + } + + moved := map[string]*sandboxFsEntry{} + prefix := src + "/" + for name, entry := range sfs.entries { + switch { + case name == src: + moved[dst] = entry + case strings.HasPrefix(name, prefix): + moved[dst+"/"+strings.TrimPrefix(name, prefix)] = entry + default: + continue + } + delete(sfs.entries, name) + } + maps.Copy(sfs.entries, moved) + return Response{Body: sandbox.RenamePathResponse{}} +} diff --git a/libs/testserver/sandbox_files_test.go b/libs/testserver/sandbox_files_test.go new file mode 100644 index 00000000000..326130a8548 --- /dev/null +++ b/libs/testserver/sandbox_files_test.go @@ -0,0 +1,115 @@ +package testserver_test + +import ( + "fmt" + "io" + "net/http" + "strings" + "sync" + "testing" + + "github.com/databricks/cli/libs/testserver" + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/client" + "github.com/databricks/databricks-sdk-go/config" + "github.com/databricks/databricks-sdk-go/service/sandbox" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const sandboxParent = "sandboxes/" + testserver.TestDefaultSandboxId + +func newSandboxAPI(t *testing.T) sandbox.SandboxInterface { + t.Helper() + server := testserver.New(t) + testserver.AddDefaultHandlers(server) + w, err := databricks.NewWorkspaceClient(&databricks.Config{ + Host: server.URL, + Token: "testtoken", + RateLimitPerSecond: 10000, + }) + require.NoError(t, err) + return w.Sandbox +} + +func upload(t *testing.T, api sandbox.SandboxInterface, path, contents string) error { + t.Helper() + return api.UploadFile(t.Context(), sandbox.UploadFileRequest{ + Parent: sandboxParent, + Path: path, + Contents: io.NopCloser(strings.NewReader(contents)), + }) +} + +func TestSandboxFilesListPagination(t *testing.T) { + api := newSandboxAPI(t) + for _, name := range []string{"c", "a", "e", "b", "d"} { + require.NoError(t, upload(t, api, "/tmp/dir/"+name, "")) + } + + resp, err := api.ListDirectoryContents(t.Context(), sandbox.ListDirectoryContentsRequest{ + Parent: sandboxParent, + Path: "/tmp/dir", + PageSize: 2, + }).Next(t.Context()) + require.NoError(t, err) + assert.Equal(t, "a", resp.Name) + + all, err := api.ListDirectoryContentsAll(t.Context(), sandbox.ListDirectoryContentsRequest{ + Parent: sandboxParent, + Path: "/tmp/dir", + PageSize: 2, + }) + require.NoError(t, err) + var names []string + for _, info := range all { + names = append(names, info.Name) + } + assert.Equal(t, []string{"a", "b", "c", "d", "e"}, names) +} + +func TestSandboxFilesUploadRejectsUnknownParameters(t *testing.T) { + // The real server rejects "overwrite", like any parameter besides "path". + server := testserver.New(t) + testserver.AddDefaultHandlers(server) + c, err := client.New(&config.Config{Host: server.URL, Token: "testtoken"}) + require.NoError(t, err) + + err = c.Do(t.Context(), http.MethodPut, "/api/2.0/sandbox-files/"+sandboxParent+"/content", + map[string]string{"Content-Type": "application/octet-stream"}, + map[string]any{"path": "/tmp/file", "overwrite": false}, + io.NopCloser(strings.NewReader("")), nil) + assert.ErrorIs(t, err, apierr.ErrInvalidParameterValue) +} + +func TestSandboxFilesRenameIsExclusive(t *testing.T) { + api := newSandboxAPI(t) + const n = 8 + for i := range n { + require.NoError(t, upload(t, api, fmt.Sprintf("/tmp/src%d", i), fmt.Sprintf("writer-%d", i))) + } + + var wg sync.WaitGroup + errs := make([]error, n) + for i := range n { + wg.Go(func() { + _, errs[i] = api.RenamePath(t.Context(), sandbox.RenamePathRequest{ + Parent: sandboxParent, + Source: fmt.Sprintf("/tmp/src%d", i), + Destination: "/tmp/dst", + }) + }) + } + wg.Wait() + + wins := 0 + for _, err := range errs { + if err == nil { + wins++ + continue + } + assert.ErrorIs(t, err, apierr.ErrAlreadyExists) + } + assert.Equal(t, 1, wins) +} From 38cd33b6784fb467f27cd7e6ae98ea285ac00f9a Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 6 Nov 2026 16:54:51 +0100 Subject: [PATCH 3/7] Close the gaps between sandbox file API semantics and the Filer contract The server rejects the overwrite upload parameter, so every write failed. Drop it and the hand-built HTTP client in favor of the typed SDK calls. Exclusive writes now upload to a temporary file and rename it into place, since rename refuses an existing destination atomically. Map the server's generic errors to the Filer contract by statting the path: reading a directory, listing a file, deleting a non-empty directory, and creating a directory over a file. Report deletes of absent paths and writes into missing parents as not found. Test the filer against the testserver fake instead of per-test stubs, which encoded server behavior that doesn't exist. Co-authored-by: Isaac --- cmd/sandbox/fs_test.go | 133 +--------- libs/filer/sandbox_client.go | 394 ++++++++++++++++++++++-------- libs/filer/sandbox_client_test.go | 272 ++++++++------------- 3 files changed, 401 insertions(+), 398 deletions(-) diff --git a/cmd/sandbox/fs_test.go b/cmd/sandbox/fs_test.go index 32af4ec3098..247c8b4f449 100644 --- a/cmd/sandbox/fs_test.go +++ b/cmd/sandbox/fs_test.go @@ -1,146 +1,23 @@ package sandbox_test import ( - "context" - "encoding/json" - "os" - "path/filepath" "strings" "testing" sandboxcmd "github.com/databricks/cli/cmd/sandbox" - "github.com/databricks/cli/libs/cmdctx" "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/env" - "github.com/databricks/cli/libs/testserver" - "github.com/databricks/databricks-sdk-go" - "github.com/databricks/databricks-sdk-go/service/sandbox" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -const filesRoute = "/api/2.0/sandbox-files/sandboxes/test-sandbox" - -func sandboxContext(t *testing.T, server *testserver.Server) context.Context { - t.Helper() - home := t.TempDir() - ctx := env.WithUserHomeDir(t.Context(), home) - w, err := databricks.NewWorkspaceClient(&databricks.Config{Host: server.URL, Token: "testtoken"}) - require.NoError(t, err) - state, err := json.Marshal(map[string]any{ - "sandboxes": map[string]any{server.URL: []map[string]string{{"id": "test-sandbox", "name": "my-sandbox"}}}, - }) - require.NoError(t, err) - require.NoError(t, os.MkdirAll(filepath.Join(home, ".databricks"), 0o700)) - require.NoError(t, os.WriteFile(filepath.Join(home, ".databricks", "sandbox.json"), state, 0o600)) - return cmdctx.SetWorkspaceClient(ctx, w) -} - -func runFsCommand(t *testing.T, ctx context.Context, name string, args ...string) (string, error) { - t.Helper() - ctx, stdout := cmdio.NewTestContextWithStdout(ctx) - cmd, _, err := sandboxcmd.New().Find([]string{"fs", name}) - require.NoError(t, err) - cmd.SetContext(ctx) - require.NoError(t, cmd.ParseFlags(args)) - err = cmd.RunE(cmd, cmd.Flags().Args()) - return stdout.String(), err -} - -func TestSandboxFsCopySkipsAndOverwrites(t *testing.T) { - server := testserver.New(t) - ctx := sandboxContext(t, server) - contents := "original" - server.Handle("GET", filesRoute+"/paths:describe", func(req testserver.Request) any { - name := req.URL.Query().Get("path") - fileType := sandbox.SandboxFileTypeSandboxFileTypeFile - if name == "/remote" { - fileType = sandbox.SandboxFileTypeSandboxFileTypeDirectory - } - return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{Path: name, FileType: fileType}} - }) - var overwrites []string - server.Handle("PUT", filesRoute+"/content", func(req testserver.Request) any { - assert.Equal(t, "/remote/file", req.URL.Query().Get("path")) - overwrite := req.URL.Query().Get("overwrite") - overwrites = append(overwrites, overwrite) - if overwrite == "false" { - return testserver.Response{StatusCode: 412, Body: map[string]string{ - "error_code": "RESOURCE_ALREADY_EXISTS", "message": "already exists", - }} - } - contents = string(req.Body) - return testserver.Response{StatusCode: 204} - }) - local := filepath.Join(t.TempDir(), "file") - require.NoError(t, os.WriteFile(local, []byte("replacement"), 0o600)) - output, err := runFsCommand(t, ctx, "cp", local, "my-sandbox:/remote/file") - require.NoError(t, err) - assert.Contains(t, output, "my-sandbox:/remote/file (skipped; already exists)") - assert.Equal(t, "original", contents) - output, err = runFsCommand(t, ctx, "cp", local, "my-sandbox:/remote/file", "--overwrite") - require.NoError(t, err) - assert.NotContains(t, output, "skipped") - assert.Equal(t, "replacement", contents) - assert.Equal(t, []string{"false", "true"}, overwrites) -} - -func TestSandboxFsRecursiveDownload(t *testing.T) { - server := testserver.New(t) - ctx := sandboxContext(t, server) - server.Handle("GET", filesRoute+"/paths:describe", func(req testserver.Request) any { - return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{ - Path: "/remote", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory, - }} - }) - server.Handle("GET", filesRoute+"/directory-entries", func(req testserver.Request) any { - if req.URL.Query().Get("path") == "/remote" { - return sandbox.ListDirectoryContentsResponse{Contents: []sandbox.SandboxFileInfo{ - {Path: "/remote/file", FileType: sandbox.SandboxFileTypeSandboxFileTypeFile}, - {Path: "/remote/sub", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory}, - }} - } - return sandbox.ListDirectoryContentsResponse{Contents: []sandbox.SandboxFileInfo{ - {Path: "/remote/sub/nested", FileType: sandbox.SandboxFileTypeSandboxFileTypeFile}, - }} - }) - server.Handle("GET", filesRoute+"/content", func(req testserver.Request) any { - return []byte(req.URL.Query().Get("path")) - }) - target := filepath.Join(t.TempDir(), "output") - _, err := runFsCommand(t, ctx, "cp", "-r", "my-sandbox:/remote", target) - require.NoError(t, err) - for _, name := range []string{"file", "sub/nested"} { - contents, err := os.ReadFile(filepath.Join(target, filepath.FromSlash(name))) - require.NoError(t, err) - assert.Equal(t, "/remote/"+name, string(contents)) - } -} - -func TestSandboxFsCatAndLs(t *testing.T) { - server := testserver.New(t) - ctx := sandboxContext(t, server) - server.Handle("GET", filesRoute+"/content", func(req testserver.Request) any { - return []byte("contents") - }) - server.Handle("GET", filesRoute+"/directory-entries", func(req testserver.Request) any { - return sandbox.ListDirectoryContentsResponse{Contents: []sandbox.SandboxFileInfo{ - {Path: "/tmp/z", FileType: sandbox.SandboxFileTypeSandboxFileTypeFile}, - {Path: "/tmp/a", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory}, - }} - }) - output, err := runFsCommand(t, ctx, "cat", "my-sandbox:/tmp/z") - require.NoError(t, err) - assert.Equal(t, "contents", output) - output, err = runFsCommand(t, ctx, "ls", "my-sandbox:/tmp", "--absolute") - require.NoError(t, err) - assert.Equal(t, "my-sandbox:/tmp/a\nmy-sandbox:/tmp/z\n", output) -} - func TestSandboxFsRejectsRelativeRemotePaths(t *testing.T) { for _, input := range []string{"my-sandbox:relative", ":/tmp/file"} { t.Run(input, func(t *testing.T) { - _, err := runFsCommand(t, t.Context(), "cat", input) + ctx, _ := cmdio.NewTestContextWithStdout(t.Context()) + cmd, _, err := sandboxcmd.New().Find([]string{"fs", "cat"}) + require.NoError(t, err) + cmd.SetContext(ctx) + err = cmd.RunE(cmd, []string{input}) require.Error(t, err) assert.Contains(t, strings.ToLower(err.Error()), "sandbox") }) diff --git a/libs/filer/sandbox_client.go b/libs/filer/sandbox_client.go index 0aafe848136..7c7cab7a76d 100644 --- a/libs/filer/sandbox_client.go +++ b/libs/filer/sandbox_client.go @@ -3,11 +3,11 @@ package filer import ( "cmp" "context" + "crypto/rand" "errors" "fmt" "io" "io/fs" - "maps" "net/http" "net/url" "path" @@ -15,198 +15,384 @@ import ( "strings" "time" - "github.com/databricks/cli/libs/auth" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/apierr" - "github.com/databricks/databricks-sdk-go/client" "github.com/databricks/databricks-sdk-go/service/sandbox" ) -// SandboxClient implements Filer using the sandbox Files APIs. +// sandboxStatus returns the HTTP status code of err if it is (or wraps) an +// [apierr.APIError], and -1 otherwise. +// +// Sandbox filesystem errors carry the same reason and message ("sandbox +// filesystem operation failed") for unrelated conditions, and their error codes +// don't always match the status (a download of a directory is 409 with error +// code BAD_REQUEST). The status is the most specific signal, but it is still +// ambiguous, so callers stat the path to confirm a condition before mapping it. +func sandboxStatus(err error) int { + if aerr, ok := errors.AsType[*apierr.APIError](err); ok { + return aerr.StatusCode + } + return -1 +} + +// sandboxPathError maps the errors that mean the same for every operation. Callers map +// operation-specific statuses first, because those need a stat to disambiguate. +func sandboxPathError(err error, absPath string) error { + switch { + case errors.Is(err, apierr.ErrNotFound): + return fileDoesNotExistError{absPath} + case sandboxStatus(err) == http.StatusForbidden: + return permissionError{absPath, err} + default: + return err + } +} + +// sandboxFileInfo implements [fs.FileInfo] for an entry in a sandbox filesystem. +type sandboxFileInfo struct { + info sandbox.SandboxFileInfo +} + +func (i sandboxFileInfo) Name() string { + return i.info.Name +} + +func (i sandboxFileInfo) Size() int64 { + return i.info.Size +} + +// Mode reports symlinks as [fs.ModeSymlink] without following them, matching +// [os.Lstat]: the metadata and listing APIs describe the link itself, and +// reporting the link's own type keeps recursive walks from following links out +// of the tree. Read and Write still follow links, because the server does. +// Only the permission bits of the Unix mode are kept; setuid, setgid and sticky +// use different bits in [fs.FileMode]. +func (i sandboxFileInfo) Mode() fs.FileMode { + mode := fs.FileMode(i.info.Mode) & fs.ModePerm + switch i.info.FileType { + case sandbox.SandboxFileTypeSandboxFileTypeFile: + case sandbox.SandboxFileTypeSandboxFileTypeDirectory: + mode |= fs.ModeDir + case sandbox.SandboxFileTypeSandboxFileTypeSymlink: + mode |= fs.ModeSymlink + case sandbox.SandboxFileTypeSandboxFileTypeOther: + mode |= fs.ModeIrregular + } + return mode +} + +func (i sandboxFileInfo) ModTime() time.Time { + return i.info.ModifyTime.AsTime() +} + +func (i sandboxFileInfo) IsDir() bool { + return i.info.FileType == sandbox.SandboxFileTypeSandboxFileTypeDirectory +} + +func (i sandboxFileInfo) Sys() any { + return i.info +} + +// SandboxClient implements the [Filer] interface for the filesystem of a +// single Databricks sandbox. +// +// The sandbox file APIs are more permissive than the Filer contract: uploads +// always replace the target and create missing parents, and deletes succeed if +// the path is absent. Where the contract is stricter, this client checks the +// path first or, for exclusive creates, uses rename (see createExclusive). type SandboxClient struct { - workspaceClient *databricks.WorkspaceClient - apiClient *client.DatabricksClient - parent string - root WorkspaceRootPath + api sandbox.SandboxInterface + + // parent is the sandbox resource name, "sandboxes/{sandbox_id}", that every + // file API call is scoped to. + parent string + + // File operations will be relative to this path. + root WorkspaceRootPath } -// NewSandboxClient returns a filer rooted at an absolute path in a sandbox. +// NewSandboxClient returns a [Filer] for the filesystem of sandbox sandboxID. +// Paths are relative to root, an absolute path inside the sandbox, and cannot +// escape it. A stopped sandbox starts automatically on the first call, which +// makes that call slow. func NewSandboxClient(w *databricks.WorkspaceClient, sandboxID, root string) (Filer, error) { + // The ID becomes a path segment of every request URL, so it can't contain "/" or be a + // relative segment that the URL would resolve away. if sandboxID == "" || sandboxID == "." || sandboxID == ".." || strings.Contains(sandboxID, "/") { return nil, fmt.Errorf("invalid sandbox ID: %q", sandboxID) } if !path.IsAbs(root) { return nil, fmt.Errorf("sandbox root must be absolute: %s", root) } - apiClient, err := client.New(w.Config) - if err != nil { - return nil, err - } return &SandboxClient{ - workspaceClient: w, - apiClient: apiClient, - parent: "sandboxes/" + url.PathEscape(sandboxID), - root: NewWorkspaceRootPath(root), + api: w.Sandbox, + parent: "sandboxes/" + url.PathEscape(sandboxID), + root: NewWorkspaceRootPath(root), }, nil } -func (w *SandboxClient) Write(ctx context.Context, name string, reader io.Reader, mode ...WriteMode) error { - absPath, err := w.root.Join(name) +func (c *SandboxClient) Write(ctx context.Context, name string, reader io.Reader, mode ...WriteMode) error { + absPath, err := c.root.Join(name) if err != nil { return err } + // The server always creates missing parent directories, but the Filer + // contract requires fs.ErrNotExist without CreateParentDirectories. A parent + // created or removed after this check only changes which error is returned. if !slices.Contains(mode, CreateParentDirectories) { dir := path.Dir(absPath) - info, err := w.Stat(ctx, path.Dir(name)) + info, err := c.stat(ctx, dir) if errors.Is(err, fs.ErrNotExist) { return noSuchDirectoryError{dir} } if err != nil { return err } + // stat doesn't follow a final symlink, but upload does, so a symlink to a + // directory is a valid parent. A symlink to a file fails the upload instead. if !info.IsDir() && info.Mode()&fs.ModeSymlink == 0 { return notADirectory{dir} } } - headers := map[string]string{"Content-Type": "application/octet-stream"} - maps.Copy(headers, auth.WorkspaceIDHeaders(w.workspaceClient.Config)) - // The pinned SDK predates the overwrite parameter. Keep the body streaming. - err = w.apiClient.Do(ctx, http.MethodPut, - "/api/2.0/sandbox-files/"+w.parent+"/content", headers, - map[string]any{"path": absPath, "overwrite": slices.Contains(mode, OverwriteIfExists)}, - io.NopCloser(reader), nil) - if aerr, ok := errors.AsType[*apierr.APIError](err); ok && aerr.StatusCode == http.StatusPreconditionFailed { + if slices.Contains(mode, OverwriteIfExists) { + return c.upload(ctx, absPath, reader) + } + + // Fail fast if the target exists, so that rerunning a copy that skips + // existing files doesn't upload every file only to lose the rename. This + // check alone is racy; createExclusive provides the guarantee. + _, err = c.stat(ctx, absPath) + if err == nil { return fileAlreadyExistsError{absPath} } + if !errors.Is(err, fs.ErrNotExist) { + return err + } + + return c.createExclusive(ctx, absPath, reader) +} + +// createExclusive writes a file that must not already exist, and guarantees +// that only one of several concurrent writers creates it. The libs/locker lock +// file relies on this guarantee. +// +// The upload API can't do this: it always replaces the target and rejects +// conditional headers (If-None-Match) with 400. The rename API can: it returns +// 409 ALREADY_EXISTS if the destination exists, and of several concurrent +// renames onto the same absent destination, exactly one succeeds. That +// atomicity is observed behavior (consistent with renameat(2) with +// RENAME_NOREPLACE), not documented; TestFilerConcurrentExclusiveWrite in +// integration/libs/filer pins it. So upload to a temporary file and rename it +// into place. +func (c *SandboxClient) createExclusive(ctx context.Context, absPath string, reader io.Reader) error { + tmpPath := exclusiveCreateTempPath(absPath) + + err := c.upload(ctx, tmpPath, reader) + if err != nil { + // A failed upload can still have created the file, if the response was + // lost after the server committed it. + c.removeTemp(ctx, tmpPath) + return err + } + + _, err = c.api.RenamePath(ctx, sandbox.RenamePathRequest{ + Parent: c.parent, + Source: tmpPath, + Destination: absPath, + }) + if err != nil { + // A failed rename leaves the temporary file in place. + c.removeTemp(ctx, tmpPath) + + // The destination exists. Another writer created it, or it is a + // directory; os.OpenFile with O_EXCL reports both as fs.ErrExist too. + if errors.Is(err, apierr.ErrAlreadyExists) { + return fileAlreadyExistsError{absPath} + } + return err + } + + return nil +} + +// exclusiveCreateTempPath returns the temporary path that createExclusive +// uploads to before renaming it to absPath. It is in the same directory because +// rename fails across filesystems (400), and the directory is the only location +// known to be on the target's filesystem. The leading dot hides it from +// listings that skip dotfiles, and the random part keeps concurrent writers of +// the same target from sharing one temporary file. +func exclusiveCreateTempPath(absPath string) string { + return path.Join(path.Dir(absPath), "."+path.Base(absPath)+"."+rand.Text()+".tmp") +} + +// removeTemp deletes a temporary file left by createExclusive. It runs even if +// ctx is cancelled, because cancellation is a common reason the write failed. +// Its error is ignored: a leftover temporary file doesn't change the outcome of +// the write, and reporting it would hide the error that caused the cleanup. +func (c *SandboxClient) removeTemp(ctx context.Context, tmpPath string) { + _, _ = c.api.DeletePath(context.WithoutCancel(ctx), sandbox.DeletePathRequest{ + Parent: c.parent, + Path: tmpPath, + }) +} + +// upload creates or replaces the file at absPath, creating missing parents. +func (c *SandboxClient) upload(ctx context.Context, absPath string, reader io.Reader) error { + err := c.api.UploadFile(ctx, sandbox.UploadFileRequest{ + Parent: c.parent, + Path: absPath, + // The caller owns reader; the SDK must not close it. + Contents: io.NopCloser(reader), + }) return sandboxPathError(err, absPath) } -func (w *SandboxClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { - absPath, err := w.root.Join(name) +func (c *SandboxClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { + absPath, err := c.root.Join(name) if err != nil { return nil, err } - response, err := w.workspaceClient.Sandbox.DownloadFile(ctx, sandbox.DownloadFileRequest{ - Parent: w.parent, + + resp, err := c.api.DownloadFile(ctx, sandbox.DownloadFileRequest{ + Parent: c.parent, Path: absPath, }) - if err != nil { - return nil, sandboxPathError(err, absPath) + if err == nil { + return resp.Contents, nil } - return response.Contents, nil + + // The server returns 409 for a directory, and also when a parent path + // component is a file. Stat to tell them apart. + if sandboxStatus(err) == http.StatusConflict { + if info, serr := c.stat(ctx, absPath); serr == nil && info.IsDir() { + return nil, notAFile{absPath} + } + } + + // The server returns 404 for a missing file or missing parent directory. It + // also returns 404 for an unknown sandbox, with the same message ("The + // sandbox or file was not found."), so that is reported as a missing file too. + return nil, sandboxPathError(err, absPath) } -func (w *SandboxClient) Delete(ctx context.Context, name string, mode ...DeleteMode) error { - absPath, err := w.root.Join(name) +func (c *SandboxClient) Delete(ctx context.Context, name string, mode ...DeleteMode) error { + absPath, err := c.root.Join(name) if err != nil { return err } - if absPath == w.root.rootPath { + + // Illegal to delete the root path. + if absPath == c.root.rootPath { return cannotDeleteRootError{} } - // DeletePath is idempotent; Filer reports a missing entry. - if _, err := w.Stat(ctx, name); err != nil { + + // The server succeeds if the path is already absent, but the Filer contract + // requires fs.ErrNotExist. The stat also tells a directory from a file, which + // the error mapping below needs. + info, err := c.stat(ctx, absPath) + if err != nil { return err } - _, err = w.workspaceClient.Sandbox.DeletePath(ctx, sandbox.DeletePathRequest{ - Parent: w.parent, + + // A recursive delete is a single server-side call, unlike the Files API + // filer, which walks the tree. + recursive := slices.Contains(mode, DeleteRecursively) + _, err = c.api.DeletePath(ctx, sandbox.DeletePathRequest{ + Parent: c.parent, Path: absPath, - Recursive: slices.Contains(mode, DeleteRecursively), + Recursive: recursive, }) + + // The server returns a generic 400 for a non-empty directory without the + // recursive flag. The stat above confirmed that the path is a directory. + if !recursive && info.IsDir() && sandboxStatus(err) == http.StatusBadRequest { + return directoryNotEmptyError{absPath} + } + return sandboxPathError(err, absPath) } -func (w *SandboxClient) ReadDir(ctx context.Context, name string) ([]fs.DirEntry, error) { - absPath, err := w.root.Join(name) +func (c *SandboxClient) ReadDir(ctx context.Context, name string) ([]fs.DirEntry, error) { + absPath, err := c.root.Join(name) if err != nil { return nil, err } - files, err := w.workspaceClient.Sandbox.ListDirectoryContentsAll(ctx, sandbox.ListDirectoryContentsRequest{ - Parent: w.parent, + + // The listing is paginated (up to 1000 entries per page); this collects all pages. + infos, err := c.api.ListDirectoryContentsAll(ctx, sandbox.ListDirectoryContentsRequest{ + Parent: c.parent, Path: absPath, }) if err != nil { + if errors.Is(err, apierr.ErrNotFound) { + return nil, noSuchDirectoryError{absPath} + } + + // The server returns a generic 400 when the path is a file. + if sandboxStatus(err) == http.StatusBadRequest { + if info, serr := c.stat(ctx, absPath); serr == nil && !info.IsDir() { + return nil, notADirectory{absPath} + } + } + return nil, sandboxPathError(err, absPath) } - entries := make([]fs.DirEntry, len(files)) - for i, file := range files { - entries[i] = fs.FileInfoToDirEntry(sandboxFileInfo{file}) + + entries := make([]fs.DirEntry, len(infos)) + for i, info := range infos { + entries[i] = fs.FileInfoToDirEntry(sandboxFileInfo{info}) } + // Sort for parity with os.ReadDir instead of relying on the server's + // documented name order. slices.SortFunc(entries, func(a, b fs.DirEntry) int { return cmp.Compare(a.Name(), b.Name()) }) return entries, nil } -func (w *SandboxClient) Mkdir(ctx context.Context, name string) error { - absPath, err := w.root.Join(name) +func (c *SandboxClient) Mkdir(ctx context.Context, name string) error { + absPath, err := c.root.Join(name) if err != nil { return err } - _, err = w.workspaceClient.Sandbox.CreateDirectory(ctx, sandbox.CreateDirectoryRequest{ - Parent: w.parent, - Path: absPath, + + _, err = c.api.CreateDirectory(ctx, sandbox.CreateDirectoryRequest{ + Parent: c.parent, + Path: absPath, + // Filer.Mkdir creates intermediate directories. Without Parents, the + // server also returns 409 if the directory already exists. Parents: true, }) + + // The server returns a generic 400 when a file exists at the path. + if sandboxStatus(err) == http.StatusBadRequest { + if info, serr := c.stat(ctx, absPath); serr == nil && !info.IsDir() { + return fileAlreadyExistsError{absPath} + } + } + return sandboxPathError(err, absPath) } -func (w *SandboxClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { - absPath, err := w.root.Join(name) +func (c *SandboxClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { + absPath, err := c.root.Join(name) if err != nil { return nil, err } - response, err := w.workspaceClient.Sandbox.GetMetadata(ctx, sandbox.GetMetadataRequest{ - Parent: w.parent, + + return c.stat(ctx, absPath) +} + +// stat describes absPath without following a final symlink; see +// [sandboxFileInfo.Mode] for why that is kept. +func (c *SandboxClient) stat(ctx context.Context, absPath string) (fs.FileInfo, error) { + resp, err := c.api.GetMetadata(ctx, sandbox.GetMetadataRequest{ + Parent: c.parent, Path: absPath, }) if err != nil { return nil, sandboxPathError(err, absPath) } - return sandboxFileInfo{*response.File}, nil -} - -func sandboxPathError(err error, name string) error { - aerr, ok := errors.AsType[*apierr.APIError](err) - if !ok { - return err - } - switch aerr.StatusCode { - case http.StatusNotFound: - return fileDoesNotExistError{name} - case http.StatusForbidden: - return permissionError{name, err} - default: - return err - } -} - -type sandboxFileInfo struct { - fi sandbox.SandboxFileInfo -} - -func (info sandboxFileInfo) Name() string { return path.Base(info.fi.Path) } -func (info sandboxFileInfo) Size() int64 { return info.fi.Size } -func (info sandboxFileInfo) Sys() any { return info.fi } - -func (info sandboxFileInfo) IsDir() bool { - return info.fi.FileType == sandbox.SandboxFileTypeSandboxFileTypeDirectory -} - -func (info sandboxFileInfo) Mode() fs.FileMode { - mode := fs.FileMode(info.fi.Mode) & fs.ModePerm - switch info.fi.FileType { - case sandbox.SandboxFileTypeSandboxFileTypeDirectory: - mode |= fs.ModeDir - case sandbox.SandboxFileTypeSandboxFileTypeSymlink: - mode |= fs.ModeSymlink - case sandbox.SandboxFileTypeSandboxFileTypeFile: - default: - mode |= fs.ModeIrregular - } - return mode -} -func (info sandboxFileInfo) ModTime() time.Time { - return info.fi.ModifyTime.AsTime() + return sandboxFileInfo{*resp.File}, nil } diff --git a/libs/filer/sandbox_client_test.go b/libs/filer/sandbox_client_test.go index 6df6d22db54..64c326906e2 100644 --- a/libs/filer/sandbox_client_test.go +++ b/libs/filer/sandbox_client_test.go @@ -1,205 +1,145 @@ package filer_test import ( - "encoding/json" + "fmt" "io" "io/fs" - "net/http" - "strconv" "strings" + "sync" "testing" - "time" - "github.com/databricks/cli/libs/auth" "github.com/databricks/cli/libs/filer" "github.com/databricks/cli/libs/testserver" "github.com/databricks/databricks-sdk-go" - "github.com/databricks/databricks-sdk-go/apierr" - sdktime "github.com/databricks/databricks-sdk-go/common/types/time" - "github.com/databricks/databricks-sdk-go/service/sandbox" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -const sandboxFilesRoute = "/api/2.0/sandbox-files/sandboxes/test-sandbox" - -func newSandboxFiler(t *testing.T, server *testserver.Server, root, workspaceID string) filer.Filer { +func newSandboxTestClient(t *testing.T, server *testserver.Server) *databricks.WorkspaceClient { t.Helper() w, err := databricks.NewWorkspaceClient(&databricks.Config{ - Host: server.URL, Token: "testtoken", WorkspaceID: workspaceID, + Host: server.URL, + Token: "testtoken", + // The SDK's default rate limit would throttle the concurrent writes below. + RateLimitPerSecond: 10000, }) require.NoError(t, err) - f, err := filer.NewSandboxClient(w, "test-sandbox", root) + return w +} + +// newSandboxTestFiler returns a sandbox filer rooted at "/tmp", backed by the testserver +// fake of the sandbox file APIs, which reproduces the server semantics the filer works around. +func newSandboxTestFiler(t *testing.T) filer.Filer { + t.Helper() + server := testserver.New(t) + testserver.AddDefaultHandlers(server) + f, err := filer.NewSandboxClient(newSandboxTestClient(t, server), testserver.TestDefaultSandboxId, "/tmp") require.NoError(t, err) return f } -func TestSandboxWriteOverwrite(t *testing.T) { - for _, overwrite := range []bool{false, true} { - t.Run(strconv.FormatBool(overwrite), func(t *testing.T) { - server := testserver.New(t) - server.Handle("PUT", sandboxFilesRoute+"/content", func(req testserver.Request) any { - assert.Equal(t, "/root/a ?#雪.txt", req.URL.Query().Get("path")) - assert.Equal(t, []string{strconv.FormatBool(overwrite)}, req.URL.Query()["overwrite"]) - assert.Equal(t, "application/octet-stream", req.Headers.Get("Content-Type")) - assert.Equal(t, "123", req.Headers.Get(auth.WorkspaceIDHeader)) - assert.Equal(t, "contents", string(req.Body)) - if !overwrite { - return testserver.Response{StatusCode: 412, Body: map[string]string{ - "error_code": "RESOURCE_ALREADY_EXISTS", "message": "already exists", - }} - } - return testserver.Response{StatusCode: 204} - }) - f := newSandboxFiler(t, server, "/root", "123") - mode := []filer.WriteMode{filer.CreateParentDirectories} - if overwrite { - mode = append(mode, filer.OverwriteIfExists) - } - err := f.Write(t.Context(), "a ?#雪.txt", strings.NewReader("contents"), mode...) - if overwrite { - require.NoError(t, err) - } else { - require.ErrorIs(t, err, fs.ErrExist) - } - }) - } +func readString(t *testing.T, f filer.Filer, name string) string { + t.Helper() + r, err := f.Read(t.Context(), name) + require.NoError(t, err) + defer r.Close() + b, err := io.ReadAll(r) + require.NoError(t, err) + return string(b) } -func TestSandboxWriteParent(t *testing.T) { - for _, fileType := range []sandbox.SandboxFileType{ - sandbox.SandboxFileTypeSandboxFileTypeDirectory, - sandbox.SandboxFileTypeSandboxFileTypeFile, - sandbox.SandboxFileTypeSandboxFileTypeSymlink, - } { - t.Run(string(fileType), func(t *testing.T) { - server := testserver.New(t) - server.Handle("GET", sandboxFilesRoute+"/paths:describe", func(req testserver.Request) any { - assert.Equal(t, "/root/sub", req.URL.Query().Get("path")) - return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{FileType: fileType}} - }) - uploaded := false - server.Handle("PUT", sandboxFilesRoute+"/content", func(req testserver.Request) any { - uploaded = true - assert.Empty(t, req.Headers.Get(auth.WorkspaceIDHeader)) - return testserver.Response{StatusCode: 204} - }) - f := newSandboxFiler(t, server, "/root", "none") - err := f.Write(t.Context(), "sub/file", strings.NewReader("content")) - if fileType != sandbox.SandboxFileTypeSandboxFileTypeFile { - require.NoError(t, err) - assert.True(t, uploaded) - } else { - require.ErrorIs(t, err, fs.ErrInvalid) - assert.False(t, uploaded) - } - }) +func TestSandboxClientRejectsInvalidArguments(t *testing.T) { + w := &databricks.WorkspaceClient{} + for _, id := range []string{"", ".", "..", "a/b"} { + _, err := filer.NewSandboxClient(w, id, "/") + assert.ErrorContains(t, err, "invalid sandbox ID") } + _, err := filer.NewSandboxClient(w, "id", "relative") + assert.ErrorContains(t, err, "sandbox root must be absolute") } -func TestSandboxReadAndMetadata(t *testing.T) { - server := testserver.New(t) - modified := time.Date(2026, 11, 5, 1, 2, 3, 123456789, time.UTC) - server.Handle("GET", sandboxFilesRoute+"/content", func(req testserver.Request) any { - assert.Equal(t, "/tmp/file", req.URL.Query().Get("path")) - return testserver.Response{Body: []byte("contents"), Headers: http.Header{"Content-Type": {"application/octet-stream"}}} - }) - server.Handle("GET", sandboxFilesRoute+"/paths:describe", func(req testserver.Request) any { - return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{ - Path: "/tmp/file", Size: 8, Mode: 0o640, ModifyTime: sdktime.New(modified), - FileType: sandbox.SandboxFileTypeSandboxFileTypeFile, - }} - }) - f := newSandboxFiler(t, server, "/", "") - r, err := f.Read(t.Context(), "/tmp/file") - require.NoError(t, err) - defer r.Close() - contents, err := io.ReadAll(r) - require.NoError(t, err) - assert.Equal(t, "contents", string(contents)) - info, err := f.Stat(t.Context(), "/tmp/file") +func TestSandboxClientWriteDoesNotOverwrite(t *testing.T) { + // The upload API always replaces, so exclusive writes go through a temporary file + // and a rename, which must not leave the temporary file behind. + ctx := t.Context() + f := newSandboxTestFiler(t) + + require.NoError(t, f.Write(ctx, "file", strings.NewReader("one"))) + assert.ErrorIs(t, f.Write(ctx, "file", strings.NewReader("two")), fs.ErrExist) + assert.Equal(t, "one", readString(t, f, "file")) + + entries, err := f.ReadDir(ctx, ".") require.NoError(t, err) - assert.Equal(t, "file", info.Name()) - assert.Equal(t, int64(8), info.Size()) - assert.Equal(t, fs.FileMode(0o640), info.Mode()) - assert.Equal(t, modified, info.ModTime()) - assert.False(t, info.IsDir()) + require.Len(t, entries, 1) + assert.Equal(t, "file", entries[0].Name()) } -func TestSandboxReadDirPagination(t *testing.T) { - server := testserver.New(t) - var tokens []string - server.Handle("GET", sandboxFilesRoute+"/directory-entries", func(req testserver.Request) any { - assert.Equal(t, "/tmp", req.URL.Query().Get("path")) - token := req.URL.Query().Get("page_token") - tokens = append(tokens, token) - if token == "" { - return sandbox.ListDirectoryContentsResponse{ - Contents: []sandbox.SandboxFileInfo{{Path: "/tmp/z", FileType: sandbox.SandboxFileTypeSandboxFileTypeSymlink}}, - NextPageToken: "page2", - } +func TestSandboxClientConcurrentExclusiveWrite(t *testing.T) { + ctx := t.Context() + f := newSandboxTestFiler(t) + + const writers = 8 + var wg sync.WaitGroup + errs := make([]error, writers) + for i := range writers { + wg.Go(func() { + errs[i] = f.Write(ctx, "file", strings.NewReader(fmt.Sprintf("writer-%d", i))) + }) + } + wg.Wait() + + winner := -1 + for i, err := range errs { + if err == nil { + assert.Equal(t, -1, winner, "more than one writer created the file") + winner = i + continue } - return sandbox.ListDirectoryContentsResponse{Contents: []sandbox.SandboxFileInfo{ - {Path: "/tmp/a", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory}, - }} - }) - f := newSandboxFiler(t, server, "/", "") - entries, err := f.ReadDir(t.Context(), "/tmp") - require.NoError(t, err) - require.Len(t, entries, 2) - assert.Equal(t, []string{"", "page2"}, tokens) - assert.Equal(t, "a", entries[0].Name()) - assert.True(t, entries[0].IsDir()) - assert.Equal(t, "z", entries[1].Name()) - assert.Equal(t, fs.ModeSymlink, entries[1].Type()) + assert.ErrorIs(t, err, fs.ErrExist) + } + require.NotEqual(t, -1, winner, "no writer created the file") + assert.Equal(t, fmt.Sprintf("writer-%d", winner), readString(t, f, "file")) } -func TestSandboxMkdirAndDelete(t *testing.T) { +func TestSandboxClientWriteMissingParent(t *testing.T) { + // The upload API always creates missing parents. + err := newSandboxTestFiler(t).Write(t.Context(), "missing/file", strings.NewReader("x")) + assert.ErrorIs(t, err, fs.ErrNotExist) +} + +func TestSandboxClientDeleteMissing(t *testing.T) { + // The delete API succeeds if the path is absent. + err := newSandboxTestFiler(t).Delete(t.Context(), "missing") + assert.ErrorIs(t, err, fs.ErrNotExist) +} + +func TestSandboxClientGenericErrors(t *testing.T) { + // The server reports each of these with a generic status, which the filer + // disambiguates with a stat. + ctx := t.Context() + f := newSandboxTestFiler(t) + require.NoError(t, f.Write(ctx, "dir/file", strings.NewReader("x"), filer.CreateParentDirectories)) + + _, err := f.Read(ctx, "dir") + assert.ErrorIs(t, err, fs.ErrInvalid, "read of a directory") + _, err = f.ReadDir(ctx, "dir/file") + assert.ErrorIs(t, err, fs.ErrInvalid, "list of a file") + assert.ErrorIs(t, f.Delete(ctx, "dir"), fs.ErrInvalid, "delete of a non-empty directory") + assert.ErrorIs(t, f.Mkdir(ctx, "dir/file"), fs.ErrExist, "mkdir onto a file") +} + +func TestSandboxClientPermissionDenied(t *testing.T) { + // The fake doesn't model filesystem permissions, so stub the response a real + // sandbox returns for a path the sandbox user can't access. server := testserver.New(t) - server.Handle("GET", sandboxFilesRoute+"/paths:describe", func(req testserver.Request) any { - return sandbox.GetMetadataResponse{File: &sandbox.SandboxFileInfo{ - Path: "/root/sub", FileType: sandbox.SandboxFileTypeSandboxFileTypeDirectory, + server.Handle("GET", "/api/2.0/sandbox-files/sandboxes/{sandbox_id}/paths:describe", func(req testserver.Request) any { + return testserver.Response{StatusCode: 403, Body: map[string]string{ + "error_code": "PERMISSION_DENIED", "message": "Access to the requested operation is denied.", }} }) - server.Handle("POST", sandboxFilesRoute+"/directories:make", func(req testserver.Request) any { - var body sandbox.CreateDirectoryRequest - require.NoError(t, json.Unmarshal(req.Body, &body)) - assert.Equal(t, "/root/sub", body.Path) - assert.True(t, body.Parents) - return sandbox.CreateDirectoryResponse{} - }) - server.Handle("POST", sandboxFilesRoute+"/paths:remove", func(req testserver.Request) any { - var body sandbox.DeletePathRequest - require.NoError(t, json.Unmarshal(req.Body, &body)) - assert.Equal(t, "/root/sub", body.Path) - assert.True(t, body.Recursive) - return sandbox.DeletePathResponse{} - }) - f := newSandboxFiler(t, server, "/root", "") - require.NoError(t, f.Mkdir(t.Context(), "sub")) - require.NoError(t, f.Delete(t.Context(), "sub", filer.DeleteRecursively)) - require.ErrorIs(t, f.Delete(t.Context(), ".", filer.DeleteRecursively), fs.ErrInvalid) - require.Error(t, f.Mkdir(t.Context(), "../escape")) -} + f, err := filer.NewSandboxClient(newSandboxTestClient(t, server), testserver.TestDefaultSandboxId, "/") + require.NoError(t, err) -func TestSandboxPathErrors(t *testing.T) { - for _, tc := range []struct { - status int - code string - want error - }{ - {404, "NOT_FOUND", fs.ErrNotExist}, - {403, "PERMISSION_DENIED", fs.ErrPermission}, - {400, "INVALID_PARAMETER_VALUE", apierr.ErrInvalidParameterValue}, - } { - t.Run(tc.code, func(t *testing.T) { - server := testserver.New(t) - server.Handle("GET", sandboxFilesRoute+"/paths:describe", func(req testserver.Request) any { - return testserver.Response{StatusCode: tc.status, Body: map[string]string{"error_code": tc.code, "message": "failed"}} - }) - f := newSandboxFiler(t, server, "/", "") - _, err := f.Stat(t.Context(), "missing") - require.ErrorIs(t, err, tc.want) - }) - } + _, err = f.Stat(t.Context(), "/root/x") + assert.ErrorIs(t, err, fs.ErrPermission) } From 530f963fc5700952f2c7af2fc0a77613f0386910 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 6 Nov 2026 16:54:51 +0100 Subject: [PATCH 4/7] Run the shared filer integration suites against a sandbox Enabled by TEST_SANDBOX_ID. Also add a suite for concurrent exclusive writes, run for the local and sandbox filers. Co-authored-by: Isaac --- integration/libs/filer/filer_test.go | 59 ++++++++++++++++++++++++++ integration/libs/filer/helpers_test.go | 35 +++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/integration/libs/filer/filer_test.go b/integration/libs/filer/filer_test.go index 58618628b7e..7a8a4b427a0 100644 --- a/integration/libs/filer/filer_test.go +++ b/integration/libs/filer/filer_test.go @@ -4,10 +4,12 @@ import ( "bytes" "context" "encoding/json" + "fmt" "io" "io/fs" "path" "strings" + "sync" "testing" "github.com/databricks/cli/internal/testutil" @@ -127,6 +129,7 @@ func TestFilerRecursiveDelete(t *testing.T) { {"dbfs", setupDbfsFiler}, {"files", setupUcVolumesFiler}, {"workspace files extensions", setupWsfsExtensionsFiler}, + {"sandbox", setupSandboxFiler}, } { t.Run(testCase.name, func(t *testing.T) { t.Parallel() @@ -229,6 +232,7 @@ func TestFilerReadWrite(t *testing.T) { {"dbfs", setupDbfsFiler}, {"files", setupUcVolumesFiler}, {"workspace files extensions", setupWsfsExtensionsFiler}, + {"sandbox", setupSandboxFiler}, } { t.Run(testCase.name, func(t *testing.T) { t.Parallel() @@ -335,6 +339,7 @@ func TestFilerReadDir(t *testing.T) { {"dbfs", setupDbfsFiler}, {"files", setupUcVolumesFiler}, {"workspace files extensions", setupWsfsExtensionsFiler}, + {"sandbox", setupSandboxFiler}, } { t.Run(testCase.name, func(t *testing.T) { t.Parallel() @@ -346,6 +351,60 @@ func TestFilerReadDir(t *testing.T) { } } +// Concurrent writes without OverwriteIfExists must let exactly one writer +// create the file. The lock file in libs/locker relies on this. +func commonFilerConcurrentExclusiveWriteTest(t *testing.T, ctx context.Context, f filer.Filer) { + const writers = 8 + + var wg sync.WaitGroup + errs := make([]error, writers) + for i := range writers { + wg.Go(func() { + errs[i] = f.Write(ctx, "file", strings.NewReader(fmt.Sprintf("writer-%d", i))) + }) + } + wg.Wait() + + winner := -1 + for i, err := range errs { + if err == nil { + assert.Equal(t, -1, winner, "more than one writer created the file") + winner = i + continue + } + assert.ErrorIs(t, err, fs.ErrExist) + } + require.NotEqual(t, -1, winner, "no writer created the file") + filerTest{t, f}.assertContents(ctx, "file", fmt.Sprintf("writer-%d", winner)) + + // Writers that lost must not leave anything behind. + entries, err := f.ReadDir(ctx, ".") + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, "file", entries[0].Name()) +} + +func TestFilerConcurrentExclusiveWrite(t *testing.T) { + t.Parallel() + + // Only filers verified to make exclusive creates atomic are listed. + for _, testCase := range []struct { + name string + f func(t testutil.TestingT) (filer.Filer, string) + }{ + {"local", setupLocalFiler}, + {"sandbox", setupSandboxFiler}, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + f, _ := testCase.f(t) + ctx := t.Context() + + commonFilerConcurrentExclusiveWriteTest(t, ctx, f) + }) + } +} + func TestFilerWorkspaceNotebook(t *testing.T) { t.Parallel() diff --git a/integration/libs/filer/helpers_test.go b/integration/libs/filer/helpers_test.go index baca230b791..35085869c17 100644 --- a/integration/libs/filer/helpers_test.go +++ b/integration/libs/filer/helpers_test.go @@ -1,6 +1,7 @@ package filer_test import ( + "context" "errors" "net/http" "os" @@ -12,6 +13,7 @@ import ( "github.com/databricks/cli/libs/filer" "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/service/sandbox" "github.com/stretchr/testify/require" ) @@ -70,3 +72,36 @@ func setupUcVolumesFiler(t testutil.TestingT) (filer.Filer, string) { return f, path.Join("dbfs:/", tmpdir) } + +func setupSandboxFiler(t testutil.TestingT) (filer.Filer, string) { + ctx, wt := acc.WorkspaceTest(t) + + sandboxID := os.Getenv("TEST_SANDBOX_ID") + if sandboxID == "" { + t.Skip("Skipping tests that require a sandbox when TEST_SANDBOX_ID is not set.") + } + + parent := "sandboxes/" + sandboxID + tmpdir := "/tmp/" + testutil.RandomName("filer-it-") + _, err := wt.W.Sandbox.CreateDirectory(ctx, sandbox.CreateDirectoryRequest{ + Parent: parent, + Path: tmpdir, + Parents: true, + }) + require.NoError(t, err) + + t.Cleanup(func() { + _, err := wt.W.Sandbox.DeletePath(context.WithoutCancel(ctx), sandbox.DeletePathRequest{ + Parent: parent, + Path: tmpdir, + Recursive: true, + }) + if err != nil { + t.Logf("Unable to remove temporary sandbox directory %s: %#v", tmpdir, err) + } + }) + + f, err := filer.NewSandboxClient(wt.W, sandboxID, tmpdir) + require.NoError(t, err) + return f, tmpdir +} From 1246b12af15557ff532a18655b0f2c8f934c244c Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 6 Nov 2026 16:59:15 +0100 Subject: [PATCH 5/7] Add acceptance tests for the sandbox fs commands One test per command (cp, ls, cat, mkdir, rm), run against the testserver fake locally. With Cloud = true they also run against the existing sandbox named by TEST_SANDBOX_ID, so a run doesn't create a sandbox; cloud environments without one skip them. Co-authored-by: Isaac --- acceptance/acceptance_test.go | 4 +++ acceptance/cmd/sandbox/fs/cat/hello.txt | 1 + acceptance/cmd/sandbox/fs/cat/out.test.toml | 1 + acceptance/cmd/sandbox/fs/cat/output.txt | 15 +++++++++ acceptance/cmd/sandbox/fs/cat/script | 8 +++++ acceptance/cmd/sandbox/fs/cp/local.txt | 1 + .../cmd/sandbox/fs/cp/localdir/file1.txt | 1 + .../cmd/sandbox/fs/cp/localdir/file2.txt | 1 + acceptance/cmd/sandbox/fs/cp/out.test.toml | 1 + acceptance/cmd/sandbox/fs/cp/output.txt | 33 +++++++++++++++++++ acceptance/cmd/sandbox/fs/cp/script | 16 +++++++++ acceptance/cmd/sandbox/fs/ls/hello.txt | 1 + acceptance/cmd/sandbox/fs/ls/out.test.toml | 1 + acceptance/cmd/sandbox/fs/ls/output.txt | 20 +++++++++++ acceptance/cmd/sandbox/fs/ls/script | 10 ++++++ acceptance/cmd/sandbox/fs/mkdir/hello.txt | 1 + acceptance/cmd/sandbox/fs/mkdir/out.test.toml | 1 + acceptance/cmd/sandbox/fs/mkdir/output.txt | 15 +++++++++ acceptance/cmd/sandbox/fs/mkdir/script | 10 ++++++ acceptance/cmd/sandbox/fs/rm/hello.txt | 1 + acceptance/cmd/sandbox/fs/rm/out.test.toml | 1 + acceptance/cmd/sandbox/fs/rm/output.txt | 27 +++++++++++++++ acceptance/cmd/sandbox/fs/rm/script | 15 +++++++++ acceptance/cmd/sandbox/fs/script.prepare | 14 ++++++++ acceptance/cmd/sandbox/fs/test.toml | 3 ++ 25 files changed, 202 insertions(+) create mode 100644 acceptance/cmd/sandbox/fs/cat/hello.txt create mode 100644 acceptance/cmd/sandbox/fs/cat/out.test.toml create mode 100644 acceptance/cmd/sandbox/fs/cat/output.txt create mode 100644 acceptance/cmd/sandbox/fs/cat/script create mode 100644 acceptance/cmd/sandbox/fs/cp/local.txt create mode 100644 acceptance/cmd/sandbox/fs/cp/localdir/file1.txt create mode 100644 acceptance/cmd/sandbox/fs/cp/localdir/file2.txt create mode 100644 acceptance/cmd/sandbox/fs/cp/out.test.toml create mode 100644 acceptance/cmd/sandbox/fs/cp/output.txt create mode 100644 acceptance/cmd/sandbox/fs/cp/script create mode 100644 acceptance/cmd/sandbox/fs/ls/hello.txt create mode 100644 acceptance/cmd/sandbox/fs/ls/out.test.toml create mode 100644 acceptance/cmd/sandbox/fs/ls/output.txt create mode 100644 acceptance/cmd/sandbox/fs/ls/script create mode 100644 acceptance/cmd/sandbox/fs/mkdir/hello.txt create mode 100644 acceptance/cmd/sandbox/fs/mkdir/out.test.toml create mode 100644 acceptance/cmd/sandbox/fs/mkdir/output.txt create mode 100644 acceptance/cmd/sandbox/fs/mkdir/script create mode 100644 acceptance/cmd/sandbox/fs/rm/hello.txt create mode 100644 acceptance/cmd/sandbox/fs/rm/out.test.toml create mode 100644 acceptance/cmd/sandbox/fs/rm/output.txt create mode 100644 acceptance/cmd/sandbox/fs/rm/script create mode 100644 acceptance/cmd/sandbox/fs/script.prepare create mode 100644 acceptance/cmd/sandbox/fs/test.toml diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 3805300a8e7..86b3c5fdb67 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -190,6 +190,7 @@ func setReplsForTestEnvVars(t *testing.T, repls *testdiff.ReplacementsContext) { "TEST_DEFAULT_WAREHOUSE_ID", "TEST_DEFAULT_CLUSTER_ID", "TEST_INSTANCE_POOL_ID", + "TEST_SANDBOX_ID", } for _, envVar := range envVars { if value := os.Getenv(envVar); value != "" { @@ -449,6 +450,9 @@ func testAccept(t *testing.T, inprocessMode bool, selectedTests []string, skipTo if os.Getenv("TEST_INSTANCE_POOL_ID") == "" { t.Setenv("TEST_INSTANCE_POOL_ID", testserver.TestDefaultInstancePoolId) } + if os.Getenv("TEST_SANDBOX_ID") == "" { + t.Setenv("TEST_SANDBOX_ID", testserver.TestDefaultSandboxId) + } } const sharedProxyHint = "; re-run with -debugsandbox to see which test caused this" diff --git a/acceptance/cmd/sandbox/fs/cat/hello.txt b/acceptance/cmd/sandbox/fs/cat/hello.txt new file mode 100644 index 00000000000..a0423896973 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cat/hello.txt @@ -0,0 +1 @@ +hello world! diff --git a/acceptance/cmd/sandbox/fs/cat/out.test.toml b/acceptance/cmd/sandbox/fs/cat/out.test.toml new file mode 100644 index 00000000000..c7c6f58ed6e --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cat/out.test.toml @@ -0,0 +1 @@ +Cloud = true diff --git a/acceptance/cmd/sandbox/fs/cat/output.txt b/acceptance/cmd/sandbox/fs/cat/output.txt new file mode 100644 index 00000000000..59daea2cbaa --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cat/output.txt @@ -0,0 +1,15 @@ + +=== cat a file +>>> [CLI] sandbox fs cat [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt +hello world! + +=== cat a directory, then a missing path +>>> [CLI] sandbox fs cat [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME] +Error: not a file: /tmp/sandbox-fs-[UNIQUE_NAME] + +Exit code: 1 + +>>> [CLI] sandbox fs cat [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/missing +Error: file does not exist: /tmp/sandbox-fs-[UNIQUE_NAME]/missing + +Exit code: 1 diff --git a/acceptance/cmd/sandbox/fs/cat/script b/acceptance/cmd/sandbox/fs/cat/script new file mode 100644 index 00000000000..7b784014b98 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cat/script @@ -0,0 +1,8 @@ +$CLI sandbox fs cp hello.txt $SANDBOX_ROOT/hello.txt > /dev/null + +title "cat a file" +trace $CLI sandbox fs cat $SANDBOX_ROOT/hello.txt + +title "cat a directory, then a missing path" +errcode trace $CLI sandbox fs cat $SANDBOX_ROOT +errcode trace $CLI sandbox fs cat $SANDBOX_ROOT/missing diff --git a/acceptance/cmd/sandbox/fs/cp/local.txt b/acceptance/cmd/sandbox/fs/cp/local.txt new file mode 100644 index 00000000000..a0423896973 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cp/local.txt @@ -0,0 +1 @@ +hello world! diff --git a/acceptance/cmd/sandbox/fs/cp/localdir/file1.txt b/acceptance/cmd/sandbox/fs/cp/localdir/file1.txt new file mode 100644 index 00000000000..d9039017ab6 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cp/localdir/file1.txt @@ -0,0 +1 @@ +file1 content diff --git a/acceptance/cmd/sandbox/fs/cp/localdir/file2.txt b/acceptance/cmd/sandbox/fs/cp/localdir/file2.txt new file mode 100644 index 00000000000..f3c77b12c6c --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cp/localdir/file2.txt @@ -0,0 +1 @@ +file2 content diff --git a/acceptance/cmd/sandbox/fs/cp/out.test.toml b/acceptance/cmd/sandbox/fs/cp/out.test.toml new file mode 100644 index 00000000000..c7c6f58ed6e --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cp/out.test.toml @@ -0,0 +1 @@ +Cloud = true diff --git a/acceptance/cmd/sandbox/fs/cp/output.txt b/acceptance/cmd/sandbox/fs/cp/output.txt new file mode 100644 index 00000000000..233fa026de7 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cp/output.txt @@ -0,0 +1,33 @@ + +=== Upload a file +>>> [CLI] sandbox fs cp local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt +local.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt + +>>> [CLI] sandbox fs cat [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt +hello world! + +=== Writing over an existing file needs --overwrite +>>> [CLI] sandbox fs cp local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt +local.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt (skipped; already exists) + +>>> [CLI] sandbox fs cp --overwrite local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt +local.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt + +=== Writing into a directory that does not exist fails +>>> [CLI] sandbox fs cp local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/missing/file.txt +Error: no such directory: /tmp/sandbox-fs-[UNIQUE_NAME]/missing + +Exit code: 1 + +=== Copy a directory recursively, and back +>>> [CLI] sandbox fs cp -r localdir [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir +localdir/file1.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir/file1.txt +localdir/file2.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir/file2.txt + +>>> [CLI] sandbox fs cp -r [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir downloaded +[TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir/file1.txt -> downloaded/file1.txt +[TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir/file2.txt -> downloaded/file2.txt + +>>> cat downloaded/file1.txt downloaded/file2.txt +file1 content +file2 content diff --git a/acceptance/cmd/sandbox/fs/cp/script b/acceptance/cmd/sandbox/fs/cp/script new file mode 100644 index 00000000000..e77e60407a0 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cp/script @@ -0,0 +1,16 @@ +title "Upload a file" +trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/file.txt +trace $CLI sandbox fs cat $SANDBOX_ROOT/file.txt + +title "Writing over an existing file needs --overwrite" +trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/file.txt +trace $CLI sandbox fs cp --overwrite local.txt $SANDBOX_ROOT/file.txt + +title "Writing into a directory that does not exist fails" +errcode trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/missing/file.txt + +title "Copy a directory recursively, and back" +trace $CLI sandbox fs cp -r localdir $SANDBOX_ROOT/dir 2>&1 | sort +trace $CLI sandbox fs cp -r $SANDBOX_ROOT/dir downloaded 2>&1 | sort +trace cat downloaded/file1.txt downloaded/file2.txt +rm -r downloaded diff --git a/acceptance/cmd/sandbox/fs/ls/hello.txt b/acceptance/cmd/sandbox/fs/ls/hello.txt new file mode 100644 index 00000000000..a0423896973 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/ls/hello.txt @@ -0,0 +1 @@ +hello world! diff --git a/acceptance/cmd/sandbox/fs/ls/out.test.toml b/acceptance/cmd/sandbox/fs/ls/out.test.toml new file mode 100644 index 00000000000..c7c6f58ed6e --- /dev/null +++ b/acceptance/cmd/sandbox/fs/ls/out.test.toml @@ -0,0 +1 @@ +Cloud = true diff --git a/acceptance/cmd/sandbox/fs/ls/output.txt b/acceptance/cmd/sandbox/fs/ls/output.txt new file mode 100644 index 00000000000..94aa1300b7c --- /dev/null +++ b/acceptance/cmd/sandbox/fs/ls/output.txt @@ -0,0 +1,20 @@ + +=== List a directory +>>> [CLI] sandbox fs ls [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME] +dir +hello.txt + +>>> [CLI] sandbox fs ls --absolute [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME] +[TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir +[TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt + +=== List a file, then a missing path +>>> [CLI] sandbox fs ls [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt +Error: not a directory: /tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt + +Exit code: 1 + +>>> [CLI] sandbox fs ls [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/missing +Error: no such directory: /tmp/sandbox-fs-[UNIQUE_NAME]/missing + +Exit code: 1 diff --git a/acceptance/cmd/sandbox/fs/ls/script b/acceptance/cmd/sandbox/fs/ls/script new file mode 100644 index 00000000000..340f1fe93eb --- /dev/null +++ b/acceptance/cmd/sandbox/fs/ls/script @@ -0,0 +1,10 @@ +$CLI sandbox fs cp hello.txt $SANDBOX_ROOT/hello.txt > /dev/null +$CLI sandbox fs mkdir $SANDBOX_ROOT/dir + +title "List a directory" +trace $CLI sandbox fs ls $SANDBOX_ROOT +trace $CLI sandbox fs ls --absolute $SANDBOX_ROOT + +title "List a file, then a missing path" +errcode trace $CLI sandbox fs ls $SANDBOX_ROOT/hello.txt +errcode trace $CLI sandbox fs ls $SANDBOX_ROOT/missing diff --git a/acceptance/cmd/sandbox/fs/mkdir/hello.txt b/acceptance/cmd/sandbox/fs/mkdir/hello.txt new file mode 100644 index 00000000000..a0423896973 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/mkdir/hello.txt @@ -0,0 +1 @@ +hello world! diff --git a/acceptance/cmd/sandbox/fs/mkdir/out.test.toml b/acceptance/cmd/sandbox/fs/mkdir/out.test.toml new file mode 100644 index 00000000000..c7c6f58ed6e --- /dev/null +++ b/acceptance/cmd/sandbox/fs/mkdir/out.test.toml @@ -0,0 +1 @@ +Cloud = true diff --git a/acceptance/cmd/sandbox/fs/mkdir/output.txt b/acceptance/cmd/sandbox/fs/mkdir/output.txt new file mode 100644 index 00000000000..dab3eb228f8 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/mkdir/output.txt @@ -0,0 +1,15 @@ + +=== Create nested directories +>>> [CLI] sandbox fs mkdir [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/a/b/c + +>>> [CLI] sandbox fs ls [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/a/b +c + +=== Creating an existing directory succeeds +>>> [CLI] sandbox fs mkdir [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/a/b/c + +=== Creating a directory over a file fails +>>> [CLI] sandbox fs mkdir [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt +Error: file already exists: /tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt + +Exit code: 1 diff --git a/acceptance/cmd/sandbox/fs/mkdir/script b/acceptance/cmd/sandbox/fs/mkdir/script new file mode 100644 index 00000000000..635408fac67 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/mkdir/script @@ -0,0 +1,10 @@ +title "Create nested directories" +trace $CLI sandbox fs mkdir $SANDBOX_ROOT/a/b/c +trace $CLI sandbox fs ls $SANDBOX_ROOT/a/b + +title "Creating an existing directory succeeds" +trace $CLI sandbox fs mkdir $SANDBOX_ROOT/a/b/c + +title "Creating a directory over a file fails" +$CLI sandbox fs cp hello.txt $SANDBOX_ROOT/hello.txt > /dev/null +errcode trace $CLI sandbox fs mkdir $SANDBOX_ROOT/hello.txt diff --git a/acceptance/cmd/sandbox/fs/rm/hello.txt b/acceptance/cmd/sandbox/fs/rm/hello.txt new file mode 100644 index 00000000000..a0423896973 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/rm/hello.txt @@ -0,0 +1 @@ +hello world! diff --git a/acceptance/cmd/sandbox/fs/rm/out.test.toml b/acceptance/cmd/sandbox/fs/rm/out.test.toml new file mode 100644 index 00000000000..c7c6f58ed6e --- /dev/null +++ b/acceptance/cmd/sandbox/fs/rm/out.test.toml @@ -0,0 +1 @@ +Cloud = true diff --git a/acceptance/cmd/sandbox/fs/rm/output.txt b/acceptance/cmd/sandbox/fs/rm/output.txt new file mode 100644 index 00000000000..21c1d2e5e4d --- /dev/null +++ b/acceptance/cmd/sandbox/fs/rm/output.txt @@ -0,0 +1,27 @@ + +=== Remove a file +>>> [CLI] sandbox fs rm [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt + +>>> [CLI] sandbox fs cat [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt +Error: file does not exist: /tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt + +Exit code: 1 + +=== Removing a non-empty directory needs -r +>>> [CLI] sandbox fs rm [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir +Error: directory not empty: /tmp/sandbox-fs-[UNIQUE_NAME]/dir + +Exit code: 1 + +>>> [CLI] sandbox fs rm -r [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir + +>>> [CLI] sandbox fs ls [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/dir +Error: no such directory: /tmp/sandbox-fs-[UNIQUE_NAME]/dir + +Exit code: 1 + +=== Removing a missing path fails +>>> [CLI] sandbox fs rm [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/missing +Error: file does not exist: /tmp/sandbox-fs-[UNIQUE_NAME]/missing + +Exit code: 1 diff --git a/acceptance/cmd/sandbox/fs/rm/script b/acceptance/cmd/sandbox/fs/rm/script new file mode 100644 index 00000000000..bc70b770402 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/rm/script @@ -0,0 +1,15 @@ +$CLI sandbox fs cp hello.txt $SANDBOX_ROOT/hello.txt > /dev/null +$CLI sandbox fs mkdir $SANDBOX_ROOT/dir +$CLI sandbox fs cp hello.txt $SANDBOX_ROOT/dir/hello.txt > /dev/null + +title "Remove a file" +trace $CLI sandbox fs rm $SANDBOX_ROOT/hello.txt +errcode trace $CLI sandbox fs cat $SANDBOX_ROOT/hello.txt + +title "Removing a non-empty directory needs -r" +errcode trace $CLI sandbox fs rm $SANDBOX_ROOT/dir +trace $CLI sandbox fs rm -r $SANDBOX_ROOT/dir +errcode trace $CLI sandbox fs ls $SANDBOX_ROOT/dir + +title "Removing a missing path fails" +errcode trace $CLI sandbox fs rm $SANDBOX_ROOT/missing diff --git a/acceptance/cmd/sandbox/fs/script.prepare b/acceptance/cmd/sandbox/fs/script.prepare new file mode 100644 index 00000000000..54bf397d678 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/script.prepare @@ -0,0 +1,14 @@ +# Cloud environments without a sandbox skip these tests. Locally, the harness sets +# TEST_SANDBOX_ID to the testserver's default sandbox. +if [ -z "${TEST_SANDBOX_ID:-}" ]; then + echo "SKIP_TEST TEST_SANDBOX_ID is not set" + exit 0 +fi + +# Each test works in its own directory under /tmp and removes it on exit. +SANDBOX_ROOT="$TEST_SANDBOX_ID:/tmp/sandbox-fs-$UNIQUE_NAME" +sandbox_fs_cleanup() { + $CLI sandbox fs rm -r "$SANDBOX_ROOT" 2>/dev/null || true +} +trap sandbox_fs_cleanup EXIT +$CLI sandbox fs mkdir "$SANDBOX_ROOT" diff --git a/acceptance/cmd/sandbox/fs/test.toml b/acceptance/cmd/sandbox/fs/test.toml new file mode 100644 index 00000000000..63fa9c9d381 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/test.toml @@ -0,0 +1,3 @@ +# Runs against the testserver fake of the sandbox file APIs locally, and against the existing +# sandbox named by TEST_SANDBOX_ID on cloud, so a run doesn't create a sandbox per test. +Cloud = true From 8aeb1b62c43d42e3ddd822adddc4047aef0f2224 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 6 Nov 2026 17:13:38 +0100 Subject: [PATCH 6/7] Address review: error mapping, temp names, help, and tests - Map rename errors other than "already exists" like upload errors. - Give the exclusive-create temporary file a fixed-length name, so long target names stay within the filesystem's name limit, and stop claiming listings hide it. - Keep cp's description of directory copies in the sandbox help, and explain why the fs command clears its help group. - Share the sandbox cache key rule (profile, else host) in cacheProfile. - Show the effect of cp --overwrite, and ignore the downloaded directory. - Assert the full error for invalid sandbox paths. - Drop the changelog fragment: the sandbox command is still hidden. Co-authored-by: Isaac --- .nextchanges/cli/sandbox-files.md | 1 - acceptance/cmd/sandbox/fs/cp/output.txt | 7 ++++-- acceptance/cmd/sandbox/fs/cp/script | 3 ++- acceptance/cmd/sandbox/fs/cp/test.toml | 1 + acceptance/cmd/sandbox/fs/cp/updated.txt | 1 + cmd/sandbox/completion.go | 10 +++----- cmd/sandbox/fs.go | 30 ++++++++++++++++-------- cmd/sandbox/fs_test.go | 17 ++++++++------ cmd/sandbox/list.go | 5 +--- cmd/sandbox/resolve.go | 11 +++++++++ libs/filer/sandbox_client.go | 25 +++++++++++--------- libs/testserver/sandbox_files.go | 3 ++- 12 files changed, 70 insertions(+), 44 deletions(-) delete mode 100644 .nextchanges/cli/sandbox-files.md create mode 100644 acceptance/cmd/sandbox/fs/cp/test.toml create mode 100644 acceptance/cmd/sandbox/fs/cp/updated.txt diff --git a/.nextchanges/cli/sandbox-files.md b/.nextchanges/cli/sandbox-files.md deleted file mode 100644 index 633dd5a69ee..00000000000 --- a/.nextchanges/cli/sandbox-files.md +++ /dev/null @@ -1 +0,0 @@ -* Add `sandbox fs cp`, `ls`, `cat`, `mkdir`, and `rm` for sandbox file transfers. ([#7372](https://github.com/databricks/cli/pull/7372)) diff --git a/acceptance/cmd/sandbox/fs/cp/output.txt b/acceptance/cmd/sandbox/fs/cp/output.txt index 233fa026de7..3dbb1a713a4 100644 --- a/acceptance/cmd/sandbox/fs/cp/output.txt +++ b/acceptance/cmd/sandbox/fs/cp/output.txt @@ -10,8 +10,11 @@ hello world! >>> [CLI] sandbox fs cp local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt local.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt (skipped; already exists) ->>> [CLI] sandbox fs cp --overwrite local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt -local.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt +>>> [CLI] sandbox fs cp --overwrite updated.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt +updated.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt + +>>> [CLI] sandbox fs cat [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt +hello again! === Writing into a directory that does not exist fails >>> [CLI] sandbox fs cp local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/missing/file.txt diff --git a/acceptance/cmd/sandbox/fs/cp/script b/acceptance/cmd/sandbox/fs/cp/script index e77e60407a0..61c5f17471d 100644 --- a/acceptance/cmd/sandbox/fs/cp/script +++ b/acceptance/cmd/sandbox/fs/cp/script @@ -4,7 +4,8 @@ trace $CLI sandbox fs cat $SANDBOX_ROOT/file.txt title "Writing over an existing file needs --overwrite" trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/file.txt -trace $CLI sandbox fs cp --overwrite local.txt $SANDBOX_ROOT/file.txt +trace $CLI sandbox fs cp --overwrite updated.txt $SANDBOX_ROOT/file.txt +trace $CLI sandbox fs cat $SANDBOX_ROOT/file.txt title "Writing into a directory that does not exist fails" errcode trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/missing/file.txt diff --git a/acceptance/cmd/sandbox/fs/cp/test.toml b/acceptance/cmd/sandbox/fs/cp/test.toml new file mode 100644 index 00000000000..ef5d1bb5092 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cp/test.toml @@ -0,0 +1 @@ +Ignore = ["downloaded"] diff --git a/acceptance/cmd/sandbox/fs/cp/updated.txt b/acceptance/cmd/sandbox/fs/cp/updated.txt new file mode 100644 index 00000000000..69d2c08e682 --- /dev/null +++ b/acceptance/cmd/sandbox/fs/cp/updated.txt @@ -0,0 +1 @@ +hello again! diff --git a/cmd/sandbox/completion.go b/cmd/sandbox/completion.go index e8c48535a3e..e7a74ab2472 100644 --- a/cmd/sandbox/completion.go +++ b/cmd/sandbox/completion.go @@ -58,15 +58,11 @@ func completeSSHKeyHashes(cmd *cobra.Command, args []string, toComplete string) } // completionProfile returns the profile name the cache is keyed under, -// matching the resolution used by the runtime commands (Profile if set, -// else Host). Returns "" if the workspace client can't be bootstrapped. +// matching the resolution used by the runtime commands (see cacheProfile). +// Returns "" if the workspace client can't be bootstrapped. func completionProfile(cmd *cobra.Command, args []string) string { if err := root.MustWorkspaceClient(cmd, args); err != nil { return "" } - w := cmdctx.WorkspaceClient(cmd.Context()) - if w.Config.Profile != "" { - return w.Config.Profile - } - return w.Config.Host + return cacheProfile(cmdctx.WorkspaceClient(cmd.Context())) } diff --git a/cmd/sandbox/fs.go b/cmd/sandbox/fs.go index 17fdf3139a5..d830a83c8f1 100644 --- a/cmd/sandbox/fs.go +++ b/cmd/sandbox/fs.go @@ -13,8 +13,23 @@ import ( "github.com/spf13/cobra" ) +// sandboxCpLong replaces the help of cp, which describes DBFS and UC paths, +// and keeps its description of how directories are copied. +const sandboxCpLong = `Copy files and directories between your local filesystem and sandboxes. + + Use SANDBOX:/absolute/path for sandbox paths. + + Recursively copying a directory will copy all files inside directory + at SOURCE_PATH to the directory at TARGET_PATH. + + When copying a file, if TARGET_PATH is a directory, the file will be created + inside the directory, otherwise the file is created at TARGET_PATH. +` + func newFsCommand() *cobra.Command { cmd := filesystem.NewWithPathResolver(sandboxFilerForPath) + // The top-level fs command belongs to a help group that the sandbox command + // doesn't define, and cobra panics on an undefined group. cmd.GroupID = "" cmd.Long = `Transfer files between your local filesystem and sandboxes. @@ -23,6 +38,9 @@ name from the local cache populated by sandbox list. Paths without a sandbox prefix refer to the local filesystem.` for _, sub := range cmd.Commands() { sub.Long = sub.Short + " Use SANDBOX:/absolute/path for sandbox paths." + if sub.Name() == "cp" { + sub.Long = sandboxCpLong + } sub.ValidArgsFunction = completeSandboxPaths(sub.ValidArgsFunction) } cmd.Example = ` databricks sandbox fs cp -r my-sandbox:/foo/bar ./local/output @@ -44,11 +62,7 @@ func sandboxFilerForPath(ctx context.Context, fullPath string) (filer.Filer, str return nil, "", fmt.Errorf("sandbox path must be absolute: use %s:/path", identifier) } w := cmdctx.WorkspaceClient(ctx) - profile := w.Config.Profile - if profile == "" { - profile = w.Config.Host - } - id, err := resolveLocalID(ctx, profile, identifier) + id, err := resolveLocalID(ctx, cacheProfile(w), identifier) if err != nil { return nil, "", err } @@ -65,11 +79,7 @@ func completeSandboxPaths(complete cobra.CompletionFunc) cobra.CompletionFunc { } completions = slices.DeleteFunc(completions, func(p string) bool { return p == "dbfs:/" }) w := cmdctx.WorkspaceClient(cmd.Context()) - profile := w.Config.Profile - if profile == "" { - profile = w.Config.Host - } - for _, s := range getSandboxes(cmd.Context(), profile) { + for _, s := range getSandboxes(cmd.Context(), cacheProfile(w)) { completions = append(completions, s.ID+":/") if s.Name != "" && s.Name != s.ID { completions = append(completions, s.Name+":/") diff --git a/cmd/sandbox/fs_test.go b/cmd/sandbox/fs_test.go index 247c8b4f449..42b3b08615d 100644 --- a/cmd/sandbox/fs_test.go +++ b/cmd/sandbox/fs_test.go @@ -1,7 +1,6 @@ package sandbox_test import ( - "strings" "testing" sandboxcmd "github.com/databricks/cli/cmd/sandbox" @@ -10,16 +9,20 @@ import ( "github.com/stretchr/testify/require" ) -func TestSandboxFsRejectsRelativeRemotePaths(t *testing.T) { - for _, input := range []string{"my-sandbox:relative", ":/tmp/file"} { - t.Run(input, func(t *testing.T) { +func TestSandboxFsRejectsInvalidRemotePaths(t *testing.T) { + for _, tc := range []struct { + input string + err string + }{ + {"my-sandbox:relative", "sandbox path must be absolute: use my-sandbox:/path"}, + {":/tmp/file", "empty sandbox identifier: use SANDBOX:/path"}, + } { + t.Run(tc.input, func(t *testing.T) { ctx, _ := cmdio.NewTestContextWithStdout(t.Context()) cmd, _, err := sandboxcmd.New().Find([]string{"fs", "cat"}) require.NoError(t, err) cmd.SetContext(ctx) - err = cmd.RunE(cmd, []string{input}) - require.Error(t, err) - assert.Contains(t, strings.ToLower(err.Error()), "sandbox") + assert.EqualError(t, cmd.RunE(cmd, []string{tc.input}), tc.err) }) } } diff --git a/cmd/sandbox/list.go b/cmd/sandbox/list.go index ef0960d21cb..189d32fb1f5 100644 --- a/cmd/sandbox/list.go +++ b/cmd/sandbox/list.go @@ -39,10 +39,7 @@ Example: return fmt.Errorf("failed to list sandboxes: %w", err) } - profile := w.Config.Profile - if profile == "" { - profile = w.Config.Host - } + profile := cacheProfile(w) // `list` returns the full set (the API client loops through every // page), so it's the cheapest place to keep local state coherent: diff --git a/cmd/sandbox/resolve.go b/cmd/sandbox/resolve.go index 21bbcb841b6..3800cbf55de 100644 --- a/cmd/sandbox/resolve.go +++ b/cmd/sandbox/resolve.go @@ -5,8 +5,19 @@ import ( "errors" "fmt" "strings" + + "github.com/databricks/databricks-sdk-go" ) +// cacheProfile returns the key the local sandbox cache is stored under: the +// profile name if set, else the host. +func cacheProfile(w *databricks.WorkspaceClient) string { + if w.Config.Profile != "" { + return w.Config.Profile + } + return w.Config.Host +} + // resolveLocalID maps a user-typed argument to a sandbox ID using only // the local state cache — no API calls. Resolution order: // diff --git a/libs/filer/sandbox_client.go b/libs/filer/sandbox_client.go index 7c7cab7a76d..06b53c6489f 100644 --- a/libs/filer/sandbox_client.go +++ b/libs/filer/sandbox_client.go @@ -111,10 +111,11 @@ type SandboxClient struct { root WorkspaceRootPath } -// NewSandboxClient returns a [Filer] for the filesystem of sandbox sandboxID. -// Paths are relative to root, an absolute path inside the sandbox, and cannot -// escape it. A stopped sandbox starts automatically on the first call, which -// makes that call slow. +// NewSandboxClient returns a [Filer] for the filesystem of sandbox sandboxID, +// rooted at root, an absolute path inside the sandbox. Names are joined to root, +// and a name that would resolve outside it is rejected (see +// [WorkspaceRootPath.Join]). A stopped sandbox starts automatically on the first +// call, which makes that call slow. func NewSandboxClient(w *databricks.WorkspaceClient, sandboxID, root string) (Filer, error) { // The ID becomes a path segment of every request URL, so it can't contain "/" or be a // relative segment that the URL would resolve away. @@ -211,7 +212,7 @@ func (c *SandboxClient) createExclusive(ctx context.Context, absPath string, rea if errors.Is(err, apierr.ErrAlreadyExists) { return fileAlreadyExistsError{absPath} } - return err + return sandboxPathError(err, absPath) } return nil @@ -220,11 +221,13 @@ func (c *SandboxClient) createExclusive(ctx context.Context, absPath string, rea // exclusiveCreateTempPath returns the temporary path that createExclusive // uploads to before renaming it to absPath. It is in the same directory because // rename fails across filesystems (400), and the directory is the only location -// known to be on the target's filesystem. The leading dot hides it from -// listings that skip dotfiles, and the random part keeps concurrent writers of -// the same target from sharing one temporary file. +// known to be on the target's filesystem. The random name keeps concurrent +// writers of the same target from sharing one temporary file, and its fixed +// length keeps it within the filesystem's name limit whatever the target's name. +// A temporary file left behind (removeTemp failed or the process was killed) is +// a regular dotfile, visible to listings and recursive copies. func exclusiveCreateTempPath(absPath string) string { - return path.Join(path.Dir(absPath), "."+path.Base(absPath)+"."+rand.Text()+".tmp") + return path.Join(path.Dir(absPath), ".databricks-"+rand.Text()+".tmp") } // removeTemp deletes a temporary file left by createExclusive. It runs even if @@ -359,8 +362,8 @@ func (c *SandboxClient) Mkdir(ctx context.Context, name string) error { _, err = c.api.CreateDirectory(ctx, sandbox.CreateDirectoryRequest{ Parent: c.parent, Path: absPath, - // Filer.Mkdir creates intermediate directories. Without Parents, the - // server also returns 409 if the directory already exists. + // Filer.Mkdir creates intermediate directories. With parents=false, + // the server also returns 409 if the directory already exists. Parents: true, }) diff --git a/libs/testserver/sandbox_files.go b/libs/testserver/sandbox_files.go index a714e87c26c..2507edd06e9 100644 --- a/libs/testserver/sandbox_files.go +++ b/libs/testserver/sandbox_files.go @@ -304,7 +304,8 @@ func (s *FakeWorkspace) SandboxFilesMkdir(req Request, sandboxID string) Respons defer s.LockUnlock()() sfs := s.sandboxFs(sandboxID) entry, underFile := sfs.lookup(p) - // Parents defaults to true, which also succeeds when the directory exists. + // An omitted parents defaults to true, per the API docs; the SDK omits false + // unless it is in ForceSendFields. With parents, an existing directory is fine. if body.Parents == nil || *body.Parents { if !sfs.mkdirAll(p) { return sandboxFsError(400, "BAD_REQUEST") From c69e06e43998377ded320cbba747ff4c07575222 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 6 Nov 2026 17:22:19 +0100 Subject: [PATCH 7/7] Address review round 2 and capture sandbox fs help - Report reading a symlink to a directory, and overwriting a directory, as "not a file" instead of the server's raw 409. - Stop the fake's mkdirAll at "/" so it terminates after a recursive delete of the root. - Show examples in each sandbox fs command's help, and say in cp's help that existing files are skipped without --overwrite. - Mark which comments rely on the API docs rather than observed behavior. - Capture --help as the first stanza of each sandbox fs acceptance test, and cover copying a file into an existing directory. Co-authored-by: Isaac --- acceptance/cmd/sandbox/fs/cat/output.txt | 19 ++++++++++ acceptance/cmd/sandbox/fs/cat/script | 3 ++ acceptance/cmd/sandbox/fs/cp/output.txt | 41 ++++++++++++++++++++++ acceptance/cmd/sandbox/fs/cp/script | 8 +++++ acceptance/cmd/sandbox/fs/ls/output.txt | 21 +++++++++++ acceptance/cmd/sandbox/fs/ls/script | 3 ++ acceptance/cmd/sandbox/fs/mkdir/output.txt | 22 ++++++++++++ acceptance/cmd/sandbox/fs/mkdir/script | 3 ++ acceptance/cmd/sandbox/fs/rm/output.txt | 20 +++++++++++ acceptance/cmd/sandbox/fs/rm/script | 3 ++ cmd/sandbox/fs.go | 14 ++++++++ libs/filer/sandbox_client.go | 29 ++++++++++----- libs/filer/sandbox_client_test.go | 2 ++ libs/testserver/sandbox_files.go | 4 +++ libs/testserver/sandbox_files_test.go | 7 ++++ 15 files changed, 190 insertions(+), 9 deletions(-) diff --git a/acceptance/cmd/sandbox/fs/cat/output.txt b/acceptance/cmd/sandbox/fs/cat/output.txt index 59daea2cbaa..41cc3c7b508 100644 --- a/acceptance/cmd/sandbox/fs/cat/output.txt +++ b/acceptance/cmd/sandbox/fs/cat/output.txt @@ -1,4 +1,23 @@ +=== Help +>>> [CLI] sandbox fs cat --help +Show file content. Use SANDBOX:/absolute/path for sandbox paths. + +Usage: + databricks sandbox fs cat FILE_PATH [flags] + +Examples: + databricks sandbox fs cat my-sandbox:/tmp/file + +Flags: + -h, --help help for cat + +Global Flags: + --debug enable debug logging + -o, --output type output type: text or json (default text) + -p, --profile string ~/.databrickscfg profile + -t, --target string bundle target to use (if applicable) + === cat a file >>> [CLI] sandbox fs cat [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt hello world! diff --git a/acceptance/cmd/sandbox/fs/cat/script b/acceptance/cmd/sandbox/fs/cat/script index 7b784014b98..ba5392b54be 100644 --- a/acceptance/cmd/sandbox/fs/cat/script +++ b/acceptance/cmd/sandbox/fs/cat/script @@ -1,3 +1,6 @@ +title "Help" +trace $CLI sandbox fs cat --help + $CLI sandbox fs cp hello.txt $SANDBOX_ROOT/hello.txt > /dev/null title "cat a file" diff --git a/acceptance/cmd/sandbox/fs/cp/output.txt b/acceptance/cmd/sandbox/fs/cp/output.txt index 3dbb1a713a4..71293b978ad 100644 --- a/acceptance/cmd/sandbox/fs/cp/output.txt +++ b/acceptance/cmd/sandbox/fs/cp/output.txt @@ -1,4 +1,38 @@ +=== Help +>>> [CLI] sandbox fs cp --help +Copy files and directories between your local filesystem and sandboxes. + + Use SANDBOX:/absolute/path for sandbox paths. + + Recursively copying a directory will copy all files inside directory + at SOURCE_PATH to the directory at TARGET_PATH. + + When copying a file, if TARGET_PATH is a directory, the file will be created + inside the directory, otherwise the file is created at TARGET_PATH. + + Files that already exist at the target are skipped unless --overwrite is set. + +Usage: + databricks sandbox fs cp SOURCE_PATH TARGET_PATH [flags] + +Examples: + databricks sandbox fs cp ./file my-sandbox:/tmp/file + databricks sandbox fs cp --overwrite ./file my-sandbox:/tmp/file + databricks sandbox fs cp -r my-sandbox:/tmp/dir ./local/dir + +Flags: + --concurrency int number of parallel copy operations (default 8) + -h, --help help for cp + --overwrite overwrite existing files + -r, --recursive recursively copy files from directory + +Global Flags: + --debug enable debug logging + -o, --output type output type: text or json (default text) + -p, --profile string ~/.databrickscfg profile + -t, --target string bundle target to use (if applicable) + === Upload a file >>> [CLI] sandbox fs cp local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt local.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt @@ -16,6 +50,13 @@ updated.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt >>> [CLI] sandbox fs cat [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/file.txt hello again! +=== Copying a file to a directory puts it inside +>>> [CLI] sandbox fs cp local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/target +local.txt -> [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/target/local.txt + +>>> [CLI] sandbox fs ls [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/target +local.txt + === Writing into a directory that does not exist fails >>> [CLI] sandbox fs cp local.txt [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/missing/file.txt Error: no such directory: /tmp/sandbox-fs-[UNIQUE_NAME]/missing diff --git a/acceptance/cmd/sandbox/fs/cp/script b/acceptance/cmd/sandbox/fs/cp/script index 61c5f17471d..3a152df8524 100644 --- a/acceptance/cmd/sandbox/fs/cp/script +++ b/acceptance/cmd/sandbox/fs/cp/script @@ -1,3 +1,6 @@ +title "Help" +trace $CLI sandbox fs cp --help + title "Upload a file" trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/file.txt trace $CLI sandbox fs cat $SANDBOX_ROOT/file.txt @@ -7,6 +10,11 @@ trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/file.txt trace $CLI sandbox fs cp --overwrite updated.txt $SANDBOX_ROOT/file.txt trace $CLI sandbox fs cat $SANDBOX_ROOT/file.txt +title "Copying a file to a directory puts it inside" +$CLI sandbox fs mkdir $SANDBOX_ROOT/target +trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/target +trace $CLI sandbox fs ls $SANDBOX_ROOT/target + title "Writing into a directory that does not exist fails" errcode trace $CLI sandbox fs cp local.txt $SANDBOX_ROOT/missing/file.txt diff --git a/acceptance/cmd/sandbox/fs/ls/output.txt b/acceptance/cmd/sandbox/fs/ls/output.txt index 94aa1300b7c..c5afff6a72c 100644 --- a/acceptance/cmd/sandbox/fs/ls/output.txt +++ b/acceptance/cmd/sandbox/fs/ls/output.txt @@ -1,4 +1,25 @@ +=== Help +>>> [CLI] sandbox fs ls --help +Lists files. Use SANDBOX:/absolute/path for sandbox paths. + +Usage: + databricks sandbox fs ls DIR_PATH [flags] + +Examples: + databricks sandbox fs ls my-sandbox:/tmp + +Flags: + --absolute Displays absolute paths. + -h, --help help for ls + -l, --long Displays full information including size, file type and modification time since Epoch in milliseconds. + +Global Flags: + --debug enable debug logging + -o, --output type output type: text or json (default text) + -p, --profile string ~/.databrickscfg profile + -t, --target string bundle target to use (if applicable) + === List a directory >>> [CLI] sandbox fs ls [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME] dir diff --git a/acceptance/cmd/sandbox/fs/ls/script b/acceptance/cmd/sandbox/fs/ls/script index 340f1fe93eb..09647c11cc5 100644 --- a/acceptance/cmd/sandbox/fs/ls/script +++ b/acceptance/cmd/sandbox/fs/ls/script @@ -1,3 +1,6 @@ +title "Help" +trace $CLI sandbox fs ls --help + $CLI sandbox fs cp hello.txt $SANDBOX_ROOT/hello.txt > /dev/null $CLI sandbox fs mkdir $SANDBOX_ROOT/dir diff --git a/acceptance/cmd/sandbox/fs/mkdir/output.txt b/acceptance/cmd/sandbox/fs/mkdir/output.txt index dab3eb228f8..13b5ea85dac 100644 --- a/acceptance/cmd/sandbox/fs/mkdir/output.txt +++ b/acceptance/cmd/sandbox/fs/mkdir/output.txt @@ -1,4 +1,26 @@ +=== Help +>>> [CLI] sandbox fs mkdir --help +Make directories. Use SANDBOX:/absolute/path for sandbox paths. + +Usage: + databricks sandbox fs mkdir DIR_PATH [flags] + +Aliases: + mkdir, mkdirs + +Examples: + databricks sandbox fs mkdir my-sandbox:/tmp/a/b + +Flags: + -h, --help help for mkdir + +Global Flags: + --debug enable debug logging + -o, --output type output type: text or json (default text) + -p, --profile string ~/.databrickscfg profile + -t, --target string bundle target to use (if applicable) + === Create nested directories >>> [CLI] sandbox fs mkdir [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/a/b/c diff --git a/acceptance/cmd/sandbox/fs/mkdir/script b/acceptance/cmd/sandbox/fs/mkdir/script index 635408fac67..eed4f679f2c 100644 --- a/acceptance/cmd/sandbox/fs/mkdir/script +++ b/acceptance/cmd/sandbox/fs/mkdir/script @@ -1,3 +1,6 @@ +title "Help" +trace $CLI sandbox fs mkdir --help + title "Create nested directories" trace $CLI sandbox fs mkdir $SANDBOX_ROOT/a/b/c trace $CLI sandbox fs ls $SANDBOX_ROOT/a/b diff --git a/acceptance/cmd/sandbox/fs/rm/output.txt b/acceptance/cmd/sandbox/fs/rm/output.txt index 21c1d2e5e4d..c896e4298af 100644 --- a/acceptance/cmd/sandbox/fs/rm/output.txt +++ b/acceptance/cmd/sandbox/fs/rm/output.txt @@ -1,4 +1,24 @@ +=== Help +>>> [CLI] sandbox fs rm --help +Remove files and directories. Use SANDBOX:/absolute/path for sandbox paths. + +Usage: + databricks sandbox fs rm PATH [flags] + +Examples: + databricks sandbox fs rm -r my-sandbox:/tmp/dir + +Flags: + -h, --help help for rm + -r, --recursive Recursively delete a non-empty directory. + +Global Flags: + --debug enable debug logging + -o, --output type output type: text or json (default text) + -p, --profile string ~/.databrickscfg profile + -t, --target string bundle target to use (if applicable) + === Remove a file >>> [CLI] sandbox fs rm [TEST_SANDBOX_ID]:/tmp/sandbox-fs-[UNIQUE_NAME]/hello.txt diff --git a/acceptance/cmd/sandbox/fs/rm/script b/acceptance/cmd/sandbox/fs/rm/script index bc70b770402..cf61c16256d 100644 --- a/acceptance/cmd/sandbox/fs/rm/script +++ b/acceptance/cmd/sandbox/fs/rm/script @@ -1,3 +1,6 @@ +title "Help" +trace $CLI sandbox fs rm --help + $CLI sandbox fs cp hello.txt $SANDBOX_ROOT/hello.txt > /dev/null $CLI sandbox fs mkdir $SANDBOX_ROOT/dir $CLI sandbox fs cp hello.txt $SANDBOX_ROOT/dir/hello.txt > /dev/null diff --git a/cmd/sandbox/fs.go b/cmd/sandbox/fs.go index d830a83c8f1..c299e604ce3 100644 --- a/cmd/sandbox/fs.go +++ b/cmd/sandbox/fs.go @@ -24,8 +24,21 @@ const sandboxCpLong = `Copy files and directories between your local filesystem When copying a file, if TARGET_PATH is a directory, the file will be created inside the directory, otherwise the file is created at TARGET_PATH. + + Files that already exist at the target are skipped unless --overwrite is set. ` +// sandboxFsExamples are shown in each command's help. +var sandboxFsExamples = map[string]string{ + "cp": ` databricks sandbox fs cp ./file my-sandbox:/tmp/file + databricks sandbox fs cp --overwrite ./file my-sandbox:/tmp/file + databricks sandbox fs cp -r my-sandbox:/tmp/dir ./local/dir`, + "ls": ` databricks sandbox fs ls my-sandbox:/tmp`, + "cat": ` databricks sandbox fs cat my-sandbox:/tmp/file`, + "mkdir": ` databricks sandbox fs mkdir my-sandbox:/tmp/a/b`, + "rm": ` databricks sandbox fs rm -r my-sandbox:/tmp/dir`, +} + func newFsCommand() *cobra.Command { cmd := filesystem.NewWithPathResolver(sandboxFilerForPath) // The top-level fs command belongs to a help group that the sandbox command @@ -41,6 +54,7 @@ sandbox prefix refer to the local filesystem.` if sub.Name() == "cp" { sub.Long = sandboxCpLong } + sub.Example = sandboxFsExamples[sub.Name()] sub.ValidArgsFunction = completeSandboxPaths(sub.ValidArgsFunction) } cmd.Example = ` databricks sandbox fs cp -r my-sandbox:/foo/bar ./local/output diff --git a/libs/filer/sandbox_client.go b/libs/filer/sandbox_client.go index 06b53c6489f..ba1e52a0415 100644 --- a/libs/filer/sandbox_client.go +++ b/libs/filer/sandbox_client.go @@ -64,7 +64,8 @@ func (i sandboxFileInfo) Size() int64 { // Mode reports symlinks as [fs.ModeSymlink] without following them, matching // [os.Lstat]: the metadata and listing APIs describe the link itself, and // reporting the link's own type keeps recursive walks from following links out -// of the tree. Read and Write still follow links, because the server does. +// of the tree. Read and Write still follow links, because the server does +// (per the API docs for download and upload). // Only the permission bits of the Unix mode are kept; setuid, setgid and sticky // use different bits in [fs.FileMode]. func (i sandboxFileInfo) Mode() fs.FileMode { @@ -114,8 +115,8 @@ type SandboxClient struct { // NewSandboxClient returns a [Filer] for the filesystem of sandbox sandboxID, // rooted at root, an absolute path inside the sandbox. Names are joined to root, // and a name that would resolve outside it is rejected (see -// [WorkspaceRootPath.Join]). A stopped sandbox starts automatically on the first -// call, which makes that call slow. +// [WorkspaceRootPath.Join]). Per the API docs, a stopped sandbox starts +// automatically on the first call, which makes that call slow. func NewSandboxClient(w *databricks.WorkspaceClient, sandboxID, root string) (Filer, error) { // The ID becomes a path segment of every request URL, so it can't contain "/" or be a // relative segment that the URL would resolve away. @@ -158,7 +159,15 @@ func (c *SandboxClient) Write(ctx context.Context, name string, reader io.Reader } if slices.Contains(mode, OverwriteIfExists) { - return c.upload(ctx, absPath, reader) + err := c.upload(ctx, absPath, reader) + // The server returns 409 when the target is a directory, and also when a + // parent path component is a file. Stat to tell them apart. + if sandboxStatus(err) == http.StatusConflict { + if info, serr := c.stat(ctx, absPath); serr == nil && info.IsDir() { + return notAFile{absPath} + } + } + return err } // Fail fast if the target exists, so that rerunning a copy that skips @@ -177,10 +186,11 @@ func (c *SandboxClient) Write(ctx context.Context, name string, reader io.Reader // createExclusive writes a file that must not already exist, and guarantees // that only one of several concurrent writers creates it. The libs/locker lock -// file relies on this guarantee. +// file relies on this guarantee from filers. // -// The upload API can't do this: it always replaces the target and rejects -// conditional headers (If-None-Match) with 400. The rename API can: it returns +// The upload API can't do this: it always replaces the target (observed), and +// per its docs rejects conditional headers such as If-None-Match with 400. The +// rename API can: it returns // 409 ALREADY_EXISTS if the destination exists, and of several concurrent // renames onto the same absent destination, exactly one succeeds. That // atomicity is observed behavior (consistent with renameat(2) with @@ -267,9 +277,10 @@ func (c *SandboxClient) Read(ctx context.Context, name string) (io.ReadCloser, e } // The server returns 409 for a directory, and also when a parent path - // component is a file. Stat to tell them apart. + // component is a file. Stat to tell them apart. Download follows symlinks + // but stat doesn't, so a 409 for a symlink means it points at a directory. if sandboxStatus(err) == http.StatusConflict { - if info, serr := c.stat(ctx, absPath); serr == nil && info.IsDir() { + if info, serr := c.stat(ctx, absPath); serr == nil && (info.IsDir() || info.Mode()&fs.ModeSymlink != 0) { return nil, notAFile{absPath} } } diff --git a/libs/filer/sandbox_client_test.go b/libs/filer/sandbox_client_test.go index 64c326906e2..1da20a52083 100644 --- a/libs/filer/sandbox_client_test.go +++ b/libs/filer/sandbox_client_test.go @@ -126,6 +126,8 @@ func TestSandboxClientGenericErrors(t *testing.T) { assert.ErrorIs(t, err, fs.ErrInvalid, "list of a file") assert.ErrorIs(t, f.Delete(ctx, "dir"), fs.ErrInvalid, "delete of a non-empty directory") assert.ErrorIs(t, f.Mkdir(ctx, "dir/file"), fs.ErrExist, "mkdir onto a file") + err = f.Write(ctx, "dir", strings.NewReader("x"), filer.OverwriteIfExists) + assert.ErrorIs(t, err, fs.ErrInvalid, "overwrite of a directory") } func TestSandboxClientPermissionDenied(t *testing.T) { diff --git a/libs/testserver/sandbox_files.go b/libs/testserver/sandbox_files.go index 2507edd06e9..acd4aefaf94 100644 --- a/libs/testserver/sandbox_files.go +++ b/libs/testserver/sandbox_files.go @@ -146,6 +146,10 @@ func (sfs *fakeSandboxFs) mkdirAll(p string) bool { break } missing = append(missing, dir) + // A recursive delete of "/" removes the root entry too. + if dir == "/" { + break + } } for _, dir := range missing { sfs.entries[dir] = newSandboxDirEntry() diff --git a/libs/testserver/sandbox_files_test.go b/libs/testserver/sandbox_files_test.go index 326130a8548..03ec5b94ffd 100644 --- a/libs/testserver/sandbox_files_test.go +++ b/libs/testserver/sandbox_files_test.go @@ -83,6 +83,13 @@ func TestSandboxFilesUploadRejectsUnknownParameters(t *testing.T) { assert.ErrorIs(t, err, apierr.ErrInvalidParameterValue) } +func TestSandboxFilesUploadAfterDeletingRoot(t *testing.T) { + api := newSandboxAPI(t) + _, err := api.DeletePath(t.Context(), sandbox.DeletePathRequest{Parent: sandboxParent, Path: "/", Recursive: true}) + require.NoError(t, err) + require.NoError(t, upload(t, api, "/tmp/file", "")) +} + func TestSandboxFilesRenameIsExclusive(t *testing.T) { api := newSandboxAPI(t) const n = 8