lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

Unify argument validation

Open spawnia opened this issue 4 years ago • 8 comments

What problem does this feature proposal attempt to solve?

The native spec-compliant validation performed by webonyx/graphql-php (!, scalars) and Lighthouse/Laravel (@rules, @validator) run in different phases and return different error shapes.

This has lead to number of issues:

  • https://github.com/nuwave/lighthouse/issues/345
  • https://github.com/nuwave/lighthouse/issues/262

Which possible solutions should be considered?

type Query {
  foo(
    nonNull: ID!
    customScalar: CustomScalar
    required: String @rules(apply: ["required"]
    email: String @rules(apply: ["email"]
  ): ID
}

query EverythingWrong {
  # Missing nonNull and required
  foo(customScalar: "wrong-format", email: "not-an-email")
}

An optimal solution would combine all kinds of validation in a single, unified step and return a consistent format for which inputs failed validation.

{
  "errors": [
    {
      "message": "Validation failed for the field [foo]",
      "extensions": {
        "validation": {
          "nonNull": ["The nonNull field is required."],
          "customScalar": ["The customScalar field must be a valid CustomScalar."],
          "required": ["The required field is required."],
          "email": ["The email field must be a valid email."]
        }
      }
    }
  ]
}

spawnia avatar Feb 27 '21 23:02 spawnia

@wimski has written up a great summary and potential solution for this issue in https://github.com/nuwave/lighthouse/issues/1830

spawnia avatar Apr 26 '21 12:04 spawnia

wimski has written up a great summary and potential solution for this issue in 1830

In all fairness, it's not so much a solution as yet, more the desired result the solution should yield. I've had a quick look in how GraphQL validation is done and I think it's going to be pretty hard to somehow hook into/extend that. But I'll definitely give it another go when I have a bit more time.

wimski avatar Apr 26 '21 12:04 wimski

Perhaps we need to swap out some of the validation rules in webonyx/graphql-php with our own implementations: https://github.com/webonyx/graphql-php/blob/bb60eb49bd77dd04b78ff7e54d0098d3f075d261/src/Validator/DocumentValidator.php#L140-L167

spawnia avatar Apr 26 '21 12:04 spawnia

Yes, the ValuesOfCorrectType to be exact. But it's hard to extend with all the private and static/self going on.

wimski avatar Apr 26 '21 12:04 wimski

You can create a PR in webonyx/graphql-php to make those into protected/static.

spawnia avatar Apr 26 '21 13:04 spawnia

Removing all the static would make things already a lot easier for sure. https://github.com/webonyx/graphql-php/issues/851

wimski avatar Apr 26 '21 13:04 wimski

3 years passed - @spawnia @wimski did you guys tried to work on it, with any results? All I want to do is to use "required" validation so it will return nice error handled by frontend.


Edit: As a "temporary" solution, I handled my case by custom error handler. Not perfect solution, but it works like it should. In case someone else comes here from google like me - here it is:

Based on https://lighthouse-php.com/master/digging-deeper/error-handling.html#registering-error-handlers

<?php

namespace App\Exceptions;

use App\Enums\ValidationError;
use GraphQL\Error\Error;
use Illuminate\Validation\ValidationException as LaravelValidationException;
use Nuwave\Lighthouse\Exceptions\ValidationException;
use Nuwave\Lighthouse\Execution\ErrorHandler;

/**
 * When we have rules with "required" it will by default it will be not returned as a validation error,
 * but instead generic GraphQL error message with complain about missing non-nullable field,
 * which is not frontend friendly.
 * There is no built-in way to handle this in Lighthouse, so we need to create a custom error handler to detect
 * such exception and convert it to a validation error.
 */
final class EmptyFieldErrorHandler implements ErrorHandler
{
    public function __invoke(?Error $error, \Closure $next): ?array
    {
        if (preg_match('/Field "(.+)" of required type "(.+)" was not provided./', $error->getMessage(), $matches)) {
            $fieldName = $matches[1];
            $error = new Error(
                $error->getMessage(),
                $error->getNodes(),
                $error->getSource(),
                $error->getPositions(),
                $error->getPath(),
                ValidationException::fromLaravel(LaravelValidationException::withMessages([
                    $fieldName => ValidationError::FIELD_NOT_PRESENT->value,
                ])),
            );
        }

        return $next($error);
    }
}

Note - ValidationError is MY own custom enum, not something from Laravel or Lighthouse. Normally for Laravel validation there should go string from validation translations. So just replace it.

also register it in lighthouse config.

chojnicki avatar May 30 '24 13:05 chojnicki

Nice idea @chojnicki, I never thought of modifying the native validation errors to make them look like Laravel validation errors. There are many other possible validation errors that would need to be added in order for this to be complete (e.g. all the scalar validations).

spawnia avatar Jun 05 '24 08:06 spawnia