Cost validator throws duplicate error for an invalid input

[rafalp]

Discussed in #934

^{Originally posted by jayeshv September 30, 2022}
Consider the following app

from ariadne import QueryType, ScalarType, gql, make_executable_schema
from ariadne.asgi import GraphQL
from ariadne.validation import cost_directive, cost_validator

type_defs = gql("""
    scalar MyScalar

    type Query {
      test(myInput: MyScalar!): String
    }
""")

query = QueryType()
my_scalar = ScalarType("MyScalar")

@my_scalar.value_parser
def validate_myscalar(value):
    if value == "a":
        raise ValueError("Invalid myscalar")
    return value


@query.field("test")
def resolve_hello(_, info, **kw):
    return "hello"


schema = make_executable_schema([type_defs, cost_directive], query, my_scalar)
app = GraphQL(schema, debug=False, validation_rules=[cost_validator(maximum_cost=5)])

for the following query, it generates two error objects.

query test {
  test(myInput: "a") 
}

{
  "error": {
    "errors": [
      {
        "message": "Argument 'myInput' has invalid value \"a\".\n\nGraphQL request:2:17\n1 | query test {\n2 |   test(myInput: \"a\")\n  |                 ^\n3 | }"
      },
      {
        "message": "Expected value of type 'MyScalar!', found \"a\"; Invalid myscalar",
        "locations": [
          {
            "line": 2,
            "column": 17
          }
        ]
      }
    ]
  }
}

Isn't the first error created by the cost_validator a redundant one? This duplicate error happens only with the extra validator.
ariadne version 0.16.1

[rafalp]

@jayeshv Thanks for reporting this. The reason there are two errors here is because there are two validators tat query fails against:

• GraphQL core's query validator that validates query against schema
• Query cost validator that expected value of one type but got another

I'm thinking that proper fix here is to only run custom validators like price check only after standard validation has passed and we know there's nothing specially funky with the query.

[jayeshv]

I'm thinking that proper fix here is to only run custom validators like price check only after standard validation has passed and we know there's nothing specially funky with the query.

I too think that is the right fix. I made a rough fix here without knowing the entire code flow.
#936
What I did was to avoid reporting errors other than the cost one.
With this fix, it is throwing both the validation and the cost errors.

I like this behavior, but I think what you are suggesting is to remove catching that exception.

[rafalp]

I'll experiment with this next week to see how we can handle this issue better. I'm not feeling comfortable about eating errors that may be caused by potentially serious issues elsewhere in validator.

[rafalp]

Apologies for the delay, but I've finally investigated this. My initial idea was to do two separate validity checks, one for spec compliance checks (those test if graphql query is valid), and other for custom validators (but only if spec checks passed), but this means running AST visitor twice, and thats going to be costful for larger queries.

I am considering three approaches:

• Don't do anything because the case here happens when there's invalid input, and its on the client to avoid this and not Ariadne to be nice.
• Don't report errors from validators for things that are validated by built-in validator set (like PR opened for this issue).
• Implement custom validate_query function that passes different report_error callback to built-in validators and different one to custom validators that only reports errors if errors list has no spec compliance errors.

I like option 1 the most and 2 the least. I'll see how much work it would be to implement 3.

[rafalp]

Alternative solutions:

• Make errors raised by cost validator same as ones raised by spec validators, then dedupe validation errors.
• Make max_errors customizable on GraphQL apps so you can limit number of validation errors to say, one.

[rafalp]

Punting this from 0.17, I don't feel this is big issue that should prevent the release.

[rafalp]

Unless this is shown to be a terrible world-ending issue, I am closing this with "current approach is better than proposed alternatives" resolution.

If somebody has better idea, I'll be happy to reconsider.