ValuesOfCorrectType is not correctly implementing the spec

[attilah]

As the spec states, this rule should validate that a given document is conforming to the schema, by verifying the type of fields for any given type.

When the schema defines a List type with non-null items and an Object is passed in the document, validation is not failing.

Schema:

type Post {
  id: ID!
  title: String
  content: String
}

input UpdatePostInput {
  id: ID!
  title: String
  content: String
}

input PostConditionInput {
  title: StringConditionInput
  content: StringConditionInput
  and: [PostConditionInput!]
}

input StringConditionInput {
  contains: String
}

type Mutation {
  updatePost(input: UpdatePostInput!, conditions: PostConditionInput): Post
}

type Query {
  listPosts : [Post!]!
}

Document:

mutation InvalidUpdate {
	updatePost(
        input: {
            id: "P1"
            title: "New Title"
            content: "New Content"
        }
        conditions: {
            and: {
                title: { contains: "ABC" }
                content: { contains: "DEF" }
            }
        }
    ) {
        id
        title
        content
    }
}

mutation ValidUpdate {
	updatePost(
        input: {
            id: "P1"
            title: "New Title"
            content: "New Content"
        }
        conditions: {
            and: [
                { title: { contains: "ABC" } }
                { content: { contains: "DEF" } }
            ]
        }
    ) {
        id
        title
        content
    }
}

Notice that and is an object instead of an array in InvalidUpdate mutation, which would be valid, if a single object would be coerced into a single element array after parse and the object would be validated against the type of the list item.

The document above is passing validation, with the following code, using the latest 14.x version of the library.

import * as fs from 'fs';
import { buildSchema, parse } from 'graphql';
import { GraphQLSchema } from 'graphql/type';
import { validate, specifiedRules } from 'graphql/validation';

const schema = fs.readFileSync('schema.graphql').toString();
const mutation = fs.readFileSync('mutation.graphql').toString();

const rules = specifiedRules;

const schemaObject: GraphQLSchema = buildSchema(schema);
const document = parse(mutation);

const validataionResult = validate(schemaObject, document, rules);

console.log(JSON.stringify(validataionResult, null, 2));

[IvanGoncharov]

When the schema defines a List type with non-null items and an Object is passed in the document, validation is not failing.

@attilah Yes, it's expected behavior:

https://graphql.github.io/graphql-spec/draft/#sel-FAHjBHFCAACE_Gh7d

If the value passed as an input to a list type is not a list and not the null value, then the result of input coercion is a list of size one, where the single item value is the result of input coercion for the list’s item type on the provided value (note this may apply recursively for nested lists).

Does this quote answer your question?

[attilah]

@IvanGoncharov so validation is "assuming" that the inputs will be coerced at the time of processing, that's the reason why it is passing the validation?

A plain parse call will not do this coercion as that method is unaware of the schema that given document is parsed against. How can one parse a document against a given schema with automatic input type coercion?

How will this snippet:

and: {
  title: { contains: "ABC" }
  content: { contains: "DEF" }
}

become this:

and: [
  { title: { contains: "ABC" } }
  { content: { contains: "DEF" } }
]

in the parsed document AST - since it is a multi-element array?

UPDATE

Manually, I was able to use the coerceInputValue function and it produced the expected results, but it's really "manual":

const condition = {
    and: {
        title: { contains: "ABC" },
        content: { contains: "DEF" }
    }
};

const inputType = <GraphQLInputType>schemaObject.getType('PostConditionInput');

const coercedValue = coerceInputValue(condition, inputType);

Another place where I've issue and it is the same is: List type directive arguments. Given the following schema:

directive @comments(values: [Comment!]!) on OBJECT | FIELD_DEFINITION

input Comment {
  text: String!
}

This document passes validation:

type ABC @comments(values: { text: "Hello" }) {
    id: ID!
}

But since here the directive argument is a List type, I cannot use the coerceInputValue function straight ahead on the Comment input type, so I found no way to even manually to the coercion in this case.

[IvanGoncharov]

so validation is "assuming" that the inputs will be coerced at the time of processing, that's the reason why it is passing the validation?

@attilah Yes
https://graphql.github.io/graphql-spec/draft/#sel-HALXDHDABAB1EzpY

Literal values must be compatible with the type expected in the position they are found as per the coercion rules defined in the Type System chapter.

A plain parse call will not do this coercion as that method is unaware of the schema that given document is parsed against. How can one parse a document against a given schema with automatic input type coercion?

Parsing has no information about the type system so it can't do coercion.
Coercion happening here: https://graphql.github.io/graphql-spec/draft/#sec-Coercing-Field-Arguments

Before we get too deep into technical details can you please explain your use case?

[attilah]

@IvanGoncharov, sure thing:

Use Case # 1
In Amplify CLI we're generating Condition input types as in the example above, that can be used as additional constraints on a mutation operation. We've the same as Filters on Queries today.
The problem is that customers when invoking these operations with a Filter in place the GraphQL operation will be valid if they pass in an object instead of an array for OR or AND, but the results will be incorrect because of the fact that an Object was passed in instead of an array that the schema defines.

Use Case # 2
With the @auth directive when we validate the customer's schema we are not getting any error if an Object was passed in for AuthRule and because of this we'll not generate any rules, and the customer is not getting any errors.

I just wanted to simplify the problem with my repro's above instead of diving deep into the actual operations we're doing.

[IvanGoncharov]

I just wanted to simplify the problem with my repro's above instead of diving deep into the actual operations we're doing.

@attilah Thanks a lot for simplifying it, it saves a lot of time on bug reports 👍

It looks like getArgumentValues can solve # 1

graphql-js/src/execution/values.js

Lines 151 to 165 in 0eaa5e6

	/**
	* Prepares an object map of argument values given a list of argument
	* definitions and list of argument AST nodes.
	*
	* Note: The returned value is a plain Object with a prototype, since it is
	* exposed to user code. Care should be taken to not pull values from the
	* Object prototype.
	*
	* @internal
	*/
	export function getArgumentValues(
	def: GraphQLField<mixed, mixed> | GraphQLDirective,
	node: FieldNode | DirectiveNode,
	variableValues?: ?ObjMap<mixed>,
	): { [argument: string]: mixed, ... } {

Note you can use visitWithTypeInfo to get field definition for every field node.

As for # 2 getDirectiveValues seems like a good match:

graphql-js/src/execution/values.js

Lines 233 to 248 in 0eaa5e6

	/**
	* Prepares an object map of argument values given a directive definition
	* and a AST node which may contain directives. Optionally also accepts a map
	* of variable values.
	*
	* If the directive does not exist on the node, returns undefined.
	*
	* Note: The returned value is a plain Object with a prototype, since it is
	* exposed to user code. Care should be taken to not pull values from the
	* Object prototype.
	*/
	export function getDirectiveValues(
	directiveDef: GraphQLDirective,
	node: { +directives?: $ReadOnlyArray<DirectiveNode>, ... },
	variableValues?: ?ObjMap<mixed>,
	): void | { [argument: string]: mixed, ... } {

BTW. you can also find me on GraphQL Slack as @i1g

[IvanGoncharov]

@attilah Can I close this issue or you still have some questions?

[attilah]

Sure, yes, I will ping you if I encounter any further issue when I get back to this.