Adding extra fields to errors about graphql HOT 25 CLOSED

I think that the proposition of @bbuck is crystal clear :)

But, to sum up I do not want to use the term "field", but rather "entries". From spec:

future versions of the spec may describe additional entries to errors.

Because if you are an error between two fields, it's an entry error no a field error.

Interface

type ErrorEntries map[string]interface{}

type Error struct {
        Message string
        Entries ErrorEntries
}

func (e *Error) Error() string {
        return e.Message
}

func (e *Error) WithEntries(entries ErrorEntries) {
        e.Entries = entries
}

func (e *Error) WithEntry(name string, value interface{}) {
        e.Entries[name] = value
}

func NewError(message string) *Error {
        return &Error{
                Message: message,
                Entries: make(ErrorEntries),
        }
}

Resolver

func (params graphql.ResolveParams) (interface{}, error) {
        return nil, graphql.NewError("Register failed").WithEntry("email", "Must be unique")
}

@sogko or @chris-ramon can you give your vision/opinion?

(as usual, a big thank you for the work)

from graphql.

I think logrus is a good place to start with this one.

logrus.WithFields(logrus.Fields{
        "one": "two",
}).Info("Hey, it's a log")

Given the nature of error (it being an interface and al) we could then do something like this:

type ErrorFields map[string]interface{}

type Error struct {
        Message string
        Fields ErrorFields
}

func (e *Error) Error() string {
        return e.Message
}

func (e *Error) WithFields(fields ErrorFields) {
        e.Fields = fields
}

func (e *Error) WithField(name string, value interface{}) {
        e.Fields[name] = value
}

func NewError(message string) *Error {
        return &Error{
                Message: message,
                Fields: make(ErrorFields),
        }
}

Usage from a Resolve:

func (params graphql.ResolveParams) (interface{}, error) {
        return nil, graphql.NewError("this failed").WithFields(graphql.ErrorFields{
                "one": "two",
        })
}

Probably needs some significant cleaning up, or works. I think it's a simple and clean API.

from graphql.

Great idea!

from graphql.

This is covered by the latest GraphQL spec. Your resolvers can now return errors that implement gqlerrors.ExtendedError to add an "extensions" field with arbitrary contents. For example:

  "errors": [
    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [ { "line": 6, "column": 7 } ],
      "path": [ "hero", "heroFriends", 1, "name" ],
      "extensions": {
        "code": "CAN_NOT_FETCH_BY_ID",
        "timestamp": "Fri Feb 9 14:33:09 UTC 2018"
      }
    }
  ]

from graphql.

The errors field in the GraphQL response is defined in the official spec as follows:

Every error must contain an entry with the key message with a string description of the error intended for the developer as a guide to understand and correct the error.

If an error can be associated to a particular point in the requested GraphQL document, it should contain an entry with the key locations with a list of locations, where each location is a map with the keys line and column, both positive numbers starting from 1 which describe the beginning of an associated syntax element.

Having extra fields is actually addressed in the spec as well:

GraphQL servers may provide additional entries to error as they choose to produce more helpful or machine‐readable errors, however future versions of the spec may describe additional entries to errors.

Would love to start a discussion if / how we want to achieve this.

Cheers!

from graphql.

This is covered by the latest GraphQL spec. Your resolvers can now return errors that implement gqlerrors.ExtendedError to add an "extensions" field with arbitrary contents. For example:

  "errors": [
    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [ { "line": 6, "column": 7 } ],
      "path": [ "hero", "heroFriends", 1, "name" ],
      "extensions": {
        "code": "CAN_NOT_FETCH_BY_ID",
        "timestamp": "Fri Feb 9 14:33:09 UTC 2018"
      }
    }
  ]

The issue has been resolved, so I'm closing it.

from graphql.

Maybe the function's signature can become:

func(p graphql.ResolveParams) (interface{}, error, interface{}) 

So, if there are any extra fields, you can do:

type extraErrors struct {
  Code string `json:"code"`
  Field string `json:"field"`
}

fields := graphql.Fields{
        "hello": &graphql.Field{
            Type: graphql.String,
            Resolve: func(p graphql.ResolveParams) (interface{}, error, interface{}) {

                extras := &extraErrors{Code: "WHOOPS_ERROR", Field: "someField"}
                return nil, errors.New("Whoops! An error occurred")
            },
        },
    }

This means that existing resolvers will need to be updated to use:

return result, nil, nil
return nil, err, nil

from graphql.

@F21 I think changing the return signature for resolve functions would not be such a great idea. The biggest reason is I would imagine that additional error information is not a terribly common requirement so most of the time it would be nil. Basically I don't the use case is large enough to warrant a significant change like that - this feature should probably be implemented in the east invasive but still decently straight-forward way possible.

