From 3eeac3904dfac5a96d65e99892f3dd39f5e7cc67 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 21 Jun 2023 16:39:38 +0200 Subject: [PATCH 1/9] [WIP] Remove support for file scheme --- cmd/fs/helpers.go | 55 ++++++------------- libs/filer/dbfs_client.go | 4 +- libs/filer/files_client.go | 4 +- libs/filer/local_client.go | 4 +- libs/filer/local_root_path.go | 27 +++++++++ libs/filer/workspace_files_client.go | 4 +- .../{root_path.go => workspace_root_path.go} | 12 ++-- ...th_test.go => workspace_root_path_test.go} | 2 +- 8 files changed, 58 insertions(+), 54 deletions(-) create mode 100644 libs/filer/local_root_path.go rename libs/filer/{root_path.go => workspace_root_path.go} (65%) rename libs/filer/{root_path_test.go => workspace_root_path_test.go} (98%) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index aecee9e083..5442fd4c7c 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -2,55 +2,32 @@ package fs import ( "context" - "fmt" - "runtime" "strings" "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/filer" ) -type Scheme string - -const ( - DbfsScheme = Scheme("dbfs") - LocalScheme = Scheme("file") - NoScheme = Scheme("") -) - func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { - parts := strings.SplitN(fullPath, ":/", 2) - if len(parts) < 2 { - return nil, "", fmt.Errorf(`no scheme specified for path %s. Please specify scheme "dbfs" or "file". Example: file:/foo/bar or file:/c:/foo/bar`, fullPath) + // Split path at : to detect any file schemes + parts := strings.SplitN(fullPath, ":", 2) + + // If dbfs file scheme is not specified, then it's a local path + if len(parts) < 2 || parts[0] != "dbfs" { + f, err := filer.NewLocalClient("") + return f, fullPath, err } - scheme := Scheme(parts[0]) + path := parts[1] - switch scheme { - case DbfsScheme: - w := root.WorkspaceClient(ctx) - // If the specified path has the "Volumes" prefix, use the Files API. - if strings.HasPrefix(path, "Volumes/") { - f, err := filer.NewFilesClient(w, "/") - return f, path, err - } - f, err := filer.NewDbfsClient(w, "/") - return f, path, err + w := root.WorkspaceClient(ctx) - case LocalScheme: - if runtime.GOOS == "windows" { - parts := strings.SplitN(path, ":", 2) - if len(parts) < 2 { - return nil, "", fmt.Errorf("no volume specfied for path: %s", path) - } - volume := parts[0] + ":" - relPath := parts[1] - f, err := filer.NewLocalClient(volume) - return f, relPath, err - } - f, err := filer.NewLocalClient("/") + // If the specified path has the "Volumes" prefix, use the Files API. + if strings.HasPrefix(path, "/Volumes/") { + f, err := filer.NewFilesClient(w, "/") return f, path, err - - default: - return nil, "", fmt.Errorf(`unsupported scheme %s specified for path %s. Please specify scheme "dbfs" or "file". Example: file:/foo/bar or file:/c:/foo/bar`, scheme, fullPath) } + + // The file is a dbfs file, and uses the DBFS APIs + f, err := filer.NewDbfsClient(w, "/") + return f, path, err } diff --git a/libs/filer/dbfs_client.go b/libs/filer/dbfs_client.go index 85f87ff5e5..440c5393e9 100644 --- a/libs/filer/dbfs_client.go +++ b/libs/filer/dbfs_client.go @@ -67,14 +67,14 @@ type DbfsClient struct { workspaceClient *databricks.WorkspaceClient // File operations will be relative to this path. - root RootPath + root WorkspaceRootPath } func NewDbfsClient(w *databricks.WorkspaceClient, root string) (Filer, error) { return &DbfsClient{ workspaceClient: w, - root: NewRootPath(root), + root: NewWorkspaceRootPath(root), }, nil } diff --git a/libs/filer/files_client.go b/libs/filer/files_client.go index 6c1f5a97ee..ee7587dcc7 100644 --- a/libs/filer/files_client.go +++ b/libs/filer/files_client.go @@ -60,7 +60,7 @@ type FilesClient struct { apiClient *client.DatabricksClient // File operations will be relative to this path. - root RootPath + root WorkspaceRootPath } func filesNotImplementedError(fn string) error { @@ -77,7 +77,7 @@ func NewFilesClient(w *databricks.WorkspaceClient, root string) (Filer, error) { workspaceClient: w, apiClient: apiClient, - root: NewRootPath(root), + root: NewWorkspaceRootPath(root), }, nil } diff --git a/libs/filer/local_client.go b/libs/filer/local_client.go index 8df59d255d..8d960c84bd 100644 --- a/libs/filer/local_client.go +++ b/libs/filer/local_client.go @@ -13,12 +13,12 @@ import ( // LocalClient implements the [Filer] interface for the local filesystem. type LocalClient struct { // File operations will be relative to this path. - root RootPath + root localRootPath } func NewLocalClient(root string) (Filer, error) { return &LocalClient{ - root: NewRootPath(root), + root: NewLocalRootPath(root), }, nil } diff --git a/libs/filer/local_root_path.go b/libs/filer/local_root_path.go new file mode 100644 index 0000000000..15a5426318 --- /dev/null +++ b/libs/filer/local_root_path.go @@ -0,0 +1,27 @@ +package filer + +import ( + "fmt" + "path/filepath" + "strings" +) + +type localRootPath struct { + rootPath string +} + +func NewLocalRootPath(root string) localRootPath { + if root == "" { + return localRootPath{""} + } + return localRootPath{filepath.Clean(root)} +} + +func (rp *localRootPath) Join(name string) (string, error) { + absPath := filepath.Join(rp.rootPath, name) + + if !strings.HasPrefix(absPath, rp.rootPath) { + return "", fmt.Errorf("relative path escapes root: %s", name) + } + return absPath, nil +} diff --git a/libs/filer/workspace_files_client.go b/libs/filer/workspace_files_client.go index 2b5e718beb..ac9bc50dd1 100644 --- a/libs/filer/workspace_files_client.go +++ b/libs/filer/workspace_files_client.go @@ -79,7 +79,7 @@ type WorkspaceFilesClient struct { apiClient *client.DatabricksClient // File operations will be relative to this path. - root RootPath + root WorkspaceRootPath } func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer, error) { @@ -92,7 +92,7 @@ func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer, workspaceClient: w, apiClient: apiClient, - root: NewRootPath(root), + root: NewWorkspaceRootPath(root), }, nil } diff --git a/libs/filer/root_path.go b/libs/filer/workspace_root_path.go similarity index 65% rename from libs/filer/root_path.go rename to libs/filer/workspace_root_path.go index bdeff5d73c..d5163924a1 100644 --- a/libs/filer/root_path.go +++ b/libs/filer/workspace_root_path.go @@ -6,23 +6,23 @@ import ( "strings" ) -// RootPath can be joined with a relative path and ensures that +// WorkspaceRootPath can be joined with a relative path and ensures that // the returned path is always a strict child of the root path. -type RootPath struct { +type WorkspaceRootPath struct { rootPath string } -// NewRootPath constructs and returns [RootPath]. +// NewWorkspaceRootPath constructs and returns [RootPath]. // The named path is cleaned on construction. -func NewRootPath(name string) RootPath { - return RootPath{ +func NewWorkspaceRootPath(name string) WorkspaceRootPath { + return WorkspaceRootPath{ rootPath: path.Clean(name), } } // Join returns the specified path name joined to the root. // It returns an error if the resulting path is not a strict child of the root path. -func (p *RootPath) Join(name string) (string, error) { +func (p *WorkspaceRootPath) Join(name string) (string, error) { absPath := path.Join(p.rootPath, name) // Don't allow escaping the specified root using relative paths. diff --git a/libs/filer/root_path_test.go b/libs/filer/workspace_root_path_test.go similarity index 98% rename from libs/filer/root_path_test.go rename to libs/filer/workspace_root_path_test.go index 965842d030..73746d274f 100644 --- a/libs/filer/root_path_test.go +++ b/libs/filer/workspace_root_path_test.go @@ -9,7 +9,7 @@ import ( func testRootPath(t *testing.T, uncleanRoot string) { cleanRoot := path.Clean(uncleanRoot) - rp := NewRootPath(uncleanRoot) + rp := NewWorkspaceRootPath(uncleanRoot) remotePath, err := rp.Join("a/b/c") assert.NoError(t, err) From 78c60d2dd76ae2287f2e3d2e1b30472987c80bb3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 23 Jun 2023 12:45:15 +0200 Subject: [PATCH 2/9] added test --- cmd/fs/helpers_test.go | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 4beda6ca12..5163292d8c 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -2,25 +2,20 @@ package fs import ( "context" - "runtime" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestNotSpecifyingVolumeForWindowsPathErrors(t *testing.T) { - if runtime.GOOS != "windows" { - t.Skip() - } +func TestFilerForPathForLocalPaths(t *testing.T) { + tmpDir := t.TempDir() - ctx := context.Background() - pathWithVolume := `file:/c:/foo/bar` - pathWOVolume := `file:/uno/dos` - - _, path, err := filerForPath(ctx, pathWithVolume) + f, path, err := filerForPath(context.Background(), tmpDir) assert.NoError(t, err) - assert.Equal(t, `/foo/bar`, path) + assert.Equal(t, tmpDir, path) - _, _, err = filerForPath(ctx, pathWOVolume) - assert.Equal(t, "no volume specfied for path: uno/dos", err.Error()) + info, err := f.Stat(context.Background(), path) + require.NoError(t, err) + assert.True(t, info.IsDir()) } From b6d4ce785ae55358a99cd0a825f98685e3e3c062 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 23 Jun 2023 13:41:35 +0200 Subject: [PATCH 3/9] added tests --- cmd/fs/cat.go | 2 +- cmd/fs/cp.go | 4 ++-- cmd/fs/helpers.go | 20 ++++++++++++++++--- cmd/fs/helpers_test.go | 45 ++++++++++++++++++++++++++++++++++++++++-- cmd/fs/ls.go | 2 +- cmd/fs/mkdir.go | 2 +- cmd/fs/rm.go | 2 +- internal/fs_test.go | 33 +++++++++++++++++++++++++++++++ 8 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 internal/fs_test.go diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index 2cdc40759f..129f8ae9ae 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -16,7 +16,7 @@ var catCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - f, path, err := filerForPath(ctx, args[0]) + f, path, err := FilerForPath(ctx, args[0]) if err != nil { return err } diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index 7d9ff24ac6..6d5b919219 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -132,14 +132,14 @@ var cpCmd = &cobra.Command{ // Get source filer and source path without scheme fullSourcePath := args[0] - sourceFiler, sourcePath, err := filerForPath(ctx, fullSourcePath) + sourceFiler, sourcePath, err := FilerForPath(ctx, fullSourcePath) if err != nil { return err } // Get target filer and target path without scheme fullTargetPath := args[1] - targetFiler, targetPath, err := filerForPath(ctx, fullTargetPath) + targetFiler, targetPath, err := FilerForPath(ctx, fullTargetPath) if err != nil { return err } diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 5442fd4c7c..42a9d5bd1e 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -2,22 +2,36 @@ package fs import ( "context" + "fmt" + "runtime" "strings" "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/filer" ) -func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { +func FilerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { // Split path at : to detect any file schemes parts := strings.SplitN(fullPath, ":", 2) - // If dbfs file scheme is not specified, then it's a local path - if len(parts) < 2 || parts[0] != "dbfs" { + // If no scheme is specified, then local path + if len(parts) < 2 { f, err := filer.NewLocalClient("") return f, fullPath, err } + // 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 { + f, err := filer.NewLocalClient("") + return f, fullPath, err + } + + if parts[0] != "dbfs" { + return nil, "", fmt.Errorf("invalid scheme: %s", parts[0]) + } + path := parts[1] w := root.WorkspaceClient(ctx) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 5163292d8c..1f4b7307e5 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -2,20 +2,61 @@ package fs import ( "context" + "runtime" "testing" + "github.com/databricks/cli/libs/filer" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestFilerForPathForLocalPaths(t *testing.T) { tmpDir := t.TempDir() + ctx := context.Background() - f, path, err := filerForPath(context.Background(), tmpDir) + f, path, err := FilerForPath(ctx, tmpDir) assert.NoError(t, err) assert.Equal(t, tmpDir, path) - info, err := f.Stat(context.Background(), path) + info, err := f.Stat(ctx, path) require.NoError(t, err) assert.True(t, info.IsDir()) } + +func TestFilerForPathForInvalidScheme(t *testing.T) { + ctx := context.Background() + + _, _, err := FilerForPath(ctx, "dbf:/a") + assert.ErrorContains(t, err, "invalid scheme") + + _, _, err = FilerForPath(ctx, "foo:a") + assert.ErrorContains(t, err, "invalid scheme") + + _, _, err = FilerForPath(ctx, "file:/a") + assert.ErrorContains(t, err, "invalid scheme") +} + +func testWindowsFilerForPath(t *testing.T, ctx context.Context, fullPath string) { + f, path, err := FilerForPath(ctx, fullPath) + assert.NoError(t, err) + + // Assert path remains unchanged + assert.Equal(t, path, fullPath) + + // Assert local client is created + _, ok := f.(*filer.LocalClient) + assert.True(t, ok) +} + +func TestFilerForWindowsLocalPaths(t *testing.T) { + if runtime.GOOS != "windows" { + t.SkipNow() + } + + ctx := context.Background() + testWindowsFilerForPath(t, ctx, `c:\abc`) + testWindowsFilerForPath(t, ctx, `c:abc`) + testWindowsFilerForPath(t, ctx, `d:\abc`) + testWindowsFilerForPath(t, ctx, `d:\abc`) + testWindowsFilerForPath(t, ctx, `f:\abc\ef`) +} diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 1226e937fb..1e3ea5d5f6 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -42,7 +42,7 @@ var lsCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - f, path, err := filerForPath(ctx, args[0]) + f, path, err := FilerForPath(ctx, args[0]) if err != nil { return err } diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index cb0491393f..e514e4ff9a 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -18,7 +18,7 @@ var mkdirCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - f, path, err := filerForPath(ctx, args[0]) + f, path, err := FilerForPath(ctx, args[0]) if err != nil { return err } diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index 21f5adb993..b75e1e8350 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -16,7 +16,7 @@ var rmCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - f, path, err := filerForPath(ctx, args[0]) + f, path, err := FilerForPath(ctx, args[0]) if err != nil { return err } diff --git a/internal/fs_test.go b/internal/fs_test.go new file mode 100644 index 0000000000..0209a6c8b8 --- /dev/null +++ b/internal/fs_test.go @@ -0,0 +1,33 @@ +package internal + +import ( + "context" + "path" + "testing" + + "github.com/databricks/cli/cmd/fs" + "github.com/databricks/databricks-sdk-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFilerForPathForDbfsPaths(t *testing.T) { + t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) + + w, err := databricks.NewWorkspaceClient() + require.NoError(t, err) + + ctx := context.Background() + tmpDir := temporaryDbfsDir(t, w) + + f, path, err := fs.FilerForPath(ctx, path.Join("dbfs:", tmpDir)) + assert.NoError(t, err) + + // assert dbfs scheme is trimmed from input path + assert.Equal(t, tmpDir, path) + + // assert filer works + stat, err := f.Stat(ctx, tmpDir) + require.NoError(t, err) + assert.True(t, stat.IsDir()) +} From 55c5cb47d7f71249ab3a86365674cdb867599f7e Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 23 Jun 2023 13:50:57 +0200 Subject: [PATCH 4/9] - --- cmd/root/auth.go | 4 ++++ internal/fs_test.go | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/root/auth.go b/cmd/root/auth.go index 61068ab384..1b52128cea 100644 --- a/cmd/root/auth.go +++ b/cmd/root/auth.go @@ -186,6 +186,10 @@ func askForAccountProfile() (string, error) { return profiles[i].Name, nil } +func SetWorkspaceClient(ctx context.Context, w *databricks.WorkspaceClient) context.Context { + return context.WithValue(ctx, &workspaceClient, w) +} + func WorkspaceClient(ctx context.Context) *databricks.WorkspaceClient { w, ok := ctx.Value(&workspaceClient).(*databricks.WorkspaceClient) if !ok { diff --git a/internal/fs_test.go b/internal/fs_test.go index 0209a6c8b8..521638d280 100644 --- a/internal/fs_test.go +++ b/internal/fs_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/databricks/cli/cmd/fs" + "github.com/databricks/cli/cmd/root" "github.com/databricks/databricks-sdk-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -14,11 +15,12 @@ import ( func TestFilerForPathForDbfsPaths(t *testing.T) { t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) + ctx := context.Background() w, err := databricks.NewWorkspaceClient() require.NoError(t, err) - ctx := context.Background() tmpDir := temporaryDbfsDir(t, w) + ctx = root.SetWorkspaceClient(ctx, w) f, path, err := fs.FilerForPath(ctx, path.Join("dbfs:", tmpDir)) assert.NoError(t, err) From 06311d0afa16177133c0b8d5f35dbd6e5787a514 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 23 Jun 2023 14:15:34 +0200 Subject: [PATCH 5/9] remove dbfs integration testt --- cmd/fs/cat.go | 2 +- cmd/fs/cp.go | 4 ++-- cmd/fs/helpers.go | 2 +- cmd/fs/helpers_test.go | 10 +++++----- cmd/fs/ls.go | 2 +- cmd/fs/mkdir.go | 2 +- cmd/fs/rm.go | 2 +- cmd/root/auth.go | 4 ---- internal/fs_test.go | 35 ----------------------------------- 9 files changed, 12 insertions(+), 51 deletions(-) delete mode 100644 internal/fs_test.go diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index 129f8ae9ae..2cdc40759f 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -16,7 +16,7 @@ var catCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - f, path, err := FilerForPath(ctx, args[0]) + f, path, err := filerForPath(ctx, args[0]) if err != nil { return err } diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index 6d5b919219..7d9ff24ac6 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -132,14 +132,14 @@ var cpCmd = &cobra.Command{ // Get source filer and source path without scheme fullSourcePath := args[0] - sourceFiler, sourcePath, err := FilerForPath(ctx, fullSourcePath) + sourceFiler, sourcePath, err := filerForPath(ctx, fullSourcePath) if err != nil { return err } // Get target filer and target path without scheme fullTargetPath := args[1] - targetFiler, targetPath, err := FilerForPath(ctx, fullTargetPath) + targetFiler, targetPath, err := filerForPath(ctx, fullTargetPath) if err != nil { return err } diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 42a9d5bd1e..ca85cf1bd9 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -10,7 +10,7 @@ import ( "github.com/databricks/cli/libs/filer" ) -func FilerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { +func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { // Split path at : to detect any file schemes parts := strings.SplitN(fullPath, ":", 2) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 1f4b7307e5..d86bd46e1e 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -14,7 +14,7 @@ func TestFilerForPathForLocalPaths(t *testing.T) { tmpDir := t.TempDir() ctx := context.Background() - f, path, err := FilerForPath(ctx, tmpDir) + f, path, err := filerForPath(ctx, tmpDir) assert.NoError(t, err) assert.Equal(t, tmpDir, path) @@ -26,18 +26,18 @@ func TestFilerForPathForLocalPaths(t *testing.T) { func TestFilerForPathForInvalidScheme(t *testing.T) { ctx := context.Background() - _, _, err := FilerForPath(ctx, "dbf:/a") + _, _, err := filerForPath(ctx, "dbf:/a") assert.ErrorContains(t, err, "invalid scheme") - _, _, err = FilerForPath(ctx, "foo:a") + _, _, err = filerForPath(ctx, "foo:a") assert.ErrorContains(t, err, "invalid scheme") - _, _, err = FilerForPath(ctx, "file:/a") + _, _, err = filerForPath(ctx, "file:/a") assert.ErrorContains(t, err, "invalid scheme") } func testWindowsFilerForPath(t *testing.T, ctx context.Context, fullPath string) { - f, path, err := FilerForPath(ctx, fullPath) + f, path, err := filerForPath(ctx, fullPath) assert.NoError(t, err) // Assert path remains unchanged diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 1e3ea5d5f6..1226e937fb 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -42,7 +42,7 @@ var lsCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - f, path, err := FilerForPath(ctx, args[0]) + f, path, err := filerForPath(ctx, args[0]) if err != nil { return err } diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index e514e4ff9a..cb0491393f 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -18,7 +18,7 @@ var mkdirCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - f, path, err := FilerForPath(ctx, args[0]) + f, path, err := filerForPath(ctx, args[0]) if err != nil { return err } diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index b75e1e8350..21f5adb993 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -16,7 +16,7 @@ var rmCmd = &cobra.Command{ RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - f, path, err := FilerForPath(ctx, args[0]) + f, path, err := filerForPath(ctx, args[0]) if err != nil { return err } diff --git a/cmd/root/auth.go b/cmd/root/auth.go index 1b52128cea..61068ab384 100644 --- a/cmd/root/auth.go +++ b/cmd/root/auth.go @@ -186,10 +186,6 @@ func askForAccountProfile() (string, error) { return profiles[i].Name, nil } -func SetWorkspaceClient(ctx context.Context, w *databricks.WorkspaceClient) context.Context { - return context.WithValue(ctx, &workspaceClient, w) -} - func WorkspaceClient(ctx context.Context) *databricks.WorkspaceClient { w, ok := ctx.Value(&workspaceClient).(*databricks.WorkspaceClient) if !ok { diff --git a/internal/fs_test.go b/internal/fs_test.go deleted file mode 100644 index 521638d280..0000000000 --- a/internal/fs_test.go +++ /dev/null @@ -1,35 +0,0 @@ -package internal - -import ( - "context" - "path" - "testing" - - "github.com/databricks/cli/cmd/fs" - "github.com/databricks/cli/cmd/root" - "github.com/databricks/databricks-sdk-go" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestFilerForPathForDbfsPaths(t *testing.T) { - t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) - - ctx := context.Background() - w, err := databricks.NewWorkspaceClient() - require.NoError(t, err) - - tmpDir := temporaryDbfsDir(t, w) - ctx = root.SetWorkspaceClient(ctx, w) - - f, path, err := fs.FilerForPath(ctx, path.Join("dbfs:", tmpDir)) - assert.NoError(t, err) - - // assert dbfs scheme is trimmed from input path - assert.Equal(t, tmpDir, path) - - // assert filer works - stat, err := f.Stat(ctx, tmpDir) - require.NoError(t, err) - assert.True(t, stat.IsDir()) -} From 12c8ab2b4e71e1223206dd55a92b0f6f5834777d Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 23 Jun 2023 14:59:47 +0200 Subject: [PATCH 6/9] add tests for local root path --- libs/filer/local_root_path_test.go | 142 +++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 libs/filer/local_root_path_test.go diff --git a/libs/filer/local_root_path_test.go b/libs/filer/local_root_path_test.go new file mode 100644 index 0000000000..f8d4041819 --- /dev/null +++ b/libs/filer/local_root_path_test.go @@ -0,0 +1,142 @@ +package filer + +import ( + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" +) + +func testUnixLocalRootPath(t *testing.T, uncleanRoot string) { + cleanRoot := filepath.Clean(uncleanRoot) + rp := NewWorkspaceRootPath(uncleanRoot) + + remotePath, err := rp.Join("a/b/c") + assert.NoError(t, err) + assert.Equal(t, cleanRoot+"/a/b/c", remotePath) + + remotePath, err = rp.Join("a/b/../d") + assert.NoError(t, err) + assert.Equal(t, cleanRoot+"/a/d", remotePath) + + remotePath, err = rp.Join("a/../c") + assert.NoError(t, err) + assert.Equal(t, cleanRoot+"/c", remotePath) + + remotePath, err = rp.Join("a/b/c/.") + assert.NoError(t, err) + assert.Equal(t, cleanRoot+"/a/b/c", remotePath) + + remotePath, err = rp.Join("a/b/c/d/./../../f/g") + assert.NoError(t, err) + assert.Equal(t, cleanRoot+"/a/b/f/g", remotePath) + + remotePath, err = rp.Join(".//a/..//./b/..") + assert.NoError(t, err) + assert.Equal(t, cleanRoot, remotePath) + + remotePath, err = rp.Join("a/b/../..") + assert.NoError(t, err) + assert.Equal(t, cleanRoot, remotePath) + + remotePath, err = rp.Join("") + assert.NoError(t, err) + assert.Equal(t, cleanRoot, remotePath) + + remotePath, err = rp.Join(".") + assert.NoError(t, err) + assert.Equal(t, cleanRoot, remotePath) + + remotePath, err = rp.Join("/") + assert.NoError(t, err) + assert.Equal(t, cleanRoot, remotePath) + + _, err = rp.Join("..") + assert.ErrorContains(t, err, `relative path escapes root: ..`) + + _, err = rp.Join("a/../..") + assert.ErrorContains(t, err, `relative path escapes root: a/../..`) + + _, err = rp.Join("./../.") + assert.ErrorContains(t, err, `relative path escapes root: ./../.`) + + _, err = rp.Join("/./.././..") + assert.ErrorContains(t, err, `relative path escapes root: /./.././..`) + + _, err = rp.Join("./../.") + assert.ErrorContains(t, err, `relative path escapes root: ./../.`) + + _, err = rp.Join("./..") + assert.ErrorContains(t, err, `relative path escapes root: ./..`) + + _, err = rp.Join("./../../..") + assert.ErrorContains(t, err, `relative path escapes root: ./../../..`) + + _, err = rp.Join("./../a/./b../../..") + assert.ErrorContains(t, err, `relative path escapes root: ./../a/./b../../..`) + + _, err = rp.Join("../..") + assert.ErrorContains(t, err, `relative path escapes root: ../..`) +} + +func TestUnixLocalRootPath(t *testing.T) { + if runtime.GOOS != "darwin" && runtime.GOOS != "linux" { + t.SkipNow() + } + + testUnixLocalRootPath(t, "/some/root/path") + testUnixLocalRootPath(t, "/some/root/path/") + testUnixLocalRootPath(t, "/some/root/path/.") + testUnixLocalRootPath(t, "/some/root/../path/") +} + +func testWindowsLocalRootPath(t *testing.T, uncleanRoot string) { + cleanRoot := filepath.Clean(uncleanRoot) + rp := NewWorkspaceRootPath(uncleanRoot) + + remotePath, err := rp.Join(`C:a\b\c`) + assert.NoError(t, err) + assert.Equal(t, cleanRoot+`\a\b\c`, remotePath) + + remotePath, err = rp.Join(`c:a\b\..\d`) + assert.NoError(t, err) + assert.Equal(t, cleanRoot+`\a\d`, remotePath) + + remotePath, err = rp.Join(`a\..\c`) + assert.NoError(t, err) + assert.Equal(t, cleanRoot+`\c`, remotePath) + + remotePath, err = rp.Join(`c:a\b\c\.`) + assert.NoError(t, err) + assert.Equal(t, cleanRoot+"/a/b/c", remotePath) + + remotePath, err = rp.Join(`c:a\b\..\..`) + assert.NoError(t, err) + assert.Equal(t, cleanRoot, remotePath) + + remotePath, err = rp.Join("") + assert.NoError(t, err) + assert.Equal(t, cleanRoot, remotePath) + + remotePath, err = rp.Join(".") + assert.NoError(t, err) + assert.Equal(t, cleanRoot, remotePath) + + _, err = rp.Join("..") + assert.ErrorContains(t, err, `relative path escapes root`) + + _, err = rp.Join(`a\..\..`) + assert.ErrorContains(t, err, `relative path escapes root`) +} + +func TestWindowsLocalRootPath(t *testing.T) { + if runtime.GOOS != "windows" { + t.SkipNow() + } + + testWindowsLocalRootPath(t, `c:\some\root\path`) + testWindowsLocalRootPath(t, `c:\some\root\path\`) + testWindowsLocalRootPath(t, `c:\some\root\path\.`) + testWindowsLocalRootPath(t, `C:\some\root\..\path\`) +} From b1a1c614f4240e910767387d54b98191f346cb41 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 23 Jun 2023 15:05:40 +0200 Subject: [PATCH 7/9] fix tests --- internal/fs_cp_test.go | 2 +- libs/filer/local_root_path_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/fs_cp_test.go b/internal/fs_cp_test.go index c91710865c..806263b662 100644 --- a/internal/fs_cp_test.go +++ b/internal/fs_cp_test.go @@ -66,7 +66,7 @@ func setupLocalFiler(t *testing.T) (filer.Filer, string) { f, err := filer.NewLocalClient(tmp) require.NoError(t, err) - return f, path.Join("file:/", filepath.ToSlash(tmp)) + return f, path.Join(filepath.ToSlash(tmp)) } func setupDbfsFiler(t *testing.T) (filer.Filer, string) { diff --git a/libs/filer/local_root_path_test.go b/libs/filer/local_root_path_test.go index f8d4041819..ea782ef657 100644 --- a/libs/filer/local_root_path_test.go +++ b/libs/filer/local_root_path_test.go @@ -10,7 +10,7 @@ import ( func testUnixLocalRootPath(t *testing.T, uncleanRoot string) { cleanRoot := filepath.Clean(uncleanRoot) - rp := NewWorkspaceRootPath(uncleanRoot) + rp := NewLocalRootPath(uncleanRoot) remotePath, err := rp.Join("a/b/c") assert.NoError(t, err) @@ -93,7 +93,7 @@ func TestUnixLocalRootPath(t *testing.T) { func testWindowsLocalRootPath(t *testing.T, uncleanRoot string) { cleanRoot := filepath.Clean(uncleanRoot) - rp := NewWorkspaceRootPath(uncleanRoot) + rp := NewLocalRootPath(uncleanRoot) remotePath, err := rp.Join(`C:a\b\c`) assert.NoError(t, err) From d1f177215a45c534345ee193f8ee389ae0ddbcc3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 23 Jun 2023 15:17:20 +0200 Subject: [PATCH 8/9] fix tests --- libs/filer/local_root_path_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libs/filer/local_root_path_test.go b/libs/filer/local_root_path_test.go index ea782ef657..1a39c4463b 100644 --- a/libs/filer/local_root_path_test.go +++ b/libs/filer/local_root_path_test.go @@ -95,11 +95,11 @@ func testWindowsLocalRootPath(t *testing.T, uncleanRoot string) { cleanRoot := filepath.Clean(uncleanRoot) rp := NewLocalRootPath(uncleanRoot) - remotePath, err := rp.Join(`C:a\b\c`) + remotePath, err := rp.Join(`a\b\c`) assert.NoError(t, err) assert.Equal(t, cleanRoot+`\a\b\c`, remotePath) - remotePath, err = rp.Join(`c:a\b\..\d`) + remotePath, err = rp.Join(`a\b\..\d`) assert.NoError(t, err) assert.Equal(t, cleanRoot+`\a\d`, remotePath) @@ -107,11 +107,11 @@ func testWindowsLocalRootPath(t *testing.T, uncleanRoot string) { assert.NoError(t, err) assert.Equal(t, cleanRoot+`\c`, remotePath) - remotePath, err = rp.Join(`c:a\b\c\.`) + remotePath, err = rp.Join(`a\b\c\.`) assert.NoError(t, err) - assert.Equal(t, cleanRoot+"/a/b/c", remotePath) + assert.Equal(t, cleanRoot+`\a\b\c`, remotePath) - remotePath, err = rp.Join(`c:a\b\..\..`) + remotePath, err = rp.Join(`a\b\..\..`) assert.NoError(t, err) assert.Equal(t, cleanRoot, remotePath) From 2e69c2aed1db4638771296b0cec4486f6be55c17 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 23 Jun 2023 15:26:40 +0200 Subject: [PATCH 9/9] fix integration tests --- internal/fs_cp_test.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/internal/fs_cp_test.go b/internal/fs_cp_test.go index 806263b662..766d6a59b4 100644 --- a/internal/fs_cp_test.go +++ b/internal/fs_cp_test.go @@ -259,21 +259,14 @@ func TestAccFsCpErrorsWhenSourceIsDirWithoutRecursiveFlag(t *testing.T) { tmpDir := temporaryDbfsDir(t, w) _, _, err = RequireErrorRun(t, "fs", "cp", "dbfs:"+tmpDir, "dbfs:/tmp") - assert.Equal(t, fmt.Sprintf("source path %s is a directory. Please specify the --recursive flag", strings.TrimPrefix(tmpDir, "/")), err.Error()) -} - -func TestAccFsCpErrorsOnNoScheme(t *testing.T) { - t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) - - _, _, err := RequireErrorRun(t, "fs", "cp", "/a", "/b") - assert.Equal(t, "no scheme specified for path /a. Please specify scheme \"dbfs\" or \"file\". Example: file:/foo/bar or file:/c:/foo/bar", err.Error()) + assert.Equal(t, fmt.Sprintf("source path %s is a directory. Please specify the --recursive flag", tmpDir), err.Error()) } func TestAccFsCpErrorsOnInvalidScheme(t *testing.T) { t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) _, _, err := RequireErrorRun(t, "fs", "cp", "dbfs:/a", "https:/b") - assert.Equal(t, "unsupported scheme https specified for path https:/b. Please specify scheme \"dbfs\" or \"file\". Example: file:/foo/bar or file:/c:/foo/bar", err.Error()) + assert.Equal(t, "invalid scheme: https", err.Error()) } func TestAccFsCpSourceIsDirectoryButTargetIsFile(t *testing.T) {