Structured errors for argument validation

[spawnia]

The following test code:

<?php

declare(strict_types=1);

namespace GraphQL\Tests;

use GraphQL\GraphQL;
use GraphQL\Type\Definition\ObjectType;
use GraphQL\Type\Definition\Type;
use GraphQL\Type\Schema;
use PHPUnit\Framework\TestCase;

class MultiValidationErrorTest extends TestCase
{
    public function testReturnsMultipleValidationErrorsForArguments(): void
    {
        $query = '
        {
          foo(bar: "asdf")
        }
        ';
        $result = GraphQL::executeQuery(self::build(), $query)->toArray();
        
        var_dump(json_encode($result));
    }
    
    public static function build(): Schema
    {
        $queryType = new ObjectType([
            'name' => 'Query',
            'fields' => [
                'foo' => [
                    'type' => Type::int(),
                    'args' => [
                        'bar' => [
                            'type' => Type::int(),
                        ],
                        'baz' => [
                            'type' => Type::nonNull(Type::int()),
                        ],
                    ],
                    'resolve' => function () {
                    },
                ],
            ],
        ]);
        
        return new Schema(['query' => $queryType]);
    }
}

Produces the following result:

{  
  "errors":[  
    {  
      "message":"Internal server error",
      "category":"internal",
      "locations":[  
        {  
          "line":3,
          "column":20
        }
      ]
    },
    {  
      "message":"Field \"foo\" argument \"baz\" of type \"Int!\" is required but not provided.",
      "category":"graphql",
      "locations":[  
        {  
          "line":3,
          "column":11
        }
      ]
    }
  ]
}

I think there are a few issues with this.

The error for the first field where a wrong input is given is treated as an internal server whereas the second error is displayed to the User. This has to do with #337 in a way. I tried fixing this behaviour by making the parseLiteral() function throw a ValidationError that implements ClientAware. However that does not make it so the error is displayed to the User.

The other issue is related to the structure of the Exception. The information about which field and which argument contain an error are buried inside of the message. While this is quite readable for a human, it is not a useful format for working with the errors programmatically. I would like a format that provides me with a structured way of accessing the validation errors, for example:

{  
  "errors":[  
    {  
      "message":"Argument validation for field \"foo\" failed.",
      "category":"validation",
      "locations":[  
        {  
          "line":3,
          "column":11
        },
	    {  
          "line":3,
          "column":20
        }
      ],
	  "path": [
		"foo"
	  ],
	  "extensions": {
		"validation": {
		  "bar": "Expected type Int, found \"sdf\"",
		  "baz": "Argument is required"
		}
	  }
    }
  ]
}

[vladar]

I guess these are two different issues?

1. The first error should be a graphql error, not an internal one
2. Changing the shape of the validation errors

As for 1 - it is clearly a bug, we should address it.
As for 2 - I would say that graphql error is not for the end user, but for the developer. It is supposed that the client developer does this kind of validation in the client code and that graphql errors are not displayed to the end-user. They are more like 400 errors in HTTP.

[robsontenorio]

@vladar

About "2", i agree with you: errors should be for developers, not for endusers.

But in this case, as well described by @spawnia, the issue is about how to better structure error messages, in order developers can friendly handle it. Because right now is hard to catch errors by "field keys". We just have a "big error message" (with multiple issues mixed) that should be parsed (maybe with some regex) in order to get a definition about each field error individually.

And I'd venture to say, it's safe to handle this in the backend instead of relying on frontend validations.

[spawnia]

Validation always has to be done on the server. Client-side is an added bonus, that provides instant feedback, but the server always has the final word. There are also some constraints that can only be validated on the server, for example the uniqueness of a field.

I do agree that the errors are meant for the developer, and yeah, i would not just dump the error JSON to the screen. What i am trying to do is to consume the validation errors provided by the server and map them back to the original form. The current structure of the error messages makes that quite hard and i think can be improved upon.

What do you think about the format i proposed?

[vladar]

Validation always has to be done on the server.

It is. But there are two layers of validation. First one is a formal validation of the contract between the client and the server schema.

