Enable Fragment Arguments (a form of parameterized fragments) during execution with flag

[mjmahone]

This is really a continuation of @daniel-nagy's work in https://github.com/graphql/graphql-js/pull/2871 and, more distantly, @sam-swarr's initial AST implementations in https://github.com/graphql/graphql-js/pull/1141, as well as @josephsavona and the entire Relay team's work from before https://github.com/graphql/graphql-spec/issues/204 was opened.

Background

https://github.com/graphql/graphql-spec/issues/204 is the original open source issue from ~5 years ago.

For the past 5+ years, Relay has had the @arguments directive, which is not spec compliant. In some sense, Relay is a dual GraphQL client: there's Relay syntax which is used to resolve data available locally on the client, and then that syntax compiles down into a spec compliant syntax to resolve data from an external source (aka a "server"), which hydrates a graph of "local" data the relay-specific resolvers operate over.

This means Relay can get away with having user-written fragments that are freed from operation-defined knowledge: Relay's fragments can be provided variable values that were never defined at the operation level, to use when resolving arguments.

This PR: A graphql-js implementation of "fragment argument replacement", a la Relay

This enables executing requests that use fragment arguments: when collecting fields on fragment spreads, the executor now replaces any variables matching fragment argument values, with the rule:

• Replace the fragment variable with the argument value from the spread location, if provided
• Otherwise, replace the fragment variable with the default value from the fragment variable definition, if there is a default value
• Otherwise, leave the variable as-is: as it is unset, the executor will treat it as an unset variable.

Note that last point, without adding better validation rules, allows an operation defined variable to "clobber" a fragment variable that was purposefully left unset. I think this is undesirable behavior.

The two new runtime errors I added were:

• If the fragment variable is defined as non-nullable, but null is passed in at the argument, throw.
• If the fragment variable is non-nullable and has no default value, but no fragment argument value was provided at the spread, throw.

With just these new errors, this behavior already ought to compose cleanly with other executor errors to disallow things like spreading the same fragment multiple times at the same level with different arguments: that would create overlapping fields. It also cleanly allows for variables to be passed in as fragment arguments.

In some sense, this is a baby step towards @IvanGoncharov's idea of adding an execution plan stage. It's unlikely you'd want to implement this execution plan using visit as I have here, but it works, and I'm trying to ensure this behavior won't add overhead unless you're actually using the new feature.

Validation Required

IMO the executor behavior is too permissive for a first iteration on the spec: it will likely be difficult for existing clients to immediately support spreading the same fragment multiple times in the same operation with different argument values. So my plan is to add additional validation rules (will add on this PR):

• All required fragment arguments are explicitly included in any fragment spread for that fragment. This duplicates the execution-time error.
• All fragment arguments provided are of the correct type: this duplicates the execution-time "arguments of correct type" error, but allows us to point specifically to the original, user-written fragment spreads as the cause of the error, rather than using the replaced values.
• Every fragment spread within an operation must have identical argument values (this could be relaxed for clients like Relay)
• Operation variables must not "shadow" fragment arguments (this ensures a fragment spread that explicitly leaves an argument unset will not have that behavior clobbered with a global variable definition).
• All argument variables defined on a fragment must be used in the same fragment (this prevents needing to support an argument stack across fragment spreads)
• (Maybe) Only constant argument values are permitted (we may want to transition clients slowly to fragment arguments, and constant-only values are much easier to support)

[mjmahone]

@IvanGoncharov would it be better to create a branch within graphql-js for this, rather than merging onto master/from my own repo?

There's some complexity in terms of validating the argument usage, given this is the first time we'll have arguments whose type is defined by a variable definition, and where those variable definitions are not visible on the operation. It might be better to have many people contribute to those new validation rules, and update existing validation rules, which I think would be easier from a repo branch (plus then I wouldn't clobber other people's commits, like I did with @daniel-nagy's initial work).

[mjmahone]

Are these "Fragment Arguments" or "Fragment Variables"?

The way I'm thinking of them is, when used within a fragment definition, they are "fragment variables", as they're variables defined on a fragment that can be used anywhere you'd use an "operation variable".

However, when exposed for use on fragment spreads, they're "fragment arguments": just like a field defines arguments that can be fulfilled by either variable or explicit values, fragments now define arguments that can be fulfilled by explicit values.

I think it would be less confusing (and easier to write validation for!) if we had the concept of "fragment interfaces" which a "Fragment implementation" can implement. The current syntax is basically creating this concept, but not explicitly defining it.

So something like

fragment Foo($v: Int) on User {
  size(x: $v)
}

Is really doing something like:

fragment interface Foo(v: Int) on User;

fragment Foo($v: Int) on User {
  size(x: $v)
}

If we had the above syntax, you'd only need to re-check a fragment spread is correct when either the spread's argument changes, or the fragment interface changes, but not when the fragment implementation changes.

In this world, it might be legal to do:

fragment interface Foo(v: Int) on User;

fragment Foo on User {
  size(x: 3)
}

i.e. create an interface that expects a specific argument, then allow implementations to not use that argument at all. This is especially helpful if you want to be able to inject, at runtime, different values for the same fragment spread depending on what context you're in (this is something I have a lot of use for, but in JS land it's probably not very desired).

