Skip to content

Exclude custom scalar literals from validation #1157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Exclude custom scalar literals from validation #1157

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions spec/Section 5 -- Validation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1306,9 +1306,10 @@ fragment resourceFragment on Resource {

- For each literal Input Value {value} in the document:
- Let {type} be the type expected in the position {value} is found.
- {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).
Comment on lines +1309 to +1312
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


**Explanatory Text**

Expand All @@ -1324,6 +1325,11 @@ algorithm ensures runtime values for variables coerce correctly. Therefore, for
the purposes of the "coercible" assertion in this validation rule, we can assume
the runtime value of each {variableUsage} is valid for usage in its position.

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.
Comment on lines +1328 to +1331
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


The type expected in a position includes the type defined by the argument a
value is provided for, the type defined by an input object field a value is
provided for, and the type of a variable definition a default value is provided
Expand Down