Fix parsing of default values in `buildASTSchema`, `extendSchema` and `buildClientSchema` #903

IvanGoncharov · 2017-06-12T08:32:13Z

In the current implementation buildASTSchema, extendSchema and buildClientSchema register following stubs as parseLiteral and parseValue:

    return new GraphQLScalarType({
      // ...

      // Note: validation calls the parse functions to determine if a
      // literal value is correct. Returning null would cause use of custom
      // scalars to always fail validation. Returning false causes them to
      // always pass validation.
      parseValue: () => false,
      parseLiteral: () => false,
    });

Because of that, graphql-js can't correctly parse default values of custom scalar types:

scalar CustomScalar

type Query {
  someField(someArg: CustomScalar = 5): String
}

So to clarify. You can override those functions after the schema is built, but the default value from IDL will be lost.

To illustrate this I've added a test case in the first commit of this PR and here is the result:

This PR solves this problem by providing reasonable defaults for parseLiteral and parseValue.

leebyron

Some questions inline

leebyron · 2017-06-16T00:06:31Z

src/utilities/scalarValueFromAST.js

+ * Create a JavaScript value from a GraphQL languate AST representation
+ * of Scalar.
+ */
+export function scalarValueFromAST(astValue: ValueNode): mixed {


This looks very similar to valueFromAST - can that be used instead?

If this function is for scalars, then it should likely not support LIST or OBJECT, correct?

This looks very similar to valueFromAST - can that be used instead?

valueFromAST builds value based on the type definition it accepts as the second argument.
In a case of a scalar, there is no such type definition.

If this function is for scalars, then it should likely not support LIST or OBJECT, correct?

AFAIK, the spec doesn't explicitly forbid complex scalars as LIST or OBJECT. Moreover, it's a widespread practice to create such scalars. More details in this PR:
graphql/graphql-spec#325

leebyron · 2017-06-16T00:08:58Z

src/type/definition.js

@@ -333,7 +334,7 @@ export class GraphQLScalarType {
  // Parses an externally provided value to use as an input.
  parseValue(value: mixed): mixed {
    const parser = this._scalarConfig.parseValue;
-    return parser && !isNullish(value) ? parser(value) : undefined;
+    return parser && !isNullish(value) ? parser(value) : value;


If a value parses as nullish, shouldn't it return clearly as undefined to not be confused with a true null value?

leebyron · 2017-06-16T00:09:55Z

src/utilities/index.js

@@ -58,6 +58,10 @@ export { valueFromAST } from './valueFromAST';
 // Create a GraphQL language AST from a JavaScript value.
 export { astFromValue } from './astFromValue';

+// Create a JavaScript value from a GraphQL languate AST representation


leebyron · 2017-12-01T20:54:58Z

Thanks for making this happen, Ivan. Great improvement

Closes #903 commit 0062abf Author: Lee Byron <[email protected]> Date: Fri Dec 1 12:49:38 2017 -0800 Amends this PR with a few additions: * Generalizes building a value from an AST, since "scalar" could be misleading, and supporting variable values within custom scalar literals can be valuable. * Replaces isNullish with isInvalid since `null` is a meaningful value as a result of literal parsing. * Exports this new utility from the top level commit 0ca6cf0 Merge: 37717b8 78e69d6 Author: Lee Byron <[email protected]> Date: Fri Dec 1 11:57:55 2017 -0800 Merge branch 'astValueToData' of https://github.com/APIs-guru/graphql-js into APIs-guru-astValueToData commit 78e69d6 Author: Ivan Goncharov <[email protected]> Date: Sun Jun 11 01:07:47 2017 +0300 Fix 'serialize' stub used in buildASTSchema commit a0688c6 Author: Ivan Goncharov <[email protected]> Date: Fri Jun 9 23:24:55 2017 +0300 Provide reasonable default version of 'parseLiteral' commit 313d66e Author: Ivan Goncharov <[email protected]> Date: Fri Jun 9 23:22:49 2017 +0300 Add test for building client schema with default value on custom scalar

facebook-github-bot added the CLA Signed label Jun 12, 2017

leebyron requested changes Jun 16, 2017

View reviewed changes

IvanGoncharov force-pushed the astValueToData branch 2 times, most recently from 8534882 to f9bcc32 Compare June 21, 2017 18:35

IvanGoncharov added 3 commits July 6, 2017 14:00

Add test for building client schema with default value on custom scalar

313d66e

Provide reasonable default version of 'parseLiteral'

a0688c6

Fix 'serialize' stub used in buildASTSchema

78e69d6

IvanGoncharov force-pushed the astValueToData branch from f9bcc32 to 78e69d6 Compare July 6, 2017 11:02

leebyron closed this in 1b711d2 Dec 1, 2017

This was referenced Feb 9, 2018

[0.12] Fix parsing of default values in build-schema webonyx/graphql-php#245

Closed

Port 0.12.3 changes from graphql-js webonyx/graphql-php#248

Merged

IvanGoncharov deleted the astValueToData branch March 9, 2018 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parsing of default values in `buildASTSchema`, `extendSchema` and `buildClientSchema` #903

Fix parsing of default values in `buildASTSchema`, `extendSchema` and `buildClientSchema` #903

IvanGoncharov commented Jun 12, 2017

leebyron left a comment

leebyron Jun 16, 2017

leebyron Jun 16, 2017

IvanGoncharov Jun 19, 2017

leebyron Jun 16, 2017

IvanGoncharov Jun 21, 2017

leebyron Jun 16, 2017

IvanGoncharov Jun 21, 2017

leebyron commented Dec 1, 2017

Fix parsing of default values in buildASTSchema, extendSchema and buildClientSchema #903

Fix parsing of default values in buildASTSchema, extendSchema and buildClientSchema #903

Conversation

IvanGoncharov commented Jun 12, 2017

leebyron left a comment

Choose a reason for hiding this comment

leebyron Jun 16, 2017

Choose a reason for hiding this comment

leebyron Jun 16, 2017

Choose a reason for hiding this comment

IvanGoncharov Jun 19, 2017

Choose a reason for hiding this comment

leebyron Jun 16, 2017

Choose a reason for hiding this comment

IvanGoncharov Jun 21, 2017

Choose a reason for hiding this comment

leebyron Jun 16, 2017

Choose a reason for hiding this comment

IvanGoncharov Jun 21, 2017

Choose a reason for hiding this comment

leebyron commented Dec 1, 2017

Fix parsing of default values in `buildASTSchema`, `extendSchema` and `buildClientSchema` #903

Fix parsing of default values in `buildASTSchema`, `extendSchema` and `buildClientSchema` #903