[vanyadymousky]

Any progress updates on this?

[yaacovCR]

Any updates on this, especially for those who unfortunately missed GraphQL conf?

[mjmahone]

Yeah the short of it is I'm back to iterating on this proposal.

I think the core plan of attack is:

• Call them "Fragment Arguments" as enabling people to use the same data shape across different use cases is the primary purpose, much like function arguments. You ought to be able to pass down literals or variables, so it makes sense to call them what they are: arguments.
  • Even though they are re-using the variable definition syntax on the fragment definition, in reality it is defining an argument for that fragment. It may make sense to use field-variable-definition syntax instead of operation-variable-definition syntax, but that's something we should probably work through in a WG session.
  • Given GraphQL grew-up in a PHP world, and adopted similar semantics, I think we will land on a similar resolution for "named arguments" as PHP's named arguments for functions: $arg to define the function/fragment argument and use the value within that one function/fragment's scope, and arg: <value to set> to pass the a value to the function/fragment argument defined as $arg.
• For now, we preserve the exact same logic around field validation: you cannot have the same field with different arguments at the same level of the tree.
  • This will limit the "modularity" of fragment arguments, in that you can re-use the same fragment in two locations that happen to be in the same operation, and cause a validation error.
  • However, that is an existing spec issue, and we should address that issue via the Fragment Modularity discussion. While I believe these two proposals, combined, create multiplicative value, that just means getting fragment arguments done will increase pressure to improve fragment modularity :)
• The above implies extending "OverlappingFieldsCanBeMerged" validation to include fragment spreads as "mergeable types", possibly with a second OverlappingFragmentSpreadsCanBeMerged rule
  • We could just rely on OverlappingFieldsCanBeMerged as it exists today, by creating "duplicated" instances of the same fragment for each spread, but I think it's cleaner to address the issue directly on the fragment spread itself, rather than on some inner field.
• To answer @leebyron's earlier question, "should a fragment variable be able to name override another variable?", I feel that the answer will inevitably resolve to "yes", but I'm definitely open to disallowing fragment arguments from overlapping with operation variables.
  • If a fragment defines an argument, everywhere that argument is used within the fragment will resolve to the fragment's argument value, not the operation defined variable value.
  • An open question: what about fragments spread by the fragment-with-an-argument that do not define a value for the variable?
    • I think the answer is "if you don't explicitly defined an argument, then you use the operation-scoped variable definition".
    • I realize this could be a cause of confusion in either direction so we need to be really crisp and ideally prevent footguns. This is the main headache that might convince me we should just disallow fragment arguments from overlapping with operation variables.
  • IMO it will be very difficult to roll this feature out if you are only ever allowed to define a fragment argument once for an entire operation's document. It's common to use the same variable name across many fragments, and the core issue we're hitting is that depending on where you are in the tree, you want that variable to have a different value (as determined by some parent fragment spread).

One hesitancy I have: we may want to enable "automatic threading", e.g. if you have:

query Example1($var: String!) {
  # Is $var implicitly passed in, or is this an error?
  ...ImplicitOrInvalid
  # Is $var implicitly passed in, or is it implicitly null?
  ...ImplicitOrNull
}
fragment ImplicitOrInvalid($var: String!) on Query {
  field(arg: $var)
}
fragment ImplicitOrNull($var: String) on Query {
  field(arg: $var)
}

My initial idea is that an explicitly defined fragment argument must be explicitly passed in from above, or else it is considered null. We could start by enforcing all fragment arguments must be explicitly set (even if you want them to be null), though I think this will get incredibly confusing when you start combining fragment arguments with default values.

All of the above is to say: I should have another iteration of this PR up shortly.

[netlify[bot]]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	8724e22f9e89224dd2669d6fc0c4b7fe34c2c8dd
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/63bf3d8a9d161a000a59803f
😎 Deploy Preview	https://deploy-preview-3152--compassionate-pike-271cb3.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 settings.

[mjmahone]

This PR is now very close to ready for review (review question, do people prefer stacked PRs, i.e. one PR per commit/idea, or PRs that have all the changes bundled together?)

We currently have:

