Exclude custom scalar literals from validation

[martinbonnin]

See https://github.com/graphql/graphql-spec/pull/1118/files#r2023188399

When validating a document, the custom scalar coercion rules are not always known. In those cases, it's impossible to validate them.

For an example, with this operation, it's impossible to detect that "InvalidDate" is not a valid Date without knowledge of the coercing rules:

{
  events(after: "InvalidDate") {
    title
    startsAt
  }
}

I think (but wouldn't bet on it) that this is already the case in graphql-js for an example?

Excluding those from the validation feels like a better reflection of the current state of things albeit probably a spec change.

[benjie]

I think this is good to address, I've made a couple suggestions 👍

[benjie]

Here we're punting the problem to execution for custom scalars, specifically to CoerceArgumentValues(), and specifically to these lines in the algorithm:

5.g. Otherwise, let value be argumentValue.
[...]
5.j.ii.1 If value cannot be coerced according to the input coercion rules of argumentType, raise a field error.
5.j.ii.2 Let coercedValue be the result of coercing value according to the input coercion rules of argumentType.

In GraphQL.js this aligns with the parseLiteral call. In particular, in the case of variable references inside of literals for custom scalars such as the JSON scalar, this results in parseLiteral being called twice and yielding two different values, one that has no variables (during validation) and one that does (during execution). So I understand the desire to skip it.

However, I think there's value in performing validation of the literal if you can, even for custom scalars, so I'd encourage incorporation of an addition:

Suggested change

	- If {type} is not a custom scalar type:
	- {value} must be coercible to {type} (with the assumption that any
	{variableUsage} nested within {value} will represent a runtime value valid
	for usage in its position).
	- If {type} is not a custom scalar type:
	- {value} must be coercible to {type} (with the assumption that any
	{variableUsage} nested within {value} will represent a runtime value valid
	for usage in its position).
	- Otherwise, if the implementation includes execution and {value} contains no
	variable usages then it is recommended to assert {value} is coercible to
	{type}.

[martinbonnin]

I understand the intent there but I don't think any implementation is doing this at the moment?

I like validation to be a pure function of typeSystemDefinitions + executableDefinition, i.e. it operates purely on build time artifacts. Recommending something else opens a new door which I'd rather keep closed, especially if we have no proof that people need this actively.

[benjie]

GraphQL.js currently validates custom scalars via parseLiteral via validation if I'm not mistaken (this is evidenced by parseLiteral being called twice), so I'd expect any implementation based on GraphQL.js to do this.

[benjie]

Here's proof using GraphQL.js that shows GraphQL.js does this. Run it with node script.mjs and you will see the "string" value is not accepted for the custom scalar, but the true value is. Custom scalars are just scalars in GraphQL.js.

script.mjs

// @ts-check
import {
  GraphQLSchema,
  GraphQLObjectType,
  GraphQLScalarType,
  validate,
  Kind,
  validateSchema,
  parse,
} from "graphql";

const CustomScalar = new GraphQLScalarType({
  name: "CustomScalar",
  parseLiteral(v) {
    if (v.kind === Kind.BOOLEAN) {
      return v.value;
    } else {
      throw new Error("Invalid");
    }
  },
  parseValue(v) {
    if (typeof v === "boolean") {
      return v;
    } else {
      throw new Error("Invalid");
    }
  },
  serialize(v) {
    return v;
  },
});
const Query = new GraphQLObjectType({
  name: "Query",
  fields: {
    test: {
      type: CustomScalar,
      args: {
        custom: {
          type: CustomScalar,
        },
      },
      resolve(_, { custom }) {
        return custom;
      },
    },
  },
});
const schema = new GraphQLSchema({
  query: Query,
});
const schemaErrors = validateSchema(schema);
if (schemaErrors.length > 0) {
  console.dir(schemaErrors);
  throw new Error("Invalid schema");
}

{
  const errors = validate(schema, parse(`{test(custom:true)}`));
  if (errors.length > 0) {
    console.dir(errors);
    throw new Error("true failed");
  }
}

{
  const errors = validate(schema, parse(`{test(custom:"string")}`));
  if (errors.length > 0) {
    console.dir(errors);
    throw new Error("string failed");
  }
}

$ node test.mjs 
[
  Error: Invalid
      at GraphQLScalarType.parseLiteral (file:///home/benjie/Dev/DELETEME/custom-scalar/test.mjs:18:13)
      at isValidValueNode (/home/benjie/Dev/DELETEME/custom-scalar/node_modules/graphql/validation/rules/ValuesOfCorrectTypeRule.js:177:30)
      at Object.StringValue (/home/benjie/Dev/DELETEME/custom-scalar/node_modules/graphql/validation/rules/ValuesOfCorrectTypeRule.js:141:28)
      at Object.enter (/home/benjie/Dev/DELETEME/custom-scalar/node_modules/graphql/language/visitor.js:301:32)
      at Object.enter (/home/benjie/Dev/DELETEME/custom-scalar/node_modules/graphql/utilities/TypeInfo.js:391:27)
      at visit (/home/benjie/Dev/DELETEME/custom-scalar/node_modules/graphql/language/visitor.js:197:21)
      at validate (/home/benjie/Dev/DELETEME/custom-scalar/node_modules/graphql/validation/validate.js:91:24)
      at file:///home/benjie/Dev/DELETEME/custom-scalar/test.mjs:66:18
      at ModuleJob.run (node:internal/modules/esm/module_job:234:25)
      at async ModuleLoader.import (node:internal/modules/esm/loader:473:24) {
    message: 'Expected value of type "CustomScalar", found "string"; Invalid',
    path: undefined,
    locations: [ [Object] ],
    extensions: [Object: null prototype] {}
  }
]

    message: 'Expected value of type "CustomScalar", found "string"; Invalid',

[benjie]

Suggested change

	Note: Custom scalar coercion rules are not always available when validating a
	document and custom scalar literal values are excluded from this validation. If
	a custom scalar literal value cannot be coerced, it will raise an execution
	error.
	Note: Custom scalar coercion rules are not always available when validating a
	document and custom scalar literal values are optional in this validation. If
	a custom scalar literal value cannot be coerced, it will raise an error during
	execution.

[martinbonnin]

I'm closing in favor of #1156 as they are really related