Skip to content

Commit 796dae1

Browse files
mgyuchtrenaudhartert-db
mgyucht
and
renaudhartert-db
authored
[Fix] Handle non-JSON errors (#1031)
## 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]>
1 parent a24a158 commit 796dae1

File tree

4 files changed

+205
-80
lines changed

4 files changed

+205
-80
lines changed

‎apierr/errors.go‎

Lines changed: 82 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ func GetAPIError(ctx context.Context, resp common.ResponseWrapper) error {
143143
if err != nil {
144144
return ReadError(resp.Response.StatusCode, err)
145145
}
146-
apiError := parseErrorFromResponse(resp.Response, resp.RequestBody.DebugBytes, responseBodyBytes)
146+
apiError := parseErrorFromResponse(ctx, resp.Response, resp.RequestBody.DebugBytes, responseBodyBytes)
147147
applyOverrides(ctx, apiError, resp.Response)
148148
return apiError
149149
}
@@ -155,14 +155,43 @@ func GetAPIError(ctx context.Context, resp common.ResponseWrapper) error {
155155
return nil
156156
}
157157

158-
func parseErrorFromResponse(resp *http.Response, requestBody, responseBody []byte) *APIError {
158+
// errorParser attempts to parse the error from the response body. If successful,
159+
// it returns a non-nil *APIError. It returns nil if parsing fails or no error is found.
160+
type errorParser func(context.Context, *http.Response, []byte) *APIError
161+
162+
// errorParsers is a list of errorParser functions that are tried in order to
163+
// parse an API error from a response body. Most errors should be parsable by
164+
// the standardErrorParser, but additional parsers can be added here for
165+
// specific error formats. The order of the parsers is not important, as the set
166+
// of errors that can be parsed by each parser should be disjoint.
167+
var errorParsers = []errorParser{
168+
standardErrorParser,
169+
stringErrorParser,
170+
htmlErrorParser,
171+
}
172+
173+
func parseErrorFromResponse(ctx context.Context, resp *http.Response, requestBody, responseBody []byte) *APIError {
159174
if len(responseBody) == 0 {
160175
return &APIError{
161176
Message: http.StatusText(resp.StatusCode),
162177
StatusCode: resp.StatusCode,
163178
}
164179
}
165180

181+
for _, parser := range errorParsers {
182+
if apiError := parser(ctx, resp, responseBody); apiError != nil {
183+
return apiError
184+
}
185+
}
186+
187+
return unknownAPIError(resp, requestBody, responseBody)
188+
}
189+
190+
// standardErrorParser is the default error parser for Databricks API errors.
191+
// It handles JSON error messages with error code, message, and details fields.
192+
// It also provides compatibility with the old API 1.2 error format and SCIM API
193+
// errors.
194+
func standardErrorParser(ctx context.Context, resp *http.Response, responseBody []byte) *APIError {
166195
// Anonymous struct used to unmarshal JSON Databricks API error responses.
167196
var errorBody struct {
168197
ErrorCode any `json:"error_code,omitempty"` // int or string
@@ -177,7 +206,8 @@ func parseErrorFromResponse(resp *http.Response, requestBody, responseBody []byt
177206
ScimType string `json:"scimType,omitempty"`
178207
}
179208
if err := json.Unmarshal(responseBody, &errorBody); err != nil {
180-
return unknownAPIError(resp, requestBody, responseBody, err)
209+
logger.Tracef(ctx, "standardErrorParser: failed to unmarshal error response: %s", err)
210+
return nil
181211
}
182212

183213
// 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
206236
}
207237
}
208238

209-
func unknownAPIError(resp *http.Response, requestBody, responseBody []byte, err error) *APIError {
239+
var stringErrorRegex = regexp.MustCompile(`^([A-Z_]+): (.*)$`)
240+
241+
// stringErrorParser parses errors of the form "STATUS_CODE: status message".
242+
// Some account-level APIs respond with this error code, e.g.
243+
// https://github.com/databricks/databricks-sdk-go/issues/998
244+
func stringErrorParser(ctx context.Context, resp *http.Response, responseBody []byte) *APIError {
245+
matches := stringErrorRegex.FindSubmatch(responseBody)
246+
if len(matches) < 3 {
247+
logger.Tracef(ctx, "stringErrorParser: failed to match error response")
248+
return nil
249+
}
250+
return &APIError{
251+
Message: string(matches[2]),
252+
ErrorCode: string(matches[1]),
253+
StatusCode: resp.StatusCode,
254+
}
255+
}
256+
257+
var htmlMessageRe = regexp.MustCompile(`<pre>(.*)</pre>`)
258+
259+
// htmlErrorParser parses HTML error responses. Some legacy APIs respond with
260+
// an HTML page in certain error cases, like when trying to create a cluster
261+
// before the worker environment is ready.
262+
func htmlErrorParser(ctx context.Context, resp *http.Response, responseBody []byte) *APIError {
263+
messageMatches := htmlMessageRe.FindStringSubmatch(string(responseBody))
264+
// No messages with <pre> </pre> format found so return a APIError
265+
if len(messageMatches) < 2 {
266+
logger.Tracef(ctx, "htmlErrorParser: no <pre> tag found in error response")
267+
return nil
268+
}
269+
210270
apiErr := &APIError{
211271
StatusCode: resp.StatusCode,
272+
Message: strings.Trim(messageMatches[1], " ."),
212273
}
213274

214275
// this is most likely HTML... since un-marshalling JSON failed
@@ -220,33 +281,40 @@ func unknownAPIError(resp *http.Response, requestBody, responseBody []byte, err
220281
apiErr.ErrorCode = strings.ReplaceAll(strings.ToUpper(strings.Trim(statusParts[1], " .")), " ", "_")
221282
}
222283

223-
stringBody := string(responseBody)
224-
messageRE := regexp.MustCompile(`<pre>(.*)</pre>`)
225-
messageMatches := messageRE.FindStringSubmatch(stringBody)
226-
// No messages with <pre> </pre> format found so return a APIError
227-
if len(messageMatches) < 2 {
228-
apiErr.Message = MakeUnexpectedError(resp, err, requestBody, responseBody).Error()
284+
return apiErr
285+
}
286+
287+
// unknownAPIError is a fallback error parser for unexpected error formats.
288+
func unknownAPIError(resp *http.Response, requestBody, responseBody []byte) *APIError {
289+
apiErr := &APIError{
290+
StatusCode: resp.StatusCode,
291+
Message: "unable to parse response. " + MakeUnexpectedResponse(resp, requestBody, responseBody),
292+
}
293+
294+
// Preserve status computation from htmlErrorParser in case of unknown error
295+
statusParts := strings.SplitN(resp.Status, " ", 2)
296+
if len(statusParts) < 2 {
297+
apiErr.ErrorCode = "UNKNOWN"
229298
} else {
230-
apiErr.Message = strings.Trim(messageMatches[1], " .")
299+
apiErr.ErrorCode = strings.ReplaceAll(strings.ToUpper(strings.Trim(statusParts[1], " .")), " ", "_")
231300
}
232301

233302
return apiErr
234303
}
235304

236-
func MakeUnexpectedError(resp *http.Response, err error, requestBody, responseBody []byte) error {
305+
func MakeUnexpectedResponse(resp *http.Response, requestBody, responseBody []byte) string {
237306
var req *http.Request
238307
if resp != nil {
239308
req = resp.Request
240309
}
241310
rts := httplog.RoundTripStringer{
242311
Request: req,
243312
Response: resp,
244-
Err: err,
245313
RequestBody: requestBody,
246314
ResponseBody: responseBody,
247315
DebugHeaders: true,
248316
DebugTruncateBytes: 10 * 1024,
249317
DebugAuthorizationHeader: false,
250318
}
251-
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())
319+
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())
252320
}

‎apierr/errors_test.go‎

Lines changed: 119 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package apierr
33
import (
44
"bytes"
55
"context"
6+
"fmt"
67
"io"
78
"net/http"
89
"net/url"
@@ -12,90 +13,146 @@ import (
1213
"github.com/stretchr/testify/assert"
1314
)
1415

15-
func TestGetAPIError_handlesEmptyResponse(t *testing.T) {
16-
resp := common.ResponseWrapper{
17-
Response: &http.Response{
18-
Request: &http.Request{
19-
Method: "GET",
20-
URL: &url.URL{
21-
Path: "/api/2.0/compute/get",
22-
},
23-
},
24-
StatusCode: http.StatusConflict,
25-
},
26-
DebugBytes: []byte{},
27-
ReadCloser: io.NopCloser(bytes.NewReader([]byte{})),
16+
func TestAPIError_transientRegexMatches(t *testing.T) {
17+
err := APIError{
18+
Message: "worker env WorkerEnvId(workerenv-XXXXX) not found",
2819
}
2920

30-
err := GetAPIError(context.Background(), resp)
31-
32-
assert.Equal(t, err.(*APIError).Message, "Conflict")
21+
assert.True(t, err.IsRetriable(context.Background()))
3322
}
3423

35-
func TestGetAPIError_appliesOverrides(t *testing.T) {
36-
resp := common.ResponseWrapper{
24+
func makeTestReponseWrapper(statusCode int, resp string) common.ResponseWrapper {
25+
return common.ResponseWrapper{
3726
Response: &http.Response{
38-
StatusCode: http.StatusBadRequest,
27+
Status: fmt.Sprintf("%d %s", statusCode, http.StatusText(statusCode)),
28+
StatusCode: statusCode,
3929
Request: &http.Request{
4030
Method: "GET",
4131
URL: &url.URL{
42-
Path: "/api/2.1/clusters/get",
32+
Path: "/api/2.0/myservice",
4333
},
4434
},
4535
},
4636
DebugBytes: []byte{},
47-
ReadCloser: io.NopCloser(bytes.NewReader([]byte(`{"error_code": "INVALID_PARAMETER_VALUE", "message": "Cluster abc does not exist"}`))),
37+
ReadCloser: io.NopCloser(bytes.NewReader([]byte(resp))),
4838
}
49-
50-
err := GetAPIError(context.Background(), resp)
51-
52-
assert.ErrorIs(t, err, ErrResourceDoesNotExist)
5339
}
5440

55-
func TestGetAPIError_parseIntErrorCode(t *testing.T) {
56-
resp := common.ResponseWrapper{
57-
Response: &http.Response{
58-
StatusCode: http.StatusBadRequest,
59-
Request: &http.Request{
60-
Method: "GET",
61-
URL: &url.URL{
62-
Path: "/api/2.1/clusters/get",
41+
func TestAPIError_GetAPIError(t *testing.T) {
42+
testCases := []struct {
43+
name string
44+
resp common.ResponseWrapper
45+
wantErrorIs error
46+
wantErrorCode string
47+
wantMessage string
48+
wantStatusCode int
49+
wantDetails []ErrorDetail
50+
}{
51+
{
52+
name: "empty response",
53+
resp: makeTestReponseWrapper(http.StatusNotFound, ""),
54+
wantErrorIs: ErrNotFound,
55+
wantErrorCode: "",
56+
wantMessage: "Not Found",
57+
wantStatusCode: http.StatusNotFound,
58+
},
59+
{
60+
name: "happy path",
61+
resp: makeTestReponseWrapper(http.StatusNotFound, `{"error_code": "RESOURCE_DOES_NOT_EXIST", "message": "Cluster abc does not exist"}`),
62+
wantErrorIs: ErrResourceDoesNotExist,
63+
wantErrorCode: "RESOURCE_DOES_NOT_EXIST",
64+
wantMessage: "Cluster abc does not exist",
65+
wantStatusCode: http.StatusNotFound,
66+
},
67+
{
68+
name: "error details",
69+
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"}}]}`),
70+
wantErrorIs: ErrResourceDoesNotExist,
71+
wantErrorCode: "RESOURCE_DOES_NOT_EXIST",
72+
wantMessage: "Cluster abc does not exist",
73+
wantStatusCode: http.StatusNotFound,
74+
wantDetails: []ErrorDetail{
75+
{
76+
Type: "type",
77+
Reason: "reason",
78+
Domain: "domain",
79+
Metadata: map[string]string{"key": "value"},
6380
},
6481
},
6582
},
66-
DebugBytes: []byte{},
67-
ReadCloser: io.NopCloser(bytes.NewReader([]byte(`{"error_code": 500, "status_code": 400, "message": "Cluster abc does not exist"}`))),
68-
}
69-
70-
err := GetAPIError(context.Background(), resp)
71-
72-
assert.ErrorIs(t, err, ErrBadRequest)
73-
assert.Equal(t, err.(*APIError).ErrorCode, "500")
74-
}
75-
76-
func TestGetAPIError_mapsPrivateLinkRedirect(t *testing.T) {
77-
resp := common.ResponseWrapper{
78-
Response: &http.Response{
79-
Request: &http.Request{
80-
URL: &url.URL{
81-
Host: "adb-12345678910.1.azuredatabricks.net",
82-
Path: "/login.html",
83-
RawQuery: "error=private-link-validation-error",
83+
{
84+
name: "string error response",
85+
resp: makeTestReponseWrapper(http.StatusBadRequest, `MALFORMED_REQUEST: vpc_endpoints malformed parameters: VPC Endpoint ... with use_case ... cannot be attached in ... list`),
86+
wantErrorIs: ErrBadRequest,
87+
wantErrorCode: "MALFORMED_REQUEST",
88+
wantMessage: "vpc_endpoints malformed parameters: VPC Endpoint ... with use_case ... cannot be attached in ... list",
89+
wantStatusCode: http.StatusBadRequest,
90+
},
91+
{
92+
name: "numeric error code",
93+
resp: makeTestReponseWrapper(http.StatusBadRequest, `{"error_code": 500, "message": "Cluster abc does not exist"}`),
94+
wantErrorIs: ErrBadRequest,
95+
wantErrorCode: "500",
96+
wantMessage: "Cluster abc does not exist",
97+
wantStatusCode: http.StatusBadRequest,
98+
},
99+
{
100+
name: "private link redirect",
101+
resp: common.ResponseWrapper{
102+
Response: &http.Response{
103+
Request: &http.Request{
104+
URL: &url.URL{
105+
Host: "adb-12345678910.1.azuredatabricks.net",
106+
Path: "/login.html",
107+
RawQuery: "error=private-link-validation-error",
108+
},
109+
},
110+
},
111+
},
112+
wantErrorIs: ErrPermissionDenied,
113+
wantErrorCode: "PRIVATE_LINK_VALIDATION_ERROR",
114+
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.",
115+
wantStatusCode: http.StatusForbidden,
116+
},
117+
{
118+
name: "applies overrides",
119+
resp: common.ResponseWrapper{
120+
Response: &http.Response{
121+
StatusCode: http.StatusBadRequest,
122+
Request: &http.Request{
123+
Method: "GET",
124+
URL: &url.URL{
125+
Path: "/api/2.1/clusters/get",
126+
},
127+
},
84128
},
129+
DebugBytes: []byte{},
130+
ReadCloser: io.NopCloser(bytes.NewReader([]byte(`{"error_code": "INVALID_PARAMETER_VALUE", "message": "Cluster abc does not exist"}`))),
85131
},
132+
wantErrorIs: ErrResourceDoesNotExist,
133+
wantErrorCode: "INVALID_PARAMETER_VALUE",
134+
wantMessage: "Cluster abc does not exist",
135+
wantStatusCode: http.StatusBadRequest,
136+
},
137+
{
138+
name: "unexpected error",
139+
resp: makeTestReponseWrapper(http.StatusInternalServerError, `unparsable error message`),
140+
wantErrorCode: "INTERNAL_SERVER_ERROR",
141+
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```",
142+
wantStatusCode: http.StatusInternalServerError,
86143
},
87144
}
88145

89-
err := GetAPIError(context.Background(), resp)
90-
91-
assert.ErrorIs(t, err, ErrPermissionDenied)
92-
assert.Equal(t, err.(*APIError).ErrorCode, "PRIVATE_LINK_VALIDATION_ERROR")
93-
}
94-
95-
func TestAPIError_transientRegexMatches(t *testing.T) {
96-
err := APIError{
97-
Message: "worker env WorkerEnvId(workerenv-XXXXX) not found",
146+
for _, tc := range testCases {
147+
t.Run(tc.name, func(t *testing.T) {
148+
got := GetAPIError(context.Background(), tc.resp).(*APIError)
149+
assert.Equal(t, tc.wantErrorCode, got.ErrorCode)
150+
assert.Equal(t, tc.wantMessage, got.Message)
151+
assert.Equal(t, tc.wantStatusCode, got.StatusCode)
152+
assert.Equal(t, tc.wantDetails, got.Details)
153+
if tc.wantErrorIs != nil {
154+
assert.ErrorIs(t, got, tc.wantErrorIs)
155+
}
156+
})
98157
}
99-
100-
assert.True(t, err.IsRetriable(context.Background()))
101158
}

0 commit comments

Comments
 (0)