RFC: Default value validation & coercion

[leebyron]

Depends on #3086
Implements graphql/graphql-spec#793

• BREAKING: Changes default values from being represented as an assumed-coerced "internal input value" to a pre-coerced "external input value" (See chart below).

  This allows programmatically provided default values to be represented in the same domain as values sent to the service via variable values, and allows it to have well defined methods for both transforming into a printed GraphQL literal string for introspection / schema printing (via valueToLiteral()) or coercing into an "internal input value" for use at runtime (via coerceInputValue())

  To support this change in value type, this PR adds two related behavioral changes:

  • Adds coercion of default values from external to internal at runtime (within coerceInputValue())
  • Removes astFromValue(), replacing it with valueToLiteral() for use in introspection and schema printing. astFromValue() performed unsafe "uncoercion" to convert an "Internal input value" directly to a "GraphQL Literal AST", where valueToLiteral() performs a well defined transform from "External input value" to "GraphQL Literal AST".

• Adds validation of default values during schema validation.

  Since assumed-coerced "internal" values may not pass "external" validation (for example, Enum values), an additional validation error has been included which attempts to detect this case and report a strategy for resolution.

---

This is the final PR in a stack which builds up to supporting this RFC behavior change:

• Add coerceInputLiteral() Add coerceInputLiteral() #3092
• Preserve defaultValue literals Preserve defaultValue literals #3074
• Preserve sources of variable values Preserve sources of variable values #3077
• Add valueToLiteral() Add valueToLiteral() #3065
• Input Value Validation Input Value Validation #3086
• RFC: Default value validation & coercion RFC: Default value validation & coercion #3049

---

Here's a broad overview of the intended end state:

[leebyron]

This actually caused a bunch of failures in the existing suite, so that's promising!

[leebyron]

This really just adds a condition, implementing this comment suggestion

I definitely need to write tests to ensure this is behaving as expected

[leebyron]

A key example of this is Enums, where the external (string) and internal (any) representations differ. Custom scalars can have the same property

[andimarek]

I don't this this is correct:
serialize is for coercing the values of resolved values (the return values from Resolver). It takes the result of a resolver and produces an result value ("result coercion" is the more correct term here).
parseValue is for coercing input runtime values (programming language specific values). The two use cases for that kind of coercing are variables values and default values. A GraphQLInputField contains a defaultValue which needs to be coerced via parseValue.

[benjie]

That would be a change in behaviour for graphql.js, but I think that change is desired - specifying that the default for an enum is "MY_CUSTOM_SORTER" rather than require('sorters').myCustomSorter (which may happen to be a function) makes sense to me - it's more in line with specifying the value via JSON within a variable.

I think Lee's solution may work to maintain the current behaviour - if you return an enum value (which may happen to be a function) from a resolver then that gets converted via serialize; similarly if you have the exact same enum value (i.e. a function) within defaultValue then using serialize to turn it back into the GraphQL enum makes sense to me.

[andimarek]

Let me expand a bit, why I think this can't work in general.

The possible output values of serialize can be completely disjunct from the input values of parseValue. They simply have an orthogonal purpose. parseValue will represent an "internal input value" and should accept a wide range of different inputs and return a good representation of the value to be received as input for Resolver.
The output of serialize on the other hand is the final result of a field.
In practice most Scalars probably accepts serialize values as input for parseValue, but this is not required at all.
I could define a Currency scalar for example which has a special CurrencyOutput as the result of serialize which is not accepted as input for parseValue.

Also there is nothing that actually forces you to implement to serialize in the first place. See https://github.com/jaydenseric/graphql-upload/blob/master/public/GraphQLUpload.js for an example.

[benjie]

Thanks for clarifying Andi; that makes sense.

[benjie]

From a quick scan this looks good - the memoization is straightforward and leverages the flexibility of JS. I was expecting to do this with a schema WeakMap with a defaults map inside it, but your way is massively more efficient.

It might be worth adding an initial _coercedDefaultValue property to the relevant places to help with V8's HiddenClasses.

Can we move the defineArguments refactoring into a separate PR (and merge it) so that the diff is smaller?

When it comes to validation I'm going to have to dedicate a longer block of time to reviewing that.

