500 code is wrong

[Big-Shark]

Hi @moufmouf

This condition looks very strange, because the "category" is a custom extension parameter, and we don't need to use it.

I think we need to use isClientSafe and return code 400 if true, or we need check, "Error" instance of ClientAware or not, and return code 400.

What do you think about it?

Also: getCode always returns 0

[moufmouf]

This condition looks very strange, because the "category" is a custom extension parameter, and we don't need to use it.

I'm trying to catch any "GraphQL query malformed" exceptions and so far, this is the best thing I found.

I think we need to use isClientSafe and return code 400 if true

isClientSafe simply means that the error message can be displayed to the user. But I think it does not mean anything regarding the HTTP error code we should use. I mean, it could be a request problem (and HTTP 400 would be ok) or a server-side problem (and HTTP 500 would also be OK).

Also: getCode always returns 0

Good catch! I actually wanted to test the code of the exception wrapped into the error (the previous exception). I'll fix this.

[Big-Shark]

I'm trying to catch any "GraphQL query malformed" exceptions and so far, this is the best thing I found.

Maybe we can create custom ExceptionInterface and extend errorInterface? If we do this, we can add one new variable for checking.

[moufmouf]

But the "malformed GraphQL" exceptions are thrown by Webonyx and we cannot force Webonyx to use this new interface...

Looking at the Webonyx/GraphQL-PHP code, I see that some errors triggered are related to the GraphQL query (should map to HTTP 400) and others are related to the schema (should map to HTTP 500).

For instance, this one should map to a HTTP 400:

$errors[] = new Error(
                    sprintf(
                        'Variable "$%s" of required type "%s" was not provided.',
                        $varName,
                        $varType
                    ),
                    [$varDefNode]
                );

This one should map to 400:

throw new Error('Cannot represent value as enum: ' . Utils::printSafe($value));

This one should map to 400:

new Error(
                                sprintf('@%s directive is missing "%s" argument.', Directive::SKIP_NAME, Directive::IF_ARGUMENT_NAME),
                                $selection
                            )

This one should map to 500:

$this->addError(Error::createLocatedError(
                            new InvariantViolation(
                                sprintf(
                                    'Schema must contain unique named types but contains multiple types named "%s". ' .
                                    'Make sure that `resolveType` function of abstract type "%s" returns the same ' .
                                    'type instance as referenced anywhere else within the schema ' .
                                    '(see http://webonyx.github.io/graphql-php/type-system/#type-registry).',
                                    $objectType,
                                    $type
                                )
                            ),
                            $ctx->shared->fieldNodes,
                            $path
                        ));

Now, for the first 3 exceptions (HTTP 400):

• isClientSafe = true
• category = self::CATEGORY_GRAPHQL

For the last exception (HTTP 500):

• isClientSafe = false
• category = self::CATEGORY_INTERNAL

I'm not very inclined in using isClientSafe to know if this is a 400 or a 500 because I have samples of my own code where I throw a 500 exception with isClientSafe = true.

I still believe the "self::CATEGORY_GRAPHQL" is the best thing we have to detect poorly written queries (unless you can find a counter-example where "self::CATEGORY_GRAPHQL" is used for a request that should be a HTTP 500.

On the other hand, I do need to fix the bug you found with getCode always returns 0. I'm working on it right now.

[moufmouf]

Ok, looking at Webonyx code, I think I found something great:

https://github.com/webonyx/graphql-php/blob/master/src/Error/Error.php#L123-L132

Basically, if there is no "previous" exception, then category = self::CATEGORY_GRAPHQL.

So maybe the good thing to test is to see whether there is a previous exception or not.
If there is no previous exception, we can assume the error is directly triggered by Webonyx and that it must be mapped to HTTP 400.

[moufmouf]

See #24