Skip to content

Commit 152e9dc

Browse files
acabarbayeacabarbaye
authored
🐛 Fix response unmarshalling issue when API has unknown fields (#115)
<!-- Copyright (C) 2020-2022 Arm Limited or its affiliates and Contributors. All rights reserved. SPDX-License-Identifier: Proprietary --> ### Description fix problem we see with clients when upgrading APIs ### 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 24f2ee0 commit 152e9dc

File tree

6 files changed

+129
-29
lines changed

6 files changed

+129
-29
lines changed

‎.secrets.baseline‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,6 @@
6666
{
6767
"path": "detect_secrets.filters.allowlist.is_line_allowlisted"
6868
},
69-
{
70-
"path": "detect_secrets.filters.common.is_baseline_file",
71-
"filename": ".secrets.baseline"
72-
},
7369
{
7470
"path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies",
7571
"min_level": 2
@@ -105,5 +101,5 @@
105101
}
106102
],
107103
"results": {},
108-
"generated_at": "2025-06-12T11:22:54Z"
104+
"generated_at": "2025-06-25T17:16:15Z"
109105
}

‎changes/20250625181408.bugfix‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
:bug: Fix response unmarshalling issue when API has unknown fields

‎utils/api/api.go‎

Lines changed: 44 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,29 +9,34 @@ package api
99
import (
1010
"context"
1111
"fmt"
12-
_http "net/http"
12+
"net/http"
1313
"reflect"
1414
"strings"
1515

16+
"github.com/perimeterx/marshmallow"
17+
1618
"github.com/ARM-software/embedded-development-services-client-utils/utils/errors"
1719
"github.com/ARM-software/golang-utils/utils/commonerrors"
1820
"github.com/ARM-software/golang-utils/utils/parallelisation"
1921
"github.com/ARM-software/golang-utils/utils/reflection"
22+
"github.com/ARM-software/golang-utils/utils/safeio"
2023
)
2124

