-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: Make GraphQL errors spec compliant #3040
fix: Make GraphQL errors spec compliant #3040
Conversation
f112cfa
to
2497113
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3040 +/- ##
===========================================
- Coverage 79.49% 79.47% -0.02%
===========================================
Files 336 336
Lines 26075 26074 -1
===========================================
- Hits 20727 20721 -6
- Misses 3878 3881 +3
- Partials 1470 1472 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
LGTM
@@ -290,14 +296,33 @@ type GQLResult struct { | |||
// | |||
// If there are values in this slice the request will likely not have run to completion | |||
// and [Data] will be nil. | |||
Errors []error `json:"errors,omitempty"` | |||
Errors []GQLError `json:"errors,omitempty"` | |||
|
|||
// Data contains the resultant data produced by the GQL request. | |||
// | |||
// It will be nil if any errors were raised during execution. | |||
Data any `json:"data"` | |||
} |
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.
suggestion: If you add a Marshal/Unmarshal func for GQLResult
you could probably drop the new types and functions. It might also be nicer for users of the Go client.
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.
I would just add a custom Marshal/Unmarshal func for GQLError
instead .
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.
I would just add a custom Marshal/Unmarshal func for GQLError instead .
I think the custom marshalling only works if its on the GQLResult and we want to keep errors as the generic error
type.
I've refactored it to use a custom marshaller and added some tests to make sure our http server returns proper responses.
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.
LGTM. The Marshal/Unmarshal change is nice.
f420c8d
to
2535659
Compare
Relevant issue(s)
Resolves #3039
Resolves #3041
Description
This PR fixes an issue with response errors not being spec compliant.
It also adds a
GQLError
type to the client package.Tasks
How has this been tested?
Added tests here https://github.com/sourcenetwork/defradb-third-party-test
Specify the platform(s) on which this was tested: