Skip to content
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

Merged
merged 8 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions cli/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,9 @@ To learn more about the DefraDB GraphQL Query Language, refer to https://docs.so
store := mustGetContextStore(cmd)
result := store.ExecRequest(cmd.Context(), request, options...)

var errors []string
for _, err := range result.GQL.Errors {
errors = append(errors, err.Error())
}
if result.Subscription == nil {
cmd.Print(REQ_RESULTS_HEADER)
return writeJSON(cmd, map[string]any{"data": result.GQL.Data, "errors": errors})
return writeJSON(cmd, result.GQL)
}
cmd.Print(SUB_RESULTS_HEADER)
for item := range result.Subscription {
Expand Down
27 changes: 26 additions & 1 deletion client/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,12 @@ func WithVariables(variables map[string]any) RequestOption {
}
}

// GQLError represents an error that was encountered during a GQL request.
type GQLError struct {
// Message contains a description of the error.
Message string `json:"message"`
}

// GQLResult represents the immediate results of a GQL request.
//
// It does not handle subscription channels. This object and its children are json serializable.
Expand All @@ -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"`
}
Copy link
Contributor

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.

Copy link
Collaborator

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 .

Copy link
Member Author

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.


// AddErrors appends a new GQLError to the result.
func (res *GQLResult) AddErrors(errors ...error) {
for _, e := range errors {
res.Errors = append(res.Errors, GQLError{Message: e.Error()})
}
}

// GetErrors parses the list of GQLErrors into go errors.
//
// Any errors that match client specific errors will be revived
// into their respective types.
func (res *GQLResult) GetErrors() []error {
errors := make([]error, len(res.Errors))
for i, e := range res.Errors {
errors[i] = ReviveError(e.Message)
}
return errors
}

// RequestResult represents the results of a GQL request.
type RequestResult struct {
// GQL contains the immediate results of the GQL request.
Expand Down
15 changes: 15 additions & 0 deletions client/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package client
import (
"fmt"

"github.com/sourcenetwork/defradb/datastore"
"github.com/sourcenetwork/defradb/errors"
)

Expand Down Expand Up @@ -174,3 +175,17 @@ func NewErrFailedToParseKind(kind []byte) error {
errors.NewKV("Kind", kind),
)
}

// ReviveError attempts to return a client specific error from
// the given message. If no matching error is found the message
// is wrapped in a new anonymous error type.
func ReviveError(message string) error {
switch message {
case ErrDocumentNotFoundOrNotAuthorized.Error():
return ErrDocumentNotFoundOrNotAuthorized
case datastore.ErrTxnConflict.Error():
return datastore.ErrTxnConflict
default:
return fmt.Errorf("%s", message)
}
}
9 changes: 8 additions & 1 deletion docs/website/references/http/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,14 @@
"properties": {
"data": {},
"errors": {
"items": {},
"items": {
"properties": {
"message": {
"type": "string"
}
},
"type": "object"
},
"type": "array"
}
},
Expand Down
28 changes: 11 additions & 17 deletions http/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,27 +383,27 @@

body, err := json.Marshal(gqlRequest)
if err != nil {
result.GQL.Errors = []error{err}
result.GQL.AddErrors(err)

Check warning on line 386 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L386

Added line #L386 was not covered by tests
return result
}

req, err := http.NewRequestWithContext(ctx, http.MethodPost, methodURL.String(), bytes.NewBuffer(body))
if err != nil {
result.GQL.Errors = []error{err}
result.GQL.AddErrors(err)

Check warning on line 392 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L392

Added line #L392 was not covered by tests
return result
}
err = c.http.setDefaultHeaders(req)

setDocEncryptionFlagIfNeeded(ctx, req)

if err != nil {
result.GQL.Errors = []error{err}
result.GQL.AddErrors(err)

Check warning on line 400 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L400

Added line #L400 was not covered by tests
return result
}

