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.
2024-08-21 13:15:25 +05:30 · 2024-08-21 13:15:25 +05:30 · a4c1ba3e28
parent 44902fa350
commit a4c1ba3e28
4 changed files with 172 additions and 75 deletions
--- 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()

--- 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
--- 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()),
--- a/libs/filer/workspace_files_extensions_client_test.go
+++ 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)
+		})
+	}
+}