Skip to content

Commit

Permalink
[Fix] Handle non-JSON errors (#1031)
Browse files Browse the repository at this point in the history 
## Changes
Some errors returned by the platform are not serialized using JSON (see
#998 for an
example). They are instead serialized in the form "<ERROR_CODE>:
<MESSAGE>". Today, the SDK cannot parse these error messages well,
resulting in a poor user experience.

This PR adds support for parsing these error messages from the platform
to the SDK. This should reduce bug reports for the SDK with respect to
unexpected response parsing. This PR also refactors the error
deserialization logic somewhat to make it more extensible in the future
for other potential error formats that are not currently handled.

## Breaking Changes
This PR renames MakeUnexpectedError() to MakeUnexpectedResponse() in the
`apierr` package. It also changes the return type to string. This makes
the message easier to incorporate into error responses that only depend
on the string representation of the error, as well as allows us to start
the message with a capital letter, as it is a complete sentence.

The error message for failed deserialization of valid responses has
changed slightly, from `unexpected error handling request` to `failed to
unmarshal response body`. The rest of the message is identical.

## Tests
Refactored unit tests to a table-driven test case, and added four new
cases: one for error details (not previously covered), one for the
regular happy path, one for unexpected responses, and one for the new
error message format.

- [ ] `make test` passing
- [ ] `make fmt` applied
- [ ] relevant integration tests applied

---------

Co-authored-by: Renaud Hartert <[email protected]>
  • Loading branch information
@mgyucht @renaudhartert-db
mgyucht and renaudhartert-db authored Aug 29, 2024
1 parent a24a158 commit 796dae1
Show file tree
Hide file tree
Showing 4 changed files with 205 additions and 80 deletions.
96 changes: 82 additions & 14 deletions apierr/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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())
}
181 changes: 119 additions & 62 deletions apierr/errors_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package apierr
import (
"bytes"
"context"
"fmt"
"io"
"net/http"
"net/url"
Expand All @@ -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()))
}
Loading

0 comments on commit 796dae1

Please sign in to comment.