nuwave / lighthouse

A framework for serving GraphQL from Laravel
https://lighthouse-php.com
MIT License
3.36k stars 437 forks source link

Unify argument validation #1751

Open spawnia opened 3 years ago

spawnia commented 3 years ago

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:

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 commented 3 years ago

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

wimski commented 3 years ago

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.

spawnia commented 3 years ago

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

wimski commented 3 years ago

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

spawnia commented 3 years ago

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

wimski commented 3 years ago

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

chojnicki commented 5 months ago

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.

spawnia commented 5 months ago

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).