diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 138443e66f5..8919fad5a36 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -10,5 +10,6 @@ ### Bundles * Add support for Lakebase database instances in DABs ([#3283](https://github.com/databricks/cli/pull/3283)) +* Improve error message for SDK/API errors ([#3379](https://github.com/databricks/cli/pull/3379)) ### API Changes diff --git a/acceptance/bundle/resources/jobs/create-error/output.txt b/acceptance/bundle/resources/jobs/create-error/output.txt index 10d49de6241..310772ff2ea 100644 --- a/acceptance/bundle/resources/jobs/create-error/output.txt +++ b/acceptance/bundle/resources/jobs/create-error/output.txt @@ -6,7 +6,12 @@ Warning: required field "new_cluster" is not set Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... Deploying resources... -Error: deploying jobs.foo: creating: Method=Jobs.Create *retries.Err *apierr.APIError StatusCode=400 ErrorCode="INVALID_PARAMETER_VALUE" Message="Shared job cluster feature is only supported in multi-task jobs." +Error: deploying jobs.foo: creating: Shared job cluster feature is only supported in multi-task jobs. (400 INVALID_PARAMETER_VALUE) + +Endpoint: POST [DATABRICKS_URL]/api/2.2/jobs/create +HTTP Status: 400 Bad Request +API error_code: INVALID_PARAMETER_VALUE +API message: Shared job cluster feature is only supported in multi-task jobs. Updating deployment state... diff --git a/bundle/terranova/tnresources/app.go b/bundle/terranova/tnresources/app.go index 62ff1bc085a..5542243a8af 100644 --- a/bundle/terranova/tnresources/app.go +++ b/bundle/terranova/tnresources/app.go @@ -33,7 +33,7 @@ func (r *ResourceApp) DoCreate(ctx context.Context) (string, error) { } waiter, err := r.client.Apps.Create(ctx, request) if err != nil { - return "", SDKError{Method: "Apps.Create", Err: err} + return "", err } // TODO: Store waiter for Wait method @@ -48,7 +48,7 @@ func (r *ResourceApp) DoUpdate(ctx context.Context, id string) error { } response, err := r.client.Apps.Update(ctx, request) if err != nil { - return SDKError{Method: "Apps.Update", Err: err} + return err } if response.Name != id { diff --git a/bundle/terranova/tnresources/database_instance.go b/bundle/terranova/tnresources/database_instance.go index 9517fedf8b1..35bd2a183f6 100644 --- a/bundle/terranova/tnresources/database_instance.go +++ b/bundle/terranova/tnresources/database_instance.go @@ -23,7 +23,7 @@ func (d *ResourceDatabaseInstance) DoCreate(ctx context.Context) (string, error) DatabaseInstance: d.config, }) if err != nil { - return "", SDKError{Method: "Database.CreateDatabaseInstance", Err: err} + return "", err } d.waiter = waiter return waiter.Response.Name, nil @@ -38,10 +38,7 @@ func (d ResourceDatabaseInstance) DoUpdate(ctx context.Context, id string) error request.DatabaseInstance.Uid = id _, err := d.client.Database.UpdateDatabaseInstance(ctx, request) - if err != nil { - return SDKError{Method: "Database.UpdateDatabaseInstance", Err: err} - } - return nil + return err } func (d *ResourceDatabaseInstance) WaitAfterCreate(ctx context.Context) error { @@ -49,10 +46,7 @@ func (d *ResourceDatabaseInstance) WaitAfterCreate(ctx context.Context) error { return nil } _, err := d.waiter.Get() - if err != nil { - return SDKError{Method: "WaitGetDatabaseInstanceDatabaseAvailable.Get", Err: err} - } - return nil + return err } func (d ResourceDatabaseInstance) WaitAfterUpdate(ctx context.Context) error { @@ -68,14 +62,10 @@ func NewResourceDatabaseInstance(client *databricks.WorkspaceClient, resource *r } func DeleteDatabaseInstance(ctx context.Context, client *databricks.WorkspaceClient, name string) error { - err := client.Database.DeleteDatabaseInstance(ctx, database.DeleteDatabaseInstanceRequest{ + return client.Database.DeleteDatabaseInstance(ctx, database.DeleteDatabaseInstanceRequest{ Name: name, Purge: true, Force: false, ForceSendFields: nil, }) - if err != nil { - return SDKError{Method: "Database.DeleteDatabaseInstanceByName", Err: err} - } - return nil } diff --git a/bundle/terranova/tnresources/job.go b/bundle/terranova/tnresources/job.go index ab0002a2b52..efb69968f6c 100644 --- a/bundle/terranova/tnresources/job.go +++ b/bundle/terranova/tnresources/job.go @@ -33,7 +33,7 @@ func (r *ResourceJob) DoCreate(ctx context.Context) (string, error) { } response, err := r.client.Jobs.Create(ctx, request) if err != nil { - return "", SDKError{Method: "Jobs.Create", Err: err} + return "", err } return strconv.FormatInt(response.JobId, 10), nil } @@ -43,11 +43,7 @@ func (r *ResourceJob) DoUpdate(ctx context.Context, id string) error { if err != nil { return err } - err = r.client.Jobs.Reset(ctx, request) - if err != nil { - return SDKError{Method: "Jobs.Reset", Err: err} - } - return nil + return r.client.Jobs.Reset(ctx, request) } func DeleteJob(ctx context.Context, client *databricks.WorkspaceClient, id string) error { @@ -55,11 +51,7 @@ func DeleteJob(ctx context.Context, client *databricks.WorkspaceClient, id strin if err != nil { return err } - err = client.Jobs.DeleteByJobId(ctx, idInt) - if err != nil { - return SDKError{Method: "Jobs.DeleteByJobId", Err: err} - } - return nil + return client.Jobs.DeleteByJobId(ctx, idInt) } func (r *ResourceJob) WaitAfterCreate(ctx context.Context) error { diff --git a/bundle/terranova/tnresources/pipeline.go b/bundle/terranova/tnresources/pipeline.go index e1f81467510..3625d25c70d 100644 --- a/bundle/terranova/tnresources/pipeline.go +++ b/bundle/terranova/tnresources/pipeline.go @@ -27,7 +27,7 @@ func (r *ResourcePipeline) Config() any { func (r *ResourcePipeline) DoCreate(ctx context.Context) (string, error) { response, err := r.client.Pipelines.Create(ctx, r.config) if err != nil { - return "", SDKError{Method: "Pipelines.Create", Err: err} + return "", err } return response.PipelineId, nil } @@ -68,19 +68,11 @@ func (r *ResourcePipeline) DoUpdate(ctx context.Context, id string) error { ForceSendFields: filterFields[pipelines.EditPipeline](r.config.ForceSendFields), } - err := r.client.Pipelines.Update(ctx, request) - if err != nil { - return SDKError{Method: "Pipelines.Update", Err: err} - } - return nil + return r.client.Pipelines.Update(ctx, request) } func DeletePipeline(ctx context.Context, client *databricks.WorkspaceClient, id string) error { - err := client.Pipelines.DeleteByPipelineId(ctx, id) - if err != nil { - return SDKError{Method: "Pipelines.DeleteByPipelineId", Err: err} - } - return nil + return client.Pipelines.DeleteByPipelineId(ctx, id) } func (r *ResourcePipeline) WaitAfterCreate(ctx context.Context) error { diff --git a/bundle/terranova/tnresources/schema.go b/bundle/terranova/tnresources/schema.go index 3c362171fa4..f041dc77b63 100644 --- a/bundle/terranova/tnresources/schema.go +++ b/bundle/terranova/tnresources/schema.go @@ -28,7 +28,7 @@ func (r *ResourceSchema) Config() any { func (r *ResourceSchema) DoCreate(ctx context.Context) (string, error) { response, err := r.client.Schemas.Create(ctx, r.config) if err != nil { - return "", SDKError{Method: "Schemas.Create", Err: err} + return "", err } return response.FullName, nil } @@ -46,7 +46,7 @@ func (r *ResourceSchema) DoUpdate(ctx context.Context, id string) error { response, err := r.client.Schemas.Update(ctx, updateRequest) if err != nil { - return SDKError{Method: "Schemas.Update", Err: err} + return err } if response.FullName != id { @@ -57,15 +57,11 @@ func (r *ResourceSchema) DoUpdate(ctx context.Context, id string) error { } func DeleteSchema(ctx context.Context, client *databricks.WorkspaceClient, id string) error { - err := client.Schemas.Delete(ctx, catalog.DeleteSchemaRequest{ + return client.Schemas.Delete(ctx, catalog.DeleteSchemaRequest{ FullName: id, Force: true, ForceSendFields: nil, }) - if err != nil { - return SDKError{Method: "Schemas.Delete", Err: err} - } - return nil } func (r *ResourceSchema) WaitAfterCreate(ctx context.Context) error { diff --git a/bundle/terranova/tnresources/sdk_error.go b/bundle/terranova/tnresources/sdk_error.go deleted file mode 100644 index 82035de5226..00000000000 --- a/bundle/terranova/tnresources/sdk_error.go +++ /dev/null @@ -1,43 +0,0 @@ -package tnresources - -import ( - "fmt" - - "github.com/databricks/databricks-sdk-go/apierr" - "github.com/databricks/databricks-sdk-go/retries" -) - -// Default String() for APIError only returns Message. -// This wrapper adds other info from the response such as StatusCode, ErrorCode and type information. -// This is motivated by working with testserver which might have incomplete or broken error messages (empty Message). -// However it would not hurt to have this info when debugging real endpoints as well. -// It also adds Method which refers to API method that was called (e.g. Jobs.Reset). - -type SDKError struct { - Method string - Err error -} - -func (e SDKError) Error() string { - if e.Method != "" { - return fmt.Sprintf("Method=%s %s", e.Method, formatSDKError(e.Err)) - } else { - return formatSDKError(e.Err) - } -} - -func formatSDKError(e error) string { - retriesErr, ok := e.(*retries.Err) - if ok { - return fmt.Sprintf("%T %s", retriesErr, formatAPIError(retriesErr.Err)) - } - return formatAPIError(e) -} - -func formatAPIError(e error) string { - apiErr, ok := e.(*apierr.APIError) - if ok { - return fmt.Sprintf("%T StatusCode=%d ErrorCode=%#v Message=%#v", apiErr, apiErr.StatusCode, apiErr.ErrorCode, apiErr.Message) - } - return e.Error() -} diff --git a/bundle/terranova/tnresources/sql_warehouse.go b/bundle/terranova/tnresources/sql_warehouse.go index 440b72d6cb0..176d4f8ea0d 100644 --- a/bundle/terranova/tnresources/sql_warehouse.go +++ b/bundle/terranova/tnresources/sql_warehouse.go @@ -28,7 +28,7 @@ func (r *ResourceSqlWarehouse) Config() any { func (r *ResourceSqlWarehouse) DoCreate(ctx context.Context) (string, error) { waiter, err := r.client.Warehouses.Create(ctx, r.config) if err != nil { - return "", SDKError{Method: "Warehouses.Create", Err: err} + return "", err } return waiter.Id, nil @@ -55,7 +55,7 @@ func (r *ResourceSqlWarehouse) DoUpdate(ctx context.Context, id string) error { waiter, err := r.client.Warehouses.Edit(ctx, request) if err != nil { - return SDKError{Method: "Warehouses.Edit", Err: err} + return err } if waiter.Id != id { @@ -76,9 +76,5 @@ func (r *ResourceSqlWarehouse) WaitAfterUpdate(ctx context.Context) error { } func DeleteSqlWarehouse(ctx context.Context, client *databricks.WorkspaceClient, oldID string) error { - err := client.Warehouses.DeleteById(ctx, oldID) - if err != nil { - return SDKError{Method: "Warehouses.DeleteById", Err: err} - } - return nil + return client.Warehouses.DeleteById(ctx, oldID) } diff --git a/bundle/terranova/tnresources/volume.go b/bundle/terranova/tnresources/volume.go index e49cfc8e6e3..56d43052193 100644 --- a/bundle/terranova/tnresources/volume.go +++ b/bundle/terranova/tnresources/volume.go @@ -32,7 +32,7 @@ func (r *ResourceVolume) Config() any { func (r *ResourceVolume) DoCreate(ctx context.Context) (string, error) { response, err := r.client.Volumes.Create(ctx, r.config) if err != nil { - return "", SDKError{Method: "Volumes.Create", Err: err} + return "", err } return response.FullName, nil } @@ -58,7 +58,7 @@ func (r *ResourceVolume) DoUpdate(ctx context.Context, id string) error { response, err := r.client.Volumes.Update(ctx, updateRequest) if err != nil { - return SDKError{Method: "Volumes.Update", Err: err} + return err } if id != response.FullName { @@ -91,7 +91,7 @@ func (r *ResourceVolume) DoUpdateWithID(ctx context.Context, id string) (string, response, err := r.client.Volumes.Update(ctx, updateRequest) if err != nil { - return "", SDKError{Method: "Volumes.Update", Err: err} + return "", err } return response.FullName, nil @@ -100,7 +100,7 @@ func (r *ResourceVolume) DoUpdateWithID(ctx context.Context, id string) (string, func DeleteVolume(ctx context.Context, client *databricks.WorkspaceClient, id string) error { err := client.Volumes.DeleteByName(ctx, id) if err != nil { - return SDKError{Method: "Volumes.Delete", Err: err} + return err } return nil } diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index 0c7699b4ef5..98335a88b93 100644 --- a/libs/diag/diagnostic.go +++ b/libs/diag/diagnostic.go @@ -48,7 +48,8 @@ func FromErr(err error) Diagnostics { return []Diagnostic{ { Severity: Error, - Summary: err.Error(), + Summary: FormatAPIErrorSummary(err), + Detail: FormatAPIErrorDetails(err), }, } } diff --git a/libs/diag/sdk_error.go b/libs/diag/sdk_error.go new file mode 100644 index 00000000000..1099dd738dc --- /dev/null +++ b/libs/diag/sdk_error.go @@ -0,0 +1,44 @@ +package diag + +import ( + "errors" + "fmt" + "strconv" + "strings" + + "github.com/databricks/databricks-sdk-go/apierr" +) + +func FormatAPIErrorSummary(e error) string { + var apiErr *apierr.APIError + if !errors.As(e, &apiErr) { + return e.Error() + } + extra := strings.TrimSpace(fmt.Sprintf("%d %s", apiErr.StatusCode, apiErr.ErrorCode)) + return e.Error() + " (" + extra + ")" +} + +func FormatAPIErrorDetails(e error) string { + var apiErr *apierr.APIError + if !errors.As(e, &apiErr) { + return "" + } + + endpoint := "n/a" + httpStatus := "" + w := apiErr.ResponseWrapper + if w != nil { + resp := w.Response + if resp != nil { + httpStatus = resp.Status + req := resp.Request + if req != nil { + endpoint = fmt.Sprintf("%s %s", req.Method, req.URL) + } + } + } + if len(httpStatus) == 0 { + httpStatus = strconv.Itoa(apiErr.StatusCode) + } + return fmt.Sprintf("Endpoint: %s\nHTTP Status: %s\nAPI error_code: %s\nAPI message: %s", endpoint, httpStatus, apiErr.ErrorCode, apiErr.Message) +} diff --git a/libs/diag/sdk_error_test.go b/libs/diag/sdk_error_test.go new file mode 100644 index 00000000000..6b51714bf87 --- /dev/null +++ b/libs/diag/sdk_error_test.go @@ -0,0 +1,85 @@ +package diag + +import ( + "errors" + "fmt" + "net/http" + "net/url" + "testing" + + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/common" + "github.com/stretchr/testify/assert" +) + +func TestFormatAPIError(t *testing.T) { + u, _ := url.Parse("https://example.com/api/2.0/foo") + request := &http.Request{Method: http.MethodPost, URL: u} + + cases := []struct { + name string + err error + expectedSummary string + expectedDetails string + }{ + { + name: "plain error (no apierr)", + err: errors.New("foo"), + expectedSummary: "foo", + expectedDetails: "", + }, + { + name: "apierr no wrapper", + err: &apierr.APIError{ + Message: "api error message", + ErrorCode: "TEST_ERROR", + StatusCode: 400, + }, + expectedSummary: "api error message (400 TEST_ERROR)", + expectedDetails: "Endpoint: n/a\nHTTP Status: 400\nAPI error_code: TEST_ERROR\nAPI message: api error message", + }, + { + name: "apierr with Response only", + err: &apierr.APIError{ + Message: "api error message", + ErrorCode: "TEST_ERROR", + StatusCode: 400, + ResponseWrapper: &common.ResponseWrapper{ + Response: &http.Response{Status: "400 Bad Request"}, + }, + }, + expectedSummary: "api error message (400 TEST_ERROR)", + expectedDetails: "Endpoint: n/a\nHTTP Status: 400 Bad Request\nAPI error_code: TEST_ERROR\nAPI message: api error message", + }, + { + name: "apierr with full Response+Request", + err: &apierr.APIError{ + Message: "api error message", + ErrorCode: "TEST_ERROR", + StatusCode: 400, + ResponseWrapper: &common.ResponseWrapper{ + Response: &http.Response{Status: "400 Bad Request", Request: request}, + }, + }, + expectedSummary: "api error message (400 TEST_ERROR)", + expectedDetails: "Endpoint: POST https://example.com/api/2.0/foo\nHTTP Status: 400 Bad Request\nAPI error_code: TEST_ERROR\nAPI message: api error message", + }, + { + name: "wrapped apierr", + err: fmt.Errorf("wrapped: %w", &apierr.APIError{ + Message: "api error message", + ErrorCode: "TEST_ERROR", + StatusCode: 500, + }), + expectedSummary: "wrapped: api error message (500 TEST_ERROR)", + expectedDetails: "Endpoint: n/a\nHTTP Status: 500\nAPI error_code: TEST_ERROR\nAPI message: api error message", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expectedSummary, FormatAPIErrorSummary(tc.err)) + assert.Equal(t, tc.expectedDetails, FormatAPIErrorDetails(tc.err)) + }) + } +}