RFC: Default value validation & coercion

[leebyron]

Depends on #3086 Implements https://github.com/graphql/graphql-spec/pull/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() #3092
• Preserve defaultValue literals #3074
• Preserve sources of variable values #3077
• Add valueToLiteral() #3065
• Input Value Validation #3086
• RFC: Default value validation & coercion #3049

---

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

[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 - https://github.com/graphql/graphql-js/issues/3051

[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]

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 https://github.com/graphql/graphql-js/issues/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

[andimarek]

I want to add an overview which helps to understand the conversation flow:

[eapache]

I'm a bit confused by "Input Value Uncoercion". If it's deprecated why is it part of the ideal end state? Also I'm not sure how it's meaningfully different from "Result Coercion"... the note says they're "very similar" but in my experience with the ruby implementation they're exactly identical and we just use the same coercion flow for both in all cases.

[andimarek]

@eapache the end state is the end state for this work: it needs to be supported for now to not break existing users. In a perfect world it would not exist at all.

serialize is a fundamental different coercion than input value coercion: while in practice they are often very similar, conceptually they are different: See https://github.com/graphql/graphql-js/pull/3049#discussion_r621539992 for some more details.

[leebyron]

An update on strategy now that this stack is complete:

The initial attempt at this PR tried to preserve existing behavior by automatically "uncoercing" default values at schema construction time. Good review feedback pointed out many problems with this approach. I've abandoned this.

Later discussion led to a direction (which @andimarek is also pursuing in GraphQLJava) where at schema construction time you can explicitly provide already coerced internal values (something like deprecatedCoercedDefaultValue as an alternative to defaultValue). I started to implement this and decided to change strategies. This does not solve the breaking change, it just provides a mitigation technique. To advertise this mitigation, we would need default value validation failure error messages to suggest using it. A few problems with this.

• I don't want to encourage using a deprecated path as a mitigation technique, especially if it results in evading validation. Consider the case where an invalid default value is provided and this mitigation is falsely suggested. If applied, the error would go away, but the invalid default value problem would persist.
• So I considered a strategy where I only suggest this deprecated mitigation if I know it's the right fix. First try to "uncoerce" the default value and re-run validation on the result. If the "uncoerced" value is valid, then suggest using deprecatedCoercedDefaultValue.
• But then there are two ways to resolve the errors. Use deprecatedCoercedDefaultValue or just supply a valid value. And since I used "uncoerce" to produce such a value that I know is valid... why not just suggest that directly? So that's what I did.

The implemented strategy here does not introduce a fallback configuration for coerced internal default values. Instead it does the following:

• Assume defaultValue is an external input value and apply typical validation.
• If any errors are produced:
  • Re-run validation on an "uncoerced" version.
  • If this produces no errors, report an error with a suggestion to use this new value.
  • Otherwise just report the original errors.

---

This means that this change is definitely a breaking change. I expect that the large majority of uses of defaultValue are already valid (all built-in scalars and likely most custom scalars). Some of custom scalar type may require some adjustment and any of Enum type (at least where internal/external values differ) will need adjustment.

[eapache]

the end state is the end state for this work: it needs to be supported for now to not break existing users. In a perfect world it would not exist at all.

Ah ok, my bad, I read it as the end state after this work and all future cleanups, breaking refactors, etc.

serialize is a fundamental different coercion than input value coercion: while in practice they are often very similar, conceptually they are different: See #3049 (comment) for some more details.

I agree that input value coercion should be allowed to accept inputs which serialize will never produce. However (and this is probably getting into a topic which should be tackled separately) I disagree with "the possible output values of serialize can be completely disjunct from the input values of parseValue". You're technically correct that the spec doesn't currently require parseValue to accept the output of serialize, but that feels like an oversight in the spec to me, not a legitimate use case.

The implemented strategy here does not introduce a fallback configuration for coerced internal default values. Instead it does the following

This is interesting. So graphql-js is moving to specifying default values as "on the wire" / "external" values, not as "application" / "internal" values? I believe graphql-ruby made the opposite transition a few years ago cc @rmosolgo? This feels somewhat less ergonomic from a schema-developer point of view, but I guess it avoids the "uncoercion" problem, which ties back into the must-parseValue-accept-the-output-of-serialize question.

[leebyron]

This is interesting. So graphql-js is moving to specifying default values as "on the wire" / "external" values, not as "application" / "internal" values?

That's exactly right. Defining a schema with SDL already worked this way (via literal values - also external) and now both SDL and programmatic define them as "external" values.

This has a bunch of upsides including the one you mentioned about avoiding needing to create a bijection between serialize and parseValue (though I agree with you that 99% of the time that should be desirable).

[leebyron]

The primary upside that this original RFC required was that a default value should have identical behavior should that same value be provided at runtime.

For default values of input objects, which itself have fields with default values, previous to this PR this goal is not met. A default value of {} would omit fields with default values (since it's a final internal value) but after this PR they now go through the same coercion as any other externally provided value

[leebyron]

Added code review responses from @benjie

[leebyron]

@graphql/implementers please take one last look at this stack of changes. I'm planning to clean this up and land soon, I'm pretty confident this is the right change. One last chance to give a compelling reason why not.

[yaacovCR]

Closed in favor of #3814