res, err := c.http.client.Do(req)
if err != nil {
result.GQL.Errors = []error{err}
result.GQL.AddErrors(err)

Check warning on line 406 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L406

Added line #L406 was not covered by tests
return result
}
if res.Header.Get("Content-Type") == "text/event-stream" {
Expand All @@ -417,16 +417,13 @@

data, err := io.ReadAll(res.Body)
if err != nil {
result.GQL.Errors = []error{err}
result.GQL.AddErrors(err)

Check warning on line 420 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L420

Added line #L420 was not covered by tests
return result
}
var response GraphQLResponse
if err = json.Unmarshal(data, &response); err != nil {
result.GQL.Errors = []error{err}
if err = json.Unmarshal(data, &result.GQL); err != nil {
result.GQL.AddErrors(err)

Check warning on line 424 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L424

Added line #L424 was not covered by tests
return result
}
result.GQL.Data = response.Data
result.GQL.Errors = response.Errors
return result
}

Expand All @@ -447,14 +444,11 @@
if err != nil {
return
}
var response GraphQLResponse
if err := json.Unmarshal(evt.Data, &response); err != nil {
return
}
resCh <- client.GQLResult{
Errors: response.Errors,
Data: response.Data,
var res client.GQLResult
if err := json.Unmarshal(evt.Data, &res); err != nil {
res.AddErrors(err)

Check warning on line 449 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L447-L449

Added lines #L447 - L449 were not covered by tests
}
resCh <- res

Check warning on line 451 in http/client.go

View check run for this annotation

Codecov / codecov/patch

http/client.go#L451

Added line #L451 was not covered by tests
}
}()

Expand Down
8 changes: 7 additions & 1 deletion http/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@

import (
"encoding/json"
"fmt"

"github.com/sourcenetwork/defradb/client"
"github.com/sourcenetwork/defradb/errors"
)

Expand Down Expand Up @@ -53,7 +55,11 @@
if err := json.Unmarshal(data, &out); err != nil {
return err
}
e.Error = parseError(out["error"])
if msg, ok := out["error"].(string); ok {
e.Error = client.ReviveError(msg)
} else {
e.Error = fmt.Errorf("%s", out)

Check warning on line 61 in http/errors.go

View check run for this annotation

Codecov / codecov/patch

http/errors.go#L61

Added line #L61 was not covered by tests
}
return nil
}

Expand Down
40 changes: 1 addition & 39 deletions http/handler_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
package http