25+
const requiredFieldError = "no value given for required property"
26+
2227
// IsCallSuccessful determines whether an API response is successful or not
23-
func IsCallSuccessful(r *_http.Response) bool {
28+
func IsCallSuccessful(r *http.Response) bool {
2429
if r == nil {
2530
return false
2631
}
27-
return r.StatusCode >= _http.StatusOK && r.StatusCode < _http.StatusMultipleChoices
32+
return r.StatusCode >= http.StatusOK && r.StatusCode < http.StatusMultipleChoices
2833
}
2934

3035
// CheckAPICallSuccess verifies whether an API response is successful or not and if not, populates an error with all the information needed.
3136
// errorContext corresponds to the description of what led to the error if error there is e.g. `Failed adding a user`.
3237
// resp corresponds to the HTTP response from a certain endpoint. The body of such response is not closed by this function.
3338
// apiErr corresponds to the error which may be returned by the HTTP client when calling the endpoint.
34-
func CheckAPICallSuccess(ctx context.Context, errorContext string, resp *_http.Response, apiErr error) (err error) {
39+
func CheckAPICallSuccess(ctx context.Context, errorContext string, resp *http.Response, apiErr error) (err error) {
3540
err = parallelisation.DetermineContextError(ctx)
3641
if err != nil {
3742
return
@@ -63,38 +68,63 @@ func CheckAPICallSuccess(ctx context.Context, errorContext string, resp *_http.R
6368
// CallAndCheckSuccess is a wrapper for making an API call and then checking success with `CheckAPICallSuccess`
6469
// errorContext corresponds to the description of what led to the error if error there is e.g. `Failed adding a user`.
6570
// apiCallFunc corresponds to a generic function that will be called to make the API call
66-
func CallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCallFunc func(ctx context.Context) (*T, *_http.Response, error)) (result *T, err error) {
71+
func CallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCallFunc func(ctx context.Context) (*T, *http.Response, error)) (result *T, err error) {
6772
if err = parallelisation.DetermineContextError(ctx); err != nil {
6873
return
6974
}
7075

7176
result, resp, apiErr := apiCallFunc(ctx)
7277
if resp != nil && resp.Body != nil {
73-
_ = resp.Body.Close()
78+
defer func() {
79+
if resp != nil && resp.Body != nil {
80+
_ = resp.Body.Close()
81+
}
82+
}()
7483
}
7584

76-
if err = CheckAPICallSuccess(ctx, errorContext, resp, apiErr); err != nil {
85+
err = checkResponse(ctx, apiErr, resp, result, errorContext)
86+
return
87+
}
88+
89+
func checkResponse(ctx context.Context, apiErr error, resp *http.Response, result any, errorContext string) (err error) {
90+
err = CheckAPICallSuccess(ctx, errorContext, resp, apiErr)
91+
if err != nil {
7792
return
7893
}
7994

8095
if apiErr != nil {
8196
err = commonerrors.WrapError(commonerrors.ErrMarshalling, apiErr, "API call was successful but an error occurred during response marshalling")
82-
return
97+
if commonerrors.CorrespondTo(apiErr, requiredFieldError) {
98+
return
99+
}
100+
if resp == nil || resp.Body == nil {
101+
return
102+
}
103+
// At this point, the marshalling problem may be due to the present of unknown fields in the response due to an API extension.
104+
// See https://github.com/OpenAPITools/openapi-generator/issues/21446
105+
var respB []byte
106+
respB, err = safeio.ReadAll(ctx, resp.Body)
107+
if err != nil {
108+
return
109+
}
110+
_, err = marshmallow.Unmarshal(respB, result, marshmallow.WithSkipPopulateStruct(false), marshmallow.WithExcludeKnownFieldsFromMap(true))
111+
if err != nil {
112+
err = commonerrors.WrapError(commonerrors.ErrMarshalling, err, "API call was successful but an error occurred during response marshalling")
113+
return
114+
}
83115
}
84-
85-
if result != nil && reflection.IsEmpty(result) {
116+
if reflection.IsEmpty(result) {
86117
err = commonerrors.New(commonerrors.ErrMarshalling, "unmarshalled response is empty")
87118
return
88119
}
89-
90120
return
91121
}
92122

93123
// GenericCallAndCheckSuccess is similar to CallAndCheckSuccess but for function returning interfaces rather than concrete types.
94124
// T must be an interface.
95125
// errorContext corresponds to the description of what led to the error if error there is e.g. `Failed adding a user`.
96126
// apiCallFunc corresponds to a generic function that will be called to make the API call
97-
func GenericCallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCallFunc func(ctx context.Context) (T, *_http.Response, error)) (result T, err error) {
127+
func GenericCallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCallFunc func(ctx context.Context) (T, *http.Response, error)) (result T, err error) {
98128
if err = parallelisation.DetermineContextError(ctx); err != nil {
99129
return
100130
}
@@ -104,7 +134,8 @@ func GenericCallAndCheckSuccess[T any](ctx context.Context, errorContext string,
104134
_ = resp.Body.Close()
105135
}
106136

107-
if err = CheckAPICallSuccess(ctx, errorContext, resp, apiErr); err != nil {
137+
err = checkResponse(ctx, apiErr, resp, result, errorContext)
138+
if err != nil {
108139
return
109140
}
110141

@@ -113,10 +144,5 @@ func GenericCallAndCheckSuccess[T any](ctx context.Context, errorContext string,
113144
return
114145
}
115146

116-
if reflection.IsEmpty(result) {
117-
err = commonerrors.New(commonerrors.ErrMarshalling, "unmarshalled response is empty")
118-
return
119-
}
120-
121147
return
122148
}

‎utils/api/api_test.go‎

Lines changed: 70 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,20 @@ package api
88
import (
99
"bytes"
1010
"context"
11+
"encoding/json"
1112
"errors"
1213
"io"
1314
_http "net/http"
1415
"testing"
1516

1617
"github.com/go-faker/faker/v4"
1718
"github.com/stretchr/testify/assert"
19+
"github.com/stretchr/testify/require"
1820

21+
"github.com/ARM-software/embedded-development-services-client/client"
1922
"github.com/ARM-software/golang-utils/utils/commonerrors"
2023
"github.com/ARM-software/golang-utils/utils/commonerrors/errortest"
24+
"github.com/ARM-software/golang-utils/utils/field"
2125
)
2226

2327
func TestIsAPICallSuccessful(t *testing.T) {
@@ -104,6 +108,66 @@ func TestCallAndCheckSuccess(t *testing.T) {
104108
assert.Equal(t, actualErr.Error(), expectedErr)
105109
})
106110

111+
t.Run("api call successful, marshalling failed due to missing required field in response", func(t *testing.T) {
112+
expectedErrorMessage := client.ErrorResponse{
113+
Fields: []client.FieldObject{{
114+
FieldName: faker.Name(),
115+
FieldPath: field.ToOptionalString(faker.Name()),
116+
Message: faker.Sentence(),
117+
}},
118+
HttpStatusCode: 200,
119+
Message: faker.Sentence(),
120+
RequestId: faker.UUIDDigit(),
121+
}
122+
response, err := expectedErrorMessage.ToMap()
123+
require.NoError(t, err)
124+
delete(response, "message")
125+
126+
reducedResponse, err := json.Marshal(response)
127+
require.NoError(t, err)
128+
129+
errorResponse := client.ErrorResponse{}
130+
errM := errorResponse.UnmarshalJSON(reducedResponse)
131+
require.Error(t, errM)
132+
133+
parentCtx := context.Background()
134+
_, err = CallAndCheckSuccess[client.ErrorResponse](parentCtx, "test",
135+
func(ctx context.Context) (*client.ErrorResponse, *_http.Response, error) {
136+
return &errorResponse, &_http.Response{StatusCode: 200, Body: io.NopCloser(bytes.NewReader(reducedResponse))}, errM
137+
})
138+
require.Error(t, err)
139+
errortest.AssertError(t, err, commonerrors.ErrMarshalling)
140+
})
141+
142+
t.Run("api call successful, strict marshalling failed but recovery", func(t *testing.T) {
143+
expectedErrorMessage := client.ErrorResponse{
144+
Fields: []client.FieldObject{{
145+
FieldName: faker.Name(),
146+
FieldPath: field.ToOptionalString(faker.Name()),
147+
Message: faker.Sentence(),
148+
}},
149+
HttpStatusCode: 200,
150+
Message: faker.Sentence(),
151+
RequestId: faker.UUIDDigit(),
152+
}
153+
response, err := expectedErrorMessage.ToMap()
154+
require.NoError(t, err)
155+
response[faker.Word()] = faker.Name()
156+
response[faker.Word()] = faker.Sentence()
157+
response[faker.Word()] = faker.Paragraph()
158+
response[faker.Word()] = faker.UUIDDigit()
159+
extendedResponse, err := json.Marshal(response)
160+
require.NoError(t, err)
161+
errMessage := "no error"
162+
parentCtx := context.Background()
163+
result, err := CallAndCheckSuccess[client.ErrorResponse](parentCtx, errMessage,
164+
func(ctx context.Context) (*client.ErrorResponse, *_http.Response, error) {
165+
return &client.ErrorResponse{}, &_http.Response{StatusCode: 200, Body: io.NopCloser(bytes.NewReader(extendedResponse))}, errors.New(errMessage)
166+
})
167+
require.NoError(t, err)
168+
assert.Equal(t, expectedErrorMessage, *result)
169+
})
170+
107171
t.Run("api call successful, empty response", func(t *testing.T) {
108172
errMessage := "no error"
109173
parentCtx := context.Background()
@@ -152,7 +216,7 @@ func TestGenericCallAndCheckSuccess(t *testing.T) {
152216
assert.Equal(t, actualErr.Error(), expectedErr)
153217
})
154218

155-
t.Run("no context error, api call successful", func(t *testing.T) {
219+
t.Run("api call successful but error marshalling", func(t *testing.T) {
156220
errMessage := "no error"
157221
parentCtx := context.Background()
158222
_, err := GenericCallAndCheckSuccess(parentCtx, errMessage,
@@ -164,7 +228,8 @@ func TestGenericCallAndCheckSuccess(t *testing.T) {
164228
}
165229
return &tmp, &_http.Response{StatusCode: 200}, errors.New(errMessage)
166230
})
167-
assert.NoError(t, err)
231+
require.Error(t, err)
232+
errortest.AssertError(t, err, commonerrors.ErrMarshalling)
168233
})
169234

170235
t.Run("api call successful, empty response", func(t *testing.T) {
@@ -178,11 +243,10 @@ func TestGenericCallAndCheckSuccess(t *testing.T) {
178243
})
179244

180245
t.Run("api call successful, incorrect response", func(t *testing.T) {
181-
errMessage := "response error"
182246
parentCtx := context.Background()
183-
_, err := GenericCallAndCheckSuccess(parentCtx, errMessage,
184-
func(ctx context.Context) (struct{}, *_http.Response, error) {
185-
return struct{}{}, &_http.Response{StatusCode: 200}, errors.New(errMessage)
247+
_, err := GenericCallAndCheckSuccess(parentCtx, "response error",
248+
func(ctx context.Context) (struct{ Blah string }, *_http.Response, error) {
249+
return struct{ Blah string }{Blah: "fsadsfs"}, &_http.Response{StatusCode: 200}, nil
186250
})
187251
errortest.AssertError(t, err, commonerrors.ErrConflict)
188252
})

‎utils/go.mod‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ require (
77
github.com/ARM-software/golang-utils/utils v1.101.0
88
github.com/go-faker/faker/v4 v4.6.1
99
github.com/go-logr/logr v1.4.3
10+
github.com/perimeterx/marshmallow v1.1.5
1011
github.com/stretchr/testify v1.10.0
1112
go.uber.org/atomic v1.11.0
1213
go.uber.org/goleak v1.3.0
@@ -42,7 +43,9 @@ require (
4243
github.com/hashicorp/go-retryablehttp v0.7.7 // indirect
4344
github.com/iamacarpet/go-win64api v0.0.0-20240507095429-873e84e85847 // indirect
4445
github.com/joho/godotenv v1.5.1 // indirect
46+
github.com/josharian/intern v1.0.0 // indirect
4547
github.com/lufia/plan9stats v0.0.0-20240909124753-873cd0166683 // indirect
48+
github.com/mailru/easyjson v0.7.7 // indirect
4649
github.com/mattn/go-colorable v0.1.13 // indirect
4750
github.com/mattn/go-isatty v0.0.20 // indirect
4851
github.com/mitchellh/go-homedir v1.1.0 // indirect

‎utils/go.sum‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ github.com/go-ole/go-ole v1.3.0 h1:Dt6ye7+vXGIKZ7Xtk4s6/xVdGDQynvom7xCFEdWr6uE=
5959
github.com/go-ole/go-ole v1.3.0/go.mod h1:5LS6F96DhAwUc7C+1HLexzMXY1xGRSryjyPPKW6zv78=
6060
github.com/go-ozzo/ozzo-validation/v4 v4.3.0 h1:byhDUpfEwjsVQb1vBunvIjh2BHQ9ead57VkAEY4V+Es=
6161
github.com/go-ozzo/ozzo-validation/v4 v4.3.0/go.mod h1:2NKgrcHl3z6cJs+3Oo940FPRiTzuqKbvfrL2RxCj6Ew=
62+
github.com/go-test/deep v1.0.8 h1:TDsG77qcSprGbC6vTN8OuXp5g+J+b5Pcguhf7Zt61VM=
63+
github.com/go-test/deep v1.0.8/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE=
6264
github.com/go-viper/mapstructure/v2 v2.2.1 h1:ZAaOCxANMuZx5RCeg0mBdEZk7DZasvvZIxtHqx8aGss=
6365
github.com/go-viper/mapstructure/v2 v2.2.1/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM=
6466
github.com/godbus/dbus v4.1.0+incompatible/go.mod h1:/YcGZj5zSblfDWMMoOzV4fas9FZnQYTkDnsGvmh2Grw=
@@ -109,12 +111,16 @@ github.com/iamacarpet/go-win64api v0.0.0-20240507095429-873e84e85847 h1:cRHZFGwI
109111
github.com/iamacarpet/go-win64api v0.0.0-20240507095429-873e84e85847/go.mod h1:B7zFQPAznj+ujXel5X+LUoK3LgY6VboCdVYHZNn7gpg=
110112
github.com/joho/godotenv v1.5.1 h1:7eLL/+HRGLY0ldzfGMeQkb7vMd0as4CfYvUVzLqw0N0=
111113
github.com/joho/godotenv v1.5.1/go.mod h1:f4LDr5Voq0i2e/R5DDNOoa2zzDfwtkZa6DnEwAbqwq4=
114+
github.com/josharian/intern v1.0.0 h1:vlS4z54oSdjm0bgjRigI+G1HpF+tI+9rE5LLzOg8HmY=
115+
github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y=
112116
github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
113117
github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
114118
github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
115119
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
116120
github.com/lufia/plan9stats v0.0.0-20240909124753-873cd0166683 h1:7UMa6KCCMjZEMDtTVdcGu0B1GmmC7QJKiCCjyTAWQy0=
117121
github.com/lufia/plan9stats v0.0.0-20240909124753-873cd0166683/go.mod h1:ilwx/Dta8jXAgpFYFvSWEMwxmbWXyiUHkd5FwyKhb5k=
122+
github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0=
123+
github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
118124
github.com/mattn/go-colorable v0.1.9/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
119125
github.com/mattn/go-colorable v0.1.12/go.mod h1:u5H1YNBxpqRaxsYJYSkiCWKzEfiAb1Gb520KVy5xxl4=
120126
github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA=
@@ -136,6 +142,8 @@ github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7J
136142
github.com/onsi/gomega v1.10.2/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo=
137143
github.com/pelletier/go-toml/v2 v2.2.3 h1:YmeHyLY8mFWbdkNWwpr+qIL2bEqT0o95WSdkNHvL12M=
138144
github.com/pelletier/go-toml/v2 v2.2.3/go.mod h1:MfCQTFTvCcUyyvvwm1+G6H/jORL20Xlb6rzQu9GuUkc=
145+
github.com/perimeterx/marshmallow v1.1.5 h1:a2LALqQ1BlHM8PZblsDdidgv1mWi1DgC2UmX50IvK2s=
146+
github.com/perimeterx/marshmallow v1.1.5/go.mod h1:dsXbUu8CRzfYP5a87xpp0xq9S3u0Vchtcl8we9tYaXw=
139147
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
140148
github.com/petermattis/goid v0.0.0-20250319124200-ccd6737f222a h1:S+AGcmAESQ0pXCUNnRH7V+bOUIgkSX5qVt2cNKCrm0Q=
141149
github.com/petermattis/goid v0.0.0-20250319124200-ccd6737f222a/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
@@ -190,6 +198,8 @@ github.com/tklauser/go-sysconf v0.3.14 h1:g5vzr9iPFFz24v2KZXs/pvpvh8/V9Fw6vQK5ZZ
190198
github.com/tklauser/go-sysconf v0.3.14/go.mod h1:1ym4lWMLUOhuBOPGtRcJm7tEGX4SCYNEEEtghGG/8uY=
191199
github.com/tklauser/numcpus v0.9.0 h1:lmyCHtANi8aRUgkckBgoDk1nHCux3n2cgkJLXdQGPDo=
192200
github.com/tklauser/numcpus v0.9.0/go.mod h1:SN6Nq1O3VychhC1npsWostA+oW+VOQTxZrS604NSRyI=
201+
github.com/ugorji/go/codec v1.2.7 h1:YPXUKf7fYbp/y8xloBqZOw2qaVggbfwMlI8WM3wZUJ0=
202+
github.com/ugorji/go/codec v1.2.7/go.mod h1:WGN1fab3R1fzQlVQTkfxVtIBhWDRqOviHU95kRgeqEY=
193203
github.com/yusufpapurcu/wmi v1.2.4 h1:zFUKzehAFReQwLys1b/iSMl+JQGSCSjtVqQn9bBrPo0=
194204
github.com/yusufpapurcu/wmi v1.2.4/go.mod h1:SBZ9tNy3G9/m5Oi98Zks0QjeHVDvuK0qfxQmPyzfmi0=
195205
github.com/zailic/slogr v0.0.2-alpha h1:ZZ+96+AOnk4L9JoPkZ6aGbGXnn90/53A9zm9JcjYSYc=

0 commit comments

Comments
 (0)