Clarify validation of custom scalar literals

[martinbonnin]

Fixes:

• Using variables in scalars #1011

Requires:

• Clarify 'Values of Correct Type' rule relates to literals #1118

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	4a04fff
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/67ee929d924a5b000839f967
😎 Deploy Preview	https://deploy-preview-1156--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[benjie]

This allows for things like the JSON scalar which GraphQL.js already supports 👍 We might need a little more explanation WHY this is the case, and also a reference to where this value is consumed/coerced/validated in the custom scalars section (in GraphQL.js it’s through the custom scalars parseLiteral method IIRC)

[martinbonnin]

Added a note here, let me know what you think.

[benjie]

Here's an alternative take that allows implementations to validate variables inside of custom scalars if they wish to implement that functionality.

[benjie]

Rather than enforcing that implementations MUST NOT validate variables within custom scalars, as this edit does, let's instead give them the choice:

Suggested change

	- If {variableUsage} is nested within a custom scalar literal value, return
	{true}.
	- Let {variableType} be the expected type of {variableDefinition}.
	- Let {locationType} be the expected type of the {Argument}, {ObjectField}, or
	{ListValue} entry where {variableUsage} is located.
	- Let {variableType} be the expected type of {variableDefinition}.
	- Let {locationType} be the expected type of the {Argument}, {ObjectField}, or
	{ListValue} entry where {variableUsage} is located.
	- If {locationType} cannot be determined, for example when {variableUsage} is
	nested within a custom scalar literal value, return {true}.

[martinbonnin]

It's technically the same? If variableUsage is nested within a literal value, then {locationType} cannot be determined and we return true ? At least in the "graphql type" acceptation.

The custom scalar may expect some kind of literal but it's not a GraphQL type per-se. I think if we want to allow validating variables within custom scalars, we'll need the coercing (see also the comment below)?

[martinbonnin]

Carried over from @benjie comment in https://github.com/graphql/graphql-spec/pull/1157/files#r2026799223

---

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}.

[benjie]

Here's a demo showing that GraphQL validates custom scalars at validation time. To run the demo, save it to a file script.mjs, install graphql and then run it with node script.mjs; the outcome is that true is accepted but "string" is rejected.

I think it's important that we don't lose this in general, even if we might not be able to require intermediate GraphQL services such as GraphiQL or a Federation Gateway to perform this same validation.

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',

[martinbonnin]

Right 👍 Excellent example.

2 follow up comments:

• Should we rule out variables? As the rest of this PR shows, variables in literals are always considered valid so a custom scalar coercing could very well decide to ignore them?
• Proposal to replace "if the implementation includes execution" by "if the coercion rules are known", which is more generic. Typically, the Apollo Kotlin compiler doesn't include execution but we may provide an API for our users to provide coercings if they want more build-time validation.

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 coercion rules are known then it is recommended to
	assert {value} is coercible to {type}.

[martinbonnin]

Carried over from @benjie comment in https://github.com/graphql/graphql-spec/pull/1157/files#r2026803566

---

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.