• AST (same as before)
• Execute
  • I updated the logic to add a concept of a "fragment spread key", which includes the spread and the arguments that will be substituted in with the fragment's selection set.
  • I ensured that "shadowed arguments" (arguments that share a name with an operation variable) cannot accidentally take the operation's value. I accomplish this by internally swapping in any unset fragment argument usages that have no default value for an $__UNSET variable.
  • substituteFragmentArguments holds the logic for this swapping. The replacement method is very straightforward, and to avoid the usage of an internal $__UNSET we'd need to thread some sort of local scope through in many places. The current way is simple, but isn't how I'd write the code if I was building from scratch.
  • Another possible solution here is to disallow arguments that have no default value from being unset: x on fragment foo($x: Int) would be a required, nullable argument, and ...foo would not be valid.
    • This is fine and avoids unnecessary $__UNSET, but it means it's impossible to use schema defined field argument default values when passing in a fragment argument (null will be explicitly null, not unset). I am OK with that, as I don't use field default arguments much and find their idioms a bit hard to explain to people. I wouldn't mind cutting off one element of our complexity tree.
• OverlappingFields validation
  • We now properly check whether fragment spreads of the same name with different arguments overlap, and if so throw an error before checking any of the child fields. This is I believe the most complex part of fragment arguments within graphql-js, and getting a long enough focus block to complete it was the limiting factor in my being able to advance this PR.

In graphql-js, I still need to:

• New: "NoUnusedFragmentArguments" rule (easy)
  • Alternatively: update NoUndefinedVariablesRule to encompass fragment args
• New: "FragmentArgumentsInAllowedPositionRule" (should be straightforward as fragment args are locally scoped)
  • Alternatively: update VariablesInAllowedPositionRule
• New: UniqueFragmentArgumentNamesRule (easy)
  • Alternatively: update UniqueVariableNamesRule
• Update: "VariablesInAllowedPositionRule" to also ensure operation variables used on fragment arguments are correct
  • I suspect this will require some changes to the ValidationContext's getRecursiveVariableUsages
• Update "ProvidedRequiredArgumentsRule" to ensure required fragment arguments are provided, same as field arguments (easy)
• Update: "NoUndefinedVariablesRule" to take into account fragment arguments that look like variables
  • Need to update either the getRecursiveVariableUsages, or need to change how this visitor works
• Update: "KnownArgumentNamesRule" (or add a FragmentArgs version) (easy)

And then I'll need to update the PR https://github.com/graphql/graphql-spec/pull/865, present to the WG, and hopefully get feedback and keep iterating.

[mjmahone]

I've updated the changes to be what I think are the minimal needed to get the behavior and validation to match the updates described by the spec changes in https://github.com/graphql/graphql-spec/pull/865

Some notes:

• I discovered that a fragment argument definition is more closely related to InputValueDefinitionNode than it is to a VariableDefinitionNode, despite superficially having the same syntax as the former. Namely, the node describes a type definition that will be used to validate variable and value usages within arguments. Unfortunately, because the syntax for fragment argument definitions is $name : Type and the format for InputValueDefinition is description? name : Type, I couldn't re-use the InputValueDefinitionNode exactly (like we can for Field arguments, Directive arguments and Input Object fields).
• TypeInfo needed to be updated to find and record all fragment argument definitions at the top of the document-level visit, otherwise we cannot look up the type definitions when traversing the argument usages within fragment spreads. In some sense, the fragment definitions create an "executable schema" of document scoped type definitions.
  • Extending this further, we could allow type extensions within an executable document, though I'm not sure that would add any real value.
• The only complex validation change was OverlappingFieldsCanBeMergedRule, as we now keep track of fragment spreads and compare same-named spreads to make sure they have identical argument values.
  • I chose to consider two spreads that have different arguments at the callsites to be different, even if resolving their argument values by using the argument default values would result in the same applied argument value. We could update this choice to allow more fragment spreads to merge, but I think being more restrictive in these cases is better. If a fragment changes the default value of one of its arguments, that could otherwise make two spreads that previously merged no longer merge. IMO better to just prevent them from merging.
• collectFields is still using the hack of "unwinding" fragment arguments by, at the fragment spread, substituting variables defined as arguments by the parent fragment with the value passed in. This leads to a minor bit of weirdness: what if the variable is unset by the parent fragment spread? The hack here is to just replace the variable with a guaranteed-to-be-unset $__UNSET.
• This hack is defined in substituteFragmentArguments()
• If we actually followed the spec collectFields algorithm exactly, and threaded the argumentValues through, we could eliminate the hack completely.

[mjmahone]

Moving this years-old stack to https://github.com/graphql/graphql-js/pull/3835 as a way of cleaning up the implementation. I've also created a new discussion in graphql-wg: https://github.com/graphql/graphql-wg/discussions/1239