Generalize REST error parsing #1494
Conversation
pkg/cmd/api/api.go
Outdated
| } | ||
| type errorResponse struct { | ||
| Message string | ||
| Errors []errorMessage |
There was a problem hiding this comment.
When unmarshaling a field whose value is unknown at compile time (here, we don't know if the type is []errorMessage or []string), it's best to use []json.RawMessage as the type, and then check whether its 1st byte is a { or a " and with that decide whether you want to decode it to an errorMessage type or a string.
The goal would be to avoid unmarshalling into one struct and then catching an error and unmarshalling into another struct, since that approach is brittle and doesn't give us a lot of flexibility.
There was a problem hiding this comment.
Totally agree with you @mislav , your approach is way more flexible. I will work on a solution.
pkg/cmd/api/api_test.go
Outdated
| Header: http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}, | ||
| }, | ||
| err: cmdutil.SilentError, | ||
| stdout: `{"errors": ["AGAIN", "FINE"]}`, |
There was a problem hiding this comment.
GraphQL error messages are always guaranteed to be in the {"message": "..."} format. If you look closely at the original issue you were aiming to fix, the error response in question is from a REST API, not a GraphQL one.
So I think your direction to fix it is in general good, it's just that this test (and the PR description) needs to be reworded to indicate that it's covering a REST response!
mmontes11
changed the title
|
changes done @mislav, check them out and let me know if they fit your approach 😃 |
mmontes11
force-pushed
the
generalize-gql-error-parsing
branch
from
8feb7b0
to
6522984
Compare
pkg/cmd/api/api.go
Outdated
| if err != nil { | ||
| return r, "", err | ||
| } | ||
| objectErrors = append(objectErrors, objectError) |
There was a problem hiding this comment.
Tip: why keep a separate slice of objectErrors when you can just append objectError.Message to stringErrors and simplify the whole logic
There was a problem hiding this comment.
Less code and unified error handling like in my first approach 👍🏻
pkg/cmd/api/api_test.go
Outdated
| @@ -265,7 +265,7 @@ func Test_apiRun(t *testing.T) { | |||
| stderr: "gh: THIS IS FINE (HTTP 400)\n", | |||
| }, | |||
| { | |||
| name: "GraphQL error", | |||
| name: "REST object error", | |||
There was a problem hiding this comment.
This is a test for a GraphQL error response. Note the request path is graphql. I don't think this test case needed renaming
There was a problem hiding this comment.
Ok, I will restore the original name.
pkg/cmd/api/api_test.go
Outdated
| options: ApiOptions{ | ||
| RequestPath: "graphql", |
There was a problem hiding this comment.
You can omit ApiOptions with RequestPath entirely here; it's only necessary for testing GraphQL responses
There was a problem hiding this comment.
Ok, in that case I will specify an error code 400 since it is a REST operation.
mmontes11
force-pushed
the
generalize-gql-error-parsing
branch
from
6522984
to
0b395e0
Compare
|
changes pushed @mislav, thank you for your detailed feedback! |
mmontes11
force-pushed
the
generalize-gql-error-parsing
branch
from
0b395e0
to
eaa9f50
Compare
mmontes11
force-pushed
the
generalize-gql-error-parsing
branch
from
eaa9f50
to
27765f9
Compare
Fixes #1474
Changes: