[Fix] Handle non-JSON errors

apierr/errors.go

@@ -143,7 +143,7 @@ func GetAPIError(ctx context.Context, resp common.ResponseWrapper) error {
 		if err != nil {
 			return ReadError(resp.Response.StatusCode, err)
 		}
-		apiError := parseErrorFromResponse(resp.Response, resp.RequestBody.DebugBytes, responseBodyBytes)
+		apiError := parseErrorFromResponse(ctx, resp.Response, resp.RequestBody.DebugBytes, responseBodyBytes)
 		applyOverrides(ctx, apiError, resp.Response)
 		return apiError
 	}
@@ -155,14 +155,43 @@ func GetAPIError(ctx context.Context, resp common.ResponseWrapper) error {
 	return nil
 }
 
-func parseErrorFromResponse(resp *http.Response, requestBody, responseBody []byte) *APIError {
+// errorParser attempts to parse the error from the response body. If successful,
+// it returns a non-nil *APIError. It returns nil if parsing fails or no error is found.
+type errorParser func(context.Context, *http.Response, []byte) *APIError
+
+// errorParsers is a list of errorParser functions that are tried in order to
+// parse an API error from a response body. Most errors should be parsable by
+// the standardErrorParser, but additional parsers can be added here for
+// specific error formats. The order of the parsers is not important, as the set
+// of errors that can be parsed by each parser should be disjoint.
+var errorParsers = []errorParser{
+	standardErrorParser,
+	stringErrorParser,
+	htmlErrorParser,
+}
+
+func parseErrorFromResponse(ctx context.Context, resp *http.Response, requestBody, responseBody []byte) *APIError {
 	if len(responseBody) == 0 {
 		return &APIError{
 			Message:    http.StatusText(resp.StatusCode),
 			StatusCode: resp.StatusCode,
 		}
 	}
 
+	for _, parser := range errorParsers {
+		if apiError := parser(ctx, resp, responseBody); apiError != nil {
+			return apiError
+		}
+	}
+
+	return unknownAPIError(resp, requestBody, responseBody)
+}
+
+// standardErrorParser is the default error parser for Databricks API errors.
+// It handles JSON error messages with error code, message, and details fields.
+// It also provides compatibility with the old API 1.2 error format and SCIM API
+// errors.
+func standardErrorParser(ctx context.Context, resp *http.Response, responseBody []byte) *APIError {
 	// Anonymous struct used to unmarshal JSON Databricks API error responses.
 	var errorBody struct {
 		ErrorCode  any           `json:"error_code,omitempty"` // int or string
@@ -177,7 +206,8 @@ func parseErrorFromResponse(resp *http.Response, requestBody, responseBody []byt
 		ScimType   string `json:"scimType,omitempty"`
 	}
 	if err := json.Unmarshal(responseBody, &errorBody); err != nil {
-		return unknownAPIError(resp, requestBody, responseBody, err)
+		logger.Tracef(ctx, "standardErrorParser: failed to unmarshal error response: %s", err)
+		return nil
 	}
 
 	// Convert API 1.2 error (which used a different format) to the new format.
@@ -206,9 +236,40 @@ func parseErrorFromResponse(resp *http.Response, requestBody, responseBody []byt
 	}
 }
 
-func unknownAPIError(resp *http.Response, requestBody, responseBody []byte, err error) *APIError {
+var stringErrorRegex = regexp.MustCompile(`^([A-Z_]+): (.*)$`)
+
+// stringErrorParser parses errors of the form "STATUS_CODE: status message".
+// Some account-level APIs respond with this error code, e.g.
+// https://github.com/databricks/databricks-sdk-go/issues/998
+func stringErrorParser(ctx context.Context, resp *http.Response, responseBody []byte) *APIError {
+	matches := stringErrorRegex.FindSubmatch(responseBody)
+	if len(matches) < 3 {
+		logger.Tracef(ctx, "stringErrorParser: failed to match error response")
+		return nil
+	}
+	return &APIError{
+		Message:    string(matches[2]),
+		ErrorCode:  string(matches[1]),
+		StatusCode: resp.StatusCode,
+	}
+}
+
+var htmlMessageRe = regexp.MustCompile(`<pre>(.*)</pre>`)
+
+// htmlErrorParser parses HTML error responses. Some legacy APIs respond with
+// an HTML page in certain error cases, like when trying to create a cluster
+// before the worker environment is ready.
+func htmlErrorParser(ctx context.Context, resp *http.Response, responseBody []byte) *APIError {
+	messageMatches := htmlMessageRe.FindStringSubmatch(string(responseBody))
+	// No messages with <pre> </pre> format found so return a APIError
+	if len(messageMatches) < 2 {
+		logger.Tracef(ctx, "htmlErrorParser: no <pre> tag found in error response")
+		return nil
+	}
+
 	apiErr := &APIError{
 		StatusCode: resp.StatusCode,
+		Message:    strings.Trim(messageMatches[1], " ."),
 	}
 
 	// this is most likely HTML... since un-marshalling JSON failed
@@ -220,33 +281,40 @@ func unknownAPIError(resp *http.Response, requestBody, responseBody []byte, err
 		apiErr.ErrorCode = strings.ReplaceAll(strings.ToUpper(strings.Trim(statusParts[1], " .")), " ", "_")
 	}
 
-	stringBody := string(responseBody)
-	messageRE := regexp.MustCompile(`<pre>(.*)</pre>`)
-	messageMatches := messageRE.FindStringSubmatch(stringBody)
-	// No messages with <pre> </pre> format found so return a APIError
-	if len(messageMatches) < 2 {
-		apiErr.Message = MakeUnexpectedError(resp, err, requestBody, responseBody).Error()
+	return apiErr
+}
+
+// unknownAPIError is a fallback error parser for unexpected error formats.
+func unknownAPIError(resp *http.Response, requestBody, responseBody []byte) *APIError {
+	apiErr := &APIError{
+		StatusCode: resp.StatusCode,
+		Message:    "unable to parse response. " + MakeUnexpectedResponse(resp, requestBody, responseBody),
+	}
+
+	// Preserve status computation from htmlErrorParser in case of unknown error
+	statusParts := strings.SplitN(resp.Status, " ", 2)
+	if len(statusParts) < 2 {
+		apiErr.ErrorCode = "UNKNOWN"
 	} else {
-		apiErr.Message = strings.Trim(messageMatches[1], " .")
+		apiErr.ErrorCode = strings.ReplaceAll(strings.ToUpper(strings.Trim(statusParts[1], " .")), " ", "_")
 	}
 
 	return apiErr
 }
 
-func MakeUnexpectedError(resp *http.Response, err error, requestBody, responseBody []byte) error {
+func MakeUnexpectedResponse(resp *http.Response, requestBody, responseBody []byte) string {
 	var req *http.Request
 	if resp != nil {
 		req = resp.Request
 	}
 	rts := httplog.RoundTripStringer{
 		Request:                  req,
 		Response:                 resp,
-		Err:                      err,
 		RequestBody:              requestBody,
 		ResponseBody:             responseBody,
 		DebugHeaders:             true,
 		DebugTruncateBytes:       10 * 1024,
 		DebugAuthorizationHeader: false,
 	}
-	return fmt.Errorf("unexpected error handling request: %w. This is likely a bug in the Databricks SDK for Go or the underlying REST API. Please report this issue with the following debugging information to the SDK issue tracker at https://github.com/databricks/databricks-sdk-go/issues. Request log:\n```\n%s\n```", err, rts.String())
+	return fmt.Sprintf("This is likely a bug in the Databricks SDK for Go or the underlying REST API. Please report this issue with the following debugging information to the SDK issue tracker at https://github.com/databricks/databricks-sdk-go/issues. Request log:\n```\n%s\n```", rts.String())
 }

apierr/errors_test.go

@@ -3,6 +3,7 @@ package apierr
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"io"
 	"net/http"
 	"net/url"
@@ -12,90 +13,146 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestGetAPIError_handlesEmptyResponse(t *testing.T) {
-	resp := common.ResponseWrapper{
-		Response: &http.Response{
-			Request: &http.Request{
-				Method: "GET",
-				URL: &url.URL{
-					Path: "/api/2.0/compute/get",
-				},
-			},
-			StatusCode: http.StatusConflict,
-		},
-		DebugBytes: []byte{},
-		ReadCloser: io.NopCloser(bytes.NewReader([]byte{})),
+func TestAPIError_transientRegexMatches(t *testing.T) {
+	err := APIError{
+		Message: "worker env WorkerEnvId(workerenv-XXXXX) not found",
 	}
 
-	err := GetAPIError(context.Background(), resp)
-
-	assert.Equal(t, err.(*APIError).Message, "Conflict")
+	assert.True(t, err.IsRetriable(context.Background()))
 }
 
-func TestGetAPIError_appliesOverrides(t *testing.T) {
-	resp := common.ResponseWrapper{
+func makeTestReponseWrapper(statusCode int, resp string) common.ResponseWrapper {
+	return common.ResponseWrapper{
 		Response: &http.Response{
-			StatusCode: http.StatusBadRequest,
+			Status:     fmt.Sprintf("%d %s", statusCode, http.StatusText(statusCode)),
+			StatusCode: statusCode,
 			Request: &http.Request{
 				Method: "GET",
 				URL: &url.URL{
-					Path: "/api/2.1/clusters/get",
+					Path: "/api/2.0/myservice",
 				},
 			},
 		},
 		DebugBytes: []byte{},
-		ReadCloser: io.NopCloser(bytes.NewReader([]byte(`{"error_code": "INVALID_PARAMETER_VALUE", "message": "Cluster abc does not exist"}`))),
+		ReadCloser: io.NopCloser(bytes.NewReader([]byte(resp))),
 	}
-
-	err := GetAPIError(context.Background(), resp)
-
-	assert.ErrorIs(t, err, ErrResourceDoesNotExist)
 }
 
-func TestGetAPIError_parseIntErrorCode(t *testing.T) {
-	resp := common.ResponseWrapper{
-		Response: &http.Response{
-			StatusCode: http.StatusBadRequest,
-			Request: &http.Request{
-				Method: "GET",
-				URL: &url.URL{
-					Path: "/api/2.1/clusters/get",
+func TestAPIError_GetAPIError(t *testing.T) {
+	testCases := []struct {
+		name           string
+		resp           common.ResponseWrapper
+		wantErrorIs    error
+		wantErrorCode  string
+		wantMessage    string
+		wantStatusCode int
+		wantDetails    []ErrorDetail
+	}{
+		{
+			name:           "empty response",
+			resp:           makeTestReponseWrapper(http.StatusNotFound, ""),
+			wantErrorIs:    ErrNotFound,
+			wantErrorCode:  "",
+			wantMessage:    "Not Found",
+			wantStatusCode: http.StatusNotFound,
+		},
+		{
+			name:           "happy path",
+			resp:           makeTestReponseWrapper(http.StatusNotFound, `{"error_code": "RESOURCE_DOES_NOT_EXIST", "message": "Cluster abc does not exist"}`),
+			wantErrorIs:    ErrResourceDoesNotExist,
+			wantErrorCode:  "RESOURCE_DOES_NOT_EXIST",
+			wantMessage:    "Cluster abc does not exist",
+			wantStatusCode: http.StatusNotFound,
+		},
+		{
+			name:           "error details",
+			resp:           makeTestReponseWrapper(http.StatusNotFound, `{"error_code": "RESOURCE_DOES_NOT_EXIST", "message": "Cluster abc does not exist", "details": [{"@type": "type", "reason": "reason", "domain": "domain", "metadata": {"key": "value"}}]}`),
+			wantErrorIs:    ErrResourceDoesNotExist,
+			wantErrorCode:  "RESOURCE_DOES_NOT_EXIST",
+			wantMessage:    "Cluster abc does not exist",
+			wantStatusCode: http.StatusNotFound,
+			wantDetails: []ErrorDetail{
+				{
+					Type:     "type",
+					Reason:   "reason",
+					Domain:   "domain",
+					Metadata: map[string]string{"key": "value"},
 				},
 			},
 		},
-		DebugBytes: []byte{},
-		ReadCloser: io.NopCloser(bytes.NewReader([]byte(`{"error_code": 500, "status_code": 400, "message": "Cluster abc does not exist"}`))),
-	}
-
-	err := GetAPIError(context.Background(), resp)
-
-	assert.ErrorIs(t, err, ErrBadRequest)
-	assert.Equal(t, err.(*APIError).ErrorCode, "500")
-}
-
-func TestGetAPIError_mapsPrivateLinkRedirect(t *testing.T) {
-	resp := common.ResponseWrapper{
-		Response: &http.Response{
-			Request: &http.Request{
-				URL: &url.URL{
-					Host:     "adb-12345678910.1.azuredatabricks.net",
-					Path:     "/login.html",
-					RawQuery: "error=private-link-validation-error",
+		{
+			name:           "string error response",
+			resp:           makeTestReponseWrapper(http.StatusBadRequest, `MALFORMED_REQUEST: vpc_endpoints malformed parameters: VPC Endpoint ... with use_case ... cannot be attached in ... list`),
+			wantErrorIs:    ErrBadRequest,
+			wantErrorCode:  "MALFORMED_REQUEST",
+			wantMessage:    "vpc_endpoints malformed parameters: VPC Endpoint ... with use_case ... cannot be attached in ... list",
+			wantStatusCode: http.StatusBadRequest,
+		},
+		{
+			name:           "numeric error code",
+			resp:           makeTestReponseWrapper(http.StatusBadRequest, `{"error_code": 500, "message": "Cluster abc does not exist"}`),
+			wantErrorIs:    ErrBadRequest,
+			wantErrorCode:  "500",
+			wantMessage:    "Cluster abc does not exist",
+			wantStatusCode: http.StatusBadRequest,
+		},
+		{
+			name: "private link redirect",
+			resp: common.ResponseWrapper{
+				Response: &http.Response{
+					Request: &http.Request{
+						URL: &url.URL{
+							Host:     "adb-12345678910.1.azuredatabricks.net",
+							Path:     "/login.html",
+							RawQuery: "error=private-link-validation-error",
+						},
+					},
+				},
+			},
+			wantErrorIs:    ErrPermissionDenied,
+			wantErrorCode:  "PRIVATE_LINK_VALIDATION_ERROR",
+			wantMessage:    "The requested workspace has Azure Private Link enabled and is not accessible from the current network. Ensure that Azure Private Link is properly configured and that your device has access to the Azure Private Link endpoint. For more information, see https://learn.microsoft.com/en-us/azure/databricks/security/network/classic/private-link-standard#authentication-troubleshooting.",
+			wantStatusCode: http.StatusForbidden,
+		},
+		{
+			name: "applies overrides",
+			resp: common.ResponseWrapper{
+				Response: &http.Response{
+					StatusCode: http.StatusBadRequest,
+					Request: &http.Request{
+						Method: "GET",
+						URL: &url.URL{
+							Path: "/api/2.1/clusters/get",
+						},
+					},
 				},
+				DebugBytes: []byte{},
+				ReadCloser: io.NopCloser(bytes.NewReader([]byte(`{"error_code": "INVALID_PARAMETER_VALUE", "message": "Cluster abc does not exist"}`))),
 			},
+			wantErrorIs:    ErrResourceDoesNotExist,
+			wantErrorCode:  "INVALID_PARAMETER_VALUE",
+			wantMessage:    "Cluster abc does not exist",
+			wantStatusCode: http.StatusBadRequest,
+		},
+		{
+			name:           "unexpected error",
+			resp:           makeTestReponseWrapper(http.StatusInternalServerError, `unparsable error message`),
+			wantErrorCode:  "INTERNAL_SERVER_ERROR",
+			wantMessage:    "unable to parse response. This is likely a bug in the Databricks SDK for Go or the underlying REST API. Please report this issue with the following debugging information to the SDK issue tracker at https://github.com/databricks/databricks-sdk-go/issues. Request log:\n```\nGET /api/2.0/myservice\n> * Host: \n<  500 Internal Server Error\n< unparsable error message\n```",
+			wantStatusCode: http.StatusInternalServerError,
 		},
 	}
 
-	err := GetAPIError(context.Background(), resp)
-
-	assert.ErrorIs(t, err, ErrPermissionDenied)
-	assert.Equal(t, err.(*APIError).ErrorCode, "PRIVATE_LINK_VALIDATION_ERROR")
-}
-
-func TestAPIError_transientRegexMatches(t *testing.T) {
-	err := APIError{
-		Message: "worker env WorkerEnvId(workerenv-XXXXX) not found",
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := GetAPIError(context.Background(), tc.resp).(*APIError)
+			assert.Equal(t, tc.wantErrorCode, got.ErrorCode)
+			assert.Equal(t, tc.wantMessage, got.Message)
+			assert.Equal(t, tc.wantStatusCode, got.StatusCode)
+			assert.Equal(t, tc.wantDetails, got.Details)
+			if tc.wantErrorIs != nil {
+				assert.ErrorIs(t, got, tc.wantErrorIs)
+			}
+		})
 	}
-
-	assert.True(t, err.IsRetriable(context.Background()))
 }