Lighthouse: Error not caught if use @rules directive along with custom scalar type

[yaquawa]

Describe the bug
Supposing we have the following schema.

type Mutation {
    createUser(
        email: Email
        password: Password
    ): User @field(resolver: "App\\GraphQL\\Type\\User@create")
}

Where Email and Password scalar types are defined as:

namespace App\GraphQL\Type\Scalar;

use GraphQL\Error\Error;
use GraphQL\Error\InvariantViolation;
use GraphQL\Language\AST\StringValueNode;
use GraphQL\Type\Definition\ScalarType;
use GraphQL\Utils\Utils;

class Email extends ScalarType
{
    public $name = 'Email';

    public $description = 'A valid email string.';

    public function serialize($value)
    {
        return $value;
    }

    public function parseValue($value)
    {
        if ( ! filter_var($value, FILTER_VALIDATE_EMAIL)) {
            throw new Error("Cannot represent following value as email: " . Utils::printSafeJson($value));
        }

        return $value;
    }

    public function parseLiteral($valueNode, array $variables = null)
    {
        if ( ! $valueNode instanceof StringValueNode) {
            throw new Error('Query error: Can only parse strings got: ' . $valueNode->kind, [$valueNode]);
        }
        if ( ! filter_var($valueNode->value, FILTER_VALIDATE_EMAIL)) {
            throw new Error("Not a valid email", [$valueNode]);
        }

        return $valueNode->value;
    }
}

namespace App\GraphQL\Type\Scalar;

use GraphQL\Error\Error;
use GraphQL\Error\InvariantViolation;
use GraphQL\Language\AST\StringValueNode;
use GraphQL\Type\Definition\ScalarType;
use GraphQL\Utils\Utils;
use Illuminate\Support\Facades\Validator;

class Password extends ScalarType
{
    public $name = 'Password';

    public $description = 'A password string which must be at least 6 letters and at most 30 letters.';

    public function serialize($value)
    {
        return $value;
    }

    public function parseValue($value)
    {
        $validator = Validator::make(['input' => $value], [
            'input' => 'min:6|max:30',
        ]);

        if ($validator->fails()) {
            throw new Error($validator->errors()->first());
        }

        return $value;
    }

    public function parseLiteral($valueNode, array $variables = null)
    {
        if ( ! $valueNode instanceof StringValueNode) {
            throw new Error("Query error: Can only parse strings got: {$valueNode->kind}", [$valueNode]);
        }

        return $this->parseValue($valueNode->value);
    }
}

Then if I make a mutation request with some invalid values:

The mutation

mutation createUser($email: Email, $password: Password) {
  createUser(email: $email, password: $password) {
    email
    id
  }
}

The variables

{
  "email": "xxx",
  "password" : "xx"
}

it'll give me the error messages for each invalid field, in this case, the email and password fields. Which is what I expected.

{
  "errors": [
    {
      "category": "graphql",
      "message": "Expected type Email; Cannot represent following value as email: \"xxx\"",
      "locations": [
        {
          "line": 1,
          "column": 21
        }
      ]
    },
    {
      "category": "graphql",
      "message": "Expected type Password; The input must be at least 6 characters.",
      "locations": [
        {
          "line": 1,
          "column": 36
        }
      ]
    }
  ]
}

However, if I use a @rules directive on the email (or password) field:

The mutation

type Mutation {
    createUser(
        email: Email @rules(apply: ["unique:users,email"])
        password: Password
    ): User @field(resolver: "App\\GraphQL\\Type\\User@create")
}

The variables
This time, the value of email has a valid email format, but because it's not the unique value in the users table, still it will be invalid.

{
  "email": "[email protected]",
  "password" : "xx"
}

I expected I'll get two error messages for both email and password, but only the password field's get backed.

{
  "errors": [
    {
      "category": "graphql",
      "message": "Expected type Password; The input must be at least 6 characters.",
      "locations": [
        {
          "line": 1,
          "column": 36
        }
      ]
    }
  ]
}

Expected behavior
Additionally, I noticed that if we get multiple error messages, we have no way to tell which error message is for which field… perhaps add an additional field property is better:

{
  "errors": [
    {
      "category": "graphql",
      "message": "Expected type Email; Cannot represent following value as email: \"xxx\"",
      "FIELD" : "EMAIL",
      "locations": [
        {
          "line": 1,
          "column": 21
        }
      ]
    },
    {
      "category": "graphql",
      "message": "Expected type Password; The input must be at least 6 characters.",
      "FIELD" : "PASSWORD",
      "locations": [
        {
          "line": 1,
          "column": 36
        }
      ]
    }
  ]
}

Environment

Lighthouse Version: dev-master
Laravel Version: 5.6
PHP Version: 7.2

[spawnia]

In the last few days i gained an understanding of why this happens webonyx/graphql-php runs the scalar validation on the query first, before it is passed on to Lighthouse where we run the Laravel-style validation.

We are currently also having a discussion on how to handle required arguments in #345 which will also require changes to how validation is run.

We might try to register the validation step through the webonyx DocumentValidator class so are able to collect all the errors before stopping the execution of the field.

[robsontenorio]

@spawnia Does it means you can provide a solution on LightHouse context with no dependency on webonyx/graphql-php?

[spawnia]

@robsontenorio The solution will depend heavily upon webonyx/graphql-php either way. At the moment i am hoping that the underlying issue can be resolved there. As a last resort, we can swap out the validation logic from there with our own. I would really try to avoid that though, as it would grow Lighthouse's responsibilies and cut us off from upstream fixes.

[robsontenorio]

By the last comment, the idea was not well accepted :(

https://github.com/webonyx/graphql-php/issues/356#issuecomment-426622320

[spawnia]

I am reclassifying this as a possible enhancement. Scalars are transparent to the client, and the proper format should also be described. Validating their proper format is part of GraphQL's validation phase, which the spec explicitly places before the execution phase.

However, i see how tying scalar validation and Laravel validation together would be helpful, but there is quite a lot of effort involved. As of now, i think there are bigger improvements to be had in other areas, but i wouldn't rule out coming back to this at some point.

[spawnia]

See https://github.com/nuwave/lighthouse/issues/1751 for a possible grand unification of argument validation.