graphql-platform icon indicating copy to clipboard operation
graphql-platform copied to clipboard

Please add sufficent error handling example with HTTP status codes to documentation

Open majda107 opened this issue 2 years ago • 6 comments

Is your feature request related to a problem?

In the current state of docs, it's unclear how the HTTP status interceptor determines the status code related to exception/returned data value in the resolver.

If you want to implement the 6a pattern and return 200 OK every time a "known" error occurs (for example validation issue), you have to do one of these things: a) always return nullable! (otherwise, you'll get 500) b) try-catch the exception -> report exception with resolverCtx.ReportError(...) -> return data/null.

Some people (including myself) don't automatically implement nullable return values, which results in non-sense 500 errors. This behavior of the HotChocolate pipeline should be documented in docs.

Note This also affects errors related to authentication, if - for example - I return user data that are C# record (non-nullable by nature) and the user isn't authorized, it shoots out 500 HTTP code. When you make the user data record nullable, you get 200 HTTP code. (correct behavior, but this 'authorize' attribute should be namely documented because normally you don't even have to return null in resolver).

The solution you'd like

Add HotChocolate HTTP status code and error behavior in docs.

Product

Hot Chocolate

majda107 avatar Nov 17 '22 01:11 majda107

This actually is a wrong assumption.

When we talk about known error, or lets say domain errors. Things that are expected with stage 6a then you do not report it as an error et all.

resolverCtx.ReportError(...) is for real GraphQL errors.

Stage 6a is about exposing errors in you schema which we would do by either annotating the exceptions that are domain errors or using the MutationResult struct.

Essentially ...

[Error<SomeInputHasWrongFormatException>]
public async Task<SomeResult> DoSomething(string someInput)

michaelstaib avatar Nov 19 '22 19:11 michaelstaib

@michaelstaib

Does this mean that both FairyBread and AppAny.HotChocolate.FluentValidation are incorrectly calling ReportError?

  • https://github.com/benmccallum/fairybread/blob/90902ded893c3d6e0631c8bfe07de7dacd28c775/src/FairyBread/DefaultValidationErrorsHandler.cs#L21
  • https://github.com/appany/AppAny.HotChocolate.FluentValidation/blob/050332bb163edab1f18a2a5a4761ec89527fe1ca/src/ValidationMiddlewares.cs#L67

If that's true:

  1. It might make sense to rename this method and/or clarify its expected use in the description. f.e. ReportGraphQlError, ReportFatalError, ReportRuntimeError, or whatever makes sense to separate it from application-level errors.
  2. How should these libraries handle errors? How do you report an application-level error without having to throw an exception and have every mutation configured to expect that exception? Is there something like ReportError for application errors?

As mentioned on Slack, a validation error when using the AppAny library is currently resulting in a 500 code, and an errors-only response (even when submitting multiple mutations in the same request, where one or more are successful). This is not good at all.

I really am looking forward to having comprehensive error handling documentation for v13, as well as part two of the video, covering error handling for queries.

Edit: There are other cases where we'd like to surface domain exceptions without having to annotate every single mutation. Maybe the mutation convention configuration could allow you to specify a list of exceptions to be included "globally"?

glen-84 avatar Jan 11 '23 12:01 glen-84

I'm currently running into this wall as well. I, personally, would prefer not to use the 6a convention (because of style preference). I tried something like:

throw new GraphQLException(new ErrorBuilder().SetCode("AccountAlreadyExists").SetMessage("Account already exists.").Build());

But that doesn't seem to go along well with the apollo client (doesn't expose the errors information to the mutate function). Probably because of the 500 status code?

John0x avatar Aug 23 '23 18:08 John0x

What about Domain Errors on queries? [Error] seem to work only for mutations.

nesherhh avatar Feb 16 '24 16:02 nesherhh

@nesherhh This is available in v14 previews. (https://github.com/ChilliCream/graphql-platform/pull/6846)

glen-84 avatar Feb 16 '24 17:02 glen-84

Postman and BCP behaves differently in terms of response http status code with FairyBread. Postman returns 200 whereas BCP returns 500 if there is any input validator fails. The reason is that Postman in GraphQL mode always sends "Accept: application/json" header in the request, which tells HC server to use legacy mode (explained here https://chillicream.com/docs/hotchocolate/v13/server/http-transport). If you add the same header in BCP it doesn't return 500 but returns 200 just like Postman. If you don't want to add the header to client request you can configure the server.

public class CustomValidationErrorsHandler : DefaultValidationErrorsHandler
{
    public const string FairyBreadErrorCode = "VALIDATION_ERROR";

    protected override IErrorBuilder CreateErrorBuilder(
        IMiddlewareContext context,
        string argumentName,
        IValidator validator,
        ValidationFailure failure)
    {
        return base.CreateErrorBuilder(context, argumentName, validator, failure)
            .SetCode(FairyBreadErrorCode); // default is "FairyBread_ValidationError"
    }
}
public class CustomHttpResponseFormatter : DefaultHttpResponseFormatter
{
    public CustomHttpResponseFormatter(HttpResponseFormatterOptions options) : base(options)
    {
    }

    protected override HttpStatusCode OnDetermineStatusCode(IQueryResult result, FormatInfo format, HttpStatusCode? proposedStatusCode)
    {
        if (result.Errors?.Count > 0 &&
            result.Errors.Any(error => error.Code == CustomValidationErrorsHandler.FairyBreadErrorCode))
        {
            return HttpStatusCode.Conflict;
        }

        return base.OnDetermineStatusCode(result, format, proposedStatusCode);
    }
}
services.AddHttpResponseFormatter(_ => new CustomHttpResponseFormatter(new HttpResponseFormatterOptions
{
    HttpTransportVersion = HttpTransportVersion.Legacy,
    Json = new JsonResultFormatterOptions
    {
        Indented = true
    }
}));

hero3616 avatar Jul 19 '24 21:07 hero3616