From 251021c41e2afc38853e6cd114a6ee9daf7bd392 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 9 Jun 2026 22:44:53 +0200 Subject: [PATCH 1/2] Skip notebooks with unmapped language in extension-aware ReadDir ReadDir in the workspace files extensions client appended an empty extension for notebooks whose language has no extension mapping, listing a name that the same filer's Stat and Read deliberately report as not found. Omit such entries from the listing and log a warning instead. Co-authored-by: Isaac --- .../workspace_files_extensions_client.go | 19 +++- .../workspace_files_extensions_client_test.go | 90 +++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/libs/filer/workspace_files_extensions_client.go b/libs/filer/workspace_files_extensions_client.go index 44e21432c51..e97041da0ce 100644 --- a/libs/filer/workspace_files_extensions_client.go +++ b/libs/filer/workspace_files_extensions_client.go @@ -124,6 +124,15 @@ func (w *WorkspaceFilesExtensionsClient) getNotebookStatByNameWithoutExt(ctx con ext = notebook.ExtensionJupyter } + // Notebooks with an unmapped language have no extension. Without one the entry + // cannot round-trip through this filer: [Stat] and [Read] deliberately report + // extension-less notebook names as not found. Return nil so [ReadDir] omits the + // entry instead of listing a name that cannot be accessed. + if ext == "" { + log.Warnf(ctx, "skipping notebook %s: no file extension is associated with notebook language %q", path.Join(w.root, name), stat.Language) + return nil, nil + } + // Modify the stat object path to include the extension. This stat object will be used // to return the fs.DirEntry object in the ReadDir method. stat.Path = stat.Path + ext @@ -205,6 +214,7 @@ func (w *WorkspaceFilesExtensionsClient) ReadDir(ctx context.Context, name strin } seenPaths := make(map[string]workspace.ObjectInfo) + result := make([]fs.DirEntry, 0, len(entries)) for i := range entries { info, err := entries[i].Info() if err != nil { @@ -218,6 +228,12 @@ func (w *WorkspaceFilesExtensionsClient) ReadDir(ctx context.Context, name strin if err != nil { return nil, err } + // A nil stat means no extension is associated with the notebook's + // language, so the entry has no name that can round-trip through + // [Stat] or [Read]. Omit it from the listing. + if stat == nil { + continue + } // Replace the entry with the new entry that includes the extension. entries[i] = wsfsDirEntry{wsfsFileInfo{ObjectInfo: stat.ObjectInfo}} } @@ -232,9 +248,10 @@ func (w *WorkspaceFilesExtensionsClient) ReadDir(ctx context.Context, name strin } } seenPaths[entries[i].Name()] = sysInfo + result = append(result, entries[i]) } - return entries, nil + return result, nil } // Note: The import API returns opaque internal errors for namespace clashes diff --git a/libs/filer/workspace_files_extensions_client_test.go b/libs/filer/workspace_files_extensions_client_test.go index b3b4861ee8e..b5e3111c8c5 100644 --- a/libs/filer/workspace_files_extensions_client_test.go +++ b/libs/filer/workspace_files_extensions_client_test.go @@ -201,3 +201,93 @@ func TestFilerWorkspaceFilesExtensionsErrorsOnDupName(t *testing.T) { }) } } + +func TestFilerWorkspaceFilesExtensionsReadDirWithUnmappedLanguage(t *testing.T) { + for _, tc := range []struct { + name string + language workspace.Language + notebookExportFormat workspace.ExportFormat + expectedNames []string + }{ + { + name: "source notebook with empty language is skipped", + language: "", + notebookExportFormat: workspace.ExportFormatSource, + expectedNames: []string{"bar.py"}, + }, + { + name: "source notebook with unknown language is skipped", + language: workspace.Language("FUTURELANG"), + notebookExportFormat: workspace.ExportFormatSource, + expectedNames: []string{"bar.py"}, + }, + { + // All languages are supported for Jupyter notebooks, so the entry is kept. + name: "jupyter notebook with unknown language keeps the .ipynb extension", + language: workspace.Language("FUTURELANG"), + notebookExportFormat: workspace.ExportFormatJupyter, + expectedNames: []string{"bar.py", "foo.ipynb"}, + }, + } { + 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: "/dir/bar.py", + ObjectType: workspace.ObjectTypeFile, + }, + { + Path: "/dir/foo", + 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: "/dir/foo", + 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": "/dir/foo", + "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, + } + + entries, err := workspaceFilesExtensionsClient.ReadDir(t.Context(), "/") + assert.NoError(t, err) + + names := make([]string, len(entries)) + for i, entry := range entries { + names[i] = entry.Name() + } + assert.Equal(t, tc.expectedNames, names) + + // assert the mocked methods were actually called, as a sanity check. + workspaceApi.AssertNumberOfCalls(t, "ListAll", 1) + mockedApiClient.AssertNumberOfCalls(t, "Do", 1) + }) + } +} From 8ee3935ea747970d16e2dce821a6239dac809c81 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Wed, 10 Jun 2026 00:29:44 +0200 Subject: [PATCH 2/2] Remove redundant comments Co-authored-by: Isaac --- libs/filer/workspace_files_extensions_client.go | 10 +++------- libs/filer/workspace_files_extensions_client_test.go | 6 +----- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/libs/filer/workspace_files_extensions_client.go b/libs/filer/workspace_files_extensions_client.go index e97041da0ce..29adbdbddf6 100644 --- a/libs/filer/workspace_files_extensions_client.go +++ b/libs/filer/workspace_files_extensions_client.go @@ -124,10 +124,8 @@ func (w *WorkspaceFilesExtensionsClient) getNotebookStatByNameWithoutExt(ctx con ext = notebook.ExtensionJupyter } - // Notebooks with an unmapped language have no extension. Without one the entry - // cannot round-trip through this filer: [Stat] and [Read] deliberately report - // extension-less notebook names as not found. Return nil so [ReadDir] omits the - // entry instead of listing a name that cannot be accessed. + // Extension-less notebook names cannot round-trip through [Stat] or [Read], + // which report them as not found, so return nil to have [ReadDir] omit the entry. if ext == "" { log.Warnf(ctx, "skipping notebook %s: no file extension is associated with notebook language %q", path.Join(w.root, name), stat.Language) return nil, nil @@ -228,9 +226,7 @@ func (w *WorkspaceFilesExtensionsClient) ReadDir(ctx context.Context, name strin if err != nil { return nil, err } - // A nil stat means no extension is associated with the notebook's - // language, so the entry has no name that can round-trip through - // [Stat] or [Read]. Omit it from the listing. + // A nil stat means the notebook's language has no extension mapping; omit the entry. if stat == nil { continue } diff --git a/libs/filer/workspace_files_extensions_client_test.go b/libs/filer/workspace_files_extensions_client_test.go index b5e3111c8c5..438ac21ec47 100644 --- a/libs/filer/workspace_files_extensions_client_test.go +++ b/libs/filer/workspace_files_extensions_client_test.go @@ -222,7 +222,6 @@ func TestFilerWorkspaceFilesExtensionsReadDirWithUnmappedLanguage(t *testing.T) expectedNames: []string{"bar.py"}, }, { - // All languages are supported for Jupyter notebooks, so the entry is kept. name: "jupyter notebook with unknown language keeps the .ipynb extension", language: workspace.Language("FUTURELANG"), notebookExportFormat: workspace.ExportFormatJupyter, @@ -233,7 +232,6 @@ func TestFilerWorkspaceFilesExtensionsReadDirWithUnmappedLanguage(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", @@ -249,8 +247,7 @@ func TestFilerWorkspaceFilesExtensionsReadDirWithUnmappedLanguage(t *testing.T) }, }, 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. + // Mock the get-status call used to figure out the notebook's file extension. statNotebook := wsfsFileInfo{ ObjectInfo: workspace.ObjectInfo{ Path: "/dir/foo", @@ -285,7 +282,6 @@ func TestFilerWorkspaceFilesExtensionsReadDirWithUnmappedLanguage(t *testing.T) } assert.Equal(t, tc.expectedNames, names) - // assert the mocked methods were actually called, as a sanity check. workspaceApi.AssertNumberOfCalls(t, "ListAll", 1) mockedApiClient.AssertNumberOfCalls(t, "Do", 1) })