[leebyron]

It might be worth adding an initial _coercedDefaultValue property to the relevant places to help with V8's HiddenClasses.

Smart

Can we move the defineArguments refactoring into a separate PR (and merge it) so that the diff is smaller?

Definitely, good suggestion #3050

[andimarek]

One interesting aspect here is that a default value declared via SDL is coerced twice:
Once via valueFromAST and then again via coerceInputValueImpl. Is my reading here correct?

I am not really sure this is a problem, but I wanted to call that out.

[leebyron]

Thanks @andimarek - #3051

[andimarek]

Similar to the other comment: serialize is the result coercion. Scalar and Enums check the validity of input values via parseValue.

[leebyron]

Heads up: this diff is about to expand in scope.

After spending some time on this today - I'm realizing that #3051 and related comments are actually quite significant and difficult to untangle from this change. The fact that defaultValue is usually but not always stored as a coerced "internal" value is causing quite a number of issues in keeping this diff small and focused.

This issue is made most apparent by Enum values, which have a different representation externally (a string value or Enum literal) and internally (any value).

Some significant changes are necessary to represent defaultValue as a pre-coerced "external" value (eg a string value for Enums). I'm realizing that our functions for converting between values and AST are intermingled with coercion and serialization.

[andimarek]

@leebyron we have actually a similar challenge in graphql java: the defaultValue is often a weird non coerced, but still mostly correct value.

IIRC one of the big roadblocks is the problem mentioned in #3051: if you defaultValue is a pre-coerced input value, how do we deal with it for Introspection and printing?

[leebyron]

Updates in this last commit:

• Rewrites default value circular reference checking as "detectDefaultValueCycle()"
• Adds test suite for default value circular references
• Moves default value validation to utility "validateInputValue()"
• Adds "uncoerceDefaultValue()" to preserve behavior of existing programmatically provided default values
• Rewrites "astFromValue()" to remove "uncoerce" and type checking behavior. It used to operate on "internal" coerced values, now it operates on assumed uncoerced values. (also re-writes a bunch of its test suite).
• Extracts "validateInputValue()" from "coerceInputValue()" so it can be used separately

---

I don't think this is near complete yet, but wanted to get the work in progress up.

[andimarek]

as outlined in a previous comment I think this is wrong: we are misusing serialize here to produce a representation for an input value. We should introduce a specific function inputValueToAst for the Coercion and leave it to the scalar itself to determine a printable representation of its input values.

[leebyron]

We talked about this, but writing it down here for posterity:

I agree this is a misuse. This "uncoerce" step should ideally not exist, and only does so to avoid introducing a breaking change to existing users.

The previous code path used astFromValue(defaultValue) - astFromValue misuses serialize in the same way. This carries over the misuse to preserve existing behavior but isolates it to this specific case.

The new code path is valueToLiteral(uncoerceDefaultValue(defaultValue))

[yaacovCR]

I am not sure this will always preserve backwards compatibility.

Some scalars throw on serialize as a way of preventing them from being erroneously used as output values. While the example I know of should never have a default value, other examples might. See jaydenseric/graphql-upload#194

I would suggest that as part of the definitive solution that you appear on the cusp of delivering:
(A) the spec and the implementation should provide a mechanism of labeling leaf values as only valid within either output or input values

Separately, I would suggest:
(B) the implementation should introduce programmatic internalDefaultValue/externalDefaultValue options for arguments where only one may be defined, with a deprecation notice for the use of the previously ambiguous defaultValue. This would preserve the current behavior as best as possible, but also provide a clear upgrade path.

[leebyron]

I am not sure this will always preserve backwards compatibility.

Some scalars throw on serialize as a way of preventing them from being erroneously used as output values.

Since the previous codepath used valueToAST which also called serialize() on these, I believe such a scalar would already exhibit this behavior, so no change. This just moves the call to serialize().

(A) the spec and the implementation should provide a mechanism of labeling leaf values as only valid within either output or input values

This is an interesting idea! It's out of scope for this RFC, but I see the use case for giving GraphQL validation this information, rather than waiting to fail at execution time.

