From 7211859e5926d5e6a33906075b9150d4dce47c83 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 9 Jun 2026 22:50:27 +0200 Subject: [PATCH 1/2] Replace error message matching with structural error checks Two sites branched on err.Error() content. cmd/apps/dev.go now matches the Apps API 404 via errors.Is with apierr.ErrNotFound, and the DBFS filer Read re-stats the path to detect directories instead of matching the SDK's error message. Co-authored-by: Isaac --- cmd/apps/dev.go | 12 ++++++-- cmd/apps/dev_test.go | 23 ++++++++++++++ libs/filer/dbfs_client.go | 12 +++++--- libs/filer/dbfs_client_test.go | 56 ++++++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 libs/filer/dbfs_client_test.go diff --git a/cmd/apps/dev.go b/cmd/apps/dev.go index 4fb86527c25..9bf74156076 100644 --- a/cmd/apps/dev.go +++ b/cmd/apps/dev.go @@ -12,7 +12,6 @@ import ( "os/exec" "os/signal" "strconv" - "strings" "syscall" "time" @@ -22,6 +21,7 @@ import ( "github.com/databricks/cli/libs/apps/vite" "github.com/databricks/cli/libs/cmdctx" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/databricks-sdk-go/apierr" "github.com/spf13/cobra" ) @@ -40,6 +40,14 @@ func isViteReady(port int) bool { return true } +// isAppNotFound reports whether err is the Apps API's 404 for an app that was +// never created or has been deleted ("App with name X does not exist or is +// deleted."). The API reports both cases with HTTP 404, so we match the +// status-based sentinel instead of the error message wording. +func isAppNotFound(err error) bool { + return errors.Is(err, apierr.ErrNotFound) +} + // detectAppNameFromBundle tries to extract the app name from a databricks.yml bundle config. // Returns the app name if found, or empty string if no bundle or no apps found. // This properly loads and initializes the bundle to resolve variables and apply prefixes. @@ -184,7 +192,7 @@ Examples: return domainErr }) if err != nil { - if strings.Contains(err.Error(), "does not exist") || strings.Contains(err.Error(), "is deleted") { + if isAppNotFound(err) { return fmt.Errorf("application '%s' has not been deployed yet. Run `databricks apps deploy` to deploy and then try again", appName) } return fmt.Errorf("failed to get app domain: %w", err) diff --git a/cmd/apps/dev_test.go b/cmd/apps/dev_test.go index a1e8deef447..8ee8d3380c6 100644 --- a/cmd/apps/dev_test.go +++ b/cmd/apps/dev_test.go @@ -1,6 +1,8 @@ package apps import ( + "errors" + "fmt" "net" "os" "testing" @@ -8,6 +10,7 @@ import ( "github.com/databricks/cli/libs/apps/vite" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/databricks-sdk-go/apierr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -44,6 +47,26 @@ func TestIsViteReady(t *testing.T) { }) } +func TestIsAppNotFound(t *testing.T) { + // Error shape returned by the Apps API for a missing or deleted app. + notFound := &apierr.APIError{ + StatusCode: 404, + ErrorCode: "NOT_FOUND", + Message: "App with name test-app does not exist or is deleted.", + } + assert.True(t, isAppNotFound(notFound)) + // GetAppDomain wraps the SDK error; the sentinel must match through the chain. + assert.True(t, isAppNotFound(fmt.Errorf("failed to get app: %w", notFound))) + + forbidden := &apierr.APIError{ + StatusCode: 403, + ErrorCode: "PERMISSION_DENIED", + Message: "user does not have permission", + } + assert.False(t, isAppNotFound(forbidden)) + assert.False(t, isAppNotFound(errors.New("app URL is empty"))) +} + func TestViteServerScriptContent(t *testing.T) { // Verify the embedded script is not empty assert.NotEmpty(t, vite.ServerScript) diff --git a/libs/filer/dbfs_client.go b/libs/filer/dbfs_client.go index ce9286ea8d9..80608a823e0 100644 --- a/libs/filer/dbfs_client.go +++ b/libs/filer/dbfs_client.go @@ -148,13 +148,15 @@ func (w *DbfsClient) Read(ctx context.Context, name string) (io.ReadCloser, erro handle, err := w.workspaceClient.Dbfs.Open(ctx, absPath, files.FileModeRead) if err != nil { - // Return error if file is a directory - if strings.Contains(err.Error(), "cannot open directory for reading") { - return nil, notAFile{absPath} - } - aerr, ok := errors.AsType[*apierr.APIError](err) if !ok { + // The SDK's Open stats the path and fails with a client-side error + // (not an *apierr.APIError, no sentinel) when it is a directory. + // Re-stat to detect that case instead of matching the SDK's error + // message text. + if info, serr := w.workspaceClient.Dbfs.GetStatusByPath(ctx, absPath); serr == nil && info.IsDir { + return nil, notAFile{absPath} + } return nil, err } diff --git a/libs/filer/dbfs_client_test.go b/libs/filer/dbfs_client_test.go new file mode 100644 index 00000000000..b75d6fe2117 --- /dev/null +++ b/libs/filer/dbfs_client_test.go @@ -0,0 +1,56 @@ +package filer + +import ( + "io/fs" + "testing" + + "github.com/databricks/cli/libs/testserver" + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/service/files" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func dbfsReadWithGetStatusResponse(t *testing.T, response any) error { + t.Helper() + + server := testserver.New(t) + server.Handle("GET", "/api/2.0/dbfs/get-status", func(req testserver.Request) any { + return response + }) + testserver.AddDefaultHandlers(server) + + client, err := databricks.NewWorkspaceClient(&databricks.Config{ + Host: server.URL, + Token: "testtoken", + }) + require.NoError(t, err) + + f, err := NewDbfsClient(client, "/test") + require.NoError(t, err) + + _, err = f.Read(t.Context(), "file") + require.Error(t, err) + return err +} + +func TestDbfsClientReadDirectory(t *testing.T) { + // The SDK's Open reports reading a directory with a client-side error that + // carries no sentinel, so the filer re-stats the path to map it to notAFile. + err := dbfsReadWithGetStatusResponse(t, files.FileInfo{ + Path: "/test/file", + IsDir: true, + }) + assert.ErrorIs(t, err, fs.ErrInvalid) +} + +func TestDbfsClientReadFileDoesNotExist(t *testing.T) { + err := dbfsReadWithGetStatusResponse(t, testserver.Response{ + StatusCode: 404, + Body: map[string]string{ + "error_code": "RESOURCE_DOES_NOT_EXIST", + "message": "test error", + }, + }) + assert.ErrorIs(t, err, fs.ErrNotExist) +} From 436aee8b7c19a045c3befd2aa919d586f10a7255 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Wed, 10 Jun 2026 00:29:50 +0200 Subject: [PATCH 2/2] Remove redundant comments Co-authored-by: Isaac --- cmd/apps/dev.go | 6 ++---- cmd/apps/dev_test.go | 2 -- libs/filer/dbfs_client.go | 6 ++---- libs/filer/dbfs_client_test.go | 2 -- 4 files changed, 4 insertions(+), 12 deletions(-) diff --git a/cmd/apps/dev.go b/cmd/apps/dev.go index 9bf74156076..d070c423a73 100644 --- a/cmd/apps/dev.go +++ b/cmd/apps/dev.go @@ -40,10 +40,8 @@ func isViteReady(port int) bool { return true } -// isAppNotFound reports whether err is the Apps API's 404 for an app that was -// never created or has been deleted ("App with name X does not exist or is -// deleted."). The API reports both cases with HTTP 404, so we match the -// status-based sentinel instead of the error message wording. +// isAppNotFound reports whether err is the Apps API's 404, which covers both a +// never-created and a deleted app. func isAppNotFound(err error) bool { return errors.Is(err, apierr.ErrNotFound) } diff --git a/cmd/apps/dev_test.go b/cmd/apps/dev_test.go index 8ee8d3380c6..fc9b292fb16 100644 --- a/cmd/apps/dev_test.go +++ b/cmd/apps/dev_test.go @@ -48,14 +48,12 @@ func TestIsViteReady(t *testing.T) { } func TestIsAppNotFound(t *testing.T) { - // Error shape returned by the Apps API for a missing or deleted app. notFound := &apierr.APIError{ StatusCode: 404, ErrorCode: "NOT_FOUND", Message: "App with name test-app does not exist or is deleted.", } assert.True(t, isAppNotFound(notFound)) - // GetAppDomain wraps the SDK error; the sentinel must match through the chain. assert.True(t, isAppNotFound(fmt.Errorf("failed to get app: %w", notFound))) forbidden := &apierr.APIError{ diff --git a/libs/filer/dbfs_client.go b/libs/filer/dbfs_client.go index 80608a823e0..63670560386 100644 --- a/libs/filer/dbfs_client.go +++ b/libs/filer/dbfs_client.go @@ -150,10 +150,8 @@ func (w *DbfsClient) Read(ctx context.Context, name string) (io.ReadCloser, erro if err != nil { aerr, ok := errors.AsType[*apierr.APIError](err) if !ok { - // The SDK's Open stats the path and fails with a client-side error - // (not an *apierr.APIError, no sentinel) when it is a directory. - // Re-stat to detect that case instead of matching the SDK's error - // message text. + // The SDK's Open fails with a client-side error carrying no sentinel + // when the path is a directory, so re-stat to detect that case. if info, serr := w.workspaceClient.Dbfs.GetStatusByPath(ctx, absPath); serr == nil && info.IsDir { return nil, notAFile{absPath} } diff --git a/libs/filer/dbfs_client_test.go b/libs/filer/dbfs_client_test.go index b75d6fe2117..ebdb4de659b 100644 --- a/libs/filer/dbfs_client_test.go +++ b/libs/filer/dbfs_client_test.go @@ -35,8 +35,6 @@ func dbfsReadWithGetStatusResponse(t *testing.T, response any) error { } func TestDbfsClientReadDirectory(t *testing.T) { - // The SDK's Open reports reading a directory with a client-side error that - // carries no sentinel, so the filer re-stats the path to map it to notAFile. err := dbfsReadWithGetStatusResponse(t, files.FileInfo{ Path: "/test/file", IsDir: true,