Originally it was supposed that a valid client always sends formally valid queries. Because it knows the schema (either ad-hoc or via introspection) and can enforce formal query and variables validity.

So when an error occurs at this level - it is a sign that the client logic is flawed. We do report such errors under graphql category, but those errors are targeted towards client developers (like "your app is broken!") and were not meant for runtime consumption.

Then when a query is formally valid - other layer of validation (app-related) occurs during an execution step.

What i am trying to do is to consume the validation errors provided by the server and map them back to the original form. The current structure of the error messages makes that quite hard and i think can be improved upon.

Originally those errors were not meant for runtime validation because you could have figured out these errors before the query was sent (i.e. via introspection or ad-hoc validation). But I do understand that it might be convenient to just rely on the server errors for pragmatic app development.

What do you think about the format i proposed?

I don't think it will work. At least not in this form. First problem is that locations is a stack, not a list. It is meant to track an error within a chain of fragments. Add couple named fragments and spreads in your query with an error deep inside a fragment with multiple references and you'll see what I mean.

Second problem is that path is execution-related. It points to an entry in resulting data, not in the query. So path like ['foo', 0, 'bar'] would point to foo[0].bar in the response data. But errors we discuss here are emited before execution and point to input path, not output (i.e. ['foo', 'bar']), so they should be named differently (maybe queryPath)

And most importantly your format targets a different use-case. Current errors serve a specific goal of debugging invalid queries. Your format serves a goal of runtime validation. Those are two different goals.

So what could be done?

I see a couple options:

1. Implement a custom validation rule which would do all validations for a field argument and store user-friendly messages and queryPath in extensions. Technically it could collect all existing errors in ValidationContext of field or argument (on leave) and group them in a single error. This rule could be optional and would be backward compatible. PR for such rule is welcome!

2. Change the client-side code to use introspection for formal validation instead.

Let me know what you think.

[spawnia]

Hey @vladar, thank you for your detailed and well thought out response.

As you said, there is a notable difference between formal validation and app-related validation. However, there is value in having a unified response format for both. This becomes especially useful when using Scalar types for validation.

First problem is that locations is a stack, not a list.

Can you point me to a resource where i can read more about this? Are there examples in the tests to look at? The spec does say that locations is in fact a list.

Second problem is that path is execution-related.

The format i propose does respect that, the path only contains the field. Of course, the arguments are not part of the response, but they are only mentioned in the extensions key.

And most importantly your format targets a different use-case.

I do agree that the use-case is different. Do you think that it is less useful for debugging queries?

If we do implement this with a custom validation rule, would that mean we have to disable the original validation rule?

[vladar]

Can you point me to a resource where i can read more about this? Are there examples in the tests to look at? The spec does say that locations is in fact a list.

You are right. In general case it is a list, so it would work. Reference implementation mostly uses it as a stack to track an error deep inside fragment chain, but there are also situations when it is used as a regular list.

The format i propose does respect that, the path only contains the field

What I am saying is that an error in the same field may produce different paths for execution error and for static validation error. For example we have a query:

{
  a {
    b {
      c(d: "e")
    }
  }
}

Then if an error happens in the field c during static validation the path would be ["a", "b", "c"], but if an error happens during execution the path for the same field could be ["a", 0, "b", 3, "c"] (if a and b are List types)

I do agree that the use-case is different. Do you think that it is less useful for debugging queries?

At least it won't work well with GraphiQL and other general-purpose tools (since specific messages are in extensions).

If we do implement this with a custom validation rule, would that mean we have to disable the original validation rule?

Given what I see in your description, this would be an aggregate rule. We could probably directly inject other rules it aggretates. Then it is up to the app developer to choose which rules to use. But we can create several rule presets to reduce the hassle.

[vladar]

Also check out this thread as it is pretty much related

[JetJsF]

It will be really helpful to throw any other exception instead of base \Exception at parseLiteral()
Can it be done?

[vladar]

@JetJsF I created a separate issue to track this #570 Feel free to submit a PR for it if you have a chance

[vladar]

Closing this one as it seems to be stale. If someone decides to play with aggregated validation rule - feel free to open a PR or a new follow up issue