Add valueToLiteral()

[leebyron]

Depends on #3077

Provides the "Value to Literal" methods in this data flow chart.

• Adds valueToLiteral() which takes an external input value and translates it to a literal, allowing for custom scalars to define this behavior.

This also adds important changes to Input Coercion, especially for custom scalars:

• The value provided to parseLiteral is now ConstValueNode and the second variables argument has been removed. For all built-in scalars this has no effect, but any custom scalars which use complex literals no longer need to do variable reconciliation manually (in fact most do not -- this has been an easy subtle bug to miss).

  This behavior is possible with the addition of replaceVariables

[leebyron]

@andimarek this is my attempt at introducing the version of "toLiteral" we discussed the other day.

[leebyron]

Open for discussion for a future change is whether replaceASTVariables should even exist. I'm not thrilled with it, but it's necessary to be a non-breaking change.

We currently allow variables to exist in the middle of a complex custom scalar:

query ($var: String) {
  example(arg: { field: $var })
}

type Query {
  example(arg: CustomScalar): String
}

scalar CustomScalar

This currently passes validation "All Variable Usages are Allowed" because if we do not know the expect type of a variable's position then we allow it - and the position in the example above is contained within a scalar - it does not have an expected type.

The validation rule in the spec does not specify this behavior, it just assumes a variable position has a type which based on this example is not necessarily a safe assumption.

We should probably make this behavior clear. Either by allowing it in the spec (and support this existing behavior) or disallow it in the spec making the above example invalid (which would allow us to assert that anything passed to parseLiteral is already ConstValue and therefore replaceASTVariables could be removed)

[andimarek]

Open for discussion for a future change is whether replaceASTVariables should even exist. I'm not thrilled with it, but it's necessary to be a non-breaking change.

We currently allow variables to exist in the middle of a complex custom scalar:

query ($var: String) {

  example(arg: { field: $var })

}



type Query {

  example(arg: CustomScalar): String

}



scalar CustomScalar

This currently passes validation "All Variable Usages are Allowed" because if we do not know the expect type of a variable's position then we allow it - and the position in the example above is contained within a scalar - it does not have an expected type.

The validation rule in the spec does not specify this behavior, it just assumes a variable position has a type which based on this example is not necessarily a safe assumption.

We should probably make this behavior clear. Either by allowing it in the spec (and support this existing behavior) or disallow it in the spec making the above example invalid (which would allow us to assert that anything passed to parseLiteral is already ConstValue and therefore replaceASTVariables could be removed)

The most popular example is the JSON Scalar which can contain variables in its literals. I don't really think we can break that behavior.

[leebyron]

That's the main use case I'm aware of, but I'm curious how common it is - it seems weird to allow one well-defined scalar to be embedded within another well-defined scalar. There's almost certainly going to be some weird edge-case behavior in there. From a usefulness point of view - I see the merit. From a clarity of execution and option-value preservation point of view, it seems wise to restrict complex scalars to be always fully formed rather than provided in pieces

We should probably clarify that in the spec either way.

[andimarek]

I want to point out a challenge regarding validation here:

This query:

query($var: ID){hello(arg: {a: $var})}

should be invalid for this schema:

  scalar JSON

  type Query {
    hello(arg: JSON): ID
  }

