-
Notifications
You must be signed in to change notification settings - Fork 0
🐛 Fix response unmarshalling issue when API has unknown fields #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| :bug: Fix response unmarshalling issue when API has unknown fields |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,29 +9,34 @@ package api | |
| import ( | ||
| "context" | ||
| "fmt" | ||
| _http "net/http" | ||
| "net/http" | ||
| "reflect" | ||
| "strings" | ||
|
|
||
| "github.com/perimeterx/marshmallow" | ||
|
|
||
| "github.com/ARM-software/embedded-development-services-client-utils/utils/errors" | ||
| "github.com/ARM-software/golang-utils/utils/commonerrors" | ||
| "github.com/ARM-software/golang-utils/utils/parallelisation" | ||
| "github.com/ARM-software/golang-utils/utils/reflection" | ||
| "github.com/ARM-software/golang-utils/utils/safeio" | ||
| ) | ||
|
|
||
| const requiredFieldError = "no value given for required property" | ||
|
|
||
| // IsCallSuccessful determines whether an API response is successful or not | ||
| func IsCallSuccessful(r *_http.Response) bool { | ||
| func IsCallSuccessful(r *http.Response) bool { | ||
| if r == nil { | ||
| return false | ||
| } | ||
| return r.StatusCode >= _http.StatusOK && r.StatusCode < _http.StatusMultipleChoices | ||
| return r.StatusCode >= http.StatusOK && r.StatusCode < http.StatusMultipleChoices | ||
| } | ||
|
|
||
| // CheckAPICallSuccess verifies whether an API response is successful or not and if not, populates an error with all the information needed. | ||
| // errorContext corresponds to the description of what led to the error if error there is e.g. `Failed adding a user`. | ||
| // resp corresponds to the HTTP response from a certain endpoint. The body of such response is not closed by this function. | ||
| // apiErr corresponds to the error which may be returned by the HTTP client when calling the endpoint. | ||
| func CheckAPICallSuccess(ctx context.Context, errorContext string, resp *_http.Response, apiErr error) (err error) { | ||
| func CheckAPICallSuccess(ctx context.Context, errorContext string, resp *http.Response, apiErr error) (err error) { | ||
| err = parallelisation.DetermineContextError(ctx) | ||
| if err != nil { | ||
| return | ||
|
|
@@ -63,22 +68,27 @@ func CheckAPICallSuccess(ctx context.Context, errorContext string, resp *_http.R | |
| // CallAndCheckSuccess is a wrapper for making an API call and then checking success with `CheckAPICallSuccess` | ||
| // errorContext corresponds to the description of what led to the error if error there is e.g. `Failed adding a user`. | ||
| // apiCallFunc corresponds to a generic function that will be called to make the API call | ||
| func CallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCallFunc func(ctx context.Context) (*T, *_http.Response, error)) (result *T, err error) { | ||
| func CallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCallFunc func(ctx context.Context) (*T, *http.Response, error)) (result *T, err error) { | ||
| if err = parallelisation.DetermineContextError(ctx); err != nil { | ||
| return | ||
| } | ||
|
|
||
| result, resp, apiErr := apiCallFunc(ctx) | ||
| if resp != nil && resp.Body != nil { | ||
| _ = resp.Body.Close() | ||
| defer func() { | ||
| if resp != nil && resp.Body != nil { | ||
| _ = resp.Body.Close() | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| if err = CheckAPICallSuccess(ctx, errorContext, resp, apiErr); err != nil { | ||
| err = CheckAPICallSuccess(ctx, errorContext, resp, apiErr) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| if apiErr != nil { | ||
| err = commonerrors.WrapError(commonerrors.ErrMarshalling, apiErr, "API call was successful but an error occurred during response marshalling") | ||
| err = checkResponseMarshalling(ctx, apiErr, resp, result) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -90,11 +100,36 @@ func CallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCal | |
| return | ||
| } | ||
|
|
||
| func checkResponseMarshalling(ctx context.Context, apiErr error, resp *http.Response, result any) (err error) { | ||
| if apiErr != nil { | ||
| err = commonerrors.WrapError(commonerrors.ErrMarshalling, apiErr, "API call was successful but an error occurred during response marshalling") | ||
| if commonerrors.CorrespondTo(apiErr, requiredFieldError) { | ||
| return | ||
| } | ||
| if resp == nil || resp.Body == nil { | ||
| return | ||
| } | ||
| // At this point, the marshalling problem may be due to the present of unknown fields in the response due to an API extension. | ||
| // See https://github.com/OpenAPITools/openapi-generator/issues/21446 | ||
| var respB []byte | ||
| respB, err = safeio.ReadAll(ctx, resp.Body) | ||
| if err != nil { | ||
| return | ||
| } | ||
| _, err = marshmallow.Unmarshal(respB, result, marshmallow.WithSkipPopulateStruct(false), marshmallow.WithExcludeKnownFieldsFromMap(true)) | ||
| if err != nil { | ||
| err = commonerrors.WrapError(commonerrors.ErrMarshalling, err, "API call was successful but an error occurred during response marshalling") | ||
| return | ||
| } | ||
| } | ||
| return | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you do this I think the function needs to have a different name since it isn't just checking, it is also doing another unmarshal attempt
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair enough |
||
| } | ||
|
|
||
| // GenericCallAndCheckSuccess is similar to CallAndCheckSuccess but for function returning interfaces rather than concrete types. | ||
| // T must be an interface. | ||
| // errorContext corresponds to the description of what led to the error if error there is e.g. `Failed adding a user`. | ||
| // apiCallFunc corresponds to a generic function that will be called to make the API call | ||
| func GenericCallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCallFunc func(ctx context.Context) (T, *_http.Response, error)) (result T, err error) { | ||
| func GenericCallAndCheckSuccess[T any](ctx context.Context, errorContext string, apiCallFunc func(ctx context.Context) (T, *http.Response, error)) (result T, err error) { | ||
| if err = parallelisation.DetermineContextError(ctx); err != nil { | ||
| return | ||
| } | ||
|
|
@@ -104,7 +139,13 @@ func GenericCallAndCheckSuccess[T any](ctx context.Context, errorContext string, | |
| _ = resp.Body.Close() | ||
| } | ||
|
|
||
| if err = CheckAPICallSuccess(ctx, errorContext, resp, apiErr); err != nil { | ||
| err = CheckAPICallSuccess(ctx, errorContext, resp, apiErr) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| err = checkResponseMarshalling(ctx, apiErr, resp, result) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is going to be its own function then you should check the statuscode too, before it was after something that did that so we knew it would be successful but here we don't and it could cause confusion if someone uses the function elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a private function though