from graphql.

@bbuck After looking at your proposal, I agree!

from graphql.

By all means the credit should lie with Sirupsen who wrote logrus as I'm just ripping off his idea.

from graphql.

@bbuck This solution looks good, also saves any regression issues for people that are already using this. Is anyone working on this? If not I'd like to take it on.

from graphql.

Hi @bbuck and @sogko

I've started implementing this issue and I'm pretty much done as per you suggestions above. However, I want to discuss how to handle marshalling of this. This new error type eventually gets converted to a FormattedError which, when running this as a web server will most likely eventually be Marshalled. Are we happy with an output format like:

{
  "message": "an error occurred",
  "fields": {
    "customOne": "valueOne",
    "customTwo": "valueTwo:
  }
}

Or do we want to try and flatten it out like:

{
  "message": "an error occurred",
  "customOne": "valueOne",
  "customTwo": "valueTwo:
}

If we want to do the latter how do we go about this in go so that it can be easily marshalled?

from graphql.

@mleonard87 I definitely think the nested approach feels a lot better than flattening it out in some way.

To your second question (even though I don't prefer this method) you'd want to implement the enconding/json.Marshaler interface which would allow the object to marshal itself in a flat structure as opposed to the automatic "nested" structure.

from graphql.

Ok, excellent. Are you happy with the name fields or would you prefer extra(s) or something else?

And finally, what is the purpose of the ErrorFields type. Would it not be more flexible to simply make the fields argument map[string]interface{} itself?

from graphql.

@mleonard87 I'm not a decision maker for this project, I'd wait for @sogko or @chris-ramon to respond before finalizing any decisions.

That being said though, I think fields is clean enough -- and I'm not familiar enough at this moment to respond on ErrorFields but I'll give it a look at soon as I can and get back with you.

from graphql.

May I throw in that the flat structure would allow to (re-)implement all other GraphQL-APIs in this point? Having a fixed key here feels a bit like an artificial restriction.

from graphql.

Any updates/decisions on this?

from graphql.

Hey guys, I'd like to know how this issue is going, and also to give my input.

For me the flattening of fields isn't such a bad idea. I have to respect an error format like so

{
  "message": "Whoops! An error occurred",
  "locations": [
  ],
  "title": "hello",
  "code": 400
}

That means @mleonard87's first solution wouldn't work out for me.

I also see this #238 PR seems to implement a similar thing.
What do you guys think ?

from graphql.

Just want to bring to your attention a PR I submitted related to this issue: #279.

This PR basically

• stores the original error in FormattedError
• uses github.com/pkg/errors in order to also have stack trace information

This enables me to render my domain errors differently, since I now have access to the actual underlying error.

from graphql.

@dvic Could you provide a usage example ?
Let's say I return a custom error, what would I need to do for this error to end up looking like my one of the examples above ?

from graphql.

@plassa-b It would look something like this where you handle the graphql query:

res := graphql.Do(params)
// instead of rendering res directly, we're going to do something with the errors
if res.HasErrors() {
	var errorsArray []interface{}
	for _, err := range res.Errors {
		// errors.Cause(...) unwraps the underling error, e.g., if errors.Wrap(someErr) was used
		// we get 'someErr' instead of the wrapped error
		if err, ok := errors.Cause(err.Cause()).(interface {
			AsPublicJSON() []byte
		}); ok {
			errorsArray = append(errorsArray, json.RawMessage(err.AsPublicJSON()))
			continue
		}
		errorsArray = append(errorsArray, err)
	}
	writeJSON(rw, map[string]interface{}{"Errors": errorsArray})
} else {
	writeJSON(rw, map[string]interface{}{"Data": res.Data})
}

in the resolvers you can then return an error of the following type:

type myErr struct {
	message string
	title   string
	code    int
}

func (e myErr) Error() string {
	return e.message
}

func (e myErr) AsPublicJSON() []byte {
	ret, err := json.Marshal(map[string]interface{}{
		"message":   e.message,
		"locations": []interface{}{},
		"title":     e.title,
		"code":      e.code,
	})
	if err != nil {
		panic(errors.WithStack(err))
	}
	return ret
}

Of course, you can also do the type check differently (on a specific error type, other methods, etc.).

from graphql.

Ok it looks great !
I found the #238 to be a more user friendly solution, but since it actually doesn't concern a lot of users, it may actually make things more confusing.
One up !

from graphql.

Hi all! What is the current status of this enhancement? We really need this to start using this graphql solution. Returning machine readable error codes - is important part of any big project.

from graphql.

@sogko or @chris-ramon can you give your vision/opinion?

Do you need a PR?

from graphql.

@ccbrown Thanks, I think that this issue can be closed ;)

from graphql.