Skip to content

Commit bc7906f

Browse files
🐛 api If the API response is successful but an error occured during marshalling, make sure to return the marshalling error (#114)
<!-- Copyright (C) 2020-2022 Arm Limited or its affiliates and Contributors. All rights reserved. SPDX-License-Identifier: Proprietary --> ### Description The `CallAndCheckSuccess` function calls this: https://github.com/ARM-software/embedded-development-services-client/blob/3175386be71f4f5f65f4a0a7f25b88fb90f22dce/client/api_fpga_jobs.go#L3178 The call itself is a 200, so you reach this part of the code: https://github.com/ARM-software/embedded-development-services-client/blob/3175386be71f4f5f65f4a0a7f25b88fb90f22dce/client/api_fpga_jobs.go#L3324 ```go err = a.client.decode(&localVarReturnValue, localVarBody, localVarHTTPResponse.Header.Get("Content-Type")) if err != nil { newErr := &GenericOpenAPIError{ body: localVarBody, error: err.Error(), } return localVarReturnValue, localVarHTTPResponse, newErr } ``` That is here https://github.com/ARM-software/embedded-development-services-client/blob/3175386be71f4f5f65f4a0a7f25b88fb90f22dce/client/client.go#L506 If this unmarshal fails (*maybe because of the strict encoding, I am not sure*) here: ```go if JsonCheck.MatchString(contentType) { if actualObj, ok := v.(interface{ GetActualInstance() interface{} }); ok { // oneOf, anyOf schemas if unmarshalObj, ok := actualObj.(interface{ UnmarshalJSON([]byte) error }); ok { // make sure it has UnmarshalJSON defined if err = unmarshalObj.UnmarshalJSON(b); err != nil { return err } } else { return errors.New("Unknown type with GetActualInstance but no unmarshalObj.UnmarshalJSON defined") } } else if err = json.Unmarshal(b, v); err != nil { // simple model return err } return nil } ``` Then you hit the error in the execute function: ```go return localVarReturnValue, localVarHTTPResponse, newErr ``` So `localVarReturnValue` will be empty, but `localVarHTTPResponse` will be a 200 response all okay, and `newErr` will be a marshalling error. Then in `CallAndCheckSuccess` you hit the `CheckAPICallSuccess` check https://github.com/ARM-software/embedded-development-services-client-utils/blob/4bf750bdcd7ffd23f4f4be2d1c39578a062f2f3e/utils/api/api.go#L76 This takes you to https://github.com/ARM-software/embedded-development-services-client-utils/blob/4bf750bdcd7ffd23f4f4be2d1c39578a062f2f3e/utils/api/api.go#L34: ```go func CheckAPICallSuccess(ctx context.Context, errorContext string, resp *_http.Response, apiErr error) (err error) { err = parallelisation.DetermineContextError(ctx) if err != nil { return } if !IsCallSuccessful(resp) { statusCode := 0 errorMessage := strings.Builder{} if resp != nil { statusCode = resp.StatusCode errorDetails, subErr := errors.FetchAPIErrorDescriptionWithContext(ctx, resp) if commonerrors.Ignore(subErr, commonerrors.ErrMarshalling) != nil { err = subErr return } if !reflection.IsEmpty(errorDetails) { errorMessage.WriteString(errorDetails) } _ = resp.Body.Close() } extra := "" if apiErr != nil { extra = fmt.Sprintf("; %v", apiErr.Error()) } err = fmt.Errorf("%v (%d): %v%v", errorContext, statusCode, errorMessage.String(), extra) } return } ``` But that operates off of the response (which is a 200) so that function gets skipped basically entirely and returns `nil` (really it should check `apiErr` at the beginning so that it returns an error). This means you get to https://github.com/ARM-software/embedded-development-services-client-utils/blob/4bf750bdcd7ffd23f4f4be2d1c39578a062f2f3e/utils/api/api.go#L80 and if the result is empty you will see the behaviour we are seeing. So in summary: ```go result, resp, apiErr := apiCallFunc(ctx) // <some result>, 200 response, decode error if resp != nil && resp.Body != nil { _ = resp.Body.Close() } if err = CheckAPICallSuccess(ctx, errorContext, resp, apiErr); err != nil { return // this won't be hit because it only checks apiErr if resp status is a bad one } // which means you get to here with an invalid result if result != nil { if reflection.IsEmpty(result) { err = commonerrors.New(commonerrors.ErrMarshalling, "unmarshalled response is empty") return } } ``` <!-- Please add any detail or context that would be useful to a reviewer. --> This change should make sure that if the API response is successful but an error occurred during marshalling then the marshalling error will be returned not an empty response error. ### Test Coverage <!-- Please put an `x` in the correct box e.g. `[x]` to indicate the testing coverage of this change. --> - [x] This change is covered by existing or additional automated tests. - [ ] Manual testing has been performed (and evidence provided) as automated testing was not feasible. - [ ] Additional tests are not required for this change (e.g. documentation update).
1 parent 4bf750b commit bc7906f

File tree

3 files changed

+18
-12
lines changed

3 files changed

+18
-12
lines changed

changes/20250620134750.bugfix

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
:bug: `api` If the API response is successful but an error occured during marshalling, make sure to return the marshalling error

utils/api/api.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,14 @@ func CallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCal
7777
return
7878
}
7979

