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]

just to be clear here: these are the raw (uncoerced variables) right?

I recommend to name it like that rawVariables or uncoercedVariables

[leebyron]

These are coerced variables, which is also the existing behavior. We have no other type system to apply within a custom scalar, so the types of the variables are used for their coercion before they're folded in.

[andimarek]

We are calling valueToLiteral here on the variables which means from my understanding it must be raw variables, as valueToLiteral is external input value (aka raw variables) to Literal.

Edit: I just saw that the type argument is missing to valueToLiteral which is the cause of my confusion: replaceAstVariables needs to have a type argument which then needs to be passed on valuesToLiteral, because otherwise it is all undefined behavior again.

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

I think this method tries to be too clever: the type argument should never be undefined and the whole conversion logic should be based on the type.

If it is a List type => take the value apart and perform it per value.
If Object type => iterate over field definitions and call it per field
If Scalar/Enum => call the valueToLiteral on its type definition.

If you have an input object as type and the value is a boolean it should raise an error: at the moment it returns a boolean literal which is wrong.

[leebyron]

I thought about this, but the problem with this approach is that we're operating on "external" pre-coerced, pre-validated values and it's entirely possible that those values do not match the types. These conversion functions are used before validation takes place in #3049. That may require us to define validation rules again in each of these functions, ideally they can be passed through so that we define validation once.

[leebyron]

We also need reasonable defaults for when we don't have a type, since that's going to be common for any custom scalars with complex definitions (like the JSON type)

[andimarek]

Took me a bit to wrap my head around, but I think our misunderstanding comes from a fundamental principles about Scalar.literalToValue/Scalar.valueToLiteral does: I think these two methods should follow the same principles as parseValue/parseLiteral and should throw errors if something is wrong. I want basically another "coercing" (already a bit overloaded term btw) which converts from Value and Literal and vice verse.

Looking at the ID implementation in this PR I see that exactly this is missing: currently it returns undefined instead of throwing GraphQLError. Building on this principle I think method is basically the equivalent of coerceInputValueImpl but going from value to literal .

The reason I want this: for example for a JSON literalToValue method : you would want to throw a clear error message for an expected Literal deep inside the provided literal. What is the alternative? Return undefined? Silently ignore?

There is also general value in having a well defined, validated literal or external value for inputs.

And as you mentioned: this would mean validated twice, yes. But I don't see logic defined twice really: all validation logic resided inside the Scalar except for Input Objects. And I am sure we can extract/generalize this logic if that is a concern.

To be specific: we check for two things when validating input objects: if all required fields are provided and not more fields are provided.

[leebyron]

I don't necessarily disagree with that. I think being more strict is useful, and I'm sure we could find a way to generalize the validation portion.

The root of the issue is still variables within custom scalars. We have no types there and all bets are off. As long as we decide to preserve that behavior we need untyped transform

[andimarek]

I don't think I follow completely regarding the variables:

We actually have a type of the variables and we could use that type to convert the External input value into an Ast Literal in a well defined manner. We could then simply replace the VariableReference ast node with the Ast Literal we got.

Custom Scalar implementation can't type check variables values anyway at the moment: they just get a variable and they basically take the value.

I think that would preserver the current behavior.

[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

[yaacovCR]

Merged as #3812