From 0095d1538cdf5e068823ec9428de6161e99673f4 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 22 Jul 2024 15:06:11 +0300 Subject: [PATCH 01/43] Add autocomplete to fs commands --- cmd/fs/cat.go | 2 + cmd/fs/cp.go | 3 ++ cmd/fs/helpers.go | 52 +++++++++++++++++++++++++- cmd/fs/ls.go | 2 + cmd/fs/mkdir.go | 2 + cmd/fs/rm.go | 2 + libs/filer/completer.go | 83 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 libs/filer/completer.go diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index 7a6f42cba2..375e019e32 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -30,5 +30,7 @@ func newCatCommand() *cobra.Command { return cmdio.Render(ctx, r) } + cmd.ValidArgsFunction = getValidArgsFunction(1) + return cmd } diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index 52feb89051..dc547598a5 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -200,5 +200,8 @@ func newCpCommand() *cobra.Command { return c.cpFileToFile(sourcePath, targetPath) } + // The copy command has two paths that can be completed (SOURCE_PATH & TARGET_PATH) + cmd.ValidArgsFunction = getValidArgsFunction(2) + return cmd } diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 43d65b5dd7..b4f973434e 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -8,6 +8,7 @@ import ( "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/filer" + "github.com/spf13/cobra" ) func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, error) { @@ -46,6 +47,55 @@ 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, "dbfs:/") + return strings.HasPrefix(path, DbfsPrefix) +} + +func getValidArgsFunction(pathArgCount int) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + cmd.SetContext(root.SkipPrompt(cmd.Context())) + + err := mustWorkspaceClient(cmd, args) + if err != nil { + return nil, cobra.ShellCompDirectiveError + } + + isValidPrefix := isDbfsPath(toComplete) + + if !isValidPrefix { + return []string{DbfsPrefix}, cobra.ShellCompDirectiveNoSpace + } + + f, path, err := filerForPath(cmd.Context(), toComplete) + if err != nil { + return nil, cobra.ShellCompDirectiveError + } + + wsc := root.WorkspaceClient(cmd.Context()) + completer := filer.NewCompleter(cmd.Context(), wsc, f) + + if len(args) < pathArgCount { + completions, directive := completer.CompleteRemotePath(path) + + // DbfsPrefix without trailing "/" + prefix := DbfsPrefix[:len(DbfsPrefix)-1] + + // Add the prefix to the completions + for i, completion := range completions { + completions[i] = fmt.Sprintf("%s%s", prefix, completion) + } + + return completions, directive + } else { + return nil, cobra.ShellCompDirectiveNoFileComp + } + } +} + +// Wrapper for [root.MustWorkspaceClient] that disables loading authentication configuration from a bundle. +func mustWorkspaceClient(cmd *cobra.Command, args []string) error { + cmd.SetContext(root.SkipLoadBundle(cmd.Context())) + return root.MustWorkspaceClient(cmd, args) } diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index cec9b98ba0..ba1c4b1687 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -89,5 +89,7 @@ func newLsCommand() *cobra.Command { `)) } + cmd.ValidArgsFunction = getValidArgsFunction(1) + return cmd } diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index 074a7543da..66e932df4e 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -28,5 +28,7 @@ func newMkdirCommand() *cobra.Command { return f.Mkdir(ctx, path) } + cmd.ValidArgsFunction = getValidArgsFunction(1) + return cmd } diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index 5f2904e715..e57d923af0 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -32,5 +32,7 @@ func newRmCommand() *cobra.Command { return f.Delete(ctx, path) } + cmd.ValidArgsFunction = getValidArgsFunction(1) + return cmd } diff --git a/libs/filer/completer.go b/libs/filer/completer.go new file mode 100644 index 0000000000..f1d3da01f7 --- /dev/null +++ b/libs/filer/completer.go @@ -0,0 +1,83 @@ +package filer + +import ( + "context" + "fmt" + "path" + "sort" + + "github.com/databricks/databricks-sdk-go" + "github.com/spf13/cobra" +) + +type completer struct { + ctx context.Context + wsc *databricks.WorkspaceClient + filer Filer +} + +// General completer that takes a Filer to complete remote paths when TAB-ing through a path. +func NewCompleter(ctx context.Context, wsc *databricks.WorkspaceClient, filer Filer) *completer { + return &completer{ctx: ctx, wsc: wsc, filer: filer} +} + +func (c *completer) CompleteRemotePath(remotePath string) ([]string, cobra.ShellCompDirective) { + _, err := c.wsc.CurrentUser.Me(c.ctx) + if err != nil { + return nil, cobra.ShellCompDirectiveError + } + + // If the user is TAB-ing their way through a path, the path in `toComplete` + // is valid and we should list nested directories. + // If the path in `toComplete` is incomplete, however, + // then we should list adjacent directories. + nested := fetchDirs(c.ctx, c.filer, remotePath) + dirs := <-nested + + if dirs == nil { + adjacent := fetchDirs(c.ctx, c.filer, path.Dir(remotePath)) + dirs = <-adjacent + } + + return dirs, cobra.ShellCompDirectiveNoSpace +} + +func fetchDirs( + ctx context.Context, + filer Filer, + remotePath string, +) <-chan []string { + ch := make(chan []string, 1) + go func() { + defer close(ch) + + entries, err := filer.ReadDir(ctx, remotePath) + if err != nil { + return + } + + dirs := []string{} + for _, entry := range entries { + if entry.IsDir() { + separator := "" + + // Ensure that the path and name have a "/" separating them + if remotePath[len(remotePath)-1] != '/' { + separator = "/" + } + + completion := fmt.Sprintf("%s%s%s", remotePath, separator, entry.Name()) + dirs = append(dirs, completion) + } + } + + // Sort completions alphabetically + sort.Slice(dirs, func(i, j int) bool { + return dirs[i] < dirs[j] + }) + + ch <- dirs + }() + + return ch +} From e8e93fd77711f595a87688bae8263d0d89810941 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 24 Jul 2024 12:51:40 +0300 Subject: [PATCH 02/43] Add tests --- cmd/fs/cat.go | 2 +- cmd/fs/cp.go | 2 +- cmd/fs/helpers.go | 18 +++-- cmd/fs/helpers_test.go | 79 +++++++++++++++++++++ cmd/fs/ls.go | 2 +- cmd/fs/mkdir.go | 2 +- cmd/fs/rm.go | 2 +- internal/testutil/filer.go | 74 ++++++++++++++++++++ libs/filer/{ => completer}/completer.go | 20 ++---- libs/filer/completer/completer_test.go | 92 +++++++++++++++++++++++++ 10 files changed, 269 insertions(+), 24 deletions(-) create mode 100644 internal/testutil/filer.go rename libs/filer/{ => completer}/completer.go (76%) create mode 100644 libs/filer/completer/completer_test.go diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index 375e019e32..b5ae411e86 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -30,7 +30,7 @@ func newCatCommand() *cobra.Command { return cmdio.Render(ctx, r) } - cmd.ValidArgsFunction = getValidArgsFunction(1) + cmd.ValidArgsFunction = getValidArgsFunction(1, filerForPath) return cmd } diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index dc547598a5..38c2f89afe 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -201,7 +201,7 @@ func newCpCommand() *cobra.Command { } // The copy command has two paths that can be completed (SOURCE_PATH & TARGET_PATH) - cmd.ValidArgsFunction = getValidArgsFunction(2) + cmd.ValidArgsFunction = getValidArgsFunction(2, filerForPath) return cmd } diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index b4f973434e..3dccfb56f4 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -8,6 +8,7 @@ import ( "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/filer" + "github.com/databricks/cli/libs/filer/completer" "github.com/spf13/cobra" ) @@ -53,12 +54,13 @@ func isDbfsPath(path string) bool { return strings.HasPrefix(path, DbfsPrefix) } -func getValidArgsFunction(pathArgCount int) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { +func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error)) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { cmd.SetContext(root.SkipPrompt(cmd.Context())) - err := mustWorkspaceClient(cmd, args) - if err != nil { + wsc := root.WorkspaceClient(cmd.Context()) + // err := mustWorkspaceClient(cmd, args) // TODO: Fix this + if wsc == nil { return nil, cobra.ShellCompDirectiveError } @@ -68,13 +70,17 @@ func getValidArgsFunction(pathArgCount int) func(cmd *cobra.Command, args []stri return []string{DbfsPrefix}, cobra.ShellCompDirectiveNoSpace } - f, path, err := filerForPath(cmd.Context(), toComplete) + filer, path, err := filerForPathFunc(cmd.Context(), toComplete) if err != nil { return nil, cobra.ShellCompDirectiveError } - wsc := root.WorkspaceClient(cmd.Context()) - completer := filer.NewCompleter(cmd.Context(), wsc, f) + _, err = wsc.CurrentUser.Me(cmd.Context()) + if err != nil { + return nil, cobra.ShellCompDirectiveError + } + + completer := completer.NewCompleter(cmd.Context(), filer) if len(args) < pathArgCount { completions, directive := completer.CompleteRemotePath(path) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index d86bd46e1e..c2702252f4 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -2,11 +2,19 @@ package fs import ( "context" + "errors" + "io/fs" "runtime" "testing" + "github.com/databricks/cli/cmd/root" + "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/filer" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/iam" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -60,3 +68,74 @@ func TestFilerForWindowsLocalPaths(t *testing.T) { testWindowsFilerForPath(t, ctx, `d:\abc`) testWindowsFilerForPath(t, ctx, `f:\abc\ef`) } + +func mockCurrentUserApi(m *mocks.MockWorkspaceClient, u *iam.User, e error) { + currentUserApi := m.GetMockCurrentUserAPI() + currentUserApi.EXPECT().Me(mock.AnythingOfType("*context.valueCtx")).Return(u, e) +} + +func setupValidArgsFunctionTest(t *testing.T) (*mocks.MockWorkspaceClient, *cobra.Command) { + m := mocks.NewMockWorkspaceClient(t) + ctx := context.Background() + ctx = root.SetWorkspaceClient(ctx, m.WorkspaceClient) + + cmd := &cobra.Command{} + cmd.SetContext(ctx) + + return m, cmd +} + +func TestGetValidArgsFunctionCompletion(t *testing.T) { + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + + mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + testutil.NewFakeDirEntry("dir", true), + }) + + validArgsFunction := getValidArgsFunction(1, mockFilerForPath) + completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") + + assert.Equal(t, []string{"dbfs:/dir"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + +func TestGetValidArgsFunctionNoDbfsPath(t *testing.T) { + _, cmd := setupValidArgsFunctionTest(t) + + mockFilerForPath := testutil.GetMockFilerForPath(t, nil) + + validArgsFunction := getValidArgsFunction(1, mockFilerForPath) + completions, directive := validArgsFunction(cmd, []string{}, "/") + + assert.Equal(t, []string{"dbfs:/"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + +func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, errors.New("Current User Error")) + mockFilerForPath := testutil.GetMockFilerForPath(t, nil) + + validArgsFunction := getValidArgsFunction(1, mockFilerForPath) + completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") + + assert.Nil(t, completions) + assert.Equal(t, cobra.ShellCompDirectiveError, directive) +} + +func TestGetValidArgsFunctionNotCompletedArgument(t *testing.T) { + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + mockFilerForPath := testutil.GetMockFilerForPath(t, nil) + + // 0 here means we don't complete any arguments + validArgsFunction := getValidArgsFunction(0, mockFilerForPath) + completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") + + assert.Nil(t, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive) +} diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index ba1c4b1687..2057d43b21 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -89,7 +89,7 @@ func newLsCommand() *cobra.Command { `)) } - cmd.ValidArgsFunction = getValidArgsFunction(1) + cmd.ValidArgsFunction = getValidArgsFunction(1, filerForPath) return cmd } diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index 66e932df4e..ef9656659c 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -28,7 +28,7 @@ func newMkdirCommand() *cobra.Command { return f.Mkdir(ctx, path) } - cmd.ValidArgsFunction = getValidArgsFunction(1) + cmd.ValidArgsFunction = getValidArgsFunction(1, filerForPath) return cmd } diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index e57d923af0..e225c1f1aa 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -32,7 +32,7 @@ func newRmCommand() *cobra.Command { return f.Delete(ctx, path) } - cmd.ValidArgsFunction = getValidArgsFunction(1) + cmd.ValidArgsFunction = getValidArgsFunction(1, filerForPath) return cmd } diff --git a/internal/testutil/filer.go b/internal/testutil/filer.go new file mode 100644 index 0000000000..a64f16264e --- /dev/null +++ b/internal/testutil/filer.go @@ -0,0 +1,74 @@ +package testutil + +import ( + "context" + "io/fs" + "testing" + "time" + + mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" + "github.com/databricks/cli/libs/filer" + "github.com/stretchr/testify/mock" +) + +type fakeDirEntry struct { + fakeFileInfo +} + +func (entry fakeDirEntry) Type() fs.FileMode { + typ := fs.ModePerm + if entry.dir { + typ |= fs.ModeDir + } + return typ +} + +func (entry fakeDirEntry) Info() (fs.FileInfo, error) { + return entry.fakeFileInfo, nil +} + +type fakeFileInfo struct { + name string + size int64 + dir bool + mode fs.FileMode +} + +func (info fakeFileInfo) Name() string { + return info.name +} + +func (info fakeFileInfo) Size() int64 { + return info.size +} + +func (info fakeFileInfo) Mode() fs.FileMode { + return info.mode +} + +func (info fakeFileInfo) ModTime() time.Time { + return time.Now() +} + +func (info fakeFileInfo) IsDir() bool { + return info.dir +} + +func (info fakeFileInfo) Sys() any { + return nil +} + +func NewFakeDirEntry(name string, dir bool) fs.DirEntry { + return fakeDirEntry{fakeFileInfo{name: name, dir: dir}} +} + +func GetMockFilerForPath(t *testing.T, entries []fs.DirEntry) func(ctx context.Context, fullPath string) (filer.Filer, string, error) { + mockFiler := mockfiler.NewMockFiler(t) + if entries != nil { + mockFiler.EXPECT().ReadDir(mock.AnythingOfType("*context.valueCtx"), mock.Anything).Return(entries, nil) + } + mockFilerForPath := func(ctx context.Context, fullPath string) (filer.Filer, string, error) { + return mockFiler, "/", nil + } + return mockFilerForPath +} diff --git a/libs/filer/completer.go b/libs/filer/completer/completer.go similarity index 76% rename from libs/filer/completer.go rename to libs/filer/completer/completer.go index f1d3da01f7..c20aec1c4c 100644 --- a/libs/filer/completer.go +++ b/libs/filer/completer/completer.go @@ -1,4 +1,4 @@ -package filer +package completer import ( "context" @@ -6,27 +6,21 @@ import ( "path" "sort" - "github.com/databricks/databricks-sdk-go" + "github.com/databricks/cli/libs/filer" "github.com/spf13/cobra" ) type completer struct { ctx context.Context - wsc *databricks.WorkspaceClient - filer Filer + filer filer.Filer } // General completer that takes a Filer to complete remote paths when TAB-ing through a path. -func NewCompleter(ctx context.Context, wsc *databricks.WorkspaceClient, filer Filer) *completer { - return &completer{ctx: ctx, wsc: wsc, filer: filer} +func NewCompleter(ctx context.Context, filer filer.Filer) *completer { + return &completer{ctx: ctx, filer: filer} } func (c *completer) CompleteRemotePath(remotePath string) ([]string, cobra.ShellCompDirective) { - _, err := c.wsc.CurrentUser.Me(c.ctx) - if err != nil { - return nil, cobra.ShellCompDirectiveError - } - // If the user is TAB-ing their way through a path, the path in `toComplete` // is valid and we should list nested directories. // If the path in `toComplete` is incomplete, however, @@ -44,7 +38,7 @@ func (c *completer) CompleteRemotePath(remotePath string) ([]string, cobra.Shell func fetchDirs( ctx context.Context, - filer Filer, + filer filer.Filer, remotePath string, ) <-chan []string { ch := make(chan []string, 1) @@ -61,7 +55,7 @@ func fetchDirs( if entry.IsDir() { separator := "" - // Ensure that the path and name have a "/" separating them + // Ensure the path and name have a "/" separating them if remotePath[len(remotePath)-1] != '/' { separator = "/" } diff --git a/libs/filer/completer/completer_test.go b/libs/filer/completer/completer_test.go new file mode 100644 index 0000000000..9e0c0837c6 --- /dev/null +++ b/libs/filer/completer/completer_test.go @@ -0,0 +1,92 @@ +package completer + +import ( + "context" + "errors" + "io/fs" + "testing" + + "github.com/databricks/cli/cmd/root" + mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" + "github.com/databricks/cli/internal/testutil" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func setup(t *testing.T) context.Context { + ctx := context.Background() + // Needed to make type context.valueCtx for mockFilerForPath + ctx = root.SetWorkspaceClient(ctx, mocks.NewMockWorkspaceClient(t).WorkspaceClient) + return ctx +} + +func TestFilerCompleterReturnsNestedDirs(t *testing.T) { + ctx := setup(t) + + mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + testutil.NewFakeDirEntry("dir", true), + }) + mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/") + + completer := NewCompleter(ctx, mockFiler) + + completions, directive := completer.CompleteRemotePath("/") + + assert.Equal(t, []string{"/dir"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + +func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { + ctx := setup(t) + + mockFiler := mockfiler.NewMockFiler(t) + + // First call to get nested dirs fails so we get the adjacent dirs instead + mockFiler.On("ReadDir", mock.AnythingOfType("*context.valueCtx"), "/wrong_path").Return(nil, errors.New("error")) + mockFiler.On("ReadDir", mock.AnythingOfType("*context.valueCtx"), "/").Return([]fs.DirEntry{ + testutil.NewFakeDirEntry("adjacent", true), + }, nil) + + completer := NewCompleter(ctx, mockFiler) + completions, directive := completer.CompleteRemotePath("/wrong_path") + + assert.Equal(t, []string{"/adjacent"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + + mockFiler.AssertExpectations(t) + +} + +func TestFilerCompleterRetainsFormatting(t *testing.T) { + ctx := setup(t) + + mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + testutil.NewFakeDirEntry("nested_dir", true), + }) + mockFiler, _, _ := mockFilerForPath(ctx, "dbfs://dir") + + completer := NewCompleter(ctx, mockFiler) + + completions, directive := completer.CompleteRemotePath("//dir//") + + assert.Equal(t, []string{"//dir//nested_dir"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + +func TestFilerCompleterAddsSeparator(t *testing.T) { + ctx := setup(t) + + mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + testutil.NewFakeDirEntry("nested_dir", true), + }) + mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/dir") + + completer := NewCompleter(ctx, mockFiler) + + completions, directive := completer.CompleteRemotePath("/dir") + + assert.Equal(t, []string{"/dir/nested_dir"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} From 300ab07613167ae17df91bd09ad701e73daafaa0 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 24 Jul 2024 12:58:34 +0300 Subject: [PATCH 03/43] Lint --- cmd/fs/helpers.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 3dccfb56f4..a76a30e5c5 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -100,8 +100,8 @@ func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Co } } -// Wrapper for [root.MustWorkspaceClient] that disables loading authentication configuration from a bundle. -func mustWorkspaceClient(cmd *cobra.Command, args []string) error { - cmd.SetContext(root.SkipLoadBundle(cmd.Context())) - return root.MustWorkspaceClient(cmd, args) -} +// // Wrapper for [root.MustWorkspaceClient] that disables loading authentication configuration from a bundle. +// func mustWorkspaceClient(cmd *cobra.Command, args []string) error { +// cmd.SetContext(root.SkipLoadBundle(cmd.Context())) +// return root.MustWorkspaceClient(cmd, args) +// } From ba64fd3f8603192647462d02e62f283b5e92eb62 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 24 Jul 2024 13:59:13 +0300 Subject: [PATCH 04/43] Fix TestGlobFileset --- cmd/fs/helpers.go | 10 +++++----- libs/{filer => }/completer/completer.go | 0 libs/{filer => }/completer/completer_test.go | 0 3 files changed, 5 insertions(+), 5 deletions(-) rename libs/{filer => }/completer/completer.go (100%) rename libs/{filer => }/completer/completer_test.go (100%) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index a76a30e5c5..be2e86cb11 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -48,10 +48,10 @@ func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, er return f, path, err } -const DbfsPrefix string = "dbfs:/" +const dbfsPrefix string = "dbfs:/" func isDbfsPath(path string) bool { - return strings.HasPrefix(path, DbfsPrefix) + return strings.HasPrefix(path, dbfsPrefix) } func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error)) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { @@ -67,7 +67,7 @@ func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Co isValidPrefix := isDbfsPath(toComplete) if !isValidPrefix { - return []string{DbfsPrefix}, cobra.ShellCompDirectiveNoSpace + return []string{dbfsPrefix}, cobra.ShellCompDirectiveNoSpace } filer, path, err := filerForPathFunc(cmd.Context(), toComplete) @@ -85,8 +85,8 @@ func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Co if len(args) < pathArgCount { completions, directive := completer.CompleteRemotePath(path) - // DbfsPrefix without trailing "/" - prefix := DbfsPrefix[:len(DbfsPrefix)-1] + // dbfsPrefix without trailing "/" + prefix := dbfsPrefix[:len(dbfsPrefix)-1] // Add the prefix to the completions for i, completion := range completions { diff --git a/libs/filer/completer/completer.go b/libs/completer/completer.go similarity index 100% rename from libs/filer/completer/completer.go rename to libs/completer/completer.go diff --git a/libs/filer/completer/completer_test.go b/libs/completer/completer_test.go similarity index 100% rename from libs/filer/completer/completer_test.go rename to libs/completer/completer_test.go From 5d6a348ec619bfe4803558a7864d6045e470bf91 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 24 Jul 2024 14:38:01 +0300 Subject: [PATCH 05/43] Comments --- cmd/fs/helpers.go | 5 +++-- libs/completer/completer.go | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index be2e86cb11..74ffad267a 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -7,8 +7,8 @@ import ( "strings" "github.com/databricks/cli/cmd/root" + "github.com/databricks/cli/libs/completer" "github.com/databricks/cli/libs/filer" - "github.com/databricks/cli/libs/filer/completer" "github.com/spf13/cobra" ) @@ -85,7 +85,8 @@ func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Co if len(args) < pathArgCount { completions, directive := completer.CompleteRemotePath(path) - // dbfsPrefix without trailing "/" + // The completions will start with a "/", so we'll prefix + // them with the dbfsPrefix without a trailing "/" (dbfs:) prefix := dbfsPrefix[:len(dbfsPrefix)-1] // Add the prefix to the completions diff --git a/libs/completer/completer.go b/libs/completer/completer.go index c20aec1c4c..9a168b444b 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -55,7 +55,9 @@ func fetchDirs( if entry.IsDir() { separator := "" - // Ensure the path and name have a "/" separating them + // Ensure the path and name have a "/" separating them. We don't use path + // utilities to concatenate the path and name because we want to preserve + // the formatting of the path the user has typed (e.g. //a/b///c) if remotePath[len(remotePath)-1] != '/' { separator = "/" } From e3f6a7309cb3a7c60142bfda522845dcfb8b4c0d Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 24 Jul 2024 15:31:25 +0300 Subject: [PATCH 06/43] Add onlyDirs --- cmd/fs/cat.go | 2 +- cmd/fs/cp.go | 2 +- cmd/fs/helpers.go | 12 +++--------- cmd/fs/helpers_test.go | 26 ++++++++++++++++++++++---- cmd/fs/ls.go | 2 +- cmd/fs/mkdir.go | 2 +- cmd/fs/rm.go | 2 +- cmd/root/auth.go | 2 ++ cmd/root/auth_options.go | 6 +++--- libs/completer/completer.go | 20 ++++++++++---------- libs/completer/completer_test.go | 8 ++++---- 11 files changed, 49 insertions(+), 35 deletions(-) diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index b5ae411e86..e85a5b02c4 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -30,7 +30,7 @@ func newCatCommand() *cobra.Command { return cmdio.Render(ctx, r) } - cmd.ValidArgsFunction = getValidArgsFunction(1, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(1, false, filerForPath) return cmd } diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index 38c2f89afe..ddec27bc2f 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -201,7 +201,7 @@ func newCpCommand() *cobra.Command { } // The copy command has two paths that can be completed (SOURCE_PATH & TARGET_PATH) - cmd.ValidArgsFunction = getValidArgsFunction(2, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(2, false, filerForPath) return cmd } diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 74ffad267a..4f3081f630 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -54,12 +54,12 @@ func isDbfsPath(path string) bool { return strings.HasPrefix(path, dbfsPrefix) } -func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error)) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { +func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error)) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + fmt.Println("GetValidArgsFunction") cmd.SetContext(root.SkipPrompt(cmd.Context())) wsc := root.WorkspaceClient(cmd.Context()) - // err := mustWorkspaceClient(cmd, args) // TODO: Fix this if wsc == nil { return nil, cobra.ShellCompDirectiveError } @@ -80,7 +80,7 @@ func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Co return nil, cobra.ShellCompDirectiveError } - completer := completer.NewCompleter(cmd.Context(), filer) + completer := completer.NewCompleter(cmd.Context(), filer, onlyDirs) if len(args) < pathArgCount { completions, directive := completer.CompleteRemotePath(path) @@ -100,9 +100,3 @@ func getValidArgsFunction(pathArgCount int, filerForPathFunc func(ctx context.Co } } } - -// // Wrapper for [root.MustWorkspaceClient] that disables loading authentication configuration from a bundle. -// func mustWorkspaceClient(cmd *cobra.Command, args []string) error { -// cmd.SetContext(root.SkipLoadBundle(cmd.Context())) -// return root.MustWorkspaceClient(cmd, args) -// } diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index c2702252f4..36dc4d6f43 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -92,9 +92,27 @@ func TestGetValidArgsFunctionCompletion(t *testing.T) { mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ testutil.NewFakeDirEntry("dir", true), + testutil.NewFakeDirEntry("file", false), }) - validArgsFunction := getValidArgsFunction(1, mockFilerForPath) + validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath) + completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") + + assert.Equal(t, []string{"dbfs:/dir", "dbfs:/file"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + +func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + + mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + testutil.NewFakeDirEntry("dir", true), + testutil.NewFakeDirEntry("file", false), + }) + + validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") assert.Equal(t, []string{"dbfs:/dir"}, completions) @@ -106,7 +124,7 @@ func TestGetValidArgsFunctionNoDbfsPath(t *testing.T) { mockFilerForPath := testutil.GetMockFilerForPath(t, nil) - validArgsFunction := getValidArgsFunction(1, mockFilerForPath) + validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath) completions, directive := validArgsFunction(cmd, []string{}, "/") assert.Equal(t, []string{"dbfs:/"}, completions) @@ -119,7 +137,7 @@ func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { mockCurrentUserApi(m, nil, errors.New("Current User Error")) mockFilerForPath := testutil.GetMockFilerForPath(t, nil) - validArgsFunction := getValidArgsFunction(1, mockFilerForPath) + validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") assert.Nil(t, completions) @@ -133,7 +151,7 @@ func TestGetValidArgsFunctionNotCompletedArgument(t *testing.T) { mockFilerForPath := testutil.GetMockFilerForPath(t, nil) // 0 here means we don't complete any arguments - validArgsFunction := getValidArgsFunction(0, mockFilerForPath) + validArgsFunction := getValidArgsFunction(0, true, mockFilerForPath) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") assert.Nil(t, completions) diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 2057d43b21..88c38da8d9 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -89,7 +89,7 @@ func newLsCommand() *cobra.Command { `)) } - cmd.ValidArgsFunction = getValidArgsFunction(1, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(1, true, filerForPath) return cmd } diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index ef9656659c..b24034aef0 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -28,7 +28,7 @@ func newMkdirCommand() *cobra.Command { return f.Mkdir(ctx, path) } - cmd.ValidArgsFunction = getValidArgsFunction(1, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(1, true, filerForPath) return cmd } diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index e225c1f1aa..5f1294ff67 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -32,7 +32,7 @@ func newRmCommand() *cobra.Command { return f.Delete(ctx, path) } - cmd.ValidArgsFunction = getValidArgsFunction(1, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(1, false, filerForPath) return cmd } diff --git a/cmd/root/auth.go b/cmd/root/auth.go index 107679105b..9dcaa752c0 100644 --- a/cmd/root/auth.go +++ b/cmd/root/auth.go @@ -152,6 +152,7 @@ func MustAccountClient(cmd *cobra.Command, args []string) error { // Helper function to create a workspace client or prompt once if the given configuration is not valid. func workspaceClientOrPrompt(ctx context.Context, cfg *config.Config, allowPrompt bool) (*databricks.WorkspaceClient, error) { w, err := databricks.NewWorkspaceClient((*databricks.Config)(cfg)) + if err == nil { err = w.Config.Authenticate(emptyHttpRequest(ctx)) } @@ -185,6 +186,7 @@ func workspaceClientOrPrompt(ctx context.Context, cfg *config.Config, allowPromp } func MustWorkspaceClient(cmd *cobra.Command, args []string) error { + fmt.Println("MustWorkspaceClient") cfg := &config.Config{} // The command-line profile flag takes precedence over DATABRICKS_CONFIG_PROFILE. diff --git a/cmd/root/auth_options.go b/cmd/root/auth_options.go index 701900d4f6..d684973374 100644 --- a/cmd/root/auth_options.go +++ b/cmd/root/auth_options.go @@ -6,7 +6,7 @@ import ( type skipPrompt int -var skipPromptKey skipPrompt +var SkipPromptKey skipPrompt // SkipPrompt allows to skip prompt for profile configuration in MustWorkspaceClient. // @@ -16,12 +16,12 @@ var skipPromptKey skipPrompt // thus the Context object seems to be the only viable option for us to configure prompt behaviour based on // the context it's executed from. func SkipPrompt(ctx context.Context) context.Context { - return context.WithValue(ctx, skipPromptKey, true) + return context.WithValue(ctx, SkipPromptKey, true) } // shouldSkipPrompt returns whether or not [SkipPrompt] has been set on the specified context. func shouldSkipPrompt(ctx context.Context) bool { - skipPrompt, ok := ctx.Value(skipPromptKey).(bool) + skipPrompt, ok := ctx.Value(SkipPromptKey).(bool) return ok && skipPrompt } diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 9a168b444b..9d248efd38 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -11,13 +11,14 @@ import ( ) type completer struct { - ctx context.Context - filer filer.Filer + ctx context.Context + filer filer.Filer + onlyDirs bool } // General completer that takes a Filer to complete remote paths when TAB-ing through a path. -func NewCompleter(ctx context.Context, filer filer.Filer) *completer { - return &completer{ctx: ctx, filer: filer} +func NewCompleter(ctx context.Context, filer filer.Filer, onlyDirs bool) *completer { + return &completer{ctx: ctx, filer: filer, onlyDirs: onlyDirs} } func (c *completer) CompleteRemotePath(remotePath string) ([]string, cobra.ShellCompDirective) { @@ -25,11 +26,11 @@ func (c *completer) CompleteRemotePath(remotePath string) ([]string, cobra.Shell // is valid and we should list nested directories. // If the path in `toComplete` is incomplete, however, // then we should list adjacent directories. - nested := fetchDirs(c.ctx, c.filer, remotePath) + nested := fetchDirs(c, remotePath) dirs := <-nested if dirs == nil { - adjacent := fetchDirs(c.ctx, c.filer, path.Dir(remotePath)) + adjacent := fetchDirs(c, path.Dir(remotePath)) dirs = <-adjacent } @@ -37,22 +38,21 @@ func (c *completer) CompleteRemotePath(remotePath string) ([]string, cobra.Shell } func fetchDirs( - ctx context.Context, - filer filer.Filer, + c *completer, remotePath string, ) <-chan []string { ch := make(chan []string, 1) go func() { defer close(ch) - entries, err := filer.ReadDir(ctx, remotePath) + entries, err := c.filer.ReadDir(c.ctx, remotePath) if err != nil { return } dirs := []string{} for _, entry := range entries { - if entry.IsDir() { + if !c.onlyDirs || entry.IsDir() { separator := "" // Ensure the path and name have a "/" separating them. We don't use path diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index 9e0c0837c6..7c8cba5a67 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -30,7 +30,7 @@ func TestFilerCompleterReturnsNestedDirs(t *testing.T) { }) mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/") - completer := NewCompleter(ctx, mockFiler) + completer := NewCompleter(ctx, mockFiler, true) completions, directive := completer.CompleteRemotePath("/") @@ -49,7 +49,7 @@ func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { testutil.NewFakeDirEntry("adjacent", true), }, nil) - completer := NewCompleter(ctx, mockFiler) + completer := NewCompleter(ctx, mockFiler, true) completions, directive := completer.CompleteRemotePath("/wrong_path") assert.Equal(t, []string{"/adjacent"}, completions) @@ -67,7 +67,7 @@ func TestFilerCompleterRetainsFormatting(t *testing.T) { }) mockFiler, _, _ := mockFilerForPath(ctx, "dbfs://dir") - completer := NewCompleter(ctx, mockFiler) + completer := NewCompleter(ctx, mockFiler, true) completions, directive := completer.CompleteRemotePath("//dir//") @@ -83,7 +83,7 @@ func TestFilerCompleterAddsSeparator(t *testing.T) { }) mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/dir") - completer := NewCompleter(ctx, mockFiler) + completer := NewCompleter(ctx, mockFiler, true) completions, directive := completer.CompleteRemotePath("/dir") From 787d563de176771ab444f1ffb21b76ab62636df4 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 24 Jul 2024 15:43:52 +0300 Subject: [PATCH 07/43] Add HasWorkspaceClient --- cmd/fs/helpers.go | 16 +++++++++++++--- cmd/root/auth.go | 6 +++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 4f3081f630..ae438cbcf5 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -56,11 +56,10 @@ func isDbfsPath(path string) bool { func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error)) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - fmt.Println("GetValidArgsFunction") cmd.SetContext(root.SkipPrompt(cmd.Context())) - wsc := root.WorkspaceClient(cmd.Context()) - if wsc == nil { + err := mustWorkspaceClient(cmd, args) // TODO: Fix this + if err != nil { return nil, cobra.ShellCompDirectiveError } @@ -75,6 +74,7 @@ func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func return nil, cobra.ShellCompDirectiveError } + wsc := root.WorkspaceClient(cmd.Context()) _, err = wsc.CurrentUser.Me(cmd.Context()) if err != nil { return nil, cobra.ShellCompDirectiveError @@ -100,3 +100,13 @@ func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func } } } + +// Wrapper for [root.MustWorkspaceClient] that disables loading authentication configuration from a bundle. +func mustWorkspaceClient(cmd *cobra.Command, args []string) error { + if root.HasWorkspaceClient(cmd.Context()) { + return nil + } + + cmd.SetContext(root.SkipLoadBundle(cmd.Context())) + return root.MustWorkspaceClient(cmd, args) +} diff --git a/cmd/root/auth.go b/cmd/root/auth.go index 9dcaa752c0..949ed5f93c 100644 --- a/cmd/root/auth.go +++ b/cmd/root/auth.go @@ -186,7 +186,6 @@ func workspaceClientOrPrompt(ctx context.Context, cfg *config.Config, allowPromp } func MustWorkspaceClient(cmd *cobra.Command, args []string) error { - fmt.Println("MustWorkspaceClient") cfg := &config.Config{} // The command-line profile flag takes precedence over DATABRICKS_CONFIG_PROFILE. @@ -232,6 +231,11 @@ func SetWorkspaceClient(ctx context.Context, w *databricks.WorkspaceClient) cont return context.WithValue(ctx, &workspaceClient, w) } +func HasWorkspaceClient(ctx context.Context) bool { + _, ok := ctx.Value(&workspaceClient).(*databricks.WorkspaceClient) + return ok +} + func SetAccountClient(ctx context.Context, a *databricks.AccountClient) context.Context { return context.WithValue(ctx, &accountClient, a) } From a43b4affb2ac425c6a4054fd64b4a0d79e2ea0a6 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 24 Jul 2024 15:57:44 +0300 Subject: [PATCH 08/43] Add file+dir test --- libs/completer/completer.go | 24 ++++++++++++------------ libs/completer/completer_test.go | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 9d248efd38..dda57712e2 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -26,18 +26,18 @@ func (c *completer) CompleteRemotePath(remotePath string) ([]string, cobra.Shell // is valid and we should list nested directories. // If the path in `toComplete` is incomplete, however, // then we should list adjacent directories. - nested := fetchDirs(c, remotePath) - dirs := <-nested + nested := fetchCompletions(c, remotePath) + completions := <-nested - if dirs == nil { - adjacent := fetchDirs(c, path.Dir(remotePath)) - dirs = <-adjacent + if completions == nil { + adjacent := fetchCompletions(c, path.Dir(remotePath)) + completions = <-adjacent } - return dirs, cobra.ShellCompDirectiveNoSpace + return completions, cobra.ShellCompDirectiveNoSpace } -func fetchDirs( +func fetchCompletions( c *completer, remotePath string, ) <-chan []string { @@ -50,7 +50,7 @@ func fetchDirs( return } - dirs := []string{} + completions := []string{} for _, entry := range entries { if !c.onlyDirs || entry.IsDir() { separator := "" @@ -63,16 +63,16 @@ func fetchDirs( } completion := fmt.Sprintf("%s%s%s", remotePath, separator, entry.Name()) - dirs = append(dirs, completion) + completions = append(completions, completion) } } // Sort completions alphabetically - sort.Slice(dirs, func(i, j int) bool { - return dirs[i] < dirs[j] + sort.Slice(completions, func(i, j int) bool { + return completions[i] < completions[j] }) - ch <- dirs + ch <- completions }() return ch diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index 7c8cba5a67..defe253945 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -56,7 +56,24 @@ func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) mockFiler.AssertExpectations(t) +} + +func TestFilerCompleterReturnsFileAndDir(t *testing.T) { + ctx := setup(t) + + mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + testutil.NewFakeDirEntry("dir", true), + testutil.NewFakeDirEntry("file", false), + }) + mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/") + + // onlyDirs=false so we should get both files and directories + completer := NewCompleter(ctx, mockFiler, false) + completions, directive := completer.CompleteRemotePath("/") + + assert.Equal(t, []string{"/dir", "/file"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } func TestFilerCompleterRetainsFormatting(t *testing.T) { From 13eb013f3d05f7a990287d50ec8943535520e311 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Thu, 25 Jul 2024 10:42:30 +0300 Subject: [PATCH 09/43] PR comments --- cmd/fs/helpers.go | 35 ++++++++++++++++---------------- libs/completer/completer_test.go | 15 ++++++++++++++ 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index ae438cbcf5..5f15a5e2fc 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -56,7 +56,8 @@ func isDbfsPath(path string) bool { func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error)) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - cmd.SetContext(root.SkipPrompt(cmd.Context())) + ctx := cmd.Context() + cmd.SetContext(root.SkipPrompt(ctx)) err := mustWorkspaceClient(cmd, args) // TODO: Fix this if err != nil { @@ -69,35 +70,35 @@ func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func return []string{dbfsPrefix}, cobra.ShellCompDirectiveNoSpace } - filer, path, err := filerForPathFunc(cmd.Context(), toComplete) + filer, path, err := filerForPathFunc(ctx, toComplete) if err != nil { return nil, cobra.ShellCompDirectiveError } - wsc := root.WorkspaceClient(cmd.Context()) - _, err = wsc.CurrentUser.Me(cmd.Context()) + wsc := root.WorkspaceClient(ctx) + _, err = wsc.CurrentUser.Me(ctx) if err != nil { return nil, cobra.ShellCompDirectiveError } - completer := completer.NewCompleter(cmd.Context(), filer, onlyDirs) + completer := completer.NewCompleter(ctx, filer, onlyDirs) - if len(args) < pathArgCount { - completions, directive := completer.CompleteRemotePath(path) + if len(args) >= -pathArgCount { + return nil, cobra.ShellCompDirectiveNoFileComp + } - // The completions will start with a "/", so we'll prefix - // them with the dbfsPrefix without a trailing "/" (dbfs:) - prefix := dbfsPrefix[:len(dbfsPrefix)-1] + completions, directive := completer.CompleteRemotePath(path) - // Add the prefix to the completions - for i, completion := range completions { - completions[i] = fmt.Sprintf("%s%s", prefix, completion) - } + // The completions will start with a "/", so we'll prefix + // them with the dbfsPrefix without a trailing "/" (dbfs:) + prefix := dbfsPrefix[:len(dbfsPrefix)-1] - return completions, directive - } else { - return nil, cobra.ShellCompDirectiveNoFileComp + // Add the prefix to the completions + for i, completion := range completions { + completions[i] = fmt.Sprintf("%s%s", prefix, completion) } + + return completions, directive } } diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index defe253945..9b15932cc7 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -58,6 +58,21 @@ func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { mockFiler.AssertExpectations(t) } +func TestFilerCompleterReadDirError(t *testing.T) { + ctx := setup(t) + + mockFiler := mockfiler.NewMockFiler(t) + mockFiler.On("ReadDir", mock.AnythingOfType("*context.valueCtx"), mock.Anything).Return(nil, errors.New("error")) + + completer := NewCompleter(ctx, mockFiler, true) + completions, directive := completer.CompleteRemotePath("/dir") + + assert.Nil(t, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + + mockFiler.AssertExpectations(t) +} + func TestFilerCompleterReturnsFileAndDir(t *testing.T) { ctx := setup(t) From 3856332967529cdbda51cca1929855cbf3fc5448 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Thu, 25 Jul 2024 12:20:23 +0300 Subject: [PATCH 10/43] Programmatically add dbfs:/Volumes --- cmd/fs/helpers.go | 39 +++++++++++++++++++++++++++------------ cmd/fs/helpers_test.go | 27 +++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 5f15a5e2fc..6edf588a2d 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -49,6 +49,7 @@ func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, er } const dbfsPrefix string = "dbfs:/" +const volumesPefix string = "dbfs:/Volumes" func isDbfsPath(path string) bool { return strings.HasPrefix(path, dbfsPrefix) @@ -56,48 +57,62 @@ func isDbfsPath(path string) bool { func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error)) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - ctx := cmd.Context() - cmd.SetContext(root.SkipPrompt(ctx)) + cmd.SetContext(root.SkipPrompt(cmd.Context())) err := mustWorkspaceClient(cmd, args) // TODO: Fix this if err != nil { return nil, cobra.ShellCompDirectiveError } - isValidPrefix := isDbfsPath(toComplete) + prefixes := []string{ + dbfsPrefix, + "/", + } + + selectedPrefix := "" + for _, p := range prefixes { + if strings.HasPrefix(toComplete, p) { + selectedPrefix = p + } + } - if !isValidPrefix { - return []string{dbfsPrefix}, cobra.ShellCompDirectiveNoSpace + if selectedPrefix == "" { + return prefixes, cobra.ShellCompDirectiveNoSpace } - filer, path, err := filerForPathFunc(ctx, toComplete) + filer, path, err := filerForPathFunc(cmd.Context(), toComplete) if err != nil { return nil, cobra.ShellCompDirectiveError } - wsc := root.WorkspaceClient(ctx) - _, err = wsc.CurrentUser.Me(ctx) + wsc := root.WorkspaceClient(cmd.Context()) + _, err = wsc.CurrentUser.Me(cmd.Context()) if err != nil { return nil, cobra.ShellCompDirectiveError } - completer := completer.NewCompleter(ctx, filer, onlyDirs) + completer := completer.NewCompleter(cmd.Context(), filer, onlyDirs) - if len(args) >= -pathArgCount { + if len(args) >= pathArgCount { return nil, cobra.ShellCompDirectiveNoFileComp } completions, directive := completer.CompleteRemotePath(path) // The completions will start with a "/", so we'll prefix - // them with the dbfsPrefix without a trailing "/" (dbfs:) - prefix := dbfsPrefix[:len(dbfsPrefix)-1] + // them with the selectedPrefix without a trailing "/" + prefix := selectedPrefix[:len(selectedPrefix)-1] // Add the prefix to the completions for i, completion := range completions { completions[i] = fmt.Sprintf("%s%s", prefix, completion) } + // If the path is a dbfs path, we should also add the Volumes prefix + if strings.HasPrefix(volumesPefix, toComplete) { + completions = append(completions, volumesPefix) + } + return completions, directive } } diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 36dc4d6f43..5622b56ed6 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -98,7 +98,8 @@ func TestGetValidArgsFunctionCompletion(t *testing.T) { validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") - assert.Equal(t, []string{"dbfs:/dir", "dbfs:/file"}, completions) + // dbfs:/Volumes is programmatically added to the completions + assert.Equal(t, []string{"dbfs:/dir", "dbfs:/file", "dbfs:/Volumes"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -115,19 +116,37 @@ func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") - assert.Equal(t, []string{"dbfs:/dir"}, completions) + assert.Equal(t, []string{"dbfs:/dir", "dbfs:/Volumes"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } -func TestGetValidArgsFunctionNoDbfsPath(t *testing.T) { +func TestGetValidArgsFunctionNoPath(t *testing.T) { _, cmd := setupValidArgsFunctionTest(t) mockFilerForPath := testutil.GetMockFilerForPath(t, nil) validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath) + completions, directive := validArgsFunction(cmd, []string{}, "") + + // Suggest both dbfs and local paths at beginning of completion + assert.Equal(t, []string{"dbfs:/", "/"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + +func TestGetValidArgsFunctionLocalPath(t *testing.T) { + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + + mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + testutil.NewFakeDirEntry("dir", true), + testutil.NewFakeDirEntry("file", false), + }) + + validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath) completions, directive := validArgsFunction(cmd, []string{}, "/") - assert.Equal(t, []string{"dbfs:/"}, completions) + assert.Equal(t, []string{"/dir", "/file"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } From c73c09b65a89664fbad38550534c64b63135006f Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Sat, 27 Jul 2024 15:56:58 +0300 Subject: [PATCH 11/43] PR feedback --- cmd/fs/cat.go | 2 +- cmd/fs/cp.go | 2 +- cmd/fs/helpers.go | 74 +++++++++++++++++--------------- cmd/fs/helpers_test.go | 62 ++++++++++++++++++-------- cmd/fs/ls.go | 2 +- cmd/fs/mkdir.go | 2 +- cmd/fs/rm.go | 2 +- internal/testutil/filer.go | 6 +-- libs/completer/completer.go | 11 ++--- libs/completer/completer_test.go | 8 ++-- 10 files changed, 99 insertions(+), 72 deletions(-) diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index e85a5b02c4..30056a3e27 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -30,7 +30,7 @@ func newCatCommand() *cobra.Command { return cmdio.Render(ctx, r) } - cmd.ValidArgsFunction = getValidArgsFunction(1, false, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(1, false, filerForPath, root.MustWorkspaceClient) return cmd } diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index ddec27bc2f..6d0395f402 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -201,7 +201,7 @@ func newCpCommand() *cobra.Command { } // The copy command has two paths that can be completed (SOURCE_PATH & TARGET_PATH) - cmd.ValidArgsFunction = getValidArgsFunction(2, false, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(2, false, filerForPath, root.MustWorkspaceClient) return cmd } diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 6edf588a2d..d9414cf702 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/completer" "github.com/databricks/cli/libs/filer" + "github.com/fatih/color" "github.com/spf13/cobra" ) @@ -50,41 +51,34 @@ func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, er const dbfsPrefix string = "dbfs:/" const volumesPefix string = "dbfs:/Volumes" +const localPefix string = "./" func isDbfsPath(path string) bool { return strings.HasPrefix(path, dbfsPrefix) } -func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error)) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { +func getValidArgsFunction( + pathArgCount int, + onlyDirs bool, + filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error), + mustWorkspaceClientFunc func(cmd *cobra.Command, args []string) error, +) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { cmd.SetContext(root.SkipPrompt(cmd.Context())) - err := mustWorkspaceClient(cmd, args) // TODO: Fix this + err := mustWorkspaceClientFunc(cmd, args) + if err != nil { return nil, cobra.ShellCompDirectiveError } - prefixes := []string{ - dbfsPrefix, - "/", - } - - selectedPrefix := "" - for _, p := range prefixes { - if strings.HasPrefix(toComplete, p) { - selectedPrefix = p - } - } - - if selectedPrefix == "" { - return prefixes, cobra.ShellCompDirectiveNoSpace - } - filer, path, err := filerForPathFunc(cmd.Context(), toComplete) if err != nil { return nil, cobra.ShellCompDirectiveError } + isDbfsPath := isDbfsPath(toComplete) + wsc := root.WorkspaceClient(cmd.Context()) _, err = wsc.CurrentUser.Me(cmd.Context()) if err != nil { @@ -99,30 +93,42 @@ func getValidArgsFunction(pathArgCount int, onlyDirs bool, filerForPathFunc func completions, directive := completer.CompleteRemotePath(path) - // The completions will start with a "/", so we'll prefix - // them with the selectedPrefix without a trailing "/" - prefix := selectedPrefix[:len(selectedPrefix)-1] - - // Add the prefix to the completions for i, completion := range completions { - completions[i] = fmt.Sprintf("%s%s", prefix, completion) + if isDbfsPath { + // The completions will start with a "/", so we'll prefix them with the + // selectedPrefix without a trailing "/" + prefix := dbfsPrefix[:len(dbfsPrefix)-1] + completions[i] = fmt.Sprintf("%s%s", prefix, completion) + } else if shouldDropLocalPrefix(toComplete, completion) { + + completions[i] = completion[len(localPefix):] + } else { + completions[i] = completion + } + } + + // If the path is a dbfs path, we try to add the dbfs:/Volumes prefix suggestion + if isDbfsPath && strings.HasPrefix(volumesPefix, toComplete) { + completions = append([]string{highlight(volumesPefix)}, completions...) + } - // If the path is a dbfs path, we should also add the Volumes prefix - if strings.HasPrefix(volumesPefix, toComplete) { - completions = append(completions, volumesPefix) + // If the path is local, we try to prepend the dbfs:/ prefix suggestion + if !isDbfsPath && strings.HasPrefix(dbfsPrefix, toComplete) { + completions = append([]string{highlight(dbfsPrefix)}, completions...) } return completions, directive } } -// Wrapper for [root.MustWorkspaceClient] that disables loading authentication configuration from a bundle. -func mustWorkspaceClient(cmd *cobra.Command, args []string) error { - if root.HasWorkspaceClient(cmd.Context()) { - return nil - } +// Drop the local prefix from completions if the path to complete doesn't +// start with it. We do this because the local filer returns paths with the +// local prefix. +func shouldDropLocalPrefix(toComplete string, completion string) bool { + return !strings.HasPrefix(toComplete, localPefix) && strings.HasPrefix(completion, localPefix) +} - cmd.SetContext(root.SkipLoadBundle(cmd.Context())) - return root.MustWorkspaceClient(cmd, args) +func highlight(c string) string { + return color.New(color.FgCyan, color.Bold).Sprintf(c) } diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 5622b56ed6..439ba1faca 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -74,6 +74,10 @@ func mockCurrentUserApi(m *mocks.MockWorkspaceClient, u *iam.User, e error) { currentUserApi.EXPECT().Me(mock.AnythingOfType("*context.valueCtx")).Return(u, e) } +func mockMustWorkspaceClientFunc(cmd *cobra.Command, args []string) error { + return nil +} + func setupValidArgsFunctionTest(t *testing.T) (*mocks.MockWorkspaceClient, *cobra.Command) { m := mocks.NewMockWorkspaceClient(t) ctx := context.Background() @@ -90,16 +94,16 @@ func TestGetValidArgsFunctionCompletion(t *testing.T) { mockCurrentUserApi(m, nil, nil) - mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + mockFilerForPath := testutil.GetMockFilerForPath(t, "/", []fs.DirEntry{ testutil.NewFakeDirEntry("dir", true), testutil.NewFakeDirEntry("file", false), }) - validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath) + validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") // dbfs:/Volumes is programmatically added to the completions - assert.Equal(t, []string{"dbfs:/dir", "dbfs:/file", "dbfs:/Volumes"}, completions) + assert.Equal(t, []string{"dbfs:/Volumes", "dbfs:/dir", "dbfs:/file"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -108,28 +112,33 @@ func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { mockCurrentUserApi(m, nil, nil) - mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + mockFilerForPath := testutil.GetMockFilerForPath(t, "/", []fs.DirEntry{ testutil.NewFakeDirEntry("dir", true), testutil.NewFakeDirEntry("file", false), }) - validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath) + validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") - assert.Equal(t, []string{"dbfs:/dir", "dbfs:/Volumes"}, completions) + assert.Equal(t, []string{"dbfs:/Volumes", "dbfs:/dir"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } func TestGetValidArgsFunctionNoPath(t *testing.T) { - _, cmd := setupValidArgsFunctionTest(t) + m, cmd := setupValidArgsFunctionTest(t) - mockFilerForPath := testutil.GetMockFilerForPath(t, nil) + mockCurrentUserApi(m, nil, nil) - validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath) - completions, directive := validArgsFunction(cmd, []string{}, "") + mockFilerForPath := testutil.GetMockFilerForPath(t, "", []fs.DirEntry{ + testutil.NewFakeDirEntry("dFile1", false), + testutil.NewFakeDirEntry("dFile2", false), + }) + + validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) + completions, directive := validArgsFunction(cmd, []string{}, "d") // Suggest both dbfs and local paths at beginning of completion - assert.Equal(t, []string{"dbfs:/", "/"}, completions) + assert.Equal(t, []string{"dbfs:/", "dFile1", "dFile2"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -138,15 +147,32 @@ func TestGetValidArgsFunctionLocalPath(t *testing.T) { mockCurrentUserApi(m, nil, nil) - mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + mockFilerForPath := testutil.GetMockFilerForPath(t, "", []fs.DirEntry{ + testutil.NewFakeDirEntry("dir", true), + testutil.NewFakeDirEntry("file", false), + }) + + validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) + completions, directive := validArgsFunction(cmd, []string{}, "") + + assert.Equal(t, []string{"dbfs:/", "dir", "file"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + +func TestGetValidArgsFunctionAbsoluteLocalPath(t *testing.T) { + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + + mockFilerForPath := testutil.GetMockFilerForPath(t, "", []fs.DirEntry{ testutil.NewFakeDirEntry("dir", true), testutil.NewFakeDirEntry("file", false), }) - validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath) + validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) completions, directive := validArgsFunction(cmd, []string{}, "/") - assert.Equal(t, []string{"/dir", "/file"}, completions) + assert.Equal(t, []string{"dir", "file"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -154,9 +180,9 @@ func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { m, cmd := setupValidArgsFunctionTest(t) mockCurrentUserApi(m, nil, errors.New("Current User Error")) - mockFilerForPath := testutil.GetMockFilerForPath(t, nil) + mockFilerForPath := testutil.GetMockFilerForPath(t, "/", nil) - validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath) + validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") assert.Nil(t, completions) @@ -167,10 +193,10 @@ func TestGetValidArgsFunctionNotCompletedArgument(t *testing.T) { m, cmd := setupValidArgsFunctionTest(t) mockCurrentUserApi(m, nil, nil) - mockFilerForPath := testutil.GetMockFilerForPath(t, nil) + mockFilerForPath := testutil.GetMockFilerForPath(t, "/", nil) // 0 here means we don't complete any arguments - validArgsFunction := getValidArgsFunction(0, true, mockFilerForPath) + validArgsFunction := getValidArgsFunction(0, true, mockFilerForPath, mockMustWorkspaceClientFunc) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") assert.Nil(t, completions) diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 88c38da8d9..9809aac80f 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -89,7 +89,7 @@ func newLsCommand() *cobra.Command { `)) } - cmd.ValidArgsFunction = getValidArgsFunction(1, true, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(1, true, filerForPath, root.MustWorkspaceClient) return cmd } diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index b24034aef0..6ac071f98a 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -28,7 +28,7 @@ func newMkdirCommand() *cobra.Command { return f.Mkdir(ctx, path) } - cmd.ValidArgsFunction = getValidArgsFunction(1, true, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(1, true, filerForPath, root.MustWorkspaceClient) return cmd } diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index 5f1294ff67..901207f9b8 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -32,7 +32,7 @@ func newRmCommand() *cobra.Command { return f.Delete(ctx, path) } - cmd.ValidArgsFunction = getValidArgsFunction(1, false, filerForPath) + cmd.ValidArgsFunction = getValidArgsFunction(1, false, filerForPath, root.MustWorkspaceClient) return cmd } diff --git a/internal/testutil/filer.go b/internal/testutil/filer.go index a64f16264e..3c62a75b9a 100644 --- a/internal/testutil/filer.go +++ b/internal/testutil/filer.go @@ -62,13 +62,13 @@ func NewFakeDirEntry(name string, dir bool) fs.DirEntry { return fakeDirEntry{fakeFileInfo{name: name, dir: dir}} } -func GetMockFilerForPath(t *testing.T, entries []fs.DirEntry) func(ctx context.Context, fullPath string) (filer.Filer, string, error) { +func GetMockFilerForPath(t *testing.T, path string, entries []fs.DirEntry) func(ctx context.Context, fullPath string) (filer.Filer, string, error) { mockFiler := mockfiler.NewMockFiler(t) if entries != nil { - mockFiler.EXPECT().ReadDir(mock.AnythingOfType("*context.valueCtx"), mock.Anything).Return(entries, nil) + mockFiler.EXPECT().ReadDir(mock.AnythingOfType("*context.valueCtx"), path).Return(entries, nil) } mockFilerForPath := func(ctx context.Context, fullPath string) (filer.Filer, string, error) { - return mockFiler, "/", nil + return mockFiler, path, nil } return mockFilerForPath } diff --git a/libs/completer/completer.go b/libs/completer/completer.go index dda57712e2..534bf807b6 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -4,7 +4,7 @@ import ( "context" "fmt" "path" - "sort" + "strings" "github.com/databricks/cli/libs/filer" "github.com/spf13/cobra" @@ -57,8 +57,8 @@ func fetchCompletions( // Ensure the path and name have a "/" separating them. We don't use path // utilities to concatenate the path and name because we want to preserve - // the formatting of the path the user has typed (e.g. //a/b///c) - if remotePath[len(remotePath)-1] != '/' { + // the formatting of whatever path the user has typed (e.g. //a/b///c) + if len(remotePath) > 0 && !strings.HasSuffix(remotePath, "/") { separator = "/" } @@ -67,11 +67,6 @@ func fetchCompletions( } } - // Sort completions alphabetically - sort.Slice(completions, func(i, j int) bool { - return completions[i] < completions[j] - }) - ch <- completions }() diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index 9b15932cc7..b0ac66a889 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -25,7 +25,7 @@ func setup(t *testing.T) context.Context { func TestFilerCompleterReturnsNestedDirs(t *testing.T) { ctx := setup(t) - mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + mockFilerForPath := testutil.GetMockFilerForPath(t, "/", []fs.DirEntry{ testutil.NewFakeDirEntry("dir", true), }) mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/") @@ -76,7 +76,7 @@ func TestFilerCompleterReadDirError(t *testing.T) { func TestFilerCompleterReturnsFileAndDir(t *testing.T) { ctx := setup(t) - mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + mockFilerForPath := testutil.GetMockFilerForPath(t, "/", []fs.DirEntry{ testutil.NewFakeDirEntry("dir", true), testutil.NewFakeDirEntry("file", false), }) @@ -94,7 +94,7 @@ func TestFilerCompleterReturnsFileAndDir(t *testing.T) { func TestFilerCompleterRetainsFormatting(t *testing.T) { ctx := setup(t) - mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + mockFilerForPath := testutil.GetMockFilerForPath(t, "//dir//", []fs.DirEntry{ testutil.NewFakeDirEntry("nested_dir", true), }) mockFiler, _, _ := mockFilerForPath(ctx, "dbfs://dir") @@ -110,7 +110,7 @@ func TestFilerCompleterRetainsFormatting(t *testing.T) { func TestFilerCompleterAddsSeparator(t *testing.T) { ctx := setup(t) - mockFilerForPath := testutil.GetMockFilerForPath(t, []fs.DirEntry{ + mockFilerForPath := testutil.GetMockFilerForPath(t, "/dir", []fs.DirEntry{ testutil.NewFakeDirEntry("nested_dir", true), }) mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/dir") From 363bd1d646dde7ecac6e2ae71c6554fc1b092529 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 29 Jul 2024 11:50:46 +0300 Subject: [PATCH 12/43] Client --- cmd/root/auth.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cmd/root/auth.go b/cmd/root/auth.go index 949ed5f93c..31af8e11fb 100644 --- a/cmd/root/auth.go +++ b/cmd/root/auth.go @@ -231,11 +231,6 @@ func SetWorkspaceClient(ctx context.Context, w *databricks.WorkspaceClient) cont return context.WithValue(ctx, &workspaceClient, w) } -func HasWorkspaceClient(ctx context.Context) bool { - _, ok := ctx.Value(&workspaceClient).(*databricks.WorkspaceClient) - return ok -} - func SetAccountClient(ctx context.Context, a *databricks.AccountClient) context.Context { return context.WithValue(ctx, &accountClient, a) } From 0716f31265f01f12b079a7881e7738c27253198d Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 29 Jul 2024 12:00:30 +0300 Subject: [PATCH 13/43] Comment --- cmd/fs/helpers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index d9414cf702..33cb2901d4 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -123,8 +123,8 @@ func getValidArgsFunction( } // Drop the local prefix from completions if the path to complete doesn't -// start with it. We do this because the local filer returns paths with the -// local prefix. +// start with it. We do this because the local filer returns paths in the +// current folder with the local prefix (./). func shouldDropLocalPrefix(toComplete string, completion string) bool { return !strings.HasPrefix(toComplete, localPefix) && strings.HasPrefix(completion, localPefix) } From 0341e8b93e7d4c294383b8cfcdb790b536f5191d Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 29 Jul 2024 17:16:56 +0300 Subject: [PATCH 14/43] Cleanup --- cmd/fs/helpers.go | 11 +++-------- cmd/fs/helpers_test.go | 8 ++++---- cmd/root/auth.go | 1 - cmd/root/auth_options.go | 6 +++--- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 33cb2901d4..d733c90089 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -9,7 +9,6 @@ import ( "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/completer" "github.com/databricks/cli/libs/filer" - "github.com/fatih/color" "github.com/spf13/cobra" ) @@ -109,13 +108,13 @@ func getValidArgsFunction( // If the path is a dbfs path, we try to add the dbfs:/Volumes prefix suggestion if isDbfsPath && strings.HasPrefix(volumesPefix, toComplete) { - completions = append([]string{highlight(volumesPefix)}, completions...) + completions = append(completions, volumesPefix) } - // If the path is local, we try to prepend the dbfs:/ prefix suggestion + // If the path is local, we try to add the dbfs:/ prefix suggestion if !isDbfsPath && strings.HasPrefix(dbfsPrefix, toComplete) { - completions = append([]string{highlight(dbfsPrefix)}, completions...) + completions = append(completions, dbfsPrefix) } return completions, directive @@ -128,7 +127,3 @@ func getValidArgsFunction( func shouldDropLocalPrefix(toComplete string, completion string) bool { return !strings.HasPrefix(toComplete, localPefix) && strings.HasPrefix(completion, localPefix) } - -func highlight(c string) string { - return color.New(color.FgCyan, color.Bold).Sprintf(c) -} diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 439ba1faca..3aa80a261d 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -103,7 +103,7 @@ func TestGetValidArgsFunctionCompletion(t *testing.T) { completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") // dbfs:/Volumes is programmatically added to the completions - assert.Equal(t, []string{"dbfs:/Volumes", "dbfs:/dir", "dbfs:/file"}, completions) + assert.Equal(t, []string{"dbfs:/dir", "dbfs:/file", "dbfs:/Volumes"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -120,7 +120,7 @@ func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") - assert.Equal(t, []string{"dbfs:/Volumes", "dbfs:/dir"}, completions) + assert.Equal(t, []string{"dbfs:/dir", "dbfs:/Volumes"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -138,7 +138,7 @@ func TestGetValidArgsFunctionNoPath(t *testing.T) { completions, directive := validArgsFunction(cmd, []string{}, "d") // Suggest both dbfs and local paths at beginning of completion - assert.Equal(t, []string{"dbfs:/", "dFile1", "dFile2"}, completions) + assert.Equal(t, []string{"dFile1", "dFile2", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -155,7 +155,7 @@ func TestGetValidArgsFunctionLocalPath(t *testing.T) { validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) completions, directive := validArgsFunction(cmd, []string{}, "") - assert.Equal(t, []string{"dbfs:/", "dir", "file"}, completions) + assert.Equal(t, []string{"dir", "file", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } diff --git a/cmd/root/auth.go b/cmd/root/auth.go index 31af8e11fb..107679105b 100644 --- a/cmd/root/auth.go +++ b/cmd/root/auth.go @@ -152,7 +152,6 @@ func MustAccountClient(cmd *cobra.Command, args []string) error { // Helper function to create a workspace client or prompt once if the given configuration is not valid. func workspaceClientOrPrompt(ctx context.Context, cfg *config.Config, allowPrompt bool) (*databricks.WorkspaceClient, error) { w, err := databricks.NewWorkspaceClient((*databricks.Config)(cfg)) - if err == nil { err = w.Config.Authenticate(emptyHttpRequest(ctx)) } diff --git a/cmd/root/auth_options.go b/cmd/root/auth_options.go index d684973374..701900d4f6 100644 --- a/cmd/root/auth_options.go +++ b/cmd/root/auth_options.go @@ -6,7 +6,7 @@ import ( type skipPrompt int -var SkipPromptKey skipPrompt +var skipPromptKey skipPrompt // SkipPrompt allows to skip prompt for profile configuration in MustWorkspaceClient. // @@ -16,12 +16,12 @@ var SkipPromptKey skipPrompt // thus the Context object seems to be the only viable option for us to configure prompt behaviour based on // the context it's executed from. func SkipPrompt(ctx context.Context) context.Context { - return context.WithValue(ctx, SkipPromptKey, true) + return context.WithValue(ctx, skipPromptKey, true) } // shouldSkipPrompt returns whether or not [SkipPrompt] has been set on the specified context. func shouldSkipPrompt(ctx context.Context) bool { - skipPrompt, ok := ctx.Value(SkipPromptKey).(bool) + skipPrompt, ok := ctx.Value(skipPromptKey).(bool) return ok && skipPrompt } From a42f121b5699257d25607b0727312a5c1ee33968 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 2 Aug 2024 10:44:22 +0300 Subject: [PATCH 15/43] Support .\ local paths with Windows --- cmd/fs/helpers.go | 23 +++++++++++++++-------- cmd/fs/helpers_test.go | 18 ++++++++++++++++++ libs/completer/completer.go | 11 +++++++++-- 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index d733c90089..0a24096421 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -50,7 +50,6 @@ func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, er const dbfsPrefix string = "dbfs:/" const volumesPefix string = "dbfs:/Volumes" -const localPefix string = "./" func isDbfsPath(path string) bool { return strings.HasPrefix(path, dbfsPrefix) @@ -98,11 +97,8 @@ func getValidArgsFunction( // selectedPrefix without a trailing "/" prefix := dbfsPrefix[:len(dbfsPrefix)-1] completions[i] = fmt.Sprintf("%s%s", prefix, completion) - } else if shouldDropLocalPrefix(toComplete, completion) { - - completions[i] = completion[len(localPefix):] } else { - completions[i] = completion + completions[i] = handleLocalPrefix(toComplete, completion) } } @@ -123,7 +119,18 @@ func getValidArgsFunction( // Drop the local prefix from completions if the path to complete doesn't // start with it. We do this because the local filer returns paths in the -// current folder with the local prefix (./). -func shouldDropLocalPrefix(toComplete string, completion string) bool { - return !strings.HasPrefix(toComplete, localPefix) && strings.HasPrefix(completion, localPefix) +// current folder with the local prefix (./ (and .\ on windows)) +func handleLocalPrefix(toComplete string, completion string) string { + shouldDrop := shouldDropLocalPrefix(toComplete, completion, "./") || + shouldDropLocalPrefix(toComplete, completion, ".\\") + + if shouldDrop { + return completion[2:] + } + + return completion +} + +func shouldDropLocalPrefix(toComplete string, completion string, localPrefix string) bool { + return !strings.HasPrefix(toComplete, localPrefix) && strings.HasPrefix(completion, localPrefix) } diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 3aa80a261d..e4aa05b2ae 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -176,6 +176,24 @@ func TestGetValidArgsFunctionAbsoluteLocalPath(t *testing.T) { assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } +func TestGetValidArgsFunctionLocalWindowsPath(t *testing.T) { + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + + mockFilerForPath := testutil.GetMockFilerForPath(t, "", []fs.DirEntry{ + testutil.NewFakeDirEntry(".\\dFile1", false), + testutil.NewFakeDirEntry(".\\dFile2", false), + }) + + validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) + completions, directive := validArgsFunction(cmd, []string{}, "d") + + // Suggest both dbfs and local paths at beginning of completion + assert.Equal(t, []string{"dFile1", "dFile2", "dbfs:/"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { m, cmd := setupValidArgsFunctionTest(t) diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 534bf807b6..185aea0955 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "path" + "runtime" "strings" "github.com/databricks/cli/libs/filer" @@ -50,6 +51,12 @@ func fetchCompletions( return } + pathSeparator := "/" + + if runtime.GOOS == "windows" { + pathSeparator = "\\" + } + completions := []string{} for _, entry := range entries { if !c.onlyDirs || entry.IsDir() { @@ -58,8 +65,8 @@ func fetchCompletions( // Ensure the path and name have a "/" separating them. We don't use path // utilities to concatenate the path and name because we want to preserve // the formatting of whatever path the user has typed (e.g. //a/b///c) - if len(remotePath) > 0 && !strings.HasSuffix(remotePath, "/") { - separator = "/" + if len(remotePath) > 0 && !strings.HasSuffix(remotePath, pathSeparator) { + separator = pathSeparator } completion := fmt.Sprintf("%s%s%s", remotePath, separator, entry.Name()) From 6d10776159af82cbce11469599b2076167324ae2 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 2 Aug 2024 11:54:36 +0300 Subject: [PATCH 16/43] Handle local Windows paths --- cmd/fs/helpers.go | 31 +++++++++++++++++++++---- cmd/fs/helpers_test.go | 38 +++++++++++++++++++++++++++++++ libs/completer/completer.go | 36 ++++++++--------------------- libs/completer/completer_test.go | 39 ++++++++++++-------------------- 4 files changed, 87 insertions(+), 57 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 0a24096421..bd14a5a9aa 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -89,23 +89,24 @@ func getValidArgsFunction( return nil, cobra.ShellCompDirectiveNoFileComp } - completions, directive := completer.CompleteRemotePath(path) + completions, dirPath, directive := completer.CompletePath(path) + + for i := range completions { + completions[i] = prependDirPath(dirPath, completions[i], !isDbfsPath) - for i, completion := range completions { if isDbfsPath { // The completions will start with a "/", so we'll prefix them with the // selectedPrefix without a trailing "/" prefix := dbfsPrefix[:len(dbfsPrefix)-1] - completions[i] = fmt.Sprintf("%s%s", prefix, completion) + completions[i] = fmt.Sprintf("%s%s", prefix, completions[i]) } else { - completions[i] = handleLocalPrefix(toComplete, completion) + completions[i] = handleLocalPrefix(toComplete, completions[i]) } } // If the path is a dbfs path, we try to add the dbfs:/Volumes prefix suggestion if isDbfsPath && strings.HasPrefix(volumesPefix, toComplete) { completions = append(completions, volumesPefix) - } // If the path is local, we try to add the dbfs:/ prefix suggestion @@ -117,6 +118,26 @@ func getValidArgsFunction( } } +// Prepend directory path to completion name: +// - Don't add a separator if the directory path is empty +// - Don't add a separator if the dir path already end with a separator +// - Use \ as separator for local Windows paths +// Note that we don't use path utilities to concatenate the path and name +// because we want to preserve the formatting of whatever path the user has +// typed (e.g. //a/b///c) +func prependDirPath(dirPath string, completion string, isLocalPath bool) string { + pathSeparator := "/" + if isLocalPath && runtime.GOOS == "windows" { + pathSeparator = "\\" + } + + separator := "" + if len(dirPath) > 0 && !strings.HasSuffix(dirPath, pathSeparator) { + separator = pathSeparator + } + return fmt.Sprintf("%s%s%s", dirPath, separator, completion) +} + // Drop the local prefix from completions if the path to complete doesn't // start with it. We do this because the local filer returns paths in the // current folder with the local prefix (./ (and .\ on windows)) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index e4aa05b2ae..5ebb3ff2d5 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -194,6 +194,44 @@ func TestGetValidArgsFunctionLocalWindowsPath(t *testing.T) { assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } +func TestGetValidArgsFunctionAddsSeparator(t *testing.T) { + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + + mockFilerForPath := testutil.GetMockFilerForPath(t, "foo", []fs.DirEntry{ + testutil.NewFakeDirEntry("nested_dir", true), + }) + + validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) + + completions, directive := validArgsFunction(cmd, []string{}, "foo") + + assert.Equal(t, []string{"foo/nested_dir"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + +func TestGetValidArgsFunctionAddsWindowsSeparator(t *testing.T) { + if runtime.GOOS != "windows" { + t.SkipNow() + } + + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + + mockFilerForPath := testutil.GetMockFilerForPath(t, "foo", []fs.DirEntry{ + testutil.NewFakeDirEntry("nested_dir", true), + }) + + validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) + + completions, directive := validArgsFunction(cmd, []string{}, "foo") + + assert.Equal(t, []string{"foo\\nested_dir"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { m, cmd := setupValidArgsFunctionTest(t) diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 185aea0955..c651ec9bd8 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -2,10 +2,7 @@ package completer import ( "context" - "fmt" "path" - "runtime" - "strings" "github.com/databricks/cli/libs/filer" "github.com/spf13/cobra" @@ -22,55 +19,40 @@ func NewCompleter(ctx context.Context, filer filer.Filer, onlyDirs bool) *comple return &completer{ctx: ctx, filer: filer, onlyDirs: onlyDirs} } -func (c *completer) CompleteRemotePath(remotePath string) ([]string, cobra.ShellCompDirective) { +func (c *completer) CompletePath(p string) ([]string, string, cobra.ShellCompDirective) { // If the user is TAB-ing their way through a path, the path in `toComplete` // is valid and we should list nested directories. // If the path in `toComplete` is incomplete, however, // then we should list adjacent directories. - nested := fetchCompletions(c, remotePath) + dirPath := p + nested := fetchCompletions(c, dirPath) completions := <-nested - if completions == nil { - adjacent := fetchCompletions(c, path.Dir(remotePath)) + dirPath = path.Dir(p) + adjacent := fetchCompletions(c, dirPath) completions = <-adjacent } - return completions, cobra.ShellCompDirectiveNoSpace + return completions, dirPath, cobra.ShellCompDirectiveNoSpace } func fetchCompletions( c *completer, - remotePath string, + path string, ) <-chan []string { ch := make(chan []string, 1) go func() { defer close(ch) - entries, err := c.filer.ReadDir(c.ctx, remotePath) + entries, err := c.filer.ReadDir(c.ctx, path) if err != nil { return } - pathSeparator := "/" - - if runtime.GOOS == "windows" { - pathSeparator = "\\" - } - completions := []string{} for _, entry := range entries { if !c.onlyDirs || entry.IsDir() { - separator := "" - - // Ensure the path and name have a "/" separating them. We don't use path - // utilities to concatenate the path and name because we want to preserve - // the formatting of whatever path the user has typed (e.g. //a/b///c) - if len(remotePath) > 0 && !strings.HasSuffix(remotePath, pathSeparator) { - separator = pathSeparator - } - - completion := fmt.Sprintf("%s%s%s", remotePath, separator, entry.Name()) - completions = append(completions, completion) + completions = append(completions, entry.Name()) } } diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index b0ac66a889..d4c7c2b6ba 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -32,9 +32,10 @@ func TestFilerCompleterReturnsNestedDirs(t *testing.T) { completer := NewCompleter(ctx, mockFiler, true) - completions, directive := completer.CompleteRemotePath("/") + completions, dirPath, directive := completer.CompletePath("/") - assert.Equal(t, []string{"/dir"}, completions) + assert.Equal(t, []string{"dir"}, completions) + assert.Equal(t, "/", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -50,9 +51,10 @@ func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { }, nil) completer := NewCompleter(ctx, mockFiler, true) - completions, directive := completer.CompleteRemotePath("/wrong_path") + completions, dirPath, directive := completer.CompletePath("/wrong_path") - assert.Equal(t, []string{"/adjacent"}, completions) + assert.Equal(t, []string{"adjacent"}, completions) + assert.Equal(t, "/", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) mockFiler.AssertExpectations(t) @@ -65,9 +67,10 @@ func TestFilerCompleterReadDirError(t *testing.T) { mockFiler.On("ReadDir", mock.AnythingOfType("*context.valueCtx"), mock.Anything).Return(nil, errors.New("error")) completer := NewCompleter(ctx, mockFiler, true) - completions, directive := completer.CompleteRemotePath("/dir") + completions, dirPath, directive := completer.CompletePath("/dir") assert.Nil(t, completions) + assert.Equal(t, "/", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) mockFiler.AssertExpectations(t) @@ -85,9 +88,10 @@ func TestFilerCompleterReturnsFileAndDir(t *testing.T) { // onlyDirs=false so we should get both files and directories completer := NewCompleter(ctx, mockFiler, false) - completions, directive := completer.CompleteRemotePath("/") + completions, dirPath, directive := completer.CompletePath("/") - assert.Equal(t, []string{"/dir", "/file"}, completions) + assert.Equal(t, []string{"dir", "file"}, completions) + assert.Equal(t, "/", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -101,24 +105,9 @@ func TestFilerCompleterRetainsFormatting(t *testing.T) { completer := NewCompleter(ctx, mockFiler, true) - completions, directive := completer.CompleteRemotePath("//dir//") + completions, dirPath, directive := completer.CompletePath("//dir//") - assert.Equal(t, []string{"//dir//nested_dir"}, completions) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) -} - -func TestFilerCompleterAddsSeparator(t *testing.T) { - ctx := setup(t) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "/dir", []fs.DirEntry{ - testutil.NewFakeDirEntry("nested_dir", true), - }) - mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/dir") - - completer := NewCompleter(ctx, mockFiler, true) - - completions, directive := completer.CompleteRemotePath("/dir") - - assert.Equal(t, []string{"/dir/nested_dir"}, completions) + assert.Equal(t, []string{"nested_dir"}, completions) + assert.Equal(t, "//dir//", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } From 7c010cc9e5904c227029958d98e778731f15126b Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 2 Aug 2024 12:12:24 +0300 Subject: [PATCH 17/43] Support both separators on Windows --- cmd/fs/helpers.go | 26 ++++++++++++++++---------- cmd/fs/helpers_test.go | 21 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index bd14a5a9aa..3b7eaf161e 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -95,8 +95,8 @@ func getValidArgsFunction( completions[i] = prependDirPath(dirPath, completions[i], !isDbfsPath) if isDbfsPath { - // The completions will start with a "/", so we'll prefix them with the - // selectedPrefix without a trailing "/" + // The dirPath will start with a "/", so we'll prefix the completions with + // the selectedPrefix without a trailing "/" prefix := dbfsPrefix[:len(dbfsPrefix)-1] completions[i] = fmt.Sprintf("%s%s", prefix, completions[i]) } else { @@ -120,21 +120,27 @@ func getValidArgsFunction( // Prepend directory path to completion name: // - Don't add a separator if the directory path is empty -// - Don't add a separator if the dir path already end with a separator -// - Use \ as separator for local Windows paths +// - Don't add a separator if the dir path already ends with a separator +// - Support \ as separator for local Windows paths // Note that we don't use path utilities to concatenate the path and name // because we want to preserve the formatting of whatever path the user has // typed (e.g. //a/b///c) func prependDirPath(dirPath string, completion string, isLocalPath bool) string { - pathSeparator := "/" - if isLocalPath && runtime.GOOS == "windows" { - pathSeparator = "\\" - } + defaultSeparator := "/" + windowsSeparator := "\\" + + isLocalWindowsPath := isLocalPath && runtime.GOOS == "windows" separator := "" - if len(dirPath) > 0 && !strings.HasSuffix(dirPath, pathSeparator) { - separator = pathSeparator + if isLocalWindowsPath && len(dirPath) > 0 && + !strings.HasSuffix(dirPath, defaultSeparator) && + !strings.HasSuffix(dirPath, windowsSeparator) { + separator = windowsSeparator + } else if !isLocalWindowsPath && len(dirPath) > 0 && + !strings.HasSuffix(dirPath, defaultSeparator) { + separator = defaultSeparator } + return fmt.Sprintf("%s%s%s", dirPath, separator, completion) } diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 5ebb3ff2d5..505d6d08a3 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -232,6 +232,27 @@ func TestGetValidArgsFunctionAddsWindowsSeparator(t *testing.T) { assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } +func TestGetValidArgsFunctionAddsDefaultSeparatorOnWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.SkipNow() + } + + m, cmd := setupValidArgsFunctionTest(t) + + mockCurrentUserApi(m, nil, nil) + + mockFilerForPath := testutil.GetMockFilerForPath(t, "foo/", []fs.DirEntry{ + testutil.NewFakeDirEntry("nested_dir", true), + }) + + validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) + + completions, directive := validArgsFunction(cmd, []string{}, "foo/") + + assert.Equal(t, []string{"foo/nested_dir"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { m, cmd := setupValidArgsFunctionTest(t) From e0d2062248d3ba8e7f96d671a2f13a2332b7383d Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 2 Aug 2024 12:17:17 +0300 Subject: [PATCH 18/43] Fix test --- cmd/fs/helpers_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 505d6d08a3..87f6472509 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -3,6 +3,7 @@ package fs import ( "context" "errors" + "fmt" "io/fs" "runtime" "testing" @@ -207,7 +208,12 @@ func TestGetValidArgsFunctionAddsSeparator(t *testing.T) { completions, directive := validArgsFunction(cmd, []string{}, "foo") - assert.Equal(t, []string{"foo/nested_dir"}, completions) + separator := "/" + if runtime.GOOS != "windows" { + separator = "\\" + } + + assert.Equal(t, []string{fmt.Sprintf("%s%s%s", "foo", separator, "nested_dir")}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } From d990fe2beace214ac7bdebcd3b5ac1ee01078e94 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 2 Aug 2024 12:17:38 +0300 Subject: [PATCH 19/43] Fix test 2 --- cmd/fs/helpers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 87f6472509..8196dcbc90 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -209,7 +209,7 @@ func TestGetValidArgsFunctionAddsSeparator(t *testing.T) { completions, directive := validArgsFunction(cmd, []string{}, "foo") separator := "/" - if runtime.GOOS != "windows" { + if runtime.GOOS == "windows" { separator = "\\" } From 1868791be526881aaea6e951e298bdbe1d4fa3cb Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 5 Aug 2024 16:04:44 +0300 Subject: [PATCH 20/43] PR feedback --- cmd/fs/helpers.go | 6 +++--- libs/completer/completer.go | 10 +++++++--- libs/completer/completer_test.go | 10 +++++----- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 3b7eaf161e..9154aabe30 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -75,15 +75,13 @@ func getValidArgsFunction( return nil, cobra.ShellCompDirectiveError } - isDbfsPath := isDbfsPath(toComplete) - wsc := root.WorkspaceClient(cmd.Context()) _, err = wsc.CurrentUser.Me(cmd.Context()) if err != nil { return nil, cobra.ShellCompDirectiveError } - completer := completer.NewCompleter(cmd.Context(), filer, onlyDirs) + completer := completer.New(cmd.Context(), filer, onlyDirs) if len(args) >= pathArgCount { return nil, cobra.ShellCompDirectiveNoFileComp @@ -91,6 +89,8 @@ func getValidArgsFunction( completions, dirPath, directive := completer.CompletePath(path) + isDbfsPath := isDbfsPath(toComplete) + for i := range completions { completions[i] = prependDirPath(dirPath, completions[i], !isDbfsPath) diff --git a/libs/completer/completer.go b/libs/completer/completer.go index c651ec9bd8..092bf053d5 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -15,7 +15,7 @@ type completer struct { } // General completer that takes a Filer to complete remote paths when TAB-ing through a path. -func NewCompleter(ctx context.Context, filer filer.Filer, onlyDirs bool) *completer { +func New(ctx context.Context, filer filer.Filer, onlyDirs bool) *completer { return &completer{ctx: ctx, filer: filer, onlyDirs: onlyDirs} } @@ -36,6 +36,8 @@ func (c *completer) CompletePath(p string) ([]string, string, cobra.ShellCompDir return completions, dirPath, cobra.ShellCompDirectiveNoSpace } +// List files and directories under the specified path. +// Returns a channel such that we can list multiple paths in parallel. func fetchCompletions( c *completer, path string, @@ -51,9 +53,11 @@ func fetchCompletions( completions := []string{} for _, entry := range entries { - if !c.onlyDirs || entry.IsDir() { - completions = append(completions, entry.Name()) + if c.onlyDirs && !entry.IsDir() { + continue } + + completions = append(completions, entry.Name()) } ch <- completions diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index d4c7c2b6ba..7618af9213 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -30,7 +30,7 @@ func TestFilerCompleterReturnsNestedDirs(t *testing.T) { }) mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/") - completer := NewCompleter(ctx, mockFiler, true) + completer := New(ctx, mockFiler, true) completions, dirPath, directive := completer.CompletePath("/") @@ -50,7 +50,7 @@ func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { testutil.NewFakeDirEntry("adjacent", true), }, nil) - completer := NewCompleter(ctx, mockFiler, true) + completer := New(ctx, mockFiler, true) completions, dirPath, directive := completer.CompletePath("/wrong_path") assert.Equal(t, []string{"adjacent"}, completions) @@ -66,7 +66,7 @@ func TestFilerCompleterReadDirError(t *testing.T) { mockFiler := mockfiler.NewMockFiler(t) mockFiler.On("ReadDir", mock.AnythingOfType("*context.valueCtx"), mock.Anything).Return(nil, errors.New("error")) - completer := NewCompleter(ctx, mockFiler, true) + completer := New(ctx, mockFiler, true) completions, dirPath, directive := completer.CompletePath("/dir") assert.Nil(t, completions) @@ -86,7 +86,7 @@ func TestFilerCompleterReturnsFileAndDir(t *testing.T) { mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/") // onlyDirs=false so we should get both files and directories - completer := NewCompleter(ctx, mockFiler, false) + completer := New(ctx, mockFiler, false) completions, dirPath, directive := completer.CompletePath("/") @@ -103,7 +103,7 @@ func TestFilerCompleterRetainsFormatting(t *testing.T) { }) mockFiler, _, _ := mockFilerForPath(ctx, "dbfs://dir") - completer := NewCompleter(ctx, mockFiler, true) + completer := New(ctx, mockFiler, true) completions, dirPath, directive := completer.CompletePath("//dir//") From c4a406c14ab4133cd51f74d2f62d361168d4f5a1 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 5 Aug 2024 16:32:25 +0300 Subject: [PATCH 21/43] Remove goroutine --- libs/completer/completer.go | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 092bf053d5..99960bcca5 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -25,12 +25,10 @@ func (c *completer) CompletePath(p string) ([]string, string, cobra.ShellCompDir // If the path in `toComplete` is incomplete, however, // then we should list adjacent directories. dirPath := p - nested := fetchCompletions(c, dirPath) - completions := <-nested + completions := fetchCompletions(c, dirPath) if completions == nil { dirPath = path.Dir(p) - adjacent := fetchCompletions(c, dirPath) - completions = <-adjacent + completions = fetchCompletions(c, dirPath) } return completions, dirPath, cobra.ShellCompDirectiveNoSpace @@ -41,27 +39,20 @@ func (c *completer) CompletePath(p string) ([]string, string, cobra.ShellCompDir func fetchCompletions( c *completer, path string, -) <-chan []string { - ch := make(chan []string, 1) - go func() { - defer close(ch) - - entries, err := c.filer.ReadDir(c.ctx, path) - if err != nil { - return - } - - completions := []string{} - for _, entry := range entries { - if c.onlyDirs && !entry.IsDir() { - continue - } +) []string { + entries, err := c.filer.ReadDir(c.ctx, path) + if err != nil { + return nil + } - completions = append(completions, entry.Name()) + completions := []string{} + for _, entry := range entries { + if c.onlyDirs && !entry.IsDir() { + continue } - ch <- completions - }() + completions = append(completions, entry.Name()) + } - return ch + return completions } From a909d1f2a80015533ebc50669efedcb77301997b Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 5 Aug 2024 16:47:36 +0300 Subject: [PATCH 22/43] Doc comments --- libs/completer/completer.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 99960bcca5..87774b7f02 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -9,12 +9,16 @@ import ( ) type completer struct { - ctx context.Context - filer filer.Filer + ctx context.Context + + // The filer to use for completing remote or local paths. + filer filer.Filer + + // CompletePath will only return directories when onlyDirs is true. onlyDirs bool } -// General completer that takes a Filer to complete remote paths when TAB-ing through a path. +// General completer that takes a filer to complete remote paths when TAB-ing through a path. func New(ctx context.Context, filer filer.Filer, onlyDirs bool) *completer { return &completer{ctx: ctx, filer: filer, onlyDirs: onlyDirs} } From 6091e0df11d6544d076bcbb95054ea6a0e9e1de7 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 5 Aug 2024 17:54:45 +0300 Subject: [PATCH 23/43] Use path and filepath for joining paths --- cmd/fs/helpers.go | 61 +++++++----------------------------------- cmd/fs/helpers_test.go | 4 +++ 2 files changed, 13 insertions(+), 52 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 9154aabe30..a2ab84ee7e 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -3,6 +3,8 @@ package fs import ( "context" "fmt" + "path" + "path/filepath" "runtime" "strings" @@ -70,7 +72,7 @@ func getValidArgsFunction( return nil, cobra.ShellCompDirectiveError } - filer, path, err := filerForPathFunc(cmd.Context(), toComplete) + filer, toCompletePath, err := filerForPathFunc(cmd.Context(), toComplete) if err != nil { return nil, cobra.ShellCompDirectiveError } @@ -87,20 +89,19 @@ func getValidArgsFunction( return nil, cobra.ShellCompDirectiveNoFileComp } - completions, dirPath, directive := completer.CompletePath(path) + completions, dirPath, directive := completer.CompletePath(toCompletePath) isDbfsPath := isDbfsPath(toComplete) for i := range completions { - completions[i] = prependDirPath(dirPath, completions[i], !isDbfsPath) + completions[i] = filepath.Join(dirPath, completions[i]) if isDbfsPath { - // The dirPath will start with a "/", so we'll prefix the completions with - // the selectedPrefix without a trailing "/" - prefix := dbfsPrefix[:len(dbfsPrefix)-1] - completions[i] = fmt.Sprintf("%s%s", prefix, completions[i]) + // Add dbfs:/ prefix to completions + completions[i] = path.Join(dbfsPrefix, completions[i]) } else { - completions[i] = handleLocalPrefix(toComplete, completions[i]) + // Clean up ./ (and .\ on Windows) from local completions + completions[i] = filepath.Clean(completions[i]) } } @@ -117,47 +118,3 @@ func getValidArgsFunction( return completions, directive } } - -// Prepend directory path to completion name: -// - Don't add a separator if the directory path is empty -// - Don't add a separator if the dir path already ends with a separator -// - Support \ as separator for local Windows paths -// Note that we don't use path utilities to concatenate the path and name -// because we want to preserve the formatting of whatever path the user has -// typed (e.g. //a/b///c) -func prependDirPath(dirPath string, completion string, isLocalPath bool) string { - defaultSeparator := "/" - windowsSeparator := "\\" - - isLocalWindowsPath := isLocalPath && runtime.GOOS == "windows" - - separator := "" - if isLocalWindowsPath && len(dirPath) > 0 && - !strings.HasSuffix(dirPath, defaultSeparator) && - !strings.HasSuffix(dirPath, windowsSeparator) { - separator = windowsSeparator - } else if !isLocalWindowsPath && len(dirPath) > 0 && - !strings.HasSuffix(dirPath, defaultSeparator) { - separator = defaultSeparator - } - - return fmt.Sprintf("%s%s%s", dirPath, separator, completion) -} - -// Drop the local prefix from completions if the path to complete doesn't -// start with it. We do this because the local filer returns paths in the -// current folder with the local prefix (./ (and .\ on windows)) -func handleLocalPrefix(toComplete string, completion string) string { - shouldDrop := shouldDropLocalPrefix(toComplete, completion, "./") || - shouldDropLocalPrefix(toComplete, completion, ".\\") - - if shouldDrop { - return completion[2:] - } - - return completion -} - -func shouldDropLocalPrefix(toComplete string, completion string, localPrefix string) bool { - return !strings.HasPrefix(toComplete, localPrefix) && strings.HasPrefix(completion, localPrefix) -} diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 8196dcbc90..5a8c2f7999 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -178,6 +178,10 @@ func TestGetValidArgsFunctionAbsoluteLocalPath(t *testing.T) { } func TestGetValidArgsFunctionLocalWindowsPath(t *testing.T) { + if runtime.GOOS != "windows" { + t.SkipNow() + } + m, cmd := setupValidArgsFunctionTest(t) mockCurrentUserApi(m, nil, nil) From 1591896f380058760401ccb5c9dc160b6569ffe0 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Mon, 5 Aug 2024 18:46:30 +0300 Subject: [PATCH 24/43] Use struct for Validate --- cmd/fs/cat.go | 3 +- cmd/fs/cp.go | 4 +- cmd/fs/helpers.go | 103 ++++++++++++++++++++++------------------- cmd/fs/helpers_test.go | 80 +++++++++++++++++++++++--------- cmd/fs/ls.go | 4 +- cmd/fs/mkdir.go | 4 +- cmd/fs/rm.go | 3 +- 7 files changed, 127 insertions(+), 74 deletions(-) diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index 30056a3e27..60c6678eeb 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -30,7 +30,8 @@ func newCatCommand() *cobra.Command { return cmdio.Render(ctx, r) } - cmd.ValidArgsFunction = getValidArgsFunction(1, false, filerForPath, root.MustWorkspaceClient) + v := NewValidArgs() + cmd.ValidArgsFunction = v.Validate return cmd } diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index 6d0395f402..a533184584 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -200,8 +200,10 @@ func newCpCommand() *cobra.Command { return c.cpFileToFile(sourcePath, targetPath) } + v := NewValidArgs() // The copy command has two paths that can be completed (SOURCE_PATH & TARGET_PATH) - cmd.ValidArgsFunction = getValidArgsFunction(2, false, filerForPath, root.MustWorkspaceClient) + v.pathArgCount = 2 + cmd.ValidArgsFunction = v.Validate return cmd } diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index a2ab84ee7e..414185fb5b 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -57,64 +57,73 @@ func isDbfsPath(path string) bool { return strings.HasPrefix(path, dbfsPrefix) } -func getValidArgsFunction( - pathArgCount int, - onlyDirs bool, - filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error), - mustWorkspaceClientFunc func(cmd *cobra.Command, args []string) error, -) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - cmd.SetContext(root.SkipPrompt(cmd.Context())) - - err := mustWorkspaceClientFunc(cmd, args) - - if err != nil { - return nil, cobra.ShellCompDirectiveError - } +type ValidArgs struct { + mustWorkspaceClientFunc func(cmd *cobra.Command, args []string) error + filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error) + pathArgCount int + onlyDirs bool +} - filer, toCompletePath, err := filerForPathFunc(cmd.Context(), toComplete) - if err != nil { - return nil, cobra.ShellCompDirectiveError - } +func NewValidArgs() *ValidArgs { + return &ValidArgs{ + mustWorkspaceClientFunc: root.MustWorkspaceClient, + filerForPathFunc: filerForPath, + pathArgCount: 1, + onlyDirs: false, + } +} - wsc := root.WorkspaceClient(cmd.Context()) - _, err = wsc.CurrentUser.Me(cmd.Context()) - if err != nil { - return nil, cobra.ShellCompDirectiveError - } +func (v *ValidArgs) Validate(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + cmd.SetContext(root.SkipPrompt(cmd.Context())) - completer := completer.New(cmd.Context(), filer, onlyDirs) + err := v.mustWorkspaceClientFunc(cmd, args) - if len(args) >= pathArgCount { - return nil, cobra.ShellCompDirectiveNoFileComp - } + if err != nil { + return nil, cobra.ShellCompDirectiveError + } - completions, dirPath, directive := completer.CompletePath(toCompletePath) + filer, toCompletePath, err := v.filerForPathFunc(cmd.Context(), toComplete) + if err != nil { + return nil, cobra.ShellCompDirectiveError + } - isDbfsPath := isDbfsPath(toComplete) + wsc := root.WorkspaceClient(cmd.Context()) + _, err = wsc.CurrentUser.Me(cmd.Context()) + if err != nil { + return nil, cobra.ShellCompDirectiveError + } - for i := range completions { - completions[i] = filepath.Join(dirPath, completions[i]) + completer := completer.New(cmd.Context(), filer, v.onlyDirs) - if isDbfsPath { - // Add dbfs:/ prefix to completions - completions[i] = path.Join(dbfsPrefix, completions[i]) - } else { - // Clean up ./ (and .\ on Windows) from local completions - completions[i] = filepath.Clean(completions[i]) - } - } + if len(args) >= v.pathArgCount { + return nil, cobra.ShellCompDirectiveNoFileComp + } - // If the path is a dbfs path, we try to add the dbfs:/Volumes prefix suggestion - if isDbfsPath && strings.HasPrefix(volumesPefix, toComplete) { - completions = append(completions, volumesPefix) - } + completions, dirPath, directive := completer.CompletePath(toCompletePath) + + isDbfsPath := isDbfsPath(toComplete) + + for i := range completions { + completions[i] = filepath.Join(dirPath, completions[i]) - // If the path is local, we try to add the dbfs:/ prefix suggestion - if !isDbfsPath && strings.HasPrefix(dbfsPrefix, toComplete) { - completions = append(completions, dbfsPrefix) + if isDbfsPath { + // Add dbfs:/ prefix to completions + completions[i] = path.Join(dbfsPrefix, completions[i]) + } else { + // Clean up ./ (and .\ on Windows) from local completions + completions[i] = filepath.Clean(completions[i]) } + } + + // If the path is a dbfs path, we try to add the dbfs:/Volumes prefix suggestion + if isDbfsPath && strings.HasPrefix(volumesPefix, toComplete) { + completions = append(completions, volumesPefix) + } - return completions, directive + // If the path is local, we try to add the dbfs:/ prefix suggestion + if !isDbfsPath && strings.HasPrefix(dbfsPrefix, toComplete) { + completions = append(completions, dbfsPrefix) } + + return completions, directive } diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 5a8c2f7999..f80c745cad 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -100,8 +100,11 @@ func TestGetValidArgsFunctionCompletion(t *testing.T) { testutil.NewFakeDirEntry("file", false), }) - validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) - completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") + v := NewValidArgs() + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + + completions, directive := v.Validate(cmd, []string{}, "dbfs:/") // dbfs:/Volumes is programmatically added to the completions assert.Equal(t, []string{"dbfs:/dir", "dbfs:/file", "dbfs:/Volumes"}, completions) @@ -118,8 +121,12 @@ func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { testutil.NewFakeDirEntry("file", false), }) - validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) - completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") + v := NewValidArgs() + v.onlyDirs = true + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + + completions, directive := v.Validate(cmd, []string{}, "dbfs:/") assert.Equal(t, []string{"dbfs:/dir", "dbfs:/Volumes"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -135,8 +142,11 @@ func TestGetValidArgsFunctionNoPath(t *testing.T) { testutil.NewFakeDirEntry("dFile2", false), }) - validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) - completions, directive := validArgsFunction(cmd, []string{}, "d") + v := NewValidArgs() + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + + completions, directive := v.Validate(cmd, []string{}, "d") // Suggest both dbfs and local paths at beginning of completion assert.Equal(t, []string{"dFile1", "dFile2", "dbfs:/"}, completions) @@ -153,8 +163,11 @@ func TestGetValidArgsFunctionLocalPath(t *testing.T) { testutil.NewFakeDirEntry("file", false), }) - validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) - completions, directive := validArgsFunction(cmd, []string{}, "") + v := NewValidArgs() + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + + completions, directive := v.Validate(cmd, []string{}, "") assert.Equal(t, []string{"dir", "file", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -170,8 +183,11 @@ func TestGetValidArgsFunctionAbsoluteLocalPath(t *testing.T) { testutil.NewFakeDirEntry("file", false), }) - validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) - completions, directive := validArgsFunction(cmd, []string{}, "/") + v := NewValidArgs() + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + + completions, directive := v.Validate(cmd, []string{}, "/") assert.Equal(t, []string{"dir", "file"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -191,8 +207,11 @@ func TestGetValidArgsFunctionLocalWindowsPath(t *testing.T) { testutil.NewFakeDirEntry(".\\dFile2", false), }) - validArgsFunction := getValidArgsFunction(1, false, mockFilerForPath, mockMustWorkspaceClientFunc) - completions, directive := validArgsFunction(cmd, []string{}, "d") + v := NewValidArgs() + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + + completions, directive := v.Validate(cmd, []string{}, "d") // Suggest both dbfs and local paths at beginning of completion assert.Equal(t, []string{"dFile1", "dFile2", "dbfs:/"}, completions) @@ -208,9 +227,12 @@ func TestGetValidArgsFunctionAddsSeparator(t *testing.T) { testutil.NewFakeDirEntry("nested_dir", true), }) - validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) + v := NewValidArgs() + v.onlyDirs = true + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - completions, directive := validArgsFunction(cmd, []string{}, "foo") + completions, directive := v.Validate(cmd, []string{}, "foo") separator := "/" if runtime.GOOS == "windows" { @@ -234,9 +256,12 @@ func TestGetValidArgsFunctionAddsWindowsSeparator(t *testing.T) { testutil.NewFakeDirEntry("nested_dir", true), }) - validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) + v := NewValidArgs() + v.onlyDirs = true + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - completions, directive := validArgsFunction(cmd, []string{}, "foo") + completions, directive := v.Validate(cmd, []string{}, "foo") assert.Equal(t, []string{"foo\\nested_dir"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -255,9 +280,12 @@ func TestGetValidArgsFunctionAddsDefaultSeparatorOnWindows(t *testing.T) { testutil.NewFakeDirEntry("nested_dir", true), }) - validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) + v := NewValidArgs() + v.onlyDirs = true + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - completions, directive := validArgsFunction(cmd, []string{}, "foo/") + completions, directive := v.Validate(cmd, []string{}, "foo/") assert.Equal(t, []string{"foo/nested_dir"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -269,8 +297,12 @@ func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { mockCurrentUserApi(m, nil, errors.New("Current User Error")) mockFilerForPath := testutil.GetMockFilerForPath(t, "/", nil) - validArgsFunction := getValidArgsFunction(1, true, mockFilerForPath, mockMustWorkspaceClientFunc) - completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") + v := NewValidArgs() + v.onlyDirs = true + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + + completions, directive := v.Validate(cmd, []string{}, "dbfs:/") assert.Nil(t, completions) assert.Equal(t, cobra.ShellCompDirectiveError, directive) @@ -282,9 +314,13 @@ func TestGetValidArgsFunctionNotCompletedArgument(t *testing.T) { mockCurrentUserApi(m, nil, nil) mockFilerForPath := testutil.GetMockFilerForPath(t, "/", nil) + v := NewValidArgs() // 0 here means we don't complete any arguments - validArgsFunction := getValidArgsFunction(0, true, mockFilerForPath, mockMustWorkspaceClientFunc) - completions, directive := validArgsFunction(cmd, []string{}, "dbfs:/") + v.pathArgCount = 0 + v.filerForPathFunc = mockFilerForPath + v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + + completions, directive := v.Validate(cmd, []string{}, "dbfs:/") assert.Nil(t, completions) assert.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive) diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 9809aac80f..3cbaa1c1aa 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -89,7 +89,9 @@ func newLsCommand() *cobra.Command { `)) } - cmd.ValidArgsFunction = getValidArgsFunction(1, true, filerForPath, root.MustWorkspaceClient) + v := NewValidArgs() + v.onlyDirs = true + cmd.ValidArgsFunction = v.Validate return cmd } diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index 6ac071f98a..ade58a01e8 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -28,7 +28,9 @@ func newMkdirCommand() *cobra.Command { return f.Mkdir(ctx, path) } - cmd.ValidArgsFunction = getValidArgsFunction(1, true, filerForPath, root.MustWorkspaceClient) + v := NewValidArgs() + v.onlyDirs = true + cmd.ValidArgsFunction = v.Validate return cmd } diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index 901207f9b8..80e4cfced1 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -32,7 +32,8 @@ func newRmCommand() *cobra.Command { return f.Delete(ctx, path) } - cmd.ValidArgsFunction = getValidArgsFunction(1, false, filerForPath, root.MustWorkspaceClient) + v := NewValidArgs() + cmd.ValidArgsFunction = v.Validate return cmd } From 84229be1f03bcec65b355f1e2302a95bc4bdc132 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 08:46:55 +0300 Subject: [PATCH 25/43] Lowercase validArgs --- cmd/fs/cat.go | 2 +- cmd/fs/cp.go | 2 +- cmd/fs/helpers.go | 8 ++++---- cmd/fs/helpers_test.go | 22 +++++++++++----------- cmd/fs/ls.go | 2 +- cmd/fs/mkdir.go | 2 +- cmd/fs/rm.go | 2 +- 7 files changed, 20 insertions(+), 20 deletions(-) diff --git a/cmd/fs/cat.go b/cmd/fs/cat.go index 60c6678eeb..28df80d70d 100644 --- a/cmd/fs/cat.go +++ b/cmd/fs/cat.go @@ -30,7 +30,7 @@ func newCatCommand() *cobra.Command { return cmdio.Render(ctx, r) } - v := NewValidArgs() + v := newValidArgs() cmd.ValidArgsFunction = v.Validate return cmd diff --git a/cmd/fs/cp.go b/cmd/fs/cp.go index a533184584..6fb3e5e6f1 100644 --- a/cmd/fs/cp.go +++ b/cmd/fs/cp.go @@ -200,7 +200,7 @@ func newCpCommand() *cobra.Command { return c.cpFileToFile(sourcePath, targetPath) } - v := NewValidArgs() + v := newValidArgs() // 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/helpers.go b/cmd/fs/helpers.go index 414185fb5b..adedaebd45 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -57,15 +57,15 @@ func isDbfsPath(path string) bool { return strings.HasPrefix(path, dbfsPrefix) } -type ValidArgs struct { +type validArgs struct { mustWorkspaceClientFunc func(cmd *cobra.Command, args []string) error filerForPathFunc func(ctx context.Context, fullPath string) (filer.Filer, string, error) pathArgCount int onlyDirs bool } -func NewValidArgs() *ValidArgs { - return &ValidArgs{ +func newValidArgs() *validArgs { + return &validArgs{ mustWorkspaceClientFunc: root.MustWorkspaceClient, filerForPathFunc: filerForPath, pathArgCount: 1, @@ -73,7 +73,7 @@ func NewValidArgs() *ValidArgs { } } -func (v *ValidArgs) Validate(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { +func (v *validArgs) Validate(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { cmd.SetContext(root.SkipPrompt(cmd.Context())) err := v.mustWorkspaceClientFunc(cmd, args) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index f80c745cad..58a52c59f6 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -100,7 +100,7 @@ func TestGetValidArgsFunctionCompletion(t *testing.T) { testutil.NewFakeDirEntry("file", false), }) - v := NewValidArgs() + v := newValidArgs() v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -121,7 +121,7 @@ func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { testutil.NewFakeDirEntry("file", false), }) - v := NewValidArgs() + v := newValidArgs() v.onlyDirs = true v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -142,7 +142,7 @@ func TestGetValidArgsFunctionNoPath(t *testing.T) { testutil.NewFakeDirEntry("dFile2", false), }) - v := NewValidArgs() + v := newValidArgs() v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -163,7 +163,7 @@ func TestGetValidArgsFunctionLocalPath(t *testing.T) { testutil.NewFakeDirEntry("file", false), }) - v := NewValidArgs() + v := newValidArgs() v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -183,7 +183,7 @@ func TestGetValidArgsFunctionAbsoluteLocalPath(t *testing.T) { testutil.NewFakeDirEntry("file", false), }) - v := NewValidArgs() + v := newValidArgs() v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -207,7 +207,7 @@ func TestGetValidArgsFunctionLocalWindowsPath(t *testing.T) { testutil.NewFakeDirEntry(".\\dFile2", false), }) - v := NewValidArgs() + v := newValidArgs() v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -227,7 +227,7 @@ func TestGetValidArgsFunctionAddsSeparator(t *testing.T) { testutil.NewFakeDirEntry("nested_dir", true), }) - v := NewValidArgs() + v := newValidArgs() v.onlyDirs = true v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -256,7 +256,7 @@ func TestGetValidArgsFunctionAddsWindowsSeparator(t *testing.T) { testutil.NewFakeDirEntry("nested_dir", true), }) - v := NewValidArgs() + v := newValidArgs() v.onlyDirs = true v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -280,7 +280,7 @@ func TestGetValidArgsFunctionAddsDefaultSeparatorOnWindows(t *testing.T) { testutil.NewFakeDirEntry("nested_dir", true), }) - v := NewValidArgs() + v := newValidArgs() v.onlyDirs = true v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -297,7 +297,7 @@ func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { mockCurrentUserApi(m, nil, errors.New("Current User Error")) mockFilerForPath := testutil.GetMockFilerForPath(t, "/", nil) - v := NewValidArgs() + v := newValidArgs() v.onlyDirs = true v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc @@ -314,7 +314,7 @@ func TestGetValidArgsFunctionNotCompletedArgument(t *testing.T) { mockCurrentUserApi(m, nil, nil) mockFilerForPath := testutil.GetMockFilerForPath(t, "/", nil) - v := NewValidArgs() + v := newValidArgs() // 0 here means we don't complete any arguments v.pathArgCount = 0 v.filerForPathFunc = mockFilerForPath diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 3cbaa1c1aa..d7eac513a5 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -89,7 +89,7 @@ func newLsCommand() *cobra.Command { `)) } - v := NewValidArgs() + v := newValidArgs() v.onlyDirs = true cmd.ValidArgsFunction = v.Validate diff --git a/cmd/fs/mkdir.go b/cmd/fs/mkdir.go index ade58a01e8..5e9ac78429 100644 --- a/cmd/fs/mkdir.go +++ b/cmd/fs/mkdir.go @@ -28,7 +28,7 @@ func newMkdirCommand() *cobra.Command { return f.Mkdir(ctx, path) } - v := NewValidArgs() + v := newValidArgs() v.onlyDirs = true cmd.ValidArgsFunction = v.Validate diff --git a/cmd/fs/rm.go b/cmd/fs/rm.go index 80e4cfced1..a133a83097 100644 --- a/cmd/fs/rm.go +++ b/cmd/fs/rm.go @@ -32,7 +32,7 @@ func newRmCommand() *cobra.Command { return f.Delete(ctx, path) } - v := NewValidArgs() + v := newValidArgs() cmd.ValidArgsFunction = v.Validate return cmd From 058183edaf3ea6e0768ef77c6de0e9f450bdeaba Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 09:07:28 +0300 Subject: [PATCH 26/43] Add integration test --- internal/completer_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 internal/completer_test.go diff --git a/internal/completer_test.go b/internal/completer_test.go new file mode 100644 index 0000000000..c06a84b579 --- /dev/null +++ b/internal/completer_test.go @@ -0,0 +1,36 @@ +package internal + +import ( + "context" + "strings" + "testing" + + _ "github.com/databricks/cli/cmd/fs" + "github.com/databricks/cli/libs/filer" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func setupCompletionFiles(t *testing.T, f filer.Filer) { + err := f.Write(context.Background(), "a/hello.txt", strings.NewReader("abc"), filer.CreateParentDirectories) + require.NoError(t, err) + err = f.Write(context.Background(), "bye.txt", strings.NewReader("def")) + require.NoError(t, err) +} + +func TestAccFsCompletion(t *testing.T) { + t.Parallel() + + f, tmpDir := setupDbfsFiler(t) + setupCompletionFiles(t, f) + + stdout, stderr := RequireSuccessfulRun(t, "__complete", "fs", "cat", tmpDir, "--output=json") + assert.Equal(t, "", stderr.String()) + + // var parsedStdout []map[string]any + // err := json.Unmarshal(stdout.Bytes(), &parsedStdout) + require.NoError(t, err) + + // assert.Len(t, parsedStdout, 2) + assert.Equal(t, "a", stdout) +} From 5df74cf0f773bae2da871bde67a6d4e699ca5dea Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 09:24:12 +0300 Subject: [PATCH 27/43] Fix error --- internal/completer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/completer_test.go b/internal/completer_test.go index c06a84b579..54ffe64836 100644 --- a/internal/completer_test.go +++ b/internal/completer_test.go @@ -29,7 +29,7 @@ func TestAccFsCompletion(t *testing.T) { // var parsedStdout []map[string]any // err := json.Unmarshal(stdout.Bytes(), &parsedStdout) - require.NoError(t, err) + // require.NoError(t, err) // assert.Len(t, parsedStdout, 2) assert.Equal(t, "a", stdout) From 08033819d48d6dd51783289b46fc9a1410643710 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 10:21:09 +0300 Subject: [PATCH 28/43] DRY up test --- cmd/fs/helpers_test.go | 177 ++++++----------------------------------- 1 file changed, 26 insertions(+), 151 deletions(-) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 58a52c59f6..d701b955e6 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -79,7 +79,7 @@ func mockMustWorkspaceClientFunc(cmd *cobra.Command, args []string) error { return nil } -func setupValidArgsFunctionTest(t *testing.T) (*mocks.MockWorkspaceClient, *cobra.Command) { +func setupCommand(t *testing.T) (*cobra.Command, *mocks.MockWorkspaceClient) { m := mocks.NewMockWorkspaceClient(t) ctx := context.Background() ctx = root.SetWorkspaceClient(ctx, m.WorkspaceClient) @@ -87,15 +87,15 @@ func setupValidArgsFunctionTest(t *testing.T) (*mocks.MockWorkspaceClient, *cobr cmd := &cobra.Command{} cmd.SetContext(ctx) - return m, cmd + return cmd, m } -func TestGetValidArgsFunctionCompletion(t *testing.T) { - m, cmd := setupValidArgsFunctionTest(t) +func setupTest(t *testing.T, path string) (*validArgs, *cobra.Command, *mocks.MockWorkspaceClient) { + cmd, m := setupCommand(t) mockCurrentUserApi(m, nil, nil) - mockFilerForPath := testutil.GetMockFilerForPath(t, "/", []fs.DirEntry{ + mockFilerForPath := testutil.GetMockFilerForPath(t, path, []fs.DirEntry{ testutil.NewFakeDirEntry("dir", true), testutil.NewFakeDirEntry("file", false), }) @@ -104,6 +104,12 @@ func TestGetValidArgsFunctionCompletion(t *testing.T) { v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + return v, cmd, m +} + +func TestGetValidArgsFunctionCompletion(t *testing.T) { + v, cmd, _ := setupTest(t, "/") + completions, directive := v.Validate(cmd, []string{}, "dbfs:/") // dbfs:/Volumes is programmatically added to the completions @@ -112,20 +118,9 @@ func TestGetValidArgsFunctionCompletion(t *testing.T) { } func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { - m, cmd := setupValidArgsFunctionTest(t) - - mockCurrentUserApi(m, nil, nil) + v, cmd, _ := setupTest(t, "/") - mockFilerForPath := testutil.GetMockFilerForPath(t, "/", []fs.DirEntry{ - testutil.NewFakeDirEntry("dir", true), - testutil.NewFakeDirEntry("file", false), - }) - - v := newValidArgs() v.onlyDirs = true - v.filerForPathFunc = mockFilerForPath - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - completions, directive := v.Validate(cmd, []string{}, "dbfs:/") assert.Equal(t, []string{"dbfs:/dir", "dbfs:/Volumes"}, completions) @@ -133,59 +128,26 @@ func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { } func TestGetValidArgsFunctionNoPath(t *testing.T) { - m, cmd := setupValidArgsFunctionTest(t) + v, cmd, _ := setupTest(t, "") - mockCurrentUserApi(m, nil, nil) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "", []fs.DirEntry{ - testutil.NewFakeDirEntry("dFile1", false), - testutil.NewFakeDirEntry("dFile2", false), - }) - - v := newValidArgs() - v.filerForPathFunc = mockFilerForPath - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - - completions, directive := v.Validate(cmd, []string{}, "d") + completions, directive := v.Validate(cmd, []string{}, "") // Suggest both dbfs and local paths at beginning of completion - assert.Equal(t, []string{"dFile1", "dFile2", "dbfs:/"}, completions) + assert.Equal(t, []string{"dir", "file", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } func TestGetValidArgsFunctionLocalPath(t *testing.T) { - m, cmd := setupValidArgsFunctionTest(t) - - mockCurrentUserApi(m, nil, nil) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "", []fs.DirEntry{ - testutil.NewFakeDirEntry("dir", true), - testutil.NewFakeDirEntry("file", false), - }) - - v := newValidArgs() - v.filerForPathFunc = mockFilerForPath - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + v, cmd, _ := setupTest(t, "") - completions, directive := v.Validate(cmd, []string{}, "") + completions, directive := v.Validate(cmd, []string{}, "d") assert.Equal(t, []string{"dir", "file", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } func TestGetValidArgsFunctionAbsoluteLocalPath(t *testing.T) { - m, cmd := setupValidArgsFunctionTest(t) - - mockCurrentUserApi(m, nil, nil) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "", []fs.DirEntry{ - testutil.NewFakeDirEntry("dir", true), - testutil.NewFakeDirEntry("file", false), - }) - - v := newValidArgs() - v.filerForPathFunc = mockFilerForPath - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc + v, cmd, _ := setupTest(t, "") completions, directive := v.Validate(cmd, []string{}, "/") @@ -193,113 +155,29 @@ func TestGetValidArgsFunctionAbsoluteLocalPath(t *testing.T) { assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } -func TestGetValidArgsFunctionLocalWindowsPath(t *testing.T) { - if runtime.GOOS != "windows" { - t.SkipNow() - } - - m, cmd := setupValidArgsFunctionTest(t) - - mockCurrentUserApi(m, nil, nil) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "", []fs.DirEntry{ - testutil.NewFakeDirEntry(".\\dFile1", false), - testutil.NewFakeDirEntry(".\\dFile2", false), - }) - - v := newValidArgs() - v.filerForPathFunc = mockFilerForPath - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - - completions, directive := v.Validate(cmd, []string{}, "d") - - // Suggest both dbfs and local paths at beginning of completion - assert.Equal(t, []string{"dFile1", "dFile2", "dbfs:/"}, completions) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) -} - func TestGetValidArgsFunctionAddsSeparator(t *testing.T) { - m, cmd := setupValidArgsFunctionTest(t) - - mockCurrentUserApi(m, nil, nil) + v, cmd, _ := setupTest(t, "parent_dir") - mockFilerForPath := testutil.GetMockFilerForPath(t, "foo", []fs.DirEntry{ - testutil.NewFakeDirEntry("nested_dir", true), - }) - - v := newValidArgs() - v.onlyDirs = true - v.filerForPathFunc = mockFilerForPath - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - - completions, directive := v.Validate(cmd, []string{}, "foo") + completions, directive := v.Validate(cmd, []string{}, "parent_dir") separator := "/" if runtime.GOOS == "windows" { separator = "\\" } - assert.Equal(t, []string{fmt.Sprintf("%s%s%s", "foo", separator, "nested_dir")}, completions) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) -} - -func TestGetValidArgsFunctionAddsWindowsSeparator(t *testing.T) { - if runtime.GOOS != "windows" { - t.SkipNow() - } - - m, cmd := setupValidArgsFunctionTest(t) - - mockCurrentUserApi(m, nil, nil) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "foo", []fs.DirEntry{ - testutil.NewFakeDirEntry("nested_dir", true), - }) - - v := newValidArgs() - v.onlyDirs = true - v.filerForPathFunc = mockFilerForPath - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - - completions, directive := v.Validate(cmd, []string{}, "foo") - - assert.Equal(t, []string{"foo\\nested_dir"}, completions) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) -} - -func TestGetValidArgsFunctionAddsDefaultSeparatorOnWindows(t *testing.T) { - if runtime.GOOS != "windows" { - t.SkipNow() - } - - m, cmd := setupValidArgsFunctionTest(t) - - mockCurrentUserApi(m, nil, nil) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "foo/", []fs.DirEntry{ - testutil.NewFakeDirEntry("nested_dir", true), - }) - - v := newValidArgs() - v.onlyDirs = true - v.filerForPathFunc = mockFilerForPath - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - - completions, directive := v.Validate(cmd, []string{}, "foo/") - - assert.Equal(t, []string{"foo/nested_dir"}, completions) + assert.Equal(t, []string{ + fmt.Sprintf("%s%s%s", "parent_dir", separator, "dir"), + fmt.Sprintf("%s%s%s", "parent_dir", separator, "file"), + }, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { - m, cmd := setupValidArgsFunctionTest(t) + cmd, m := setupCommand(t) mockCurrentUserApi(m, nil, errors.New("Current User Error")) - mockFilerForPath := testutil.GetMockFilerForPath(t, "/", nil) v := newValidArgs() - v.onlyDirs = true - v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc completions, directive := v.Validate(cmd, []string{}, "dbfs:/") @@ -309,15 +187,12 @@ func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { } func TestGetValidArgsFunctionNotCompletedArgument(t *testing.T) { - m, cmd := setupValidArgsFunctionTest(t) + cmd, m := setupCommand(t) mockCurrentUserApi(m, nil, nil) - mockFilerForPath := testutil.GetMockFilerForPath(t, "/", nil) v := newValidArgs() - // 0 here means we don't complete any arguments v.pathArgCount = 0 - v.filerForPathFunc = mockFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc completions, directive := v.Validate(cmd, []string{}, "dbfs:/") From fb90be922b5f602130527db22c87895c92d2b060 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 14:51:25 +0300 Subject: [PATCH 29/43] Use fakeFiler in tests --- cmd/fs/helpers_test.go | 78 +++++----------- internal/testutil/filer.go | 74 ---------------- libs/completer/completer_test.go | 103 ++++++---------------- libs/filer/fake_filer.go | 147 +++++++++++++++++++++++++++++++ libs/filer/fs_test.go | 132 ++------------------------- 5 files changed, 202 insertions(+), 332 deletions(-) delete mode 100644 internal/testutil/filer.go create mode 100644 libs/filer/fake_filer.go diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index d701b955e6..4678ff4ee6 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -3,13 +3,10 @@ package fs import ( "context" "errors" - "fmt" - "io/fs" "runtime" "testing" "github.com/databricks/cli/cmd/root" - "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/filer" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/iam" @@ -90,85 +87,58 @@ func setupCommand(t *testing.T) (*cobra.Command, *mocks.MockWorkspaceClient) { return cmd, m } -func setupTest(t *testing.T, path string) (*validArgs, *cobra.Command, *mocks.MockWorkspaceClient) { +func setupTest(t *testing.T) (*validArgs, *cobra.Command, *mocks.MockWorkspaceClient) { cmd, m := setupCommand(t) mockCurrentUserApi(m, nil, nil) - mockFilerForPath := testutil.GetMockFilerForPath(t, path, []fs.DirEntry{ - testutil.NewFakeDirEntry("dir", true), - testutil.NewFakeDirEntry("file", false), - }) + fakeFilerForPath := func(ctx context.Context, fullPath string) (filer.Filer, string, error) { + fakeFiler := filer.NewFakeFiler(map[string]filer.FakeFileInfo{ + "dir": {name: "root", dir: true}, + "dir/dirA": {dir: true}, + "dir/dirB": {dir: true}, + "dir/fileA": {}, + }) + return fakeFiler, fullPath, nil + } v := newValidArgs() - v.filerForPathFunc = mockFilerForPath + v.filerForPathFunc = fakeFilerForPath v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc return v, cmd, m } func TestGetValidArgsFunctionCompletion(t *testing.T) { - v, cmd, _ := setupTest(t, "/") - - completions, directive := v.Validate(cmd, []string{}, "dbfs:/") - - // dbfs:/Volumes is programmatically added to the completions - assert.Equal(t, []string{"dbfs:/dir", "dbfs:/file", "dbfs:/Volumes"}, completions) + v, cmd, _ := setupTest(t) + completions, directive := v.Validate(cmd, []string{}, "dir") + assert.Equal(t, []string{"dir/dirA", "dir/dirB", "dir/fileA"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { - v, cmd, _ := setupTest(t, "/") - + v, cmd, _ := setupTest(t) v.onlyDirs = true - completions, directive := v.Validate(cmd, []string{}, "dbfs:/") - - assert.Equal(t, []string{"dbfs:/dir", "dbfs:/Volumes"}, completions) + completions, directive := v.Validate(cmd, []string{}, "dir") + assert.Equal(t, []string{"dir/dirA", "dir/dirB"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } -func TestGetValidArgsFunctionNoPath(t *testing.T) { - v, cmd, _ := setupTest(t, "") +func TestGetValidArgsFunctionDbfsPath(t *testing.T) { + v, cmd, _ := setupTest(t) completions, directive := v.Validate(cmd, []string{}, "") - // Suggest both dbfs and local paths at beginning of completion - assert.Equal(t, []string{"dir", "file", "dbfs:/"}, completions) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) -} - -func TestGetValidArgsFunctionLocalPath(t *testing.T) { - v, cmd, _ := setupTest(t, "") - - completions, directive := v.Validate(cmd, []string{}, "d") - - assert.Equal(t, []string{"dir", "file", "dbfs:/"}, completions) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) -} - -func TestGetValidArgsFunctionAbsoluteLocalPath(t *testing.T) { - v, cmd, _ := setupTest(t, "") - - completions, directive := v.Validate(cmd, []string{}, "/") - - assert.Equal(t, []string{"dir", "file"}, completions) + assert.Equal(t, []string{"dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } -func TestGetValidArgsFunctionAddsSeparator(t *testing.T) { - v, cmd, _ := setupTest(t, "parent_dir") - - completions, directive := v.Validate(cmd, []string{}, "parent_dir") +func TestGetValidArgsFunctionVolumesPath(t *testing.T) { + v, cmd, _ := setupTest(t) - separator := "/" - if runtime.GOOS == "windows" { - separator = "\\" - } + completions, directive := v.Validate(cmd, []string{}, "dbfs:/") - assert.Equal(t, []string{ - fmt.Sprintf("%s%s%s", "parent_dir", separator, "dir"), - fmt.Sprintf("%s%s%s", "parent_dir", separator, "file"), - }, completions) + assert.Equal(t, []string{"dbfs:/Volumes"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } diff --git a/internal/testutil/filer.go b/internal/testutil/filer.go deleted file mode 100644 index 3c62a75b9a..0000000000 --- a/internal/testutil/filer.go +++ /dev/null @@ -1,74 +0,0 @@ -package testutil - -import ( - "context" - "io/fs" - "testing" - "time" - - mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" - "github.com/databricks/cli/libs/filer" - "github.com/stretchr/testify/mock" -) - -type fakeDirEntry struct { - fakeFileInfo -} - -func (entry fakeDirEntry) Type() fs.FileMode { - typ := fs.ModePerm - if entry.dir { - typ |= fs.ModeDir - } - return typ -} - -func (entry fakeDirEntry) Info() (fs.FileInfo, error) { - return entry.fakeFileInfo, nil -} - -type fakeFileInfo struct { - name string - size int64 - dir bool - mode fs.FileMode -} - -func (info fakeFileInfo) Name() string { - return info.name -} - -func (info fakeFileInfo) Size() int64 { - return info.size -} - -func (info fakeFileInfo) Mode() fs.FileMode { - return info.mode -} - -func (info fakeFileInfo) ModTime() time.Time { - return time.Now() -} - -func (info fakeFileInfo) IsDir() bool { - return info.dir -} - -func (info fakeFileInfo) Sys() any { - return nil -} - -func NewFakeDirEntry(name string, dir bool) fs.DirEntry { - return fakeDirEntry{fakeFileInfo{name: name, dir: dir}} -} - -func GetMockFilerForPath(t *testing.T, path string, entries []fs.DirEntry) func(ctx context.Context, fullPath string) (filer.Filer, string, error) { - mockFiler := mockfiler.NewMockFiler(t) - if entries != nil { - mockFiler.EXPECT().ReadDir(mock.AnythingOfType("*context.valueCtx"), path).Return(entries, nil) - } - mockFilerForPath := func(ctx context.Context, fullPath string) (filer.Filer, string, error) { - return mockFiler, path, nil - } - return mockFilerForPath -} diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index 7618af9213..fab2fc270d 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -2,112 +2,59 @@ package completer import ( "context" - "errors" - "io/fs" "testing" "github.com/databricks/cli/cmd/root" - mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" - "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/filer" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" ) -func setup(t *testing.T) context.Context { +func setupCompleter(t *testing.T, onlyDirs bool) *completer { ctx := context.Background() // Needed to make type context.valueCtx for mockFilerForPath ctx = root.SetWorkspaceClient(ctx, mocks.NewMockWorkspaceClient(t).WorkspaceClient) - return ctx -} - -func TestFilerCompleterReturnsNestedDirs(t *testing.T) { - ctx := setup(t) - mockFilerForPath := testutil.GetMockFilerForPath(t, "/", []fs.DirEntry{ - testutil.NewFakeDirEntry("dir", true), - }) - mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/") + fakeFiler := filer.PrepFakeFiler() - completer := New(ctx, mockFiler, true) + return New(ctx, fakeFiler, onlyDirs) +} - completions, dirPath, directive := completer.CompletePath("/") +func TestFilerCompleterReturnsNestedDirs(t *testing.T) { + completer := setupCompleter(t, true) + completions, dirPath, directive := completer.CompletePath("dir") - assert.Equal(t, []string{"dir"}, completions) - assert.Equal(t, "/", dirPath) + assert.Equal(t, []string{"dirA", "dirB"}, completions) + assert.Equal(t, "dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { - ctx := setup(t) - - mockFiler := mockfiler.NewMockFiler(t) + completer := setupCompleter(t, true) + completions, dirPath, directive := completer.CompletePath("dir/wrong_dir") - // First call to get nested dirs fails so we get the adjacent dirs instead - mockFiler.On("ReadDir", mock.AnythingOfType("*context.valueCtx"), "/wrong_path").Return(nil, errors.New("error")) - mockFiler.On("ReadDir", mock.AnythingOfType("*context.valueCtx"), "/").Return([]fs.DirEntry{ - testutil.NewFakeDirEntry("adjacent", true), - }, nil) - - completer := New(ctx, mockFiler, true) - completions, dirPath, directive := completer.CompletePath("/wrong_path") - - assert.Equal(t, []string{"adjacent"}, completions) - assert.Equal(t, "/", dirPath) + assert.Equal(t, []string{"dirA", "dirB"}, completions) + assert.Equal(t, "dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) - - mockFiler.AssertExpectations(t) } -func TestFilerCompleterReadDirError(t *testing.T) { - ctx := setup(t) +func TestFilerCompleterReturnsNestedDirsAndFiles(t *testing.T) { + completer := setupCompleter(t, false) + completions, dirPath, directive := completer.CompletePath("dir") - mockFiler := mockfiler.NewMockFiler(t) - mockFiler.On("ReadDir", mock.AnythingOfType("*context.valueCtx"), mock.Anything).Return(nil, errors.New("error")) - - completer := New(ctx, mockFiler, true) - completions, dirPath, directive := completer.CompletePath("/dir") - - assert.Nil(t, completions) - assert.Equal(t, "/", dirPath) + assert.Equal(t, []string{"dirA", "dirB", "fileA"}, completions) + assert.Equal(t, "dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) - - mockFiler.AssertExpectations(t) } -func TestFilerCompleterReturnsFileAndDir(t *testing.T) { - ctx := setup(t) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "/", []fs.DirEntry{ - testutil.NewFakeDirEntry("dir", true), - testutil.NewFakeDirEntry("file", false), - }) - mockFiler, _, _ := mockFilerForPath(ctx, "dbfs:/") - - // onlyDirs=false so we should get both files and directories - completer := New(ctx, mockFiler, false) +func TestFilerCompleterNoCompletions(t *testing.T) { + completer := setupCompleter(t, true) + completions, dirPath, directive := completer.CompletePath("wrong_dir/wrong_dir") - completions, dirPath, directive := completer.CompletePath("/") - - assert.Equal(t, []string{"dir", "file"}, completions) - assert.Equal(t, "/", dirPath) + assert.Nil(t, completions) + assert.Equal(t, "wrong_dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } -func TestFilerCompleterRetainsFormatting(t *testing.T) { - ctx := setup(t) - - mockFilerForPath := testutil.GetMockFilerForPath(t, "//dir//", []fs.DirEntry{ - testutil.NewFakeDirEntry("nested_dir", true), - }) - mockFiler, _, _ := mockFilerForPath(ctx, "dbfs://dir") - - completer := New(ctx, mockFiler, true) - - completions, dirPath, directive := completer.CompletePath("//dir//") - - assert.Equal(t, []string{"nested_dir"}, completions) - assert.Equal(t, "//dir//", dirPath) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) -} +// func TestFilerCompleterReadDirError(t *testing.T) { diff --git a/libs/filer/fake_filer.go b/libs/filer/fake_filer.go new file mode 100644 index 0000000000..0bb1a26421 --- /dev/null +++ b/libs/filer/fake_filer.go @@ -0,0 +1,147 @@ +package filer + +import ( + "context" + "fmt" + "io" + "io/fs" + "path" + "sort" + "strings" + "time" +) + +type FakeDirEntry struct { + FakeFileInfo +} + +func (entry FakeDirEntry) Type() fs.FileMode { + typ := fs.ModePerm + if entry.dir { + typ |= fs.ModeDir + } + return typ +} + +func (entry FakeDirEntry) Info() (fs.FileInfo, error) { + return entry.FakeFileInfo, nil +} + +type FakeFileInfo struct { + name string + size int64 + dir bool + mode fs.FileMode +} + +func (info FakeFileInfo) Name() string { + return info.name +} + +func (info FakeFileInfo) Size() int64 { + return info.size +} + +func (info FakeFileInfo) Mode() fs.FileMode { + return info.mode +} + +func (info FakeFileInfo) ModTime() time.Time { + return time.Now() +} + +func (info FakeFileInfo) IsDir() bool { + return info.dir +} + +func (info FakeFileInfo) Sys() any { + return nil +} + +type FakeFiler struct { + entries map[string]FakeFileInfo +} + +func (f *FakeFiler) Write(ctx context.Context, p string, reader io.Reader, mode ...WriteMode) error { + return fmt.Errorf("not implemented") +} + +func (f *FakeFiler) Read(ctx context.Context, p string) (io.ReadCloser, error) { + _, ok := f.entries[p] + if !ok { + return nil, fs.ErrNotExist + } + + return io.NopCloser(strings.NewReader("foo")), nil +} + +func (f *FakeFiler) Delete(ctx context.Context, p string, mode ...DeleteMode) error { + return fmt.Errorf("not implemented") +} + +func (f *FakeFiler) ReadDir(ctx context.Context, p string) ([]fs.DirEntry, error) { + entry, ok := f.entries[p] + if !ok { + return nil, fs.ErrNotExist + } + + if !entry.dir { + return nil, fs.ErrInvalid + } + + // Find all entries contained in the specified directory `p`. + var out []fs.DirEntry + for k, v := range f.entries { + if k == p || path.Dir(k) != p { + continue + } + + out = append(out, FakeDirEntry{v}) + } + + sort.Slice(out, func(i, j int) bool { return out[i].Name() < out[j].Name() }) + return out, nil +} + +func (f *FakeFiler) Mkdir(ctx context.Context, path string) error { + return fmt.Errorf("not implemented") +} + +func (f *FakeFiler) Stat(ctx context.Context, path string) (fs.FileInfo, error) { + entry, ok := f.entries[path] + if !ok { + return nil, fs.ErrNotExist + } + + return entry, nil +} + +func NewFakeFiler(entries map[string]FakeFileInfo) *FakeFiler { + fakeFiler := &FakeFiler{ + entries: entries, + } + + for k, v := range fakeFiler.entries { + if v.name != "" { + continue + } + v.name = path.Base(k) + fakeFiler.entries[k] = v + } + + return fakeFiler +} + +// TODO: Remove this function +func PrepFakeFiler() *FakeFiler { + return NewFakeFiler(map[string]FakeFileInfo{ + "dir": {name: "root", dir: true}, + "dir/dirA": {dir: true}, + "dir/dirB": {dir: true}, + "dir/fileA": {size: 3}, + }) +} + +func NewFakeFileInfo(dir bool) FakeFileInfo { + return FakeFileInfo{dir: dir} +} diff --git a/libs/filer/fs_test.go b/libs/filer/fs_test.go index 03ed312b46..32d75f692a 100644 --- a/libs/filer/fs_test.go +++ b/libs/filer/fs_test.go @@ -2,124 +2,14 @@ package filer import ( "context" - "fmt" "io" "io/fs" - "path" - "sort" - "strings" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -type fakeDirEntry struct { - fakeFileInfo -} - -func (entry fakeDirEntry) Type() fs.FileMode { - typ := fs.ModePerm - if entry.dir { - typ |= fs.ModeDir - } - return typ -} - -func (entry fakeDirEntry) Info() (fs.FileInfo, error) { - return entry.fakeFileInfo, nil -} - -type fakeFileInfo struct { - name string - size int64 - dir bool - mode fs.FileMode -} - -func (info fakeFileInfo) Name() string { - return info.name -} - -func (info fakeFileInfo) Size() int64 { - return info.size -} - -func (info fakeFileInfo) Mode() fs.FileMode { - return info.mode -} - -func (info fakeFileInfo) ModTime() time.Time { - return time.Now() -} - -func (info fakeFileInfo) IsDir() bool { - return info.dir -} - -func (info fakeFileInfo) Sys() any { - return nil -} - -type fakeFiler struct { - entries map[string]fakeFileInfo -} - -func (f *fakeFiler) Write(ctx context.Context, p string, reader io.Reader, mode ...WriteMode) error { - return fmt.Errorf("not implemented") -} - -func (f *fakeFiler) Read(ctx context.Context, p string) (io.ReadCloser, error) { - _, ok := f.entries[p] - if !ok { - return nil, fs.ErrNotExist - } - - return io.NopCloser(strings.NewReader("foo")), nil -} - -func (f *fakeFiler) Delete(ctx context.Context, p string, mode ...DeleteMode) error { - return fmt.Errorf("not implemented") -} - -func (f *fakeFiler) ReadDir(ctx context.Context, p string) ([]fs.DirEntry, error) { - entry, ok := f.entries[p] - if !ok { - return nil, fs.ErrNotExist - } - - if !entry.dir { - return nil, fs.ErrInvalid - } - - // Find all entries contained in the specified directory `p`. - var out []fs.DirEntry - for k, v := range f.entries { - if k == p || path.Dir(k) != p { - continue - } - - out = append(out, fakeDirEntry{v}) - } - - sort.Slice(out, func(i, j int) bool { return out[i].Name() < out[j].Name() }) - return out, nil -} - -func (f *fakeFiler) Mkdir(ctx context.Context, path string) error { - return fmt.Errorf("not implemented") -} - -func (f *fakeFiler) Stat(ctx context.Context, path string) (fs.FileInfo, error) { - entry, ok := f.entries[path] - if !ok { - return nil, fs.ErrNotExist - } - - return entry, nil -} - func TestFsImplementsFS(t *testing.T) { var _ fs.FS = &filerFS{} } @@ -145,22 +35,12 @@ func TestFsDirImplementsFsReadDirFile(t *testing.T) { } func fakeFS() fs.FS { - fakeFiler := &fakeFiler{ - entries: map[string]fakeFileInfo{ - ".": {name: "root", dir: true}, - "dirA": {dir: true}, - "dirB": {dir: true}, - "fileA": {size: 3}, - }, - } - - for k, v := range fakeFiler.entries { - if v.name != "" { - continue - } - v.name = path.Base(k) - fakeFiler.entries[k] = v - } + fakeFiler := NewFakeFiler(map[string]FakeFileInfo{ + ".": {name: "root", dir: true}, + "dirA": {dir: true}, + "dirB": {dir: true}, + "fileA": {size: 3}, + }) return NewFS(context.Background(), fakeFiler) } From d802ba771dda7da4342b62522cfebc0b3bce4588 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 15:02:33 +0300 Subject: [PATCH 30/43] Remove PrepFakeFiler --- cmd/fs/helpers_test.go | 6 ++--- libs/completer/completer_test.go | 7 +++++- libs/filer/fake_filer.go | 38 ++++++++++---------------------- 3 files changed, 21 insertions(+), 30 deletions(-) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 4678ff4ee6..abcdceb11f 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -94,9 +94,9 @@ func setupTest(t *testing.T) (*validArgs, *cobra.Command, *mocks.MockWorkspaceCl fakeFilerForPath := func(ctx context.Context, fullPath string) (filer.Filer, string, error) { fakeFiler := filer.NewFakeFiler(map[string]filer.FakeFileInfo{ - "dir": {name: "root", dir: true}, - "dir/dirA": {dir: true}, - "dir/dirB": {dir: true}, + "dir": {FakeName: "root", FakeDir: true}, + "dir/dirA": {FakeDir: true}, + "dir/dirB": {FakeDir: true}, "dir/fileA": {}, }) return fakeFiler, fullPath, nil diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index fab2fc270d..627ef96af5 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -16,7 +16,12 @@ func setupCompleter(t *testing.T, onlyDirs bool) *completer { // Needed to make type context.valueCtx for mockFilerForPath ctx = root.SetWorkspaceClient(ctx, mocks.NewMockWorkspaceClient(t).WorkspaceClient) - fakeFiler := filer.PrepFakeFiler() + fakeFiler := filer.NewFakeFiler(map[string]filer.FakeFileInfo{ + "dir": {FakeName: "root", FakeDir: true}, + "dir/dirA": {FakeDir: true}, + "dir/dirB": {FakeDir: true}, + "dir/fileA": {}, + }) return New(ctx, fakeFiler, onlyDirs) } diff --git a/libs/filer/fake_filer.go b/libs/filer/fake_filer.go index 0bb1a26421..25e29ae6a5 100644 --- a/libs/filer/fake_filer.go +++ b/libs/filer/fake_filer.go @@ -17,7 +17,7 @@ type FakeDirEntry struct { func (entry FakeDirEntry) Type() fs.FileMode { typ := fs.ModePerm - if entry.dir { + if entry.FakeDir { typ |= fs.ModeDir } return typ @@ -28,22 +28,22 @@ func (entry FakeDirEntry) Info() (fs.FileInfo, error) { } type FakeFileInfo struct { - name string - size int64 - dir bool - mode fs.FileMode + FakeName string + FakeSize int64 + FakeDir bool + FakeMode fs.FileMode } func (info FakeFileInfo) Name() string { - return info.name + return info.FakeName } func (info FakeFileInfo) Size() int64 { - return info.size + return info.FakeSize } func (info FakeFileInfo) Mode() fs.FileMode { - return info.mode + return info.FakeMode } func (info FakeFileInfo) ModTime() time.Time { @@ -51,7 +51,7 @@ func (info FakeFileInfo) ModTime() time.Time { } func (info FakeFileInfo) IsDir() bool { - return info.dir + return info.FakeDir } func (info FakeFileInfo) Sys() any { @@ -85,7 +85,7 @@ func (f *FakeFiler) ReadDir(ctx context.Context, p string) ([]fs.DirEntry, error return nil, fs.ErrNotExist } - if !entry.dir { + if !entry.FakeDir { return nil, fs.ErrInvalid } @@ -122,26 +122,12 @@ func NewFakeFiler(entries map[string]FakeFileInfo) *FakeFiler { } for k, v := range fakeFiler.entries { - if v.name != "" { + if v.FakeName != "" { continue } - v.name = path.Base(k) + v.FakeName = path.Base(k) fakeFiler.entries[k] = v } return fakeFiler } - -// TODO: Remove this function -func PrepFakeFiler() *FakeFiler { - return NewFakeFiler(map[string]FakeFileInfo{ - "dir": {name: "root", dir: true}, - "dir/dirA": {dir: true}, - "dir/dirB": {dir: true}, - "dir/fileA": {size: 3}, - }) -} - -func NewFakeFileInfo(dir bool) FakeFileInfo { - return FakeFileInfo{dir: dir} -} From 5261a2cd412420d83c5ed5e2a9e094eac79bf7f9 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 15:34:25 +0300 Subject: [PATCH 31/43] Remove call to current user --- cmd/fs/helpers.go | 15 +++++---------- cmd/fs/helpers_test.go | 28 +--------------------------- libs/completer/completer.go | 20 ++++++++++++-------- libs/completer/completer_test.go | 12 ++++++++---- libs/filer/fake_filer.go | 2 +- 5 files changed, 27 insertions(+), 50 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index adedaebd45..608cf0f48a 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -87,19 +87,17 @@ func (v *validArgs) Validate(cmd *cobra.Command, args []string, toComplete strin return nil, cobra.ShellCompDirectiveError } - wsc := root.WorkspaceClient(cmd.Context()) - _, err = wsc.CurrentUser.Me(cmd.Context()) - if err != nil { - return nil, cobra.ShellCompDirectiveError - } - completer := completer.New(cmd.Context(), filer, v.onlyDirs) if len(args) >= v.pathArgCount { return nil, cobra.ShellCompDirectiveNoFileComp } - completions, dirPath, directive := completer.CompletePath(toCompletePath) + completions, dirPath, directive, err := completer.CompletePath(toCompletePath) + + if err != nil { + return nil, cobra.ShellCompDirectiveError + } isDbfsPath := isDbfsPath(toComplete) @@ -109,9 +107,6 @@ func (v *validArgs) Validate(cmd *cobra.Command, args []string, toComplete strin if isDbfsPath { // Add dbfs:/ prefix to completions completions[i] = path.Join(dbfsPrefix, completions[i]) - } else { - // Clean up ./ (and .\ on Windows) from local completions - completions[i] = filepath.Clean(completions[i]) } } diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index abcdceb11f..704647a7d0 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -2,17 +2,14 @@ package fs import ( "context" - "errors" "runtime" "testing" "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/filer" "github.com/databricks/databricks-sdk-go/experimental/mocks" - "github.com/databricks/databricks-sdk-go/service/iam" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -67,11 +64,6 @@ func TestFilerForWindowsLocalPaths(t *testing.T) { testWindowsFilerForPath(t, ctx, `f:\abc\ef`) } -func mockCurrentUserApi(m *mocks.MockWorkspaceClient, u *iam.User, e error) { - currentUserApi := m.GetMockCurrentUserAPI() - currentUserApi.EXPECT().Me(mock.AnythingOfType("*context.valueCtx")).Return(u, e) -} - func mockMustWorkspaceClientFunc(cmd *cobra.Command, args []string) error { return nil } @@ -90,8 +82,6 @@ func setupCommand(t *testing.T) (*cobra.Command, *mocks.MockWorkspaceClient) { func setupTest(t *testing.T) (*validArgs, *cobra.Command, *mocks.MockWorkspaceClient) { cmd, m := setupCommand(t) - mockCurrentUserApi(m, nil, nil) - fakeFilerForPath := func(ctx context.Context, fullPath string) (filer.Filer, string, error) { fakeFiler := filer.NewFakeFiler(map[string]filer.FakeFileInfo{ "dir": {FakeName: "root", FakeDir: true}, @@ -142,24 +132,8 @@ func TestGetValidArgsFunctionVolumesPath(t *testing.T) { assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } -func TestGetValidArgsFunctionNoCurrentUser(t *testing.T) { - cmd, m := setupCommand(t) - - mockCurrentUserApi(m, nil, errors.New("Current User Error")) - - v := newValidArgs() - v.mustWorkspaceClientFunc = mockMustWorkspaceClientFunc - - completions, directive := v.Validate(cmd, []string{}, "dbfs:/") - - assert.Nil(t, completions) - assert.Equal(t, cobra.ShellCompDirectiveError, directive) -} - func TestGetValidArgsFunctionNotCompletedArgument(t *testing.T) { - cmd, m := setupCommand(t) - - mockCurrentUserApi(m, nil, nil) + cmd, _ := setupCommand(t) v := newValidArgs() v.pathArgCount = 0 diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 87774b7f02..73e826b535 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -23,19 +23,19 @@ func New(ctx context.Context, filer filer.Filer, onlyDirs bool) *completer { return &completer{ctx: ctx, filer: filer, onlyDirs: onlyDirs} } -func (c *completer) CompletePath(p string) ([]string, string, cobra.ShellCompDirective) { +func (c *completer) CompletePath(p string) ([]string, string, cobra.ShellCompDirective, error) { // If the user is TAB-ing their way through a path, the path in `toComplete` // is valid and we should list nested directories. // If the path in `toComplete` is incomplete, however, // then we should list adjacent directories. dirPath := p - completions := fetchCompletions(c, dirPath) - if completions == nil { + completions, err := fetchCompletions(c, dirPath) + if completions == nil && err == nil { dirPath = path.Dir(p) - completions = fetchCompletions(c, dirPath) + completions, err = fetchCompletions(c, dirPath) } - return completions, dirPath, cobra.ShellCompDirectiveNoSpace + return completions, dirPath, cobra.ShellCompDirectiveNoSpace, err } // List files and directories under the specified path. @@ -43,10 +43,14 @@ func (c *completer) CompletePath(p string) ([]string, string, cobra.ShellCompDir func fetchCompletions( c *completer, path string, -) []string { +) ([]string, error) { entries, err := c.filer.ReadDir(c.ctx, path) if err != nil { - return nil + if _, ok := err.(filer.NoSuchDirectoryError); ok { + return nil, nil + } else { + return nil, err + } } completions := []string{} @@ -58,5 +62,5 @@ func fetchCompletions( completions = append(completions, entry.Name()) } - return completions + return completions, nil } diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index 627ef96af5..beca2ebebf 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -28,38 +28,42 @@ func setupCompleter(t *testing.T, onlyDirs bool) *completer { func TestFilerCompleterReturnsNestedDirs(t *testing.T) { completer := setupCompleter(t, true) - completions, dirPath, directive := completer.CompletePath("dir") + completions, dirPath, directive, err := completer.CompletePath("dir") assert.Equal(t, []string{"dirA", "dirB"}, completions) assert.Equal(t, "dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + assert.Nil(t, err) } func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { completer := setupCompleter(t, true) - completions, dirPath, directive := completer.CompletePath("dir/wrong_dir") + completions, dirPath, directive, err := completer.CompletePath("dir/wrong_dir") assert.Equal(t, []string{"dirA", "dirB"}, completions) assert.Equal(t, "dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + assert.Nil(t, err) } func TestFilerCompleterReturnsNestedDirsAndFiles(t *testing.T) { completer := setupCompleter(t, false) - completions, dirPath, directive := completer.CompletePath("dir") + completions, dirPath, directive, err := completer.CompletePath("dir") assert.Equal(t, []string{"dirA", "dirB", "fileA"}, completions) assert.Equal(t, "dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + assert.Nil(t, err) } func TestFilerCompleterNoCompletions(t *testing.T) { completer := setupCompleter(t, true) - completions, dirPath, directive := completer.CompletePath("wrong_dir/wrong_dir") + completions, dirPath, directive, err := completer.CompletePath("wrong_dir/wrong_dir") assert.Nil(t, completions) assert.Equal(t, "wrong_dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + assert.Nil(t, err) } // func TestFilerCompleterReadDirError(t *testing.T) { diff --git a/libs/filer/fake_filer.go b/libs/filer/fake_filer.go index 25e29ae6a5..a99ac9fc01 100644 --- a/libs/filer/fake_filer.go +++ b/libs/filer/fake_filer.go @@ -82,7 +82,7 @@ func (f *FakeFiler) Delete(ctx context.Context, p string, mode ...DeleteMode) er func (f *FakeFiler) ReadDir(ctx context.Context, p string) ([]fs.DirEntry, error) { entry, ok := f.entries[p] if !ok { - return nil, fs.ErrNotExist + return nil, NoSuchDirectoryError{p} } if !entry.FakeDir { From 3514151d719c57b446e8e26445044c50cc084241 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 16:17:07 +0300 Subject: [PATCH 32/43] Add integration test --- internal/completer_test.go | 23 +++++++---------------- internal/fs_ls_test.go | 2 -- internal/helpers.go | 4 ++-- libs/filer/fs_test.go | 8 ++++---- 4 files changed, 13 insertions(+), 24 deletions(-) diff --git a/internal/completer_test.go b/internal/completer_test.go index 54ffe64836..eba22f9b02 100644 --- a/internal/completer_test.go +++ b/internal/completer_test.go @@ -2,6 +2,7 @@ package internal import ( "context" + "fmt" "strings" "testing" @@ -11,26 +12,16 @@ import ( "github.com/stretchr/testify/require" ) -func setupCompletionFiles(t *testing.T, f filer.Filer) { - err := f.Write(context.Background(), "a/hello.txt", strings.NewReader("abc"), filer.CreateParentDirectories) - require.NoError(t, err) - err = f.Write(context.Background(), "bye.txt", strings.NewReader("def")) +func setupCompletionFile(t *testing.T, f filer.Filer) { + err := f.Write(context.Background(), "dir1/file1.txt", strings.NewReader("abc"), filer.CreateParentDirectories) require.NoError(t, err) } func TestAccFsCompletion(t *testing.T) { - t.Parallel() - f, tmpDir := setupDbfsFiler(t) - setupCompletionFiles(t, f) - - stdout, stderr := RequireSuccessfulRun(t, "__complete", "fs", "cat", tmpDir, "--output=json") - assert.Equal(t, "", stderr.String()) - - // var parsedStdout []map[string]any - // err := json.Unmarshal(stdout.Bytes(), &parsedStdout) - // require.NoError(t, err) + setupCompletionFile(t, f) - // assert.Len(t, parsedStdout, 2) - assert.Equal(t, "a", stdout) + stdout, _ := RequireSuccessfulRun(t, "__complete", "fs", "ls", tmpDir) + expectedOutput := fmt.Sprintf("%s/dir1\n:2\n", tmpDir) + assert.Equal(t, expectedOutput, stdout.String()) } diff --git a/internal/fs_ls_test.go b/internal/fs_ls_test.go index 994a4a4255..05623cc18f 100644 --- a/internal/fs_ls_test.go +++ b/internal/fs_ls_test.go @@ -124,10 +124,8 @@ func TestAccFsLsOnFile(t *testing.T) { func TestAccFsLsOnEmptyDir(t *testing.T) { t.Parallel() - for _, testCase := range fsTests { tc := testCase - t.Run(tc.name, func(t *testing.T) { t.Parallel() diff --git a/internal/helpers.go b/internal/helpers.go index 972a2322b5..a0e9520a3a 100644 --- a/internal/helpers.go +++ b/internal/helpers.go @@ -455,7 +455,6 @@ func TemporaryDbfsDir(t *testing.T, w *databricks.WorkspaceClient) string { t.Logf("Creating DBFS folder:%s", path) err := w.Dbfs.MkdirsByPath(ctx, path) require.NoError(t, err) - t.Cleanup(func() { t.Logf("Removing DBFS folder:%s", path) err := w.Dbfs.Delete(ctx, files.Delete{ @@ -585,12 +584,13 @@ func setupWsfsExtensionsFiler(t *testing.T) (filer.Filer, string) { } func setupDbfsFiler(t *testing.T) (filer.Filer, string) { - t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) + // t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) w, err := databricks.NewWorkspaceClient() require.NoError(t, err) tmpDir := TemporaryDbfsDir(t, w) + f, err := filer.NewDbfsClient(w, tmpDir) require.NoError(t, err) diff --git a/libs/filer/fs_test.go b/libs/filer/fs_test.go index 32d75f692a..a74c10f0bb 100644 --- a/libs/filer/fs_test.go +++ b/libs/filer/fs_test.go @@ -36,10 +36,10 @@ func TestFsDirImplementsFsReadDirFile(t *testing.T) { func fakeFS() fs.FS { fakeFiler := NewFakeFiler(map[string]FakeFileInfo{ - ".": {name: "root", dir: true}, - "dirA": {dir: true}, - "dirB": {dir: true}, - "fileA": {size: 3}, + ".": {FakeName: "root", FakeDir: true}, + "dirA": {FakeDir: true}, + "dirB": {FakeDir: true}, + "fileA": {FakeSize: 3}, }) return NewFS(context.Background(), fakeFiler) From 2057aad315bd3a18857ad654a87a81e705673af1 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Tue, 6 Aug 2024 16:24:16 +0300 Subject: [PATCH 33/43] Put back GetEnvOrSkipTest --- internal/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/helpers.go b/internal/helpers.go index a0e9520a3a..3dfa69ab8c 100644 --- a/internal/helpers.go +++ b/internal/helpers.go @@ -584,7 +584,7 @@ func setupWsfsExtensionsFiler(t *testing.T) (filer.Filer, string) { } func setupDbfsFiler(t *testing.T) (filer.Filer, string) { - // t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) + t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) w, err := databricks.NewWorkspaceClient() require.NoError(t, err) From 25e4443964208d20db108f1a8b409c3e10dd9f05 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 7 Aug 2024 11:18:14 +0300 Subject: [PATCH 34/43] PR feedback --- cmd/fs/helpers.go | 37 +++++-------------- cmd/fs/helpers_test.go | 36 +++++++++---------- internal/completer_test.go | 2 +- libs/completer/completer.go | 55 ++++++++++++++++++++++++---- libs/completer/completer_test.go | 61 +++++++++++++++++++++++++------- 5 files changed, 123 insertions(+), 68 deletions(-) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 608cf0f48a..848cd9d041 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -3,8 +3,6 @@ package fs import ( "context" "fmt" - "path" - "path/filepath" "runtime" "strings" @@ -50,8 +48,7 @@ func filerForPath(ctx context.Context, fullPath string) (filer.Filer, string, er return f, path, err } -const dbfsPrefix string = "dbfs:/" -const volumesPefix string = "dbfs:/Volumes" +const dbfsPrefix string = "dbfs:" func isDbfsPath(path string) bool { return strings.HasPrefix(path, dbfsPrefix) @@ -77,7 +74,6 @@ func (v *validArgs) Validate(cmd *cobra.Command, args []string, toComplete strin cmd.SetContext(root.SkipPrompt(cmd.Context())) err := v.mustWorkspaceClientFunc(cmd, args) - if err != nil { return nil, cobra.ShellCompDirectiveError } @@ -87,37 +83,22 @@ func (v *validArgs) Validate(cmd *cobra.Command, args []string, toComplete strin return nil, cobra.ShellCompDirectiveError } - completer := completer.New(cmd.Context(), filer, v.onlyDirs) - if len(args) >= v.pathArgCount { return nil, cobra.ShellCompDirectiveNoFileComp } - completions, dirPath, directive, err := completer.CompletePath(toCompletePath) - - if err != nil { - return nil, cobra.ShellCompDirectiveError - } + completer := completer.New(cmd.Context(), filer, v.onlyDirs) + // Dbfs should have a prefix and always use the "/" separator isDbfsPath := isDbfsPath(toComplete) - - for i := range completions { - completions[i] = filepath.Join(dirPath, completions[i]) - - if isDbfsPath { - // Add dbfs:/ prefix to completions - completions[i] = path.Join(dbfsPrefix, completions[i]) - } + if isDbfsPath { + completer.SetPrefix(dbfsPrefix) + completer.SetIsLocalPath(false) } - // If the path is a dbfs path, we try to add the dbfs:/Volumes prefix suggestion - if isDbfsPath && strings.HasPrefix(volumesPefix, toComplete) { - completions = append(completions, volumesPefix) - } - - // If the path is local, we try to add the dbfs:/ prefix suggestion - if !isDbfsPath && strings.HasPrefix(dbfsPrefix, toComplete) { - completions = append(completions, dbfsPrefix) + completions, directive, err := completer.CompletePath(toCompletePath) + if err != nil { + return nil, cobra.ShellCompDirectiveError } return completions, directive diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 704647a7d0..5f227411b9 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -89,6 +89,11 @@ func setupTest(t *testing.T) (*validArgs, *cobra.Command, *mocks.MockWorkspaceCl "dir/dirB": {FakeDir: true}, "dir/fileA": {}, }) + + if isDbfsPath(fullPath) { + return fakeFiler, "dir", nil + } + return fakeFiler, fullPath, nil } @@ -99,36 +104,27 @@ func setupTest(t *testing.T) (*validArgs, *cobra.Command, *mocks.MockWorkspaceCl return v, cmd, m } -func TestGetValidArgsFunctionCompletion(t *testing.T) { +func TestGetValidArgsFunctionDbfsCompletion(t *testing.T) { v, cmd, _ := setupTest(t) - completions, directive := v.Validate(cmd, []string{}, "dir") - assert.Equal(t, []string{"dir/dirA", "dir/dirB", "dir/fileA"}, completions) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) -} -func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { - v, cmd, _ := setupTest(t) - v.onlyDirs = true - completions, directive := v.Validate(cmd, []string{}, "dir") - assert.Equal(t, []string{"dir/dirA", "dir/dirB"}, completions) + completions, directive := v.Validate(cmd, []string{}, "dbfs:/dir") + + assert.Equal(t, []string{"dbfs:/dir/dirA/", "dbfs:/dir/dirB/", "dbfs:/dir/fileA"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } -func TestGetValidArgsFunctionDbfsPath(t *testing.T) { +func TestGetValidArgsFunctionLocalCompletion(t *testing.T) { v, cmd, _ := setupTest(t) - - completions, directive := v.Validate(cmd, []string{}, "") - - assert.Equal(t, []string{"dbfs:/"}, completions) + completions, directive := v.Validate(cmd, []string{}, "dir") + assert.Equal(t, []string{"dir/dirA/", "dir/dirB/", "dir/fileA", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } -func TestGetValidArgsFunctionVolumesPath(t *testing.T) { +func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { v, cmd, _ := setupTest(t) - - completions, directive := v.Validate(cmd, []string{}, "dbfs:/") - - assert.Equal(t, []string{"dbfs:/Volumes"}, completions) + v.onlyDirs = true + completions, directive := v.Validate(cmd, []string{}, "dbfs:/dir") + assert.Equal(t, []string{"dbfs:/dir/dirA/", "dbfs:/dir/dirB/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } diff --git a/internal/completer_test.go b/internal/completer_test.go index eba22f9b02..0f7d2093d6 100644 --- a/internal/completer_test.go +++ b/internal/completer_test.go @@ -22,6 +22,6 @@ func TestAccFsCompletion(t *testing.T) { setupCompletionFile(t, f) stdout, _ := RequireSuccessfulRun(t, "__complete", "fs", "ls", tmpDir) - expectedOutput := fmt.Sprintf("%s/dir1\n:2\n", tmpDir) + expectedOutput := fmt.Sprintf("%s/dir1/\n:2\n", tmpDir) assert.Equal(t, expectedOutput, stdout.String()) } diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 73e826b535..030c89520b 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -3,6 +3,7 @@ package completer import ( "context" "path" + "path/filepath" "github.com/databricks/cli/libs/filer" "github.com/spf13/cobra" @@ -16,35 +17,47 @@ type completer struct { // CompletePath will only return directories when onlyDirs is true. onlyDirs bool + + prefix string + + isLocalPath bool } // General completer that takes a filer to complete remote paths when TAB-ing through a path. func New(ctx context.Context, filer filer.Filer, onlyDirs bool) *completer { - return &completer{ctx: ctx, filer: filer, onlyDirs: onlyDirs} + return &completer{ctx: ctx, filer: filer, onlyDirs: onlyDirs, prefix: "", isLocalPath: true} } -func (c *completer) CompletePath(p string) ([]string, string, cobra.ShellCompDirective, error) { +func (c *completer) SetPrefix(p string) { + c.prefix = p +} + +func (c *completer) SetIsLocalPath(i bool) { + c.isLocalPath = i +} + +func (c *completer) CompletePath(p string) ([]string, cobra.ShellCompDirective, error) { // If the user is TAB-ing their way through a path, the path in `toComplete` // is valid and we should list nested directories. // If the path in `toComplete` is incomplete, however, // then we should list adjacent directories. dirPath := p completions, err := fetchCompletions(c, dirPath) - if completions == nil && err == nil { + if completions == nil && err == nil && dirPath != path.Dir(p) { dirPath = path.Dir(p) completions, err = fetchCompletions(c, dirPath) } - return completions, dirPath, cobra.ShellCompDirectiveNoSpace, err + return completions, cobra.ShellCompDirectiveNoSpace, err } // List files and directories under the specified path. // Returns a channel such that we can list multiple paths in parallel. func fetchCompletions( c *completer, - path string, + dirPath string, ) ([]string, error) { - entries, err := c.filer.ReadDir(c.ctx, path) + entries, err := c.filer.ReadDir(c.ctx, dirPath) if err != nil { if _, ok := err.(filer.NoSuchDirectoryError); ok { return nil, nil @@ -53,13 +66,41 @@ func fetchCompletions( } } + trailingSeparator := "/" + joinFunc := path.Join + + // Use filepath functions if we are in a local path. + if c.isLocalPath { + joinFunc = filepath.Join + trailingSeparator = string(filepath.Separator) + } + completions := []string{} for _, entry := range entries { if c.onlyDirs && !entry.IsDir() { continue } - completions = append(completions, entry.Name()) + completion := joinFunc(dirPath, entry.Name()) + + // Prepend prefix if it has been set + if c.prefix != "" { + completion = joinFunc(c.prefix, completion) + } + + // Add trailing separator for directories. + if entry.IsDir() { + completion += trailingSeparator + } + + completions = append(completions, completion) + } + + print("frooop", c.isLocalPath) + + // If the path is local, we add the dbfs:/ prefix suggestion as an option + if c.isLocalPath { + completions = append(completions, "dbfs:/") } return completions, nil diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index beca2ebebf..dd895c52ed 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -2,6 +2,7 @@ package completer import ( "context" + "runtime" "testing" "github.com/databricks/cli/cmd/root" @@ -23,45 +24,81 @@ func setupCompleter(t *testing.T, onlyDirs bool) *completer { "dir/fileA": {}, }) - return New(ctx, fakeFiler, onlyDirs) + completer := New(ctx, fakeFiler, onlyDirs) + completer.SetIsLocalPath(false) + return completer +} + +func TestFilerCompleterSetsPrefix(t *testing.T) { + completer := setupCompleter(t, true) + completer.SetPrefix("dbfs:") + completions, directive, err := completer.CompletePath("dir") + + assert.Equal(t, []string{"dbfs:/dir/dirA/", "dbfs:/dir/dirB/"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + assert.Nil(t, err) } func TestFilerCompleterReturnsNestedDirs(t *testing.T) { completer := setupCompleter(t, true) - completions, dirPath, directive, err := completer.CompletePath("dir") + completions, directive, err := completer.CompletePath("dir") - assert.Equal(t, []string{"dirA", "dirB"}, completions) - assert.Equal(t, "dir", dirPath) + assert.Equal(t, []string{"dir/dirA/", "dir/dirB/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) assert.Nil(t, err) } func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { completer := setupCompleter(t, true) - completions, dirPath, directive, err := completer.CompletePath("dir/wrong_dir") + completions, directive, err := completer.CompletePath("dir/wrong_dir") - assert.Equal(t, []string{"dirA", "dirB"}, completions) - assert.Equal(t, "dir", dirPath) + assert.Equal(t, []string{"dir/dirA/", "dir/dirB/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) assert.Nil(t, err) } func TestFilerCompleterReturnsNestedDirsAndFiles(t *testing.T) { completer := setupCompleter(t, false) - completions, dirPath, directive, err := completer.CompletePath("dir") + completions, directive, err := completer.CompletePath("dir") + + assert.Equal(t, []string{"dir/dirA/", "dir/dirB/", "dir/fileA"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + assert.Nil(t, err) +} + +func TestFilerCompleterAddsDbfsPath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip() + } + + completer := setupCompleter(t, true) + completer.SetIsLocalPath(true) + completions, directive, err := completer.CompletePath("dir") + + assert.Equal(t, []string{"dir/dirA/", "dir/dirB/", "dbfs:/"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) + assert.Nil(t, err) +} + +func TestFilerCompleterWindowsSeparator(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip() + } + + completer := setupCompleter(t, true) + completer.SetIsLocalPath(true) + completions, directive, err := completer.CompletePath("dir") - assert.Equal(t, []string{"dirA", "dirB", "fileA"}, completions) - assert.Equal(t, "dir", dirPath) + assert.Equal(t, []string{"dir\\dirA\\", "dir\\dirB\\", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) assert.Nil(t, err) } func TestFilerCompleterNoCompletions(t *testing.T) { completer := setupCompleter(t, true) - completions, dirPath, directive, err := completer.CompletePath("wrong_dir/wrong_dir") + completions, directive, err := completer.CompletePath("wrong_dir/wrong_dir") assert.Nil(t, completions) - assert.Equal(t, "wrong_dir", dirPath) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) assert.Nil(t, err) } From 1b168aeeb5160b932c028b83cccc4db8ce728692 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 7 Aug 2024 11:21:57 +0300 Subject: [PATCH 35/43] Cleanup --- libs/completer/completer.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/libs/completer/completer.go b/libs/completer/completer.go index 030c89520b..d4b49bcca7 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -96,8 +96,6 @@ func fetchCompletions( completions = append(completions, completion) } - print("frooop", c.isLocalPath) - // If the path is local, we add the dbfs:/ prefix suggestion as an option if c.isLocalPath { completions = append(completions, "dbfs:/") From c2ae3f05d2fed04b0e14562896df9df58195dead Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 7 Aug 2024 11:53:12 +0300 Subject: [PATCH 36/43] Simplify path.Dir logic --- cmd/fs/helpers_test.go | 16 ++++------- libs/completer/completer.go | 47 ++++++++++++-------------------- libs/completer/completer_test.go | 18 ++++++------ libs/filer/fake_filer.go | 1 + 4 files changed, 31 insertions(+), 51 deletions(-) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 5f227411b9..5ad46fd302 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -3,6 +3,7 @@ package fs import ( "context" "runtime" + "strings" "testing" "github.com/databricks/cli/cmd/root" @@ -89,12 +90,7 @@ func setupTest(t *testing.T) (*validArgs, *cobra.Command, *mocks.MockWorkspaceCl "dir/dirB": {FakeDir: true}, "dir/fileA": {}, }) - - if isDbfsPath(fullPath) { - return fakeFiler, "dir", nil - } - - return fakeFiler, fullPath, nil + return fakeFiler, strings.TrimPrefix(fullPath, "dbfs:/"), nil } v := newValidArgs() @@ -106,16 +102,14 @@ func setupTest(t *testing.T) (*validArgs, *cobra.Command, *mocks.MockWorkspaceCl func TestGetValidArgsFunctionDbfsCompletion(t *testing.T) { v, cmd, _ := setupTest(t) - - completions, directive := v.Validate(cmd, []string{}, "dbfs:/dir") - + completions, directive := v.Validate(cmd, []string{}, "dbfs:/dir/") assert.Equal(t, []string{"dbfs:/dir/dirA/", "dbfs:/dir/dirB/", "dbfs:/dir/fileA"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } func TestGetValidArgsFunctionLocalCompletion(t *testing.T) { v, cmd, _ := setupTest(t) - completions, directive := v.Validate(cmd, []string{}, "dir") + completions, directive := v.Validate(cmd, []string{}, "dir/") assert.Equal(t, []string{"dir/dirA/", "dir/dirB/", "dir/fileA", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } @@ -123,7 +117,7 @@ func TestGetValidArgsFunctionLocalCompletion(t *testing.T) { func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { v, cmd, _ := setupTest(t) v.onlyDirs = true - completions, directive := v.Validate(cmd, []string{}, "dbfs:/dir") + completions, directive := v.Validate(cmd, []string{}, "dbfs:/dir/") assert.Equal(t, []string{"dbfs:/dir/dirA/", "dbfs:/dir/dirB/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } diff --git a/libs/completer/completer.go b/libs/completer/completer.go index d4b49bcca7..afe8b395a5 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -4,6 +4,7 @@ import ( "context" "path" "path/filepath" + "strings" "github.com/databricks/cli/libs/filer" "github.com/spf13/cobra" @@ -37,35 +38,6 @@ func (c *completer) SetIsLocalPath(i bool) { } func (c *completer) CompletePath(p string) ([]string, cobra.ShellCompDirective, error) { - // If the user is TAB-ing their way through a path, the path in `toComplete` - // is valid and we should list nested directories. - // If the path in `toComplete` is incomplete, however, - // then we should list adjacent directories. - dirPath := p - completions, err := fetchCompletions(c, dirPath) - if completions == nil && err == nil && dirPath != path.Dir(p) { - dirPath = path.Dir(p) - completions, err = fetchCompletions(c, dirPath) - } - - return completions, cobra.ShellCompDirectiveNoSpace, err -} - -// List files and directories under the specified path. -// Returns a channel such that we can list multiple paths in parallel. -func fetchCompletions( - c *completer, - dirPath string, -) ([]string, error) { - entries, err := c.filer.ReadDir(c.ctx, dirPath) - if err != nil { - if _, ok := err.(filer.NoSuchDirectoryError); ok { - return nil, nil - } else { - return nil, err - } - } - trailingSeparator := "/" joinFunc := path.Join @@ -75,12 +47,27 @@ func fetchCompletions( trailingSeparator = string(filepath.Separator) } + // If the user is TAB-ing their way through a path and the + // path ends in a trailing slash, we should list nested directories. + // If the path is incomplete, however, then we should list adjacent + // directories. + dirPath := p + if !strings.HasSuffix(p, trailingSeparator) { + dirPath = path.Dir(p) + } + + entries, err := c.filer.ReadDir(c.ctx, dirPath) + if err != nil { + return nil, cobra.ShellCompDirectiveError, err + } + completions := []string{} for _, entry := range entries { if c.onlyDirs && !entry.IsDir() { continue } + // Join directory path and entry name completion := joinFunc(dirPath, entry.Name()) // Prepend prefix if it has been set @@ -101,5 +88,5 @@ func fetchCompletions( completions = append(completions, "dbfs:/") } - return completions, nil + return completions, cobra.ShellCompDirectiveNoSpace, err } diff --git a/libs/completer/completer_test.go b/libs/completer/completer_test.go index dd895c52ed..c533f0b6cf 100644 --- a/libs/completer/completer_test.go +++ b/libs/completer/completer_test.go @@ -32,7 +32,7 @@ func setupCompleter(t *testing.T, onlyDirs bool) *completer { func TestFilerCompleterSetsPrefix(t *testing.T) { completer := setupCompleter(t, true) completer.SetPrefix("dbfs:") - completions, directive, err := completer.CompletePath("dir") + completions, directive, err := completer.CompletePath("dir/") assert.Equal(t, []string{"dbfs:/dir/dirA/", "dbfs:/dir/dirB/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -41,7 +41,7 @@ func TestFilerCompleterSetsPrefix(t *testing.T) { func TestFilerCompleterReturnsNestedDirs(t *testing.T) { completer := setupCompleter(t, true) - completions, directive, err := completer.CompletePath("dir") + completions, directive, err := completer.CompletePath("dir/") assert.Equal(t, []string{"dir/dirA/", "dir/dirB/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -50,7 +50,7 @@ func TestFilerCompleterReturnsNestedDirs(t *testing.T) { func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { completer := setupCompleter(t, true) - completions, directive, err := completer.CompletePath("dir/wrong_dir") + completions, directive, err := completer.CompletePath("dir/wrong_path") assert.Equal(t, []string{"dir/dirA/", "dir/dirB/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -59,7 +59,7 @@ func TestFilerCompleterReturnsAdjacentDirs(t *testing.T) { func TestFilerCompleterReturnsNestedDirsAndFiles(t *testing.T) { completer := setupCompleter(t, false) - completions, directive, err := completer.CompletePath("dir") + completions, directive, err := completer.CompletePath("dir/") assert.Equal(t, []string{"dir/dirA/", "dir/dirB/", "dir/fileA"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -73,7 +73,7 @@ func TestFilerCompleterAddsDbfsPath(t *testing.T) { completer := setupCompleter(t, true) completer.SetIsLocalPath(true) - completions, directive, err := completer.CompletePath("dir") + completions, directive, err := completer.CompletePath("dir/") assert.Equal(t, []string{"dir/dirA/", "dir/dirB/", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -87,7 +87,7 @@ func TestFilerCompleterWindowsSeparator(t *testing.T) { completer := setupCompleter(t, true) completer.SetIsLocalPath(true) - completions, directive, err := completer.CompletePath("dir") + completions, directive, err := completer.CompletePath("dir/") assert.Equal(t, []string{"dir\\dirA\\", "dir\\dirB\\", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) @@ -99,8 +99,6 @@ func TestFilerCompleterNoCompletions(t *testing.T) { completions, directive, err := completer.CompletePath("wrong_dir/wrong_dir") assert.Nil(t, completions) - assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) - assert.Nil(t, err) + assert.Equal(t, cobra.ShellCompDirectiveError, directive) + assert.Error(t, err) } - -// func TestFilerCompleterReadDirError(t *testing.T) { diff --git a/libs/filer/fake_filer.go b/libs/filer/fake_filer.go index a99ac9fc01..0e650ff605 100644 --- a/libs/filer/fake_filer.go +++ b/libs/filer/fake_filer.go @@ -80,6 +80,7 @@ func (f *FakeFiler) Delete(ctx context.Context, p string, mode ...DeleteMode) er } func (f *FakeFiler) ReadDir(ctx context.Context, p string) ([]fs.DirEntry, error) { + p = strings.TrimSuffix(p, "/") entry, ok := f.entries[p] if !ok { return nil, NoSuchDirectoryError{p} From e5b32fc1aaeec0fc7cecc843e4b888be939529b9 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 7 Aug 2024 12:02:15 +0300 Subject: [PATCH 37/43] Comments --- libs/completer/completer.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/completer/completer.go b/libs/completer/completer.go index afe8b395a5..569286ca38 100644 --- a/libs/completer/completer.go +++ b/libs/completer/completer.go @@ -19,8 +19,11 @@ type completer struct { // CompletePath will only return directories when onlyDirs is true. onlyDirs bool + // Prefix to prepend to completions. prefix string + // Whether the path is local or remote. If the path is local we use the `filepath` + // package for path manipulation. Otherwise we use the `path` package. isLocalPath bool } From 81775a4e1bf6f1e3d06cb6109c77621b9c3c74b5 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 7 Aug 2024 12:04:09 +0300 Subject: [PATCH 38/43] Cleanup --- internal/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/helpers.go b/internal/helpers.go index 3dfa69ab8c..972a2322b5 100644 --- a/internal/helpers.go +++ b/internal/helpers.go @@ -455,6 +455,7 @@ func TemporaryDbfsDir(t *testing.T, w *databricks.WorkspaceClient) string { t.Logf("Creating DBFS folder:%s", path) err := w.Dbfs.MkdirsByPath(ctx, path) require.NoError(t, err) + t.Cleanup(func() { t.Logf("Removing DBFS folder:%s", path) err := w.Dbfs.Delete(ctx, files.Delete{ @@ -590,7 +591,6 @@ func setupDbfsFiler(t *testing.T) (filer.Filer, string) { require.NoError(t, err) tmpDir := TemporaryDbfsDir(t, w) - f, err := filer.NewDbfsClient(w, tmpDir) require.NoError(t, err) From 57ba28833efc7da5dc3bc0426ab0d06c22dc2d66 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Wed, 7 Aug 2024 12:23:56 +0300 Subject: [PATCH 39/43] Add windows test --- cmd/fs/helpers_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cmd/fs/helpers_test.go b/cmd/fs/helpers_test.go index 5ad46fd302..10b4aa1604 100644 --- a/cmd/fs/helpers_test.go +++ b/cmd/fs/helpers_test.go @@ -108,12 +108,27 @@ func TestGetValidArgsFunctionDbfsCompletion(t *testing.T) { } func TestGetValidArgsFunctionLocalCompletion(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip() + } + v, cmd, _ := setupTest(t) completions, directive := v.Validate(cmd, []string{}, "dir/") assert.Equal(t, []string{"dir/dirA/", "dir/dirB/", "dir/fileA", "dbfs:/"}, completions) assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) } +func TestGetValidArgsFunctionLocalCompletionWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip() + } + + v, cmd, _ := setupTest(t) + completions, directive := v.Validate(cmd, []string{}, "dir/") + assert.Equal(t, []string{"dir\\dirA\\", "dir\\dirB\\", "dir\\fileA", "dbfs:/"}, completions) + assert.Equal(t, cobra.ShellCompDirectiveNoSpace, directive) +} + func TestGetValidArgsFunctionCompletionOnlyDirs(t *testing.T) { v, cmd, _ := setupTest(t) v.onlyDirs = true From 0eac4e72b38b35c4eac6a0a472fa7486dcec5f13 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 9 Aug 2024 11:00:46 +0300 Subject: [PATCH 40/43] PR comments --- cmd/fs/helpers.go | 10 +++++----- internal/fs_ls_test.go | 2 ++ libs/{ => filer}/completer/completer.go | 0 libs/{ => filer}/completer/completer_test.go | 0 4 files changed, 7 insertions(+), 5 deletions(-) rename libs/{ => filer}/completer/completer.go (100%) rename libs/{ => filer}/completer/completer_test.go (100%) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go index 848cd9d041..bda3239cf2 100644 --- a/cmd/fs/helpers.go +++ b/cmd/fs/helpers.go @@ -7,8 +7,8 @@ import ( "strings" "github.com/databricks/cli/cmd/root" - "github.com/databricks/cli/libs/completer" "github.com/databricks/cli/libs/filer" + "github.com/databricks/cli/libs/filer/completer" "github.com/spf13/cobra" ) @@ -73,6 +73,10 @@ func newValidArgs() *validArgs { func (v *validArgs) Validate(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { cmd.SetContext(root.SkipPrompt(cmd.Context())) + if len(args) >= v.pathArgCount { + return nil, cobra.ShellCompDirectiveNoFileComp + } + err := v.mustWorkspaceClientFunc(cmd, args) if err != nil { return nil, cobra.ShellCompDirectiveError @@ -83,10 +87,6 @@ func (v *validArgs) Validate(cmd *cobra.Command, args []string, toComplete strin return nil, cobra.ShellCompDirectiveError } - if len(args) >= v.pathArgCount { - return nil, cobra.ShellCompDirectiveNoFileComp - } - completer := completer.New(cmd.Context(), filer, v.onlyDirs) // Dbfs should have a prefix and always use the "/" separator diff --git a/internal/fs_ls_test.go b/internal/fs_ls_test.go index 05623cc18f..994a4a4255 100644 --- a/internal/fs_ls_test.go +++ b/internal/fs_ls_test.go @@ -124,8 +124,10 @@ func TestAccFsLsOnFile(t *testing.T) { func TestAccFsLsOnEmptyDir(t *testing.T) { t.Parallel() + for _, testCase := range fsTests { tc := testCase + t.Run(tc.name, func(t *testing.T) { t.Parallel() diff --git a/libs/completer/completer.go b/libs/filer/completer/completer.go similarity index 100% rename from libs/completer/completer.go rename to libs/filer/completer/completer.go diff --git a/libs/completer/completer_test.go b/libs/filer/completer/completer_test.go similarity index 100% rename from libs/completer/completer_test.go rename to libs/filer/completer/completer_test.go From 6c85d5198af722300dd3ce0b4dfb709e8de806ab Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 9 Aug 2024 11:55:31 +0300 Subject: [PATCH 41/43] Fix TestGlobFileset --- libs/fileset/glob_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libs/fileset/glob_test.go b/libs/fileset/glob_test.go index 70b9c444b2..72f9f652b1 100644 --- a/libs/fileset/glob_test.go +++ b/libs/fileset/glob_test.go @@ -32,7 +32,8 @@ func TestGlobFileset(t *testing.T) { files, err := g.All() require.NoError(t, err) - require.Equal(t, len(files), len(entries)) + // +1 as there's one folder in ../filer + require.Equal(t, len(files)+1, len(entries)) for _, f := range files { exists := slices.ContainsFunc(entries, func(de fs.DirEntry) bool { return de.Name() == path.Base(f.Relative) @@ -62,7 +63,8 @@ func TestGlobFilesetWithRelativeRoot(t *testing.T) { files, err := g.All() require.NoError(t, err) - require.Equal(t, len(files), len(entries)) + // +1 as there's one folder in ../filer + require.Equal(t, len(files)+1, len(entries)) } func TestGlobFilesetRecursively(t *testing.T) { From 324e4ab714aa7acd9bf56a9a26ca40a52bb6e767 Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 9 Aug 2024 11:58:42 +0300 Subject: [PATCH 42/43] Try again --- libs/fileset/glob_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/fileset/glob_test.go b/libs/fileset/glob_test.go index 72f9f652b1..473fd51e81 100644 --- a/libs/fileset/glob_test.go +++ b/libs/fileset/glob_test.go @@ -33,7 +33,7 @@ func TestGlobFileset(t *testing.T) { require.NoError(t, err) // +1 as there's one folder in ../filer - require.Equal(t, len(files)+1, len(entries)) + require.Equal(t, len(files), len(entries)+1) for _, f := range files { exists := slices.ContainsFunc(entries, func(de fs.DirEntry) bool { return de.Name() == path.Base(f.Relative) @@ -64,7 +64,7 @@ func TestGlobFilesetWithRelativeRoot(t *testing.T) { files, err := g.All() require.NoError(t, err) // +1 as there's one folder in ../filer - require.Equal(t, len(files)+1, len(entries)) + require.Equal(t, len(files), len(entries)+1) } func TestGlobFilesetRecursively(t *testing.T) { From 354f4b9655131d632451c38242f210e195267ddf Mon Sep 17 00:00:00 2001 From: Anders Rex Date: Fri, 9 Aug 2024 12:31:33 +0300 Subject: [PATCH 43/43] Dont use ../filer in test --- libs/fileset/glob_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/libs/fileset/glob_test.go b/libs/fileset/glob_test.go index 473fd51e81..8418df73a2 100644 --- a/libs/fileset/glob_test.go +++ b/libs/fileset/glob_test.go @@ -20,7 +20,7 @@ func collectRelativePaths(files []File) []string { } func TestGlobFileset(t *testing.T) { - root := vfs.MustNew("../filer") + root := vfs.MustNew("./") entries, err := root.ReadDir(".") require.NoError(t, err) @@ -33,7 +33,7 @@ func TestGlobFileset(t *testing.T) { require.NoError(t, err) // +1 as there's one folder in ../filer - require.Equal(t, len(files), len(entries)+1) + require.Equal(t, len(files), len(entries)) for _, f := range files { exists := slices.ContainsFunc(entries, func(de fs.DirEntry) bool { return de.Name() == path.Base(f.Relative) @@ -52,7 +52,7 @@ func TestGlobFileset(t *testing.T) { } func TestGlobFilesetWithRelativeRoot(t *testing.T) { - root := vfs.MustNew("../filer") + root := vfs.MustNew("../set") entries, err := root.ReadDir(".") require.NoError(t, err) @@ -63,8 +63,7 @@ func TestGlobFilesetWithRelativeRoot(t *testing.T) { files, err := g.All() require.NoError(t, err) - // +1 as there's one folder in ../filer - require.Equal(t, len(files), len(entries)+1) + require.Equal(t, len(files), len(entries)) } func TestGlobFilesetRecursively(t *testing.T) {