import (
"bytes"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -279,43 +278,6 @@ type GraphQLRequest struct {
Variables map[string]any `json:"variables"`
}

type GraphQLResponse struct {
Data any `json:"data"`
Errors []error `json:"errors,omitempty"`
}

func (res GraphQLResponse) MarshalJSON() ([]byte, error) {
var errors []string
for _, err := range res.Errors {
errors = append(errors, err.Error())
}
return json.Marshal(map[string]any{"data": res.Data, "errors": errors})
}

func (res *GraphQLResponse) UnmarshalJSON(data []byte) error {
// decode numbers to json.Number
dec := json.NewDecoder(bytes.NewBuffer(data))
dec.UseNumber()

var out map[string]any
if err := dec.Decode(&out); err != nil {
return err
}
res.Data = out["data"]

// fix errors type to match tests
switch t := out["errors"].(type) {
case []any:
for _, v := range t {
res.Errors = append(res.Errors, parseError(v))
}
default:
res.Errors = nil
}

return nil
}

func (s *storeHandler) ExecRequest(rw http.ResponseWriter, req *http.Request) {
store := req.Context().Value(dbContextKey).(client.Store)

Expand Down Expand Up @@ -343,7 +305,7 @@ func (s *storeHandler) ExecRequest(rw http.ResponseWriter, req *http.Request) {
result := store.ExecRequest(req.Context(), request.Query, options...)

if result.Subscription == nil {
responseJSON(rw, http.StatusOK, GraphQLResponse{result.GQL.Data, result.GQL.Errors})
responseJSON(rw, http.StatusOK, result.GQL)
return
}
flusher, ok := rw.(http.Flusher)
Expand Down
2 changes: 1 addition & 1 deletion http/openapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ var openApiSchemas = map[string]any{
"collection_delete": &CollectionDeleteRequest{},
"peer_info": &peer.AddrInfo{},
"graphql_request": &GraphQLRequest{},
"graphql_response": &GraphQLResponse{},
"graphql_response": &client.GQLResult{},
"backup_config": &client.BackupConfig{},
"collection": &client.CollectionDescription{},
"schema": &client.SchemaDescription{},
Expand Down
15 changes: 0 additions & 15 deletions http/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,8 @@ package http

import (
"encoding/json"
"fmt"
"io"
"net/http"

"github.com/sourcenetwork/defradb/client"
"github.com/sourcenetwork/defradb/datastore"
)

func requestJSON(req *http.Request, out any) error {
Expand All @@ -39,14 +35,3 @@ func responseJSON(rw http.ResponseWriter, status int, data any) {
log.ErrorE("failed to write response", err)
}
}

func parseError(msg any) error {
switch msg {
case client.ErrDocumentNotFoundOrNotAuthorized.Error():
return client.ErrDocumentNotFoundOrNotAuthorized
case datastore.ErrTxnConflict.Error():
return datastore.ErrTxnConflict
default:
return fmt.Errorf("%s", msg)
}
}
8 changes: 4 additions & 4 deletions internal/db/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
res := &client.RequestResult{}
ast, err := db.parser.BuildRequestAST(request)
if err != nil {
res.GQL.Errors = []error{err}
res.GQL.AddErrors(err)
return res
}
if db.parser.IsIntrospection(ast) {
Expand All @@ -31,13 +31,13 @@

parsedRequest, errors := db.parser.Parse(ast, options)
if len(errors) > 0 {
res.GQL.Errors = errors
res.GQL.AddErrors(errors...)
return res
}

pub, err := db.handleSubscription(ctx, parsedRequest)
if err != nil {
res.GQL.Errors = []error{err}
res.GQL.AddErrors(err)

Check warning on line 40 in internal/db/request.go

View check run for this annotation

Codecov / codecov/patch

internal/db/request.go#L40

Added line #L40 was not covered by tests
return res
}

Expand All @@ -52,7 +52,7 @@

results, err := planner.RunRequest(ctx, parsedRequest)
if err != nil {
res.GQL.Errors = []error{err}
res.GQL.AddErrors(err)
}
res.GQL.Data = results
return res
Expand Down
4 changes: 2 additions & 2 deletions internal/db/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
ctx, txn, err := ensureContextTxn(ctx, db, false)
if err != nil {
res := &client.RequestResult{}
res.GQL.Errors = []error{err}
res.GQL.AddErrors(err)

Check warning on line 28 in internal/db/store.go

View check run for this annotation

Codecov / codecov/patch

internal/db/store.go#L28

Added line #L28 was not covered by tests
return res
}
defer txn.Discard(ctx)
Expand All @@ -41,7 +41,7 @@
}

if err := txn.Commit(ctx); err != nil {
res.GQL.Errors = []error{err}
res.GQL.AddErrors(err)
return res
}

Expand Down
2 changes: 1 addition & 1 deletion internal/db/subscriptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func (db *db) handleSubscription(ctx context.Context, r *request.Request) (<-cha
}
res := client.GQLResult{}
if err != nil {
res.Errors = []error{err}
res.AddErrors(err)
}
res.Data = result

Expand Down
7 changes: 3 additions & 4 deletions internal/request/graphql/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,12 @@

res := &client.RequestResult{
GQL: client.GQLResult{
Data: r.Data,
Errors: make([]error, len(r.Errors)),
Data: r.Data,
},
}

for i, err := range r.Errors {
res.GQL.Errors[i] = err
for _, err := range r.Errors {
res.GQL.AddErrors(err)

Check warning on line 80 in internal/request/graphql/parser.go

View check run for this annotation

Codecov / codecov/patch

internal/request/graphql/parser.go#L80

Added line #L80 was not covered by tests
}

return res
Expand Down
Loading
Loading