[BBO-290] Improve error handling of mutations#4
Conversation
pedrocunial
left a comment
pedrocunial
left a comment
There was a problem hiding this comment.
As always, looks great! Left a suggestion that we might want to act on a later PR/fix! Gratz!
| errors := g.errors | ||
|
|
||
| if len(errors) > 0 { | ||
| return errors[len(errors)-1].ErrCode() |
There was a problem hiding this comment.
Are graphql codes different than http codes status code? If so, the return of this function may be of """""""varying types""""""", which would reduce the interchangeability (is this even a word?) between our error types by their handlers.
My suggestion: Create a "code type" specific to our lib (it could be the graphql codes, http codes or even a new one) so that our callers can use it without needing to relly on some sort of reflection to check the error type/struct.
PS: This doesn't need to be done in this PR, it could very well be done in another one since it would be a very clearly breaking change.
There was a problem hiding this comment.
graphql errors are way more specific than normal http errors, some can be mapped to http errors, like 404, 500, etc but others are specific to the request (kyc returns bad_image when documents fail) I chose to leave this generic to enable this freedom that graphql gives.
we could implement the mapping of the most common fields inside our error utils or the client that implement this project, wdyt?
| var gr *graphResponse | ||
| switch op.(type) { | ||
| case *Mutation: | ||
| var results struct { | ||
| Data map[string]graphMutationPayload | ||
| } | ||
|
|
||
| if err := json.NewDecoder(&buf).Decode(&results); err != nil { | ||
| return NewExecutionError(errors.Wrap(err, "decoding response")) | ||
| } | ||
| gr = &graphResponse{} | ||
|
|
||
| for _, result := range results.Data { | ||
| if !result.Successful { | ||
| messages := result.Messages | ||
| errors := make([]graphErr, len(messages)) | ||
|
|
||
| for i, message := range messages { | ||
| errors[i] = graphErr{ | ||
| Message: emptyOrString(message.Message), | ||
| Code: message.Code, | ||
| } | ||
| } | ||
|
|
||
| gr.Errors = append(gr.Errors, errors...) | ||
| } else { | ||
| err := mapstructure.Decode(results.Data, &resp) | ||
| if err != nil { | ||
| return NewExecutionError(errors.Wrap(err, "decoding response")) | ||
| } | ||
| } | ||
| // The code above only supports payloads with a single mutation | ||
| break | ||
| } | ||
|
|
||
| default: | ||
| gr = &graphResponse{Data: resp} | ||
| if err := json.NewDecoder(&buf).Decode(&gr); err != nil { | ||
| return NewExecutionError(errors.Wrap(err, "decoding response")) | ||
| } |
There was a problem hiding this comment.
Suggestion: You could remove this usage of reflection by moving the decoding logic into a Decode method of each of your possible interfaces
There was a problem hiding this comment.
More specifically, the Decode implementation for the Mutation type would be the first case of this switch, and the default case would be the Request type implementation
There was a problem hiding this comment.
I tried setting up this way but I was faced with the same issue, when decoding resp we need to pass the structure where the data must be decoded, which at compile time would be of the type interface {}, because of this, I couldn't access the attributes of a concrete struct for example results.Data. Similarly, in test we use map[string]interface{} but we could also use a struct, in one of the test examples, and afaik there is no way of doing a direct conversion which means we needed to do some sort of reflection anyways. I would have ended up implementing something very similar to what this lib delivers.
The thing that I'm not happy about is that we go through the json twice, first to decode it as a mutationPayload and a second time to populate the resp =\
2 participants
Description
This PR modifies the
Runmethod to handle errors in mutation requests, previously the mutations were always successful because theErrorsfield is in a different place inside the mutation responses.Changes
NewMutationto handle mutation operationsNotes
structtypes for the responses, making this compatible with the way we build the responses in BFF