at least according to the implementation of parseLiteral in the custom scalar (https://github.com/taion/graphql-type-json/blob/master/src/index.js#L45) because ID is not an allowed Literal for a JSON value.

But because it is passed in as variable the custom Scalar just accepts the coerced variable.

This comes fundamentally from the fact that the type system treats Scalars as leaf types with no further inside. But variables inside custom Scalars break this assumption.

Note: this was tested against the current version. This PR would probably fixed that problem as far as I can see. So that would be great @leebyron

[leebyron]

Yeah you've got this exactly right. What does it mean to have a variable within a leaf value? There is no way to represent that in the type system.

[andimarek]

One important detail:

What should the formal relationship between valueToLiteral and literalToValue be?

For example for ID: This code does valueToLiteral(literalToValue(AstInt(123))) == AstInt(123) but also valueToLiteral(literalToValue(AstString("123"))) == AstInt(123)

This means valueToLiteral maps two different values to the same Literal (In math speak: it is not injective) and both functions are not the inverse to each other.

If we want to make both functions the inverse to each other we would require that different literals always produce different external input values and different external input values produce different literals. I don't think we can do that as this is a constraint we have not had so far and we would break existing code.

This probably means we require that for every valid literal x literalToValue(valueToLiteral(literalToValue(x))) is possible and for every valid input value y valueToLiteral(literalToValue(valueToLiteral(y))) is possible, but not more.

[andimarek]

I want to state that I think getting rid of variable references inside complex ast literal is the right way to preserve the current behaviour while making custom scalar implementations easier and more robust. Great work @leebyron

[andimarek]

Looking at your flow chat: I fully agree regarding the new well defined behavior defined by valueToLiteral and literalToValue. 👍

I commented inline, but want to highlight it here again: I think we are trying to conserve some not well defined behavior which we don't have to: valueFromAst (and the revsere) should be deprecated and left alone imho. It should never be used anymore inside GraphQL.js execution itself as fas as I can tell.

[leebyron]

valueFromAst is misnamed, as it does "uncoercion" and needs to stay in use until we can fix default values in #3049 - it's deprecated in that PR

[leebyron]

I've updated this with a pretty major refactor after my last discussion with @andimarek.

• valueToLiteral() and literalToValue() now require a type and do basic validation to ensure these functions are well defined. There are still defaults defined for custom scalars which use a variation on the original.
• All built-in scalars and enums define these methods now. (They do in terms of the default methods with additional validation, as this was a simpler implementation than reproducing that logic per type)
• replaceVariables() now uses the types of the provided variables (and their default values) to ensure we're consistently using "external input values" and not "internal coerced input values" and well defined behavior.

[andimarek]

literal to value needed?

I changed my mind a bit and I am not convinced literalToValue is worth adding after going through GraphQL Java and looking at this PR: literalToValue is only used by valueFromAst as fallback if a Scalar doesn't define parseLiteral. And inside literalToValue we have a fallback for Scalars which don't define literalToValue. But if a Scalar doesn't define parseLiteral it will very likely also not define literalToValue as the latter is only added now. Which means literalToValue is no really needed for valueFromAst.

The only real usage I can see is that users want to convert the default value from external input values to literals, but this would require a different level of error handling I think, because this would make this function public.

Related to that: In GraphQL Java we plan to offer to set defaultValues either as Literal or external input value. GraphQL.js will only offer the former, correct? If you would offer to set the default value as external input value too, there would be no need anymore for literalToValue afaiu.

Error handling

When will the default values literals be validated? I think we need a schema validation for that, as we currently only have Query validation level.

Reading the code it seems that valuteToLiteral assumes that the external value is valid.

Documenting constrains

We should clearly document that parseValue and valueToLiteral must accept the same inputs, same with parseLiteral and literalToValue (if we keep it)

[leebyron]

I am not convinced literalToValue is worth adding. literalToValue is only used by valueFromAst as fallback if a Scalar doesn't define parseLiteral.

Yeah, it's really feels there for completeness. Especially after the stack of diffs before this one, this is the sole use case.

But if a Scalar doesn't define parseLiteral it will very likely also not define literalToValue as the latter is only added now. Which means literalToValue is no really needed for valueFromAst.

Definitionally true since this PR hasn't landed yet :)

An alternative would be to revert back to the previous behavior of using valueFromASTUntyped (which is more or less what we now have defined as defaultScalarLiteralToValue - to your point) and require parseLiteral to be defined if literalToValue is also defined. This would avoid a scenario where a custom scalar defines parseLiteral but not literalToValue which seems to be the only purpose for this function at the moment.

Related to that: In GraphQL Java we plan to offer to set defaultValues either as Literal or external input value. GraphQL.js will only offer the former, correct? If you would offer to set the default value as external input value too, there would be no need anymore for literalToValue afaiu.

This is introduced earlier in the PR stack in #3074. Once the entire stack is complete you will be able to provide a default value as a literal, an external input value, or (deprecated existing cases) as an assumed-coerced internal input value.

When will the default values literals be validated? I think we need a schema validation for that, as we currently only have Query validation level. Reading the code it seems that valuteToLiteral assumes that the external value is valid.

This isn't done in this PR, but in the one stacked above it in #3049. Since existing default values are assumed coerced internal input values, there's nothing to validate just yet. Once we can get them defined properly, then we should validate them as part of schema validation, which is part of that RFC.

valueToLiteral does not assume the external value is valid, it returns undefined if it encounters a value that cannot be converted to a literal in a well defined way according to the provided type.

Documenting constraints

Agreed. I'll expand on the documentation for these methods

[andimarek]

This is introduced earlier in the PR stack in #3074. Once the entire stack is complete you will be able to provide a default value as a literal, an external input value, or (deprecated existing cases) as an assumed-coerced internal input value.

Sounds good, but then you need to differentiate between these three states in getCoercedDefaultValue. I can't see this happening so far, but maybe I missing something?

[leebyron]

Yeah it's a bit hard to follow since defaultValues are still stored as coerced internal values. getCoercedDefaultValue checks both cases: first that .value is defined (which until #3049 is coerced internal value), then that .literal is defined.

I'm working on a serious refactor of #3049 which incorporates all of this feedback that hopefully will make exactly this far more clear

[leebyron]

@andimarek - removed literalToValue and improved on documentation. Proper schema validation comes in a later PR