Clarify 'Values of Correct Type' rule relates to literals

[benjie]

An Input Value is defined as being either a variable (if not const) or one of the literal types (IntValue, FloatValue, StringValue, BooleanValue, NullValue, EnumValue, ListValue or ObjectValue).

The rule "Values of Correct Type" states:

• For each input Value value in the document:
  • Let type be the type expected in the position value is found.
  • value must be coercible to type.

However, an input value can be a variable, and variable coercion is handled at runtime (by CoerceVariableValues). Further, we already have a rule that validates that variables are only used in the positions in which they are allowed: All Variable Usages Are Allowed.

It seems to me that "Values of Correct Type" only meant to handle literal input values, so I've added the word "literal" for clarity. I've also expanded the explanation to reference where to look for variable input value validation. Since input coercion for Input Object references "runtime value", and validation doesn't have access to runtime values, I've made explicit the assumption that values represented by variables will be of the requisite type.

This is an alternative solution to, and

• fixes Add note above variables to Values of Correct Type rule #1113

Thank you to @yaacovCR for pointing out this deficiency 🙌

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	daf28a2
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/675ad664ce26e700087770eb
😎 Deploy Preview	https://deploy-preview-1118--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[benjie]

@graphql/tsc I would love to get your input on this change.

[yaacovCR]

There are two conditions mentioned below, that the variable usage is allowed and that the variable value will coerce at runtime, but only one assumption is mentioned here. Is that intentional?

Suggested change

	{variableUsage} nested within {value} will represent a runtime value of the
	{variableUsage} nested within {value} is allowed and will represent a runtime value of the

[benjie]

It is deliberate: coercion happens at runtime where variables are already resolved; for us to run coercion during validation we need to run a slightly modified coercion - the modification being that it assumes (singular assumption) each variable reference it sees (something coercion would not normally understand) is equivalent to an arbitrary runtime value of that variable's type. It does not need to assume that the variable is allowed, from its point of view the variable exists whether it's allowed or not.

The foundation of that singular assumption is the behaviour of "All Variable Usages Are Allowed" combined with "Coercing Variable Values" - these aren't assumptions, these are specified behaviours.

[yaacovCR]

Let me try to work through it with 2 examples according to the way I understand your language. That might help you point out where I don't understand or potentially clarify if there is a problem.

EXAMPLE A:

1. {value} := {$varA}
2. Expected type at the position of {value} := String
3. Type of $varA := String
4. Let $varA represents a runtime value of String such as "SomeString"
5. {value} := "SomeString"
6. "SomeString" is coercible to String
7. VALIDATION SUCCESS!

EXAMPLE B:

1. {value} := {$varA}
2. Expected type at the position of {value} := Number
3. Type of $varA := String
4. Let $varA represents a runtime value of String such as "SomeString"
5. {value} := "SomeString"
6. "SomeString" is NOT coercible to Number
7. VALIDATION FILURE!

In the implementation and in #1113, the idea would be that we would get VALIDATION SUCCESS in BOTH cases because the other rules/assumptions take care of variables.

[benjie]

We’re talking about literal values here, so your examples should factor that in. I don’t know what your curly braces represent, I’m assuming not objects since dollar keys are not allowed. Example B fails validation according to both your example and the text of this PR, so I’m not sure what you mean by it passes validation? Validation doesn’t have access to runtime values so steps 4-6 are confusing - are you meaning “an arbitrary value of type String” rather than the runtime value, perhaps?

[yaacovCR]

Let me try again, sticking with just the problematic Example B:

query SomeQuery($someVariable: String) {
  someField(someArgOfNumberTypeLetsSayFloat: $someVariable)
}

According to the proposed new algorithm for this rule by this PR

{value} must be coercible to {type} (with the assumption that any
{variableUsage} nested within {value} will represent a runtime value of the
referenced variable's type).

So for example, $someVariable must be coercible to Float with the assumption that the variable usage of $someVariable nested within $someVariable (it's the only thing) will represent a runtime value of type String. But that's not going to work, as String does not coerce to Float, so this operation would fail validation by this rule, as edited.

Example B fails validation according to both your example and the text of this PR, so I’m not sure what you mean by it passes validation?

What I mean is that Example B and my hopefully more clear version above -- in my opinion -- SHOULD pass validation according to this rule. This rule should assume that Variables are in their Correct Position (because we have a separate rule for that) and that the variables coerce without error (because that will be and must be taken care of runtime). That's what the implementation at graphql-js assumes and what I was going for in #1113.

I've reopened #1113 as I think there is a difference in intent for the two PRs as to what Values of Correct Type should cover.

[benjie]

We've two rules and two algorithms we particularly care about here:

Values of Correct Type

Needs clarification that this relates to literal values IMO.

Importantly: note that literal input object values can be supplied in positions expecting custom scalars, such as the JSON scalar, and those literal input object values may well have variable values inside of them.

All Variable Usages Are Allowed

I've not read the relevant GraphQL JS source code, but I suspect that when it gets to step 2 in IsVariableUsageAllowed it cannot find the expected type for a variable usage within a custom scalar (because there isn't one) and gives up without error.

The rule only relates to explicit schema positions, and "inside a custom scalar" isn't one, thus we cannot rely on All Variable Usages Are Allowed within custom scalars.

CoerceVariableValues()

Ensures that the variables at runtime coerce to the values they say. Doesn't care where they're used.

CoerceArgumentValues()

Coerces the argument values "according to the input coercion rules of argumentType" - which is allowed to include consuming variables nested inside of the arguments (critical for lists and input objects, for example, but also important for our JSON custom scalar).

---

For lists and input objects you're definitely right that the type of these variables doesn't need to be checked by this rule - we could say:

(with the assumption that any {variableUsage} nested within {value} will represent a runtime value of the type expected in that position)."

However, as established above, there is no "type expected in that position" for variable references inside of custom scalars.

My thought was that by saying it's of the expected variable's type we'd get around this - but you're right that (for values like [1, $string, 3] versus an [Int] position) this now causes the error to effectively be raised twice - once in All Variable Usages Are Allowed and once in Values of Correct Type. Good catch!

I think we can just not worry about validating variable references inside custom scalars, leave that for runtime coercion inside CoerceArgumentValues(). I'm not aware of any custom scalars that accept input object literals and yet place variable type constraints on specific fields within them? I'm not sure if GraphQL.js even tries to do value coercion for custom JSON scalars when performing the Values of Correct Type check?

Evaluating for oneOf; we can assume that any variables have the expected type in that position, we just need to be clear that expected type in a oneof field position is a non-nullable variant of the field type... So yeah I think that covers it too. I'll tweak the wording.