(B) the implementation should introduce programmatic internalDefaultValue/externalDefaultValue options for arguments where only one may be defined, with a deprecation notice for the use of the previously ambiguous defaultValue. This would preserve the current behavior as best as possible, but also provide a clear upgrade path.

I agree with this! I'd like for this "uncoerce" method to have a reasonably short shelf life and to be able to be removed in a future major version, but I'm looking for a solution that allows the end state to look correct. IMHO having to provide externalDefaultValue for the foreseeable future would be too confusing.

I'm not opposed to introducing a more modest breaking change, but would look to @IvanGoncharov for guidance on when and how to introduce that. Some ideas:

1. Require defaultValue to validate as an external input value, but provide a new deprecatedCoercedDefaultValue field which uses this new method for preserving behavior.

   Any problematic internal defaultValue would need to manually edit their schema to either fix the value or use this new field. The schema validation error message could reference this field so fixing an existing schema is self-documenting.

2. Provide a "flag" like "allowDeprecatedCoercedDefaultValues" when constructing a schema which applies the behavior currently in this PR. That would be a much easier change to make (one place, instead of per-call-site) but doesn't help highlight the long-term necessary changes and unnecessarily would run "uncoerce" on already uncoerced values.

I'll note that most defaultValue tend to be simple built-in scalars, so it's very likely that the vast majority will upgrade to a version including this change and not need to do anything at all. defaultValue for Enum or some Custom Scalars will need to be addressed.

I bet @andimarek has thoughts on this as well

[yaacovCR]

I think option 1 is best and imho that it is very important to fix the reference implementation to reflect the spec properly.

Thanks for your additional comments above, much appreciated.

[andimarek]

Let me say, that I am really happy that we tackle this issue. Great work @leebyron @benjie

In order to understand the other comments I made more clearly I will try to paint a more complete picture of what I believe is missing and we should do.

