Skip to content

Clarify validation of custom scalar literals #1156

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Clarify validation of custom scalar literals #1156

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8efb84c
Exclude custom scalar literals from validation
martinbonnin Apr 2, 2025
f34e98b
Clarify that variables are always allowed when part of a custom scala…
martinbonnin Apr 2, 2025
bf42aa1
add note
martinbonnin Apr 3, 2025
e324a00
Update spec/Section 5 -- Validation.md
martinbonnin Apr 17, 2025
0741216
formatting
martinbonnin Apr 17, 2025
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
27 changes: 24 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
Contributor Author

@martinbonnin martinbonnin Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


**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 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 Expand Up @@ -1884,6 +1890,8 @@ variable.

IsVariableUsageAllowed(variableDefinition, variableUsage):

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)?

Expand Down Expand Up @@ -1974,6 +1982,19 @@ query nonNullListToList($nonNullBooleanList: [Boolean]!) {
}
```

When using variables nested within custom scalars literals, the expected type is
unknown, and variable usages are always allowed. The actual value is coerced at
runtime using the custom scalar coercion rules. In the following case, the
`user` argument expects a `JSON` custom scalar, and it is valid for it to
reference variables:

```graphql example
mutation updateUserName($name: String!) {
# This usage of the $name variable is valid
updateUser(user: { name: $name })
}
```

However, a nullable list cannot be passed to a non-null list:

```graphql counter-example
Expand Down