From a4c1ba3e2827abca29034115436d441310b7ee33 Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Wed, 21 Aug 2024 13:15:25 +0530 Subject: [PATCH] Use API mocks for duplicate path errors in workspace files extensions client (#1690) ## Changes `TestAccFilerWorkspaceFilesExtensionsErrorsOnDupName` recently started failing in our nightlies because the upstream `import` API was changed to [prohibit conflicting file paths](https://docs.databricks.com/en/release-notes/product/2024/august.html#files-can-no-longer-have-identical-names-in-workspace-folders). Because existing conflicting file paths can still be grandfathered in, we need to retain coverage for the test. To do this, this PR: 1. Removes the failing `TestAccFilerWorkspaceFilesExtensionsErrorsOnDupName` 2. Add an equivalent unit test with the `list` and `get-status` API calls mocked. --- internal/filer_test.go | 62 ------- libs/filer/workspace_files_client.go | 26 +-- .../workspace_files_extensions_client.go | 8 +- .../workspace_files_extensions_client_test.go | 151 ++++++++++++++++++ 4 files changed, 172 insertions(+), 75 deletions(-) create mode 100644 libs/filer/workspace_files_extensions_client_test.go diff --git a/internal/filer_test.go b/internal/filer_test.go index 27530425..bc4c9480 100644 --- a/internal/filer_test.go +++ b/internal/filer_test.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "errors" - "fmt" "io" "io/fs" "path" @@ -722,67 +721,6 @@ func TestAccFilerWorkspaceFilesExtensionsStat(t *testing.T) { assert.ErrorIs(t, err, fs.ErrNotExist) } -func TestAccFilerWorkspaceFilesExtensionsErrorsOnDupName(t *testing.T) { - t.Parallel() - - tcases := []struct { - files []struct{ name, content string } - name string - }{ - { - name: "python", - files: []struct{ name, content string }{ - {"foo.py", "print('foo')"}, - {"foo.py", "# Databricks notebook source\nprint('foo')"}, - }, - }, - { - name: "r", - files: []struct{ name, content string }{ - {"foo.r", "print('foo')"}, - {"foo.r", "# Databricks notebook source\nprint('foo')"}, - }, - }, - { - name: "sql", - files: []struct{ name, content string }{ - {"foo.sql", "SELECT 'foo'"}, - {"foo.sql", "-- Databricks notebook source\nSELECT 'foo'"}, - }, - }, - { - name: "scala", - files: []struct{ name, content string }{ - {"foo.scala", "println('foo')"}, - {"foo.scala", "// Databricks notebook source\nprintln('foo')"}, - }, - }, - // We don't need to test this for ipynb notebooks. The import API - // fails when the file extension is .ipynb but the content is not a - // valid juptyer notebook. - } - - for i := range tcases { - tc := tcases[i] - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - ctx := context.Background() - wf, tmpDir := setupWsfsExtensionsFiler(t) - - for _, f := range tc.files { - err := wf.Write(ctx, f.name, strings.NewReader(f.content), filer.CreateParentDirectories) - require.NoError(t, err) - } - - _, err := wf.ReadDir(ctx, ".") - assert.ErrorAs(t, err, &filer.DuplicatePathError{}) - assert.ErrorContains(t, err, fmt.Sprintf("failed to read files from the workspace file system. Duplicate paths encountered. Both NOTEBOOK at %s and FILE at %s resolve to the same name %s. Changing the name of one of these objects will resolve this issue", path.Join(tmpDir, "foo"), path.Join(tmpDir, tc.files[0].name), tc.files[0].name)) - }) - } - -} - func TestAccWorkspaceFilesExtensionsDirectoriesAreNotNotebooks(t *testing.T) { t.Parallel() diff --git a/libs/filer/workspace_files_client.go b/libs/filer/workspace_files_client.go index e911f440..d8ab5a6b 100644 --- a/libs/filer/workspace_files_client.go +++ b/libs/filer/workspace_files_client.go @@ -102,13 +102,21 @@ func (info *wsfsFileInfo) MarshalJSON() ([]byte, error) { return marshal.Marshal(info) } +// Interface for *client.DatabricksClient from the Databricks Go SDK. Abstracted +// as an interface to allow for mocking in tests. +type apiClient interface { + Do(ctx context.Context, method, path string, + headers map[string]string, request, response any, + visitors ...func(*http.Request) error) error +} + // WorkspaceFilesClient implements the files-in-workspace API. // NOTE: This API is available for files under /Repos if a workspace has files-in-repos enabled. // It can access any workspace path if files-in-workspace is enabled. -type WorkspaceFilesClient struct { +type workspaceFilesClient struct { workspaceClient *databricks.WorkspaceClient - apiClient *client.DatabricksClient + apiClient apiClient // File operations will be relative to this path. root WorkspaceRootPath @@ -120,7 +128,7 @@ func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer, return nil, err } - return &WorkspaceFilesClient{ + return &workspaceFilesClient{ workspaceClient: w, apiClient: apiClient, @@ -128,7 +136,7 @@ func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer, }, nil } -func (w *WorkspaceFilesClient) Write(ctx context.Context, name string, reader io.Reader, mode ...WriteMode) error { +func (w *workspaceFilesClient) Write(ctx context.Context, name string, reader io.Reader, mode ...WriteMode) error { absPath, err := w.root.Join(name) if err != nil { return err @@ -198,7 +206,7 @@ func (w *WorkspaceFilesClient) Write(ctx context.Context, name string, reader io return err } -func (w *WorkspaceFilesClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { +func (w *workspaceFilesClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { absPath, err := w.root.Join(name) if err != nil { return nil, err @@ -222,7 +230,7 @@ func (w *WorkspaceFilesClient) Read(ctx context.Context, name string) (io.ReadCl return w.workspaceClient.Workspace.Download(ctx, absPath) } -func (w *WorkspaceFilesClient) Delete(ctx context.Context, name string, mode ...DeleteMode) error { +func (w *workspaceFilesClient) Delete(ctx context.Context, name string, mode ...DeleteMode) error { absPath, err := w.root.Join(name) if err != nil { return err @@ -266,7 +274,7 @@ func (w *WorkspaceFilesClient) Delete(ctx context.Context, name string, mode ... return err } -func (w *WorkspaceFilesClient) ReadDir(ctx context.Context, name string) ([]fs.DirEntry, error) { +func (w *workspaceFilesClient) ReadDir(ctx context.Context, name string) ([]fs.DirEntry, error) { absPath, err := w.root.Join(name) if err != nil { return nil, err @@ -299,7 +307,7 @@ func (w *WorkspaceFilesClient) ReadDir(ctx context.Context, name string) ([]fs.D return wsfsDirEntriesFromObjectInfos(objects), nil } -func (w *WorkspaceFilesClient) Mkdir(ctx context.Context, name string) error { +func (w *workspaceFilesClient) Mkdir(ctx context.Context, name string) error { dirPath, err := w.root.Join(name) if err != nil { return err @@ -309,7 +317,7 @@ func (w *WorkspaceFilesClient) Mkdir(ctx context.Context, name string) error { }) } -func (w *WorkspaceFilesClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { +func (w *workspaceFilesClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { absPath, err := w.root.Join(name) if err != nil { return nil, err diff --git a/libs/filer/workspace_files_extensions_client.go b/libs/filer/workspace_files_extensions_client.go index 844e736b..b24ecf7e 100644 --- a/libs/filer/workspace_files_extensions_client.go +++ b/libs/filer/workspace_files_extensions_client.go @@ -133,14 +133,14 @@ func (w *workspaceFilesExtensionsClient) getNotebookStatByNameWithoutExt(ctx con }, nil } -type DuplicatePathError struct { +type duplicatePathError struct { oi1 workspace.ObjectInfo oi2 workspace.ObjectInfo commonName string } -func (e DuplicatePathError) Error() string { +func (e duplicatePathError) Error() string { return fmt.Sprintf("failed to read files from the workspace file system. Duplicate paths encountered. Both %s at %s and %s at %s resolve to the same name %s. Changing the name of one of these objects will resolve this issue", e.oi1.ObjectType, e.oi1.Path, e.oi2.ObjectType, e.oi2.Path, e.commonName) } @@ -157,7 +157,7 @@ func (e ReadOnlyError) Error() string { // delete, and stat notebooks (and files in general) in the workspace, using their paths // with the extension included. // -// The ReadDir method returns a DuplicatePathError if this traditional file system view is +// The ReadDir method returns a duplicatePathError if this traditional file system view is // not possible. For example, a Python notebook called foo and a Python file called `foo.py` // would resolve to the same path `foo.py` in a tradition file system. // @@ -220,7 +220,7 @@ func (w *workspaceFilesExtensionsClient) ReadDir(ctx context.Context, name strin // Error if we have seen this path before in the current directory. // If not seen before, add it to the seen paths. if _, ok := seenPaths[entries[i].Name()]; ok { - return nil, DuplicatePathError{ + return nil, duplicatePathError{ oi1: seenPaths[entries[i].Name()], oi2: sysInfo, commonName: path.Join(name, entries[i].Name()), diff --git a/libs/filer/workspace_files_extensions_client_test.go b/libs/filer/workspace_files_extensions_client_test.go new file mode 100644 index 00000000..321c4371 --- /dev/null +++ b/libs/filer/workspace_files_extensions_client_test.go @@ -0,0 +1,151 @@ +package filer + +import ( + "context" + "net/http" + "testing" + + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/databricks/databricks-sdk-go/service/workspace" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +// Mocks client.DatabricksClient from the databricks-sdk-go package. +type mockApiClient struct { + mock.Mock +} + +func (m *mockApiClient) Do(ctx context.Context, method, path string, + headers map[string]string, request any, response any, + visitors ...func(*http.Request) error) error { + args := m.Called(ctx, method, path, headers, request, response, visitors) + + // Set the http response from a value provided in the mock call. + p := response.(*wsfsFileInfo) + *p = args.Get(1).(wsfsFileInfo) + return args.Error(0) +} + +func TestFilerWorkspaceFilesExtensionsErrorsOnDupName(t *testing.T) { + for _, tc := range []struct { + name string + language workspace.Language + notebookExportFormat workspace.ExportFormat + notebookPath string + filePath string + expectedError string + }{ + { + name: "python source notebook and file", + language: workspace.LanguagePython, + notebookExportFormat: workspace.ExportFormatSource, + notebookPath: "/dir/foo", + filePath: "/dir/foo.py", + expectedError: "failed to read files from the workspace file system. Duplicate paths encountered. Both NOTEBOOK at /dir/foo and FILE at /dir/foo.py resolve to the same name /foo.py. Changing the name of one of these objects will resolve this issue", + }, + { + name: "python jupyter notebook and file", + language: workspace.LanguagePython, + notebookExportFormat: workspace.ExportFormatJupyter, + notebookPath: "/dir/foo", + filePath: "/dir/foo.py", + // Jupyter notebooks would correspond to foo.ipynb so an error is not expected. + expectedError: "", + }, + { + name: "scala source notebook and file", + language: workspace.LanguageScala, + notebookExportFormat: workspace.ExportFormatSource, + notebookPath: "/dir/foo", + filePath: "/dir/foo.scala", + expectedError: "failed to read files from the workspace file system. Duplicate paths encountered. Both NOTEBOOK at /dir/foo and FILE at /dir/foo.scala resolve to the same name /foo.scala. Changing the name of one of these objects will resolve this issue", + }, + { + name: "r source notebook and file", + language: workspace.LanguageR, + notebookExportFormat: workspace.ExportFormatSource, + notebookPath: "/dir/foo", + filePath: "/dir/foo.r", + expectedError: "failed to read files from the workspace file system. Duplicate paths encountered. Both NOTEBOOK at /dir/foo and FILE at /dir/foo.r resolve to the same name /foo.r. Changing the name of one of these objects will resolve this issue", + }, + { + name: "sql source notebook and file", + language: workspace.LanguageSql, + notebookExportFormat: workspace.ExportFormatSource, + notebookPath: "/dir/foo", + filePath: "/dir/foo.sql", + expectedError: "failed to read files from the workspace file system. Duplicate paths encountered. Both NOTEBOOK at /dir/foo and FILE at /dir/foo.sql resolve to the same name /foo.sql. Changing the name of one of these objects will resolve this issue", + }, + { + name: "python jupyter notebook and file", + language: workspace.LanguagePython, + notebookExportFormat: workspace.ExportFormatJupyter, + notebookPath: "/dir/foo", + filePath: "/dir/foo.ipynb", + expectedError: "failed to read files from the workspace file system. Duplicate paths encountered. Both NOTEBOOK at /dir/foo and FILE at /dir/foo.ipynb resolve to the same name /foo.ipynb. Changing the name of one of these objects will resolve this issue", + }, + } { + t.Run(tc.name, func(t *testing.T) { + mockedWorkspaceClient := mocks.NewMockWorkspaceClient(t) + mockedApiClient := mockApiClient{} + + // Mock the workspace API's ListAll method. + workspaceApi := mockedWorkspaceClient.GetMockWorkspaceAPI() + workspaceApi.EXPECT().ListAll(mock.Anything, workspace.ListWorkspaceRequest{ + Path: "/dir", + }).Return([]workspace.ObjectInfo{ + { + Path: tc.filePath, + Language: tc.language, + ObjectType: workspace.ObjectTypeFile, + }, + { + Path: tc.notebookPath, + Language: tc.language, + ObjectType: workspace.ObjectTypeNotebook, + }, + }, nil) + + // Mock bespoke API calls to /api/2.0/workspace/get-status, that are + // used to figure out the right file extension for the notebook. + statNotebook := wsfsFileInfo{ + ObjectInfo: workspace.ObjectInfo{ + Path: tc.notebookPath, + Language: tc.language, + ObjectType: workspace.ObjectTypeNotebook, + }, + ReposExportFormat: tc.notebookExportFormat, + } + + mockedApiClient.On("Do", mock.Anything, http.MethodGet, "/api/2.0/workspace/get-status", map[string]string(nil), map[string]string{ + "path": tc.notebookPath, + "return_export_info": "true", + }, mock.AnythingOfType("*filer.wsfsFileInfo"), []func(*http.Request) error(nil)).Return(nil, statNotebook) + + workspaceFilesClient := workspaceFilesClient{ + workspaceClient: mockedWorkspaceClient.WorkspaceClient, + apiClient: &mockedApiClient, + root: NewWorkspaceRootPath("/dir"), + } + + workspaceFilesExtensionsClient := workspaceFilesExtensionsClient{ + workspaceClient: mockedWorkspaceClient.WorkspaceClient, + wsfs: &workspaceFilesClient, + } + + _, err := workspaceFilesExtensionsClient.ReadDir(context.Background(), "/") + + if tc.expectedError == "" { + assert.NoError(t, err) + } else { + assert.ErrorAs(t, err, &duplicatePathError{}) + assert.EqualError(t, err, tc.expectedError) + } + + // assert the mocked methods were actually called, as a sanity check. + workspaceApi.AssertNumberOfCalls(t, "ListAll", 1) + mockedApiClient.AssertNumberOfCalls(t, "Do", 1) + }) + } +}