Currently we have three different coercion methods: parseLiteral, parseValue and serialize. (As I explained in #2357 I don't think these are great names, but this is another topic I will ignore here).

parseLiteral and parseValue produce "internal input values", while serialize accepts "internal output values" and produce a coerced result value. We are dealing here with two different sets of input values: while I understand that in practice there is often an overlap or they are even the same, in general we can't assume this. The other important aspect to highlight is that the domain of parseValue and parseLiteral is really the same: every value provided via SDL should also be possible programmatically and vice versa.

Default values for arguments or Input object fields can be created in two different ways: via SDL or programmatically directly.

A value specified via SDL can be coerced via parseLiteral into an "internal input value" and a programmatic provided value can be coerced via parseValue. As outlined in #3051 there is a challenge that SDL creation is leading to a double coercion: parseValue(parseLiteral(x)). I will come back to it later.

The missing part here becomes obvious when we look at Introspection default values or in SDL printing.
As specified by the spec default values in Introspection results should be printed as SDL:

defaultValue may return a String encoding (using the GraphQL language) of the default value

But we currently have no way of actually producing a Literal value from an "internal input value". This means there is actually no way of printing a custom Scalar default value correctly.

The solution I see is making the Scalar responsible for that by adding another coercion method: lets call it toLiteral for now.

The relationship between parseLiteral, parseValue and toLiteral would be that the result of parseLiteral and parseValue would be accepted by toLiteral and the result of toLiteral would be accepted by parseLiteral:

toLiteral(parseValue(x)) and toLiteral(parseLiteral(x)) and parseLiteral(toLiteral(x))

Regarding the double coercion problem mentioned above: parseValue(parseLiteral(x)) is not valid composing of functions in general: the result of parseLiteral doesn't have to be accepted by parseValue.

I think we must clearly define what GraphQLArgument.defaultValue,GraphQLInputObjectField.defaultValue means: is that an already coerced "internal input value" or is that an external input value which can be coerced via parseValue. The problem here is the SDL Literal values can't be transformed into an external input value in general (this would require the counterpart to toLiteral: a toValue method which then can be used as toValue(parseLiteral(x))) .
As alternative we could define defaultValue as Literal, which mean we can transform programmatic values via toLiteral(parseValue(x)) into it and then coerce it via parseLiteral. But I guess that would be a breaking change. I am not sure sure what the best solution here is tbh.

Looking for some feedback, especially if we agree on the fundamental aspects of what I described here. Thanks!

[leebyron]

I’m with you in principle but I think we can take a much simpler approach if we can represent defaultValue as an “external input value”, much like variables. We already have all the necessary well-defined methods for dealing with these in AST and introspection. We should only need to define how external input values become coerced and how internal output values become coerced. These are the only two defined by the spec. The primary problem is that we have a bunch of existing uses of graphql.JS which provide defaultValue as an internal input value instead of an external input value. We need a graphql.JS specific (outside the bounds of the spec) way to reverse input coercion specifically and only for this case.

[andimarek]

I agree that defining defaultValue as an "external input value" is the closest definition to the current reality. And of course our aim should be not break anything currently working.

I am happy that you say agree in principal, but I think our mental models around Coercion are not completely the same. For example I think that the spec should be more clear about the different kinds of coercions and coercion is "underspecified" in the spec.

There is no one rule how to coerce input values, because we have two very different input coercion ways: from AST values which should be clearly defined, but also from programmatic values, which is very technology dependent. The combination of JS, JSON serialization and JS like GraphQL syntax makes it very easy to look at think they are the same, but I think it is very important to distinguish it clearly. As you said yourself:

I'm realizing that our functions for converting between values and AST are intermingled with coercion and serialization.

But in order to get more clarity let me give a specific example Scalar to make the discussion more concrete: I define a EmployeeReference scalar which is externally represented as Base64 encoded Number, but internally represented as {employeeNumber: <number>, department: <departmentId>} (this is JS a Object).

parseLiteral: (Any StringLiteral) => Base64 decode, lookup number and department, produce internal value
parseValue: (Any JS String) => Base64 decode, lookup number and department, produce internal value

Now if I define a SDL:

type Query  {
    salary(employee: EmployeeReference = "MTIzNA=="): String
}

The current PR would then go on and invokeparseLiteral and produce {employeeNumber: "123", department="HQ"} as "internal input value".
This would then be treated as "external input value" and then parseValue would try to parse it and it would fail, because it is not a base64 encoded string.

What is the current idea how we deal with that?

[andimarek]

I want to share what GraphQL Java is planning to do about this issue in our code base, because fundamentally we have exactly the same challenge as graphql.js.

We have GraphQLArgument.defaultValue (the same problem applies for Input object, but lets focus only on Arguments, as it is literally the same problem) which are not guaranteed to be coerced. Or to put it differently: it is expected that defaultValue is an "internal input value". If the value is provided via SDL this works because when we parse the Schema we generate a coerced internal value (by calling parseLiteral ultimately). Same as in graphql.js via valueFromAst.

But we want to always coerce default values going forward, but also don't break existing usages. This is why we will make default value coercing optional: if you set GraphQLArgument.defaultValue the value will be marked as "we trust you, that this is a coerced internal input value". We will mark this method as deprecated and encourage users to provide us with "external input values" going forward, which we will coerce at execution time. This optional default value coercing will also the SDL code to continue as before.

This means defaultValue will be InternalInputValue | ExternalInputValue.

As explained before for the defaultValue printing challenge (for Introspection and printing SDL) we will introduce a toLiteral coercing method, which will allow us to convert internal input values into Literal, which we then use to produce a String. If defaultValue is InternalInputValue it will be applied directly: printLiteral(toLiteral(defaultValue)). If defaultValue is an external input value it will be: printLiteral(toLiteral(parseValue(defaultValue)))).

It may not look so nice as solution first, but this dual state of defaultValue being either a InternalInputValue or ExternalInputValue allows us to keep the existing behavior if you set the value programmatically, keep the existing code path for SDL schemas, but also make it more more correct by actually coercing default values if not explicitly prevented.

[leebyron]

That mostly matches what I'm planning here - I just want to make sure we're clear on terminology because I think toLiteral is the wrong idea. Literals are a GraphQL language concept. I'd expect something named toLiteral to return either a GraphQL literal AST or a printed GraphQL literal. That leaps through a bunch of concepts at once.

This means defaultValue will be InternalInputValue | ExternalInputValue.

This exactly matches what I'm proposing here.