80-
if result != nil {
81-
if reflection.IsEmpty(result) {
82-
err = commonerrors.New(commonerrors.ErrMarshalling, "unmarshalled response is empty")
83-
return
84-
}
80+
if apiErr != nil {
81+
err = commonerrors.WrapError(commonerrors.ErrMarshalling, apiErr, "API call was successful but an error occurred during response marshalling")
82+
return
83+
}
84+
85+
if result != nil && reflection.IsEmpty(result) {
86+
err = commonerrors.New(commonerrors.ErrMarshalling, "unmarshalled response is empty")
87+
return
8588
}
8689

8790
return

utils/api/api_test.go

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ func TestCallAndCheckSuccess(t *testing.T) {
8787
cancelCtx()
8888
_, actualErr := CallAndCheckSuccess(ctx, errMessage,
8989
func(ctx context.Context) (*struct{}, *_http.Response, error) {
90-
return nil, &_http.Response{Body: io.NopCloser(bytes.NewReader(nil))}, errors.New(errMessage)
90+
return nil, &_http.Response{Body: io.NopCloser(bytes.NewReader(nil))}, nil
9191
})
9292
errortest.AssertError(t, actualErr, commonerrors.ErrCancelled)
9393
})
@@ -104,24 +104,26 @@ func TestCallAndCheckSuccess(t *testing.T) {
104104
assert.Equal(t, actualErr.Error(), expectedErr)
105105
})
106106

107-
t.Run("no context error, api call successful", func(t *testing.T) {
107+
t.Run("api call successful, empty response", func(t *testing.T) {
108108
errMessage := "no error"
109109
parentCtx := context.Background()
110110
_, err := CallAndCheckSuccess(parentCtx, errMessage,
111111
func(ctx context.Context) (*struct{}, *_http.Response, error) {
112-
return nil, &_http.Response{StatusCode: 200}, errors.New(errMessage)
112+
return &struct{}{}, &_http.Response{StatusCode: 200}, errors.New(errMessage)
113113
})
114-
assert.NoError(t, err)
114+
errortest.AssertError(t, err, commonerrors.ErrMarshalling)
115+
errortest.AssertErrorDescription(t, err, "API call was successful but an error occurred during response marshalling")
115116
})
116117

117-
t.Run("api call successful, empty response", func(t *testing.T) {
118-
errMessage := "response error"
118+
t.Run("api call successful, broken response decode", func(t *testing.T) {
119+
errMessage := "no error"
119120
parentCtx := context.Background()
120121
_, err := CallAndCheckSuccess(parentCtx, errMessage,
121122
func(ctx context.Context) (*struct{}, *_http.Response, error) {
122-
return &struct{}{}, &_http.Response{StatusCode: 200}, errors.New(errMessage)
123+
return &struct{}{}, &_http.Response{StatusCode: 200}, nil
123124
})
124125
errortest.AssertError(t, err, commonerrors.ErrMarshalling)
126+
errortest.AssertErrorDescription(t, err, "unmarshalled response is empty")
125127
})
126128
}
127129

0 commit comments

Comments
 (0)