Revisit approach to user errors

[vladar]

Currently we have GraphQL\Error\UserError and GraphQL\Error\Error which serve similar purpose - to represent error which can be shown to end-user. It's better use one of them: GraphQL\Error\Error.

Another thing to consider: we must throw different exceptions in scalar types: basically parseValue deals with user input, so in case of invalid input we should throw exception which should be directly reported to end-user.

But exceptions in serialize should not be reported to end-users as such exception means internal logic error.

So to recap:

• Replace all entries of GraphQL\Error\UserError with GraphQL\Error\Error
• Make sure parseValue of all scalar types throws GraphQL\Error\Error on invalid input value
• Make sure serialize of all scalar types throws GraphQL\Error\InvariantViolation on invalid output value

[janschoenherr]

To add to this issue. Why does GraphQL try to catch all exceptions from user land code. Wouldn't it be better to catch specific GraphQL errors only. Currently it is not possible to handle custom exceptions that are thrown e.g. in the resolve callbacks.

[vladar]

@janschoenherr This question often arises in graphql-js repo as well. The general answer is that all of your exceptions are available in ExecutionResult after execution, so you can filter them before displaying to a client, log, re-throw, etc.

But can you elaborate a bit more on what you are trying to achieve with regards to error handling that isn't possible now?

[vladar]

Also, the reason why custom errors are caught is that error in a single nullable field is not fatal for the whole query. Since the field is marked as nullable we can safely return null for this specific field vs killing the whole query.

[janschoenherr]

Thanks I found the Exception in the ExecutionResult. However it seems a bit cumbersome to pull the original Exception from the "Exception Chain".

We are currently doing authorization logic in the resolve functions and are e.g. trying to throw an "Unauthorized" 403 Exception. The handling of that Exception is outside the GraphQL Execution.

Or a simple case, where there is a PDO Database Exception. I'm not sure you'd want that in the errors result.

In case you'd want to throw a catchable Exception, it could be required to be a GraphQL Exception.

[vladar]

The problem is that if you catch only known exceptions you will make the whole query fail in unexpected situations even if only one minor nullable field throws.

If you can provide a formally valid result to end-user - why should you kill the whole query in such case?

So the actual problem is that you really don't want to deliver error messages of "unsafe" exceptions to clients (as it happens today).

You can do this already with custom error filtering and formatting. But default error formatting should be also safe and don't expose sensitive error messages.

In version 0.10.0 default error formatter will display "Internal error" message for resolver exceptions unless exception implements GraphQL\Error\ClientAware interface (it is a breaking change, but documented in UPGRADE.md). It is similar to this project.

I also consider introducing GraphQL\Error\CriticalError interface so that when exception implementing such interface is thrown anywhere during execution - it is re-thrown by the executor and whole execution process is stopped.

You should be able to catch such exceptions in outer try{} catch() statement. Please let me know if that would solve your issues?

[janschoenherr]

In general custom error filtering and formatting work fine. They introduce extra work on the user's part though.

However to me it seems like the whole query should indeed fail in unexpected situations - even for a minor nullable field. The data might be incomplete and not usable anyway.

So the actual problem is that you really don't want to deliver error messages of "unsafe" exceptions to clients (as it happens today).

That's one part, yes. It seems to be solved with 0.10.0 as you've described.

The other part is, that I do want the whole query to fail in some cases. For example the user not being authorized to make the query.

'fields' => [
        'hero' => [
            'type' => $characterInterface,
            'args' => [
                'episode' => [
                    'description' => 'If omitted, returns the hero of the whole saga. If provided, returns the hero of that particular episode.',
                    'type' => $episodeEnum
                ]
            ],
            'resolve' => function ($root, $args, $context) {

                if (!$context['user']->hasPermission('get_starwars_hero')) {
                    abort(401, 'Unauthorized action.'); // throws Framework's HTTPException
                }

                return StarWarsData::getHero(isset($args['episode']) ? $args['episode'] : null);
            },
        ]
]

I also consider introducing GraphQL\Error\CriticalError interface so that when exception implementing such interface is thrown anywhere during execution - it is re-thrown by the executor and whole execution process is stopped.

That probably wouldn't help. As you'd have to again wrap the original Exception, if e.g. you are not the owner of the Exception. It might come from some library, framework or PHP itself.

To me it comes down to catching all Exceptions vs. GraphQL\Error\Error only.

Maybe this behavior could be introduced by a configuration setting.

[vladar]

However to me it seems like the whole query should indeed fail in unexpected situations - even for a minor nullable field. The data might be incomplete and not usable anyway.

That is wrong. GraphQL schema is a contract between a client and a server. And if server schema says, that field can be null, it means that client must take nullability into account anyway.

If the data is incomplete without the field, this field must be explicitly defined as notNull.

[vladar]

And you can actually do what you need even now. Maybe it requires some efforts to filter and format exceptions, but at least you can do it.

Say throwing 401 is possible by scanning ExecutionResult and re-throwing HTTPException if found after execution.

But if the default is not to catch exceptions and to throw even on minor errors in nullable fields - then other projects will suffer without any options to solve their problems (like our project for example, which can tolerate partial data, but not 500 errors on every minor field error).

And we won't have any options because an exception that is not caught will break Execution process without any way to resume it. So you have options now, but with your proposal, we will have none.

This choice to error handling is actually made in graphql-js, so I guess Facebook engineers faced with the same dilemma. You might be also interested in reading related discussions in graphql-js repository for insights. Like: graphql/graphql-js#225, graphql/graphql-js#773

[janschoenherr]

Thanks for the links and your thorough explanations!

[vladar]

Added new docs on error handling which should make the life a bit easier.

[janschoenherr]

Thanks, having the Debug::RETHROW_INTERNAL_EXCEPTIONS flag is great!