If defaultValue is an external input value it will be: printLiteral(toLiteral(parseValue(defaultValue)))).

This seems wrong if parseValue does input coercion. Introspection should print a pre-coerced external input value.

I think I need a flow chart to illustrate all the value states and the functions for moving between them. That might help guide the discussion

[leebyron]

Another thing to add here - seems like we agree that our goal is to find a way to move to a better method of representing defaultValue while maintaining support for existing services.

The solution I see is making the Scalar responsible for that by adding another coercion method: lets call it toLiteral for now.

My primary problem with this is that existing services will have to define something new in order to continue to function. My goal is to introduce this without a breaking change or a change that requires additional work to adopt. Requiring scalars to implement this new function would hinder this goal

[andimarek]

Maybe we can split this into two tasks. Introducing toLiteral is maybe not strictly required to solve the default coercion problem, but it is for #3051 in my understanding.

I'd expect something named toLiteral to return either a GraphQL literal AST or a printed GraphQL literal.

Yes absolutely:toLiteral should return an AST object. toLiteral is the inverse of parseLiteral.

If defaultValue is an external input value it will be: printLiteral(toLiteral(parseValue(defaultValue)))).

This seems wrong if parseValue does input coercion. Introspection should print a pre-coerced external input value.

That is exactly what toLiteral returns: it returns a pre coerced AST.

The flow for this expression is (from inside out)
"External input value"
-parseValue->
"Internal input value"
-toLiteral->
"External input value as AST/pre coerced AST"
-printLiteral->
"String representation of pre coerced default value in SDL"

My primary problem with this is that existing services will have to define something new in order to continue to function. My goal is to introduce this without a breaking change or a change that requires additional work to adopt. Requiring scalars to implement this new function would hinder this goal

I agree that requiring all Scalars suddenly to implement this is not a good approach. I am not suggesting to break everybody, but give Scalars at least the possibility to produce correct Ast representations. toLiteral could be optional and the current behavior could be the fallback for example.

[leebyron]

EDIT: This graphic is no longer totally accurate:

#3074 (in the stack referenced at the original comment) implements "New Introspection Path for SDL defined Schema" - it's non breaking and fixes a related but different bug (#3051).

The "Input Value Un-coercion" has been changed to only be used to create validation error messages, not produce values which will be used at runtime.

Here's a broad overview of the intended end state: (See PR description)

Here's a focus on the default value introspection path before and after this RFC:

[leebyron]

Some things to note in those flow diagrams above:

• The flow you're defining as toLiteral, or the a method for converting "Internal input value" to "External input value as AST", is represented as two steps here. First "Input Uncoerce" that converts an "Internal input value" to an "External input value" and then "Value to AST" which produces an External Literal AST from an External Value.

  GraphQL.js already defines the latter as a generally useful utility for working with GraphQL literals, and the former we would not want to expose as a utility since it's only purpose should be to solve this deprecated flow.

  We could combine these steps into one routine called toLiteral, but if we did we'd probably want to similarly isolate it's availability.

• Input Coercion and Input Validation are defined twice (to your point above about parseValue and parseLiteral being the same domain). It should probably be safe to convert "External Input Value" to "GraphQL Literal AST" via "Value To AST" and then one use one of these paths, though that might have some performance implications. For coercion this is probably the right tradeoff. For validation, I'm exploring only needing a single definition that only applies on AST.

[andimarek]

Thanks a lot for the picture @leebyron .

In order to get more clarity, let me just focus on one specific aspect which I don't fully agree with:

The box Ast to value (implemented in valueFromAst) uses parseLiteral to produce an "External input value" in your diagram.
The same parseLiteral is used in Input Literal coercion to produce an "internal input value".

I don't think both things can be true at the same time: parseLiteral produces an "internal input value" in my understanding and we can't use it to go from "Ast Literal" to "External Input Value".

What do you think?

[leebyron]

I totally agree. This is currently broken, or at least poorly named. There's still a lot of work left to clean up this PR, but the next change coming will essentially rename "valueFromAST" to "coerceInputLiteral". GraphQL.js defineds "valueFromASTUntyped" which is a better fit for the "AST to Value" step in the picture

[yaacovCR]

Closed in favor of #3814