handler: adds formatErrorFn

chris-ramon commented 5 years ago

Overview

Built on top of: https://github.com/graphql-go/handler/pull/46 — thanks a lot @racerxdl :+1:
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 commented 5 years ago

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

limoli commented 5 years ago

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 commented 5 years ago

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)
}

limoli commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

graphql-go / handler

handler: adds formatErrorFn #58

Overview

Test plan