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

handler: adds formatErrorFn #58

Merged
merged 5 commits into from
Dec 3, 2018
Merged

handler: adds formatErrorFn #58

merged 5 commits into from
Dec 3, 2018

Conversation

chris-ramon
Copy link
Member

@chris-ramon chris-ramon commented Dec 3, 2018

Overview

  • Built on top of: Added Support for CustomErrorFormatter #46 — thanks a lot @racerxdl 👍

  • Now that graphql-go/graphql added support for FormattedError.OriginalError() this PR adds support for adding a custom error formatter via Handler.formatErrorFn which matches the reference implementation:

    graphqlHTTP({
      schema: GraphQLSchema,
      graphiql?: ?boolean,
      rootValue?: ?any,
      context?: ?any,
      pretty?: ?boolean,
      formatError?: ?Function, <----------------
      validationRules?: ?Array<any>,
    }): Middleware

    Ref: Link

Test plan

  • Unit tests
  • Adds Handler.formatErrorFn tests

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 87.437% when pulling 202b989 on formatErrorFn into 28550b4 on master.

@chris-ramon chris-ramon merged commit 12f536e into master Dec 3, 2018
@chris-ramon chris-ramon deleted the formatErrorFn branch December 3, 2018 03:20
@limoli
Copy link

limoli commented Dec 3, 2018

Sorry but I think that the feature was misunderstood.
I have just tried to implement something with the new feature and it is almost useless in this way.

Current wrong implementation

func(err gqlerrors.FormattedError) gqlerrors.FormattedError {
???
}

Correct implementation from #46

func CustomFormat(err error) gqlerrors.FormattedError {
        // Example of management
	switch err := err.(type) {
	case *gqlerrors.Error:
		cf := CustomFormat(err.OriginalError)
		cf.Locations = err.Locations
		cf.Path = err.Path
		return cf
	case gqlerrors.ExtendedError:
		return gqlerrors.FormatError(err)
	case *QuantoError.ErrorObject:
		return err.ToFormattedError()
	default:
		return gqlerrors.FormatError(err)
	}
}

The feature must provide error interface as input. If I return a custom error from a resolver, the error that I get as input in CustomFormattedFunction must have the same custom type (if casted from error interface) that I returned in the resolver.
In few words, the returned error from resolver, must not be handled! Just passed to the custom format function if set.

@chris-ramon
Copy link
Member Author

Hi @limoli, thanks for following up the PR and trying it out.

  • Config.CustomErrorFormatter was renamed to Config.FormatErrorFn for
    consistency with ( Config.RootObjectFn & Config.ResultCallbackFn )

  • FormatErrorFn function signature is func(err gqlerrors.FormattedError) gqlerrors.FormattedError for near parity with function formatError(error: GraphQLError): GraphQLFormattedError from graphql-js, actually we are not using GraphQLError but instead GraphQLFormattedError because is the public struct error, we do have GraphQLError as (gqlerrors.Error) but is for internal use.

  • I've created a working example, if it is required to access to the error interface, OriginalError() should be use:

package main

import (
	"errors"
	"log"
	"net/http"

	"github.com/graphql-go/graphql"
	"github.com/graphql-go/graphql/gqlerrors"

	"github.com/graphql-go/handler"
)

type CustomError struct {
	error
	code string
}

func (e CustomError) Error() string {
	return e.error.Error()
}

func (e CustomError) ToFormattedError(err gqlerrors.FormattedError) gqlerrors.FormattedError {
	return gqlerrors.FormattedError{
		Message:   err.Message,
		Locations: err.Locations,
		Path:      err.Path,
		Extensions: map[string]interface{}{
			"custom_format": true,
			"code":          e.code,
		},
	}
}

var RootQuery = graphql.NewObject(graphql.ObjectConfig{
	Name: "Query",
	Fields: graphql.Fields{
		"demo": &graphql.Field{
			Type: graphql.String,
			Resolve: func(p graphql.ResolveParams) (interface{}, error) {
				err := &CustomError{
					error: errors.New("demo error"),
					code:  "ERROR_CODE",
				}
				return nil, err
			},
		},
	},
})

func main() {
	schema, err := graphql.NewSchema(graphql.SchemaConfig{
		Query: RootQuery,
	})
	if err != nil {
		log.Fatal(err)
	}

	h := handler.New(&handler.Config{
		Schema:   &schema,
		Pretty:   true,
		GraphiQL: true,
		FormatErrorFn: func(err gqlerrors.FormattedError) gqlerrors.FormattedError {
			if err.OriginalError() == nil {
				return err
			}

			switch originalError := err.OriginalError().(type) {
			case *CustomError:
				return originalError.ToFormattedError(err)
			default:
				return err
			}

		},
	})

	http.Handle("/graphql", h)
	log.Println("server running on :8080")
	http.ListenAndServe(":8080", nil)
}

image

@limoli
Copy link

limoli commented Dec 3, 2018

Hi @chris-ramon, thanks for the explanation!
Now I understood even if I would advice you to not follow Typescript implementation too much since Golang has own structures and interfaces.
To be honest, I think that the abstraction of error interface is unbeatable and as you can see the GraphQL Typescript error uses inheritance.

class GraphQLError extends Error {
 constructor(
   message: string,
   nodes?: Array<any>,
   stack?: ?string,
   source?: Source,
   positions?: Array<number>,
   originalError?: ?Error,
   extensions?: ?{ [key: string]: mixed }
 )
}

For that reasons, the following code is definitely a bad practice:

type CustomError struct {
	error
	code string
}

As error is a interface, you have to respect this architecture style, implementing it and writing something like this:

type CustomError struct {
	code string
        message string
}

func (e CustomError) Error() string {
        return fmt.Sprintf("%s: %s", e.code, e.message)
}

In the current case, this doesn't make any sense:

func (e CustomError) Error() string {
	return e.error.Error()
}

If you follow this approach, you write less (and better) code, because the following is totally removed:

func (e CustomError) ToFormattedError(err gqlerrors.FormattedError) gqlerrors.FormattedError {
	return gqlerrors.FormattedError{
		Message:   err.Message,
		Locations: err.Locations,
		Path:      err.Path,
		Extensions: map[string]interface{}{
			"custom_format": true,
			"code":          e.code,
		},
	}
}

And you have just to update the FormatErrorFn function like this:

h := handler.New(&handler.Config{
		Schema:   &schema,
		Pretty:   true,
		GraphiQL: true,
		FormatErrorFn: func(err error) gqlerrors.FormattedError {
                        // Own Implementation
		},
	})

I think that the library should provide the basic abstractions to let us to build our customisation over. This logics is like saying : "You receive an error from a resolver and you are able to customise it if you can return to me a gqlerrors.FormattedError".

@limoli
Copy link

limoli commented Dec 6, 2018

I have tried to check little how to do it, but handler is very coupled to GraphqlErrors. This doesn't allow me to change things easily keeping all the useful graphQL error information. So I keep your implementation.

My current use of this feature

func CustomErrorHandler(err gqlerrors.FormattedError) gqlerrors.FormattedError {
	switch customErr := err.OriginalError().(type) {
	case *translation.TranslatableError:
		err.Message = customErr.Translate()
                break
	}

	return err
}

@chris-ramon
Copy link
Member Author

chris-ramon commented Dec 10, 2018

Thanks for the great feedback and following up the PRs @limoli.

I've sent a PR for consolidating this, I def. agree the correct function signature should be as you described:

  • Handler.formatErrorFn func(err error) gqlerrors.FormattedError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants