Hotchocolate: Improving the ErrorFilter API

[huysentruitw]

Limitations

There are two limitation for the IErrorFilter API that I would like to discuss:

1. It should be possible to set a HTTP status code from the ErrorFilter. I understand that with GraphQL we're treating HTTP StatusCode's as a transport layer concern and not a GraphQL concern. However, for certain errors, we really need to be able to set the HTTP StatusCode. An example for us is when the user tries to perform an unauthorized action, we really want to return a 401 HTTP StatusCode so the front-end can act on a standardized thing instead of depending on the custom code AUTH_NOT_AUTHORIZED.

2. It should be possible to return multiple errors for one incoming error. An example here is, when we want to process a ValidationException from FluentValidation, we want to return an IError for each validation error, currently we only translate the first one, but it would be nice if we could translate them all.

Proposals

1. Adding a WithHttpStatusCode extension method on IError or register the ErrorFilter as scoped so we can use the IHttpContextAccessor.

2. We could modify the interface from IError OnError(IError error) to IEnumerable<IError> OnError(IError error), but that's a breaking change we probably not want. An other solution would be to introduce an AggregateError class that can hold multiple errors, while still deriving from IError (similar to the AggregateException in the .NET framework).

Please tell me what you think?

[huysentruitw]

For the record, our current error filter looks like this, where you see that for FluentValidation exceptions, we can only transform the first validation error:

internal sealed class ErrorFilter : IErrorFilter
{
    private static readonly Regex SnakeCasingRegex = new Regex("([A-Z])", RegexOptions.Compiled);

    public IError OnError(IError error)
    {
        if (error.Exception == null)
            return error;

        return EnrichErrorForFluentValidationException(error)
            ?? EnrichErrorForException(error);
    }

    private static IError EnrichErrorForFluentValidationException(IError error)
    {
        if (!(error.Exception is ValidationException validationException))
            return null;

        if (!validationException.Errors.Any())
            return null;

        ValidationFailure firstValidationFailure = validationException.Errors.First();

        var data = firstValidationFailure.FormattedMessagePlaceholderValues
            .ToDictionary(x => ToCamelCasing(x.Key), x => x.Value);

        return error
            .WithCode(ToSnakeCasing(firstValidationFailure.ErrorCode))
            .AddExtension("data", data);
    }

    private static IError EnrichErrorForException(IError error)
    {
        string code = GetCodeForException(error.Exception);
        IReadOnlyDictionary<string, object> data = GetExceptionData(error.Exception);

        return error
            .WithCode(code)
            .AddExtension("data", data);
    }

    private static string GetCodeForException(Exception exception)
    {
        var exceptionName = exception.GetType().Name;

        if (exceptionName.EndsWith("Exception"))
            exceptionName = exceptionName[..^9];

        return ToSnakeCasing(exceptionName);
    }

    private static IReadOnlyDictionary<string, object> GetExceptionData(Exception exception)
    {
        Type exceptionType = exception.GetType();

        return exceptionType
            .GetProperties(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)
            .ToDictionary(propertyInfo => ToCamelCasing(propertyInfo.Name), propertyInfo => propertyInfo.GetValue(exception));
    }

    private static string ToSnakeCasing(string input)
        => SnakeCasingRegex.Replace(input, "_$0").TrimStart('_').ToUpper();

    private static string ToCamelCasing(string input)
        => char.ToLower(input[0]) + input.Substring(1);
}

[michaelstaib]

It should be possible to set a HTTP status code from the ErrorFilter. I understand that with GraphQL we're treating HTTP StatusCode's as a transport layer concern and not a GraphQL concern. However, for certain errors, we really need to be able to set the HTTP StatusCode. An example for us is when the user tries to perform an unauthorized action, we really want to return a 401 HTTP StatusCode so the front-end can act on a standardized thing instead of depending on the custom code AUTH_NOT_AUTHORIZED.

This is solved.

https://github.com/ChilliCream/hotchocolate/blob/66bfb78f45c33aa5ff5a283c64ad49fdb83f3d0a/src/HotChocolate/AspNetCore/src/AspNetCore/Utilities/DefaultHttpResultSerializer.cs#L59

You can inherit from DefaultHttpResultSerializer and add custom StatusCode handling. For this store props on the context and then use them to interpret status codes.

[michaelstaib]

You can also inspect the errors in this case, it's up to you.

[michaelstaib]

It should be possible to return multiple errors for one incoming error. An example here is, when we want to process a ValidationException from FluentValidation, we want to return an IError for each validation error, currently we only translate the first one, but it would be nice if we could translate them all.

Why do you not throw multiple in the resolver?

new GraphQLException(new[] { error1, error2 })

[huysentruitw]

It should be possible to return multiple errors for one incoming error. An example here is, when we want to process a ValidationException from FluentValidation, we want to return an IError for each validation error, currently we only translate the first one, but it would be nice if we could translate them all.

Why do you not throw multiple in the resolver?

new GraphQLException(new[] { error1, error2 })

We validate incoming commands in our commandhandler using FluentValidation. The exception is being throw by the FluentValidation library while calling ValidateAndThrowAsync which always throws one ValidationException that contains one or more validation errors. So this is outside of our control. We would like to avoid introducing the concept of GraphQL in our command-handler logic and would love to keep exception/error transformation at a single place in our code, namely this ErrorFilter.

[huysentruitw]

It should be possible to set a HTTP status code from the ErrorFilter. I understand that with GraphQL we're treating HTTP StatusCode's as a transport layer concern and not a GraphQL concern. However, for certain errors, we really need to be able to set the HTTP StatusCode. An example for us is when the user tries to perform an unauthorized action, we really want to return a 401 HTTP StatusCode so the front-end can act on a standardized thing instead of depending on the custom code AUTH_NOT_AUTHORIZED.

This is solved.

https://github.com/ChilliCream/hotchocolate/blob/66bfb78f45c33aa5ff5a283c64ad49fdb83f3d0a/src/HotChocolate/AspNetCore/src/AspNetCore/Utilities/DefaultHttpResultSerializer.cs#L59

You can inherit from DefaultHttpResultSerializer and add custom StatusCode handling. For this store props on the context and then use them to interpret status codes.

Thanks for this information, we're currently on version 10.5.3, so do I understand correctly that this will be possible starting from version 11?

[michaelstaib]

Thanks for this information, we're currently on version 10.5.3, so do I understand correctly that this will be possible starting from version 11?

Yes, it is possible with the current 11.0.0-rc.2

[michaelstaib]

We validate incoming commands in our commandhandler using FluentValidation.

I understand, never thought about it this way :D

I will add this issue to the backlog for 11.1. We already have a feature freeze on 11.0.

[huysentruitw]

Thank you for considering this. If you have an idea in mind on how you would like this to be implemented, I'm willing to work on that.

[AradAral]

It should be possible to return multiple errors for one incoming error. An example here is, when we want to process a ValidationException from FluentValidation, we want to return an IError for each validation error, currently we only translate the first one, but it would be nice if we could translate them all.

This is very much needed, we encountered a similar scenario recently, looking forward to having this feature in v11.1.