From 64c843e221fd41595c6b2873133a0949f91c2256 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 8 Nov 2024 21:08:12 +0100 Subject: [PATCH 1/4] Fix workspace extensions filer accidentally reading notebook --- internal/filer_test.go | 38 +++++++++++++++++++ internal/helpers.go | 7 ++-- .../workspace_files_extensions_client.go | 29 +++++++++++++- 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/internal/filer_test.go b/internal/filer_test.go index bc4c94808e..207ce68e82 100644 --- a/internal/filer_test.go +++ b/internal/filer_test.go @@ -736,6 +736,44 @@ func TestAccWorkspaceFilesExtensionsDirectoriesAreNotNotebooks(t *testing.T) { assert.ErrorIs(t, err, fs.ErrNotExist) } +func TestAccWorkspaceFilesExtensionsNotebooksAreNotReadAsFiles(t *testing.T) { + t.Parallel() + + ctx := context.Background() + wf, _ := setupWsfsExtensionsFiler(t) + + // Create a notebook + err := wf.Write(ctx, "foo.ipynb", strings.NewReader(jupyterNotebookContent1)) + require.NoError(t, err) + + // Reading foo should fail. Even though the WSFS name for the notebook is foo + // reading the notebook should only work with the .ipynb extension. + _, err = wf.Read(ctx, "foo") + assert.ErrorIs(t, err, fs.ErrNotExist) + + _, err = wf.Read(ctx, "foo.ipynb") + assert.NoError(t, err) +} + +func TestAccWorkspaceFilesExtensionsNotebooksAreNotStatAsFiles(t *testing.T) { + t.Parallel() + + ctx := context.Background() + wf, _ := setupWsfsExtensionsFiler(t) + + // Create a notebook + err := wf.Write(ctx, "foo.ipynb", strings.NewReader(jupyterNotebookContent1)) + require.NoError(t, err) + + // Reading foo should fail. Even though the WSFS name for the notebook is foo + // reading the notebook should only work with the .ipynb extension. + _, err = wf.Stat(ctx, "foo") + assert.ErrorIs(t, err, fs.ErrNotExist) + + _, err = wf.Read(ctx, "foo.ipynb") + assert.NoError(t, err) +} + func TestAccWorkspaceFilesExtensions_ExportFormatIsPreserved(t *testing.T) { t.Parallel() diff --git a/internal/helpers.go b/internal/helpers.go index 3bf3877575..2c917127c1 100644 --- a/internal/helpers.go +++ b/internal/helpers.go @@ -581,11 +581,10 @@ func setupWsfsFiler(t *testing.T) (filer.Filer, string) { } func setupWsfsExtensionsFiler(t *testing.T) (filer.Filer, string) { - t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) + _, wt := acc.WorkspaceTest(t) - w := databricks.Must(databricks.NewWorkspaceClient()) - tmpdir := TemporaryWorkspaceDir(t, w) - f, err := filer.NewWorkspaceFilesExtensionsClient(w, tmpdir) + tmpdir := TemporaryWorkspaceDir(t, wt.W) + f, err := filer.NewWorkspaceFilesExtensionsClient(wt.W, tmpdir) require.NoError(t, err) return f, tmpdir diff --git a/libs/filer/workspace_files_extensions_client.go b/libs/filer/workspace_files_extensions_client.go index b24ecf7eef..ace0799683 100644 --- a/libs/filer/workspace_files_extensions_client.go +++ b/libs/filer/workspace_files_extensions_client.go @@ -245,6 +245,17 @@ func (w *workspaceFilesExtensionsClient) Write(ctx context.Context, name string, // Try to read the file as a regular file. If the file is not found, try to read it as a notebook. func (w *workspaceFilesExtensionsClient) Read(ctx context.Context, name string) (io.ReadCloser, error) { + // Ensure that the file / notebook exists. We do this check here to avoid reading + // the content of a notebook called `foo` when the user actually wanted + // to read the content of a file called `foo`. + // + // To read the content of a notebook called `foo` in the workspace the user + // should use the name with the extension included like `foo.ipynb` or `foo.sql`. + _, err := w.Stat(ctx, name) + if err != nil { + return nil, err + } + r, err := w.wsfs.Read(ctx, name) // If the file is not found, it might be a notebook. @@ -301,6 +312,22 @@ func (w *workspaceFilesExtensionsClient) Delete(ctx context.Context, name string func (w *workspaceFilesExtensionsClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { info, err := w.wsfs.Stat(ctx, name) + // If an object is found and it is not a notebook, return the stat object. This check is done + // to avoid returning the stat for a notebook called `foo` when the user actually + // wanted to stat a file called `foo`. + // + // To stat the metadata of a notebook called `foo` in the workspace the user + // should use the name with the extension included like `foo.ipynb` or `foo.sql`. + if err == nil && info.Sys().(workspace.ObjectInfo).ObjectType != workspace.ObjectTypeNotebook { + return info, nil + } + + // If a notebook is found by the workspace files client, without having stripped + // the extension, this implies that no file with the same name exists. + if err == nil && info.Sys().(workspace.ObjectInfo).ObjectType == workspace.ObjectTypeNotebook { + return nil, FileDoesNotExistError{name} + } + // If the file is not found, it might be a notebook. if errors.As(err, &FileDoesNotExistError{}) { stat, serr := w.getNotebookStatByNameWithExt(ctx, name) @@ -316,7 +343,7 @@ func (w *workspaceFilesExtensionsClient) Stat(ctx context.Context, name string) return wsfsFileInfo{ObjectInfo: stat.ObjectInfo}, nil } - return info, err + return nil, err } // Note: The import API returns opaque internal errors for namespace clashes From f8f1e8b5dba6dd0920b42eafe08c3a2b0d9532a3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 14 Nov 2024 16:58:02 +0100 Subject: [PATCH 2/4] update fixture used --- internal/filer_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/filer_test.go b/internal/filer_test.go index f9f4e8840f..44605bacb6 100644 --- a/internal/filer_test.go +++ b/internal/filer_test.go @@ -730,7 +730,7 @@ func TestAccWorkspaceFilesExtensionsNotebooksAreNotReadAsFiles(t *testing.T) { wf, _ := setupWsfsExtensionsFiler(t) // Create a notebook - err := wf.Write(ctx, "foo.ipynb", strings.NewReader(jupyterNotebookContent1)) + err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb"))) require.NoError(t, err) // Reading foo should fail. Even though the WSFS name for the notebook is foo @@ -749,7 +749,7 @@ func TestAccWorkspaceFilesExtensionsNotebooksAreNotStatAsFiles(t *testing.T) { wf, _ := setupWsfsExtensionsFiler(t) // Create a notebook - err := wf.Write(ctx, "foo.ipynb", strings.NewReader(jupyterNotebookContent1)) + err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb"))) require.NoError(t, err) // Reading foo should fail. Even though the WSFS name for the notebook is foo From 350cb8e4db8965152030c1d3de3528d44390f5a1 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 14 Nov 2024 17:04:55 +0100 Subject: [PATCH 3/4] move the success case down --- .../workspace_files_extensions_client.go | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/libs/filer/workspace_files_extensions_client.go b/libs/filer/workspace_files_extensions_client.go index 31def35671..65485a46f8 100644 --- a/libs/filer/workspace_files_extensions_client.go +++ b/libs/filer/workspace_files_extensions_client.go @@ -311,22 +311,6 @@ func (w *workspaceFilesExtensionsClient) Delete(ctx context.Context, name string func (w *workspaceFilesExtensionsClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) { info, err := w.wsfs.Stat(ctx, name) - // If an object is found and it is not a notebook, return the stat object. This check is done - // to avoid returning the stat for a notebook called `foo` when the user actually - // wanted to stat a file called `foo`. - // - // To stat the metadata of a notebook called `foo` in the workspace the user - // should use the name with the extension included like `foo.ipynb` or `foo.sql`. - if err == nil && info.Sys().(workspace.ObjectInfo).ObjectType != workspace.ObjectTypeNotebook { - return info, nil - } - - // If a notebook is found by the workspace files client, without having stripped - // the extension, this implies that no file with the same name exists. - if err == nil && info.Sys().(workspace.ObjectInfo).ObjectType == workspace.ObjectTypeNotebook { - return nil, FileDoesNotExistError{name} - } - // If the file is not found, it might be a notebook. if errors.As(err, &FileDoesNotExistError{}) { stat, serr := w.getNotebookStatByNameWithExt(ctx, name) @@ -342,7 +326,24 @@ func (w *workspaceFilesExtensionsClient) Stat(ctx context.Context, name string) return wsfsFileInfo{ObjectInfo: stat.ObjectInfo}, nil } - return nil, err + if err != nil { + return nil, err + } + + // If an object is found and it is a notebook, return a FileDoesNotExistError. + // If a notebook is found by the workspace files client, without having stripped + // the extension, this implies that no file with the same name exists. + // + // This check is done to avoid returning the stat for a notebook called `foo` + // when the user actually wanted to stat a file called `foo`. + // + // To stat the metadata of a notebook called `foo` in the workspace the user + // should use the name with the extension included like `foo.ipynb` or `foo.sql`. + if info.Sys().(workspace.ObjectInfo).ObjectType == workspace.ObjectTypeNotebook { + return nil, FileDoesNotExistError{name} + } + + return info, nil } // Note: The import API returns opaque internal errors for namespace clashes From 58349b209c9574165c9b9cfd7a055e14143055bf Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 14 Nov 2024 17:18:26 +0100 Subject: [PATCH 4/4] add support for delete' --- internal/filer_test.go | 25 ++++++++++++++++--- .../workspace_files_extensions_client.go | 13 +++++++++- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/internal/filer_test.go b/internal/filer_test.go index 44605bacb6..a2760d9113 100644 --- a/internal/filer_test.go +++ b/internal/filer_test.go @@ -752,12 +752,31 @@ func TestAccWorkspaceFilesExtensionsNotebooksAreNotStatAsFiles(t *testing.T) { err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb"))) require.NoError(t, err) - // Reading foo should fail. Even though the WSFS name for the notebook is foo - // reading the notebook should only work with the .ipynb extension. + // Stating foo should fail. Even though the WSFS name for the notebook is foo + // stating the notebook should only work with the .ipynb extension. _, err = wf.Stat(ctx, "foo") assert.ErrorIs(t, err, fs.ErrNotExist) - _, err = wf.Read(ctx, "foo.ipynb") + _, err = wf.Stat(ctx, "foo.ipynb") + assert.NoError(t, err) +} + +func TestAccWorkspaceFilesExtensionsNotebooksAreNotDeletedAsFiles(t *testing.T) { + t.Parallel() + + ctx := context.Background() + wf, _ := setupWsfsExtensionsFiler(t) + + // Create a notebook + err := wf.Write(ctx, "foo.ipynb", strings.NewReader(readFile(t, "testdata/notebooks/py1.ipynb"))) + require.NoError(t, err) + + // Deleting foo should fail. Even though the WSFS name for the notebook is foo + // deleting the notebook should only work with the .ipynb extension. + err = wf.Delete(ctx, "foo") + assert.ErrorIs(t, err, fs.ErrNotExist) + + err = wf.Delete(ctx, "foo.ipynb") assert.NoError(t, err) } diff --git a/libs/filer/workspace_files_extensions_client.go b/libs/filer/workspace_files_extensions_client.go index 65485a46f8..2a60520911 100644 --- a/libs/filer/workspace_files_extensions_client.go +++ b/libs/filer/workspace_files_extensions_client.go @@ -287,7 +287,18 @@ func (w *workspaceFilesExtensionsClient) Delete(ctx context.Context, name string return ReadOnlyError{"delete"} } - err := w.wsfs.Delete(ctx, name, mode...) + // Ensure that the file / notebook exists. We do this check here to avoid + // deleting the a notebook called `foo` when the user actually wanted to + // delete a file called `foo`. + // + // To delete a notebook called `foo` in the workspace the user should use the + // name with the extension included like `foo.ipynb` or `foo.sql`. + _, err := w.Stat(ctx, name) + if err != nil { + return err + } + + err = w.wsfs.Delete(ctx, name, mode...) // If the file is not found, it might be a notebook. if errors.As(err, &FileDoesNotExistError{}) {