OneOf late validation

[jbellenger]

Describe the bug
OneOf values seem to get validated lazily when a DataFetcher accesses its arguments. This allows OneOf types to be used incorrectly and to not raise a validation error, which can happen when using a PropertyDataFetcher or a DataFetcher that ignores its arguments.

This seems not in keeping with section 6.11 of the spec, which requires that all validation happen before execution:

only requests which pass all validation rules should be executed. If validation errors are known, they should be reported in the list of “errors” in the response and the request must fail without execution

I was expecting OneOf value validation to be implemented in graphql.validation.rules and to be handled earlier, at the same stage as rules like ArgumentsOfCorrectType. I'd be interested in implementing a rule to do this, though I wanted to first check to see if this was a deliberate decision or if there's something I'm missing.

To Reproduce
Here's a PR with a test case that uses a OneOf value with multiple keys. The test case fails because no validation error is raised.

    def "rejects invalid OneOf values before invoking data fetchers"() {
        def sdl = '''
            type Query {
                f(arg: OneOf): Boolean
            }
            
            input OneOf @oneOf { a: Int b: Int }
        '''

        def graphQLSchema = TestUtil.schema(sdl)
        def graphQL = GraphQL.newGraphQL(graphQLSchema).build()

        when:
        def er = graphQL.execute('{ f(arg : {a: 0, b: 0}) }')
        then:
        !er.errors.isEmpty()
        er.errors[0].message == "Exception while fetching data (/f) : Exactly one key must be specified for OneOf type 'OneOf'."
    }

[bbakerman]

We do the validation late execution because of variables - we have not coerced variables at the time the validation rules run.

We could do literals at validation time for things like arg : {a: 0, b: 0} say but cant we do variables arg : $var and perhaps we also cant do arg : {a: $v1, b: $v2} if $v was null (or maybe we can - because it could be the present of any value not just non null ones.

We are lazy later because why coerce values if they are never accessed. In fact its an existential question - if you never access and argument - does it matter that its values might have been invalid ? Schrödinger's argument!!

We could do something like look at the arg types and see if they are @oneOf and then coerce aggressively.

However it will costs us processing time to descend the type structure to find @oneOf types (remember they can be deeply in the input type say and if we spend that time looks to see if we should co-erce them perhaps we should just co-erce always - eg we cant make optimisation saving

[bbakerman]

Looking into the graphql-js impl - they do it in two places

validateOneOfInputObjectField in validate.ts is called during validation and examines AST elements - Later in coerceInputValue.ts they call

if (type.isOneOf) {
      const keys = Object.keys(coercedValue);
      if (keys.length !== 1) {
        onError(
          pathToArray(path),
          inputValue,
          new GraphQLError(
            `Exactly one key must be specified for OneOf type "${type.name}".`,
          ),
        );
      }

[jbellenger]

Thanks for looking into this Brad! And thanks for putting together #3577.

We do the validation late execution because of variables - we have not coerced variables at the time the validation rules run.
We are lazy later because why coerce values if they are never accessed.

I think this is true in the sense that variable values are not coerced during the validation stage, but less accurate in suggesting that coercion is always lazy and driven by resolvers.

It looks like variable coercion is done eagerly, outside of the validation framework and before execution in Execution.execute

To double check this, here's a test case showing that:

1. coercion errors are raised for variable values, even if they are never accessed
2. OneOf's are not validated if they are never accessed (a bug, I think)

    def "validation with oneof variable with invalid type and invalid structure"() {
        def sdl = '''
            input OneOf @oneOf { a: Int, b: Int }
            type Outer { inner(oneof: OneOf!): Boolean }
            type Query { outer: Outer }
        '''

        DataFetcher outer = { env -> return null }
        def graphQL = TestUtil.graphQL(sdl, [Query: [outer: outer]]).build()

        def query = '''
            query ($oneof: OneOf!) { 
              outer {
                 # these variables are never accessed by a data fetcher because 
                 # Query.outer always returns null
                 inner(oneof: $oneof)
              }
            }
        '''

        when:
        def er = graphQL.execute(
            // these variables are invalid in 2 senses:
            // 1: "asdf" cannot be coerced to an Int value
            // 2: the value of OneOf includes more than 1 key
            newExecutionInput(query).variables([oneof: [a: "asdf", b: 1]])
        )

        then:
        // Ideally, errors should include entries that both "asdf" is
        // not a valid int, and that the OneOf has an invalid structure.
        //
        // This catches the int coercion error but not the oneof structural error
        er.errors.size() == 2
    }

This is all to say that I think that the performance hit of coercing variables, even ones that are never read by a data fetcher, is already being paid.

It seems like a OneOf structural validation, even for variables, could be doable with out significant perf cost?

[bbakerman]

So just to state where we are up to - my PR will cover the case where a literal AST element has too many values (eg { a : "x" b: "y"}) so now we are talking about pure variables $oneOfVar : OneOfInputTypeSay

As for

outer {
                 # these variables are never accessed by a data fetcher because 
                 # Query.outer always returns null
                 inner(oneof: $oneof)
              }

Remember in the above if we never visit a sub child, the execution engine never traverses to the sub child.

Yeah I guess we traversing the AST tree anyway during validation and would could coerce as we do that and check variables just for certain @oneOf types say.

However one of the problems with access data in the validation phase is that of stored documents. If some one is using persisted queries or just pre parsed docs via graphql.execution.preparsed.PreparsedDocumentProvider then the validate step is not performed on subsequent invocations.

So checking in the validation phase will catch a bad @oneOf variable in the first pass but not on future passes.

So it has to happen in the execution phase. And hence we would have to eagerly go over the query tree ahead of time to catch the case you have above. Which is doable but BUT what we do today - today if a field returns null, we never visit its child fields.

Hmmm mayb e there is a way - as I type and investigate - the JS code seems to hit the oneOf checks during the "execute" stage when it gets the map of coerced variables. Hmmm we should probably put code in there as well...

Let me dig more

[bbakerman]

See #3580

This will cause oneOf validation to happen at execution time when we build the "coerced variables" map - this is just like how graphql-js does it

Note:

// Ideally, errors should include entries that both "asdf" is
// not a valid int, and that the OneOf has an invalid structure.

We still do not do this - we use execeptions during variable coercion so the first bad bit of input throws an exception and hence you still will only get 1 error - in this case the bad number / string input check first