Fragment Arguments: RFC + Implementation Details

[mjmahone]

We now have PRs for Fragment Arguments in graphql-js: graphql/graphql-js#3835 and graphql-spec: graphql/graphql-spec#1010

This discussion will include threads for all open questions and TODOs, and should be the source of truth for discussion about what is left to do or consider, while the PRs should be focused on discussion around the reference and spec implementations.

[mjmahone]

Concern: Variable Shadowing and Scope

Scope: Local ✅

In the current implementation, fragment argument values are scoped to the fragment that has the variable definition. This means you can only use a variable with the name of a fragment variable definition inside that same fragment (not within a parent, child, or sibling fragment). From all the discussions so far, this seems generally correct.

Alternatives are:

• Global, i.e. a fragment argument is just syntactic sugar for an operation variable.
  • This is what we originally implemented at Meta in our native clients, and it was terrible for all the reasons you can think of. Relay chose to use local scope and transform local fragment variables into specific-to-that-fragment operation variables, which is functionally equivalent to locally scoping them.
• Recursively local, i.e. the variable takes on any parent fragment argument definition
  • This preserves the concept of the value being some sort of recursively scoped variable.
  • However, keeping track of what's happening and preventing merge conflicts becomes a lot more difficult.

Shadowed Variables: Allowed❓

In the current implementation, we allow shadowing both global variables and parent-fragment-defined variables.

So this is legal:

query Q($a: Int) {
  x(a: $a)
  ...B
}
fragment B($a: String) on Query {
  stringOrNull(a: $a)
  ...C(a: $a)
}
fragment C($a: String = "Red") on Query {
  requiredString(a: $a)
}

Within Q any $a will have the value set by the operation variables. Within B $a will, for this operation, be unset, and unfortunately it is valid but within C $a will also be "Red" because the parent value passed into C's a: argument is unset.

We could disallow this class of shadowing, but I'm currently for allowing shadowing, for a few reasons:

• It is allowed in most programming languages, so it would be unnatural to disallow it.
• Because fragment variables are locally scoped on the definition they're being used, there's no real ambiguity: the value is always the one that was defined and immediately passed in, not the operation-scoped value.
• Disallowing shadowing will cause many more validation errors: I am against an operation becoming invalid just because you happen to spread a fragment that re-uses the same name for a variable in a different context. This defeats some of the benefit of fragment modularity.

[rivantsov]

I agree with shadowing (so yes on this, fragment's variable shadows operation variable with the same name).

But now let me put my compiler-freak purist hat on and add a comment regarding variable scoping in general. There are two basic scoping (or name-binding) models known: lexical (static) scoping, and dynamic scoping. Wikipedia article explains it very well.

Quote:

In practice, with lexical scope a name is resolved by searching the local lexical context, then if that fails, by searching the outer lexical context, and so on; whereas with dynamic scope, a name is resolved by searching the local execution context, then if that fails, by searching the outer execution context, and so on, progressing up the call stack.

Most languages today use lexical scoping; dynamic scoping was used in LISP long ago - but there's nothing wrong with it, in general.

In the current GraphQL spec version, which allows using operation variables in fragments (but no fragment args/vars yet), we clearly use dynamic scoping - variable ref is bound at runtime to the current operation variable - which is up the call stack.

Now with fragment variables, it looks like dynamic scoping but not quite. As far as I understand, with fragments invoking each other, in current proposal when binding a variable inside fragment, we check this fragment's variable first, and then go directly to calling operation's variables ignoring all fragments in the call stack. This looks like a modified dynamic scoping - we are inventing our own custom scoping rule.

Something to think about. Maybe let's just stick to 'standard' dynamic scoping, and check all the intermediate fragments' scopes? I do not see anything wrong with it; at least there's a wikipedia article that explains how it works.

[josephsavona]

+1 for local scopes and for allowing shadowing, as you noted this is how Relay works and it's proven to be a good balance. Global scopes have the steep pitfalls you mentioned, and recursively local scopes introduce action-at-a-distance and break local reasoning.

Disallowing shadowing wouldn't be horrible, but it also breaks local reasoning. I can't use a name that makes sense locally just because someone else used that name elsewhere.

[josephsavona]

@rivantsov I think you're conflating resolution of values with resolution of variables. GraphQL is lexically scoped with a single scope at the operation level. Variables in the source can be resolved statically to which variable definition they correspond to — that's the definition of lexical (static) scoping. What we're proposing here is to introduce an additional layer of lexical scoping, at the fragment boundary. The rules for variable resolution then become: attempt to resolve the variable against the fragment definitions, if not bound there then go to the next lexical scope, which is the query, and resolve them there. Note that fragments may "call" each other at runtime, but fragment spreads do not affect the lexical scope used to resolve variable identifiers to their definitions.

[rivantsov]

I think there's confusion here. Executable Doc can contain multiple operations (queries, mutations) and fragments. Fragments are not inside operations, they are side-by-side: Document Therefore lexical binding is not possible (fragment is not inside operation lexically).

Multiple fragments are side-by-side with multiple operations; one fragment maybe referenced in multiple operations, so variable ref inside fragment is not bound statically to particular operation variable, there might be several same-named variables in multiple operations. So binding happens at runtime, to current operation and its variable. It is dynamic scoping. That's the way I see it in the spec - am I confused?

[mjmahone]

(fragment is not inside operation lexically)

I'm not sure this is true, and I think it depends on your actual server implementation. You server can build up a static variable scope per operation. Does it look identical to classical lexical scoping rules? Maybe not: you need to squint a bit, but if we split documents into a one-operation-per-document set of documents (which each contain an operation and all used fragments), then the variable definitions can be static within that operation. At execution time, you can know exactly where and how the variables are bound. This breaks the idea of c-style "lexical bounds within braces", but GraphQL isn't C nor is it a C-like language, and the choice that variable definitions at the operation scope break out of their curly braces prevents us from having C-style-scoping in general.

However, because the spec describes the algorithms to use and the behavior expected, you can achieve the same executable behavior by either using "static" scoping or "dynamic" scoping today. Whether you choose to determine the variable binding at compile-time, before the variables have values set, or at execution time, you'll get the same result.

I do agree we made it harder to resolve variable scope by having all variables defined by the operation rather than either the document or locally to each definition: a better model for GraphQL V2 would be to have definition-bound variables completely locally scoped, and document-bound variables for variables that aren't locally scoped. However, because we don't have that model, and because there's already confusion about which specific variable binding to apply when you read a GraphQL document, this in my opinion is why we need to keep fragment variables' valid scope to be local to the fragment itself.

Whether you implement your executor using dynamic or static scoping, we want the result to be identical. If we allow you to define a variable $x in fragment A, in which you spread a fragment B, if you use lexical scoping, you'd expect $x in B to be bound to the operation variable defined at the operation scope, whereas if you use dynamic scoping, it'd be $x from A.

By saying "when you encounter a variable, when figuring out the definition it's bound to first look at the fragment, and if it's not there look at the operation", we define a concrete algorithm that can be implemented either at compile time or execution time and produce the exact same result.

Are we going to be a great case study for lexical or dynamic scoping in university courses? No, probably not. But theoretical purity must bend to real world needs, and requiring recursive dynamic scoping just because that's what GraphQL looks to have with respect to operation variables would be IMO a mistake: fully featured dynamic scoping is, in almost every language you encounter it in, hard to reason about, and making GraphQL more powerful with more powerful difficult to reason about features seems backwards, when we can jsut say "Yeah, if you're using dynamic scoping, here is how we limit that power so people make fewer mistakes." Just because we can describe in fewer words how things work with a footgun in place doesn't mean we should keep the footgun when a few more words will take it away entirely.

[mjmahone]

Concern: Variable Definition or Argument Definition?

In the current PRs I re-use Argument for the name of the thing that tells you what the scoped local value is replacing at the fragment spread site, i.e. the thing called arg in ...Foo(arg: $var). For the fragment definition site, it's currently called a Variable Definition for the thing $arg in fragment Foo($arg: Type) on Bar.

Relay currently defines them as @arguments and @argumentDefinitions, however at the AST and Relay-IR level, they are Argument and VariableDefinition.

Given the syntax is identical to Operation level Variable Definition, I've preserved Variable Definition at the definition site, Variable for usage of the variable within the fragment, and Argument for when a fragment spread is setting a locally scoped variable to some value.

I've tried implementations and Spec edits with both Argument Definition (creating a new ArgumentDefinition AST kind), and the current state seems reasonable to me, but this concern may be worth further discussion. Feel free to discuss here and also add inline comments on the graphql-spec PR if there are any confusing bits you can point to!

[mjmahone]

Concern: Algorithm Options for Execution

I've explored two different algorithms that are isomorphic in result.

Option 1: Passing In Scope

The first is to have true scoped variables, where we consider there to be an "operation" scope and "fragment local" scope. Whenever we encounter a fragment spread, we update the scope by replacing the set of parent local variable values with the new local variable values.

So instead of function definitions like:

CollectFields(objectType, selectionSet, variableValues, visitedFragments):

we have something like:

CollectFields(objectType, selectionSet, operationVariableValues, fragmentVariableValues, visitedFragments):

This extra argument needs to be passed into all functions that checks variableValues or recursively uses a function that does.

Option 2: Replacing Fragment Selection Set AST

This is functionally equivalent, but instead of keeping track of our current scope, whenever we encounter a fragment spread we figure out what the fragment-scoped variable values are by copying and visiting the copied fragment's selection set: whenever we find a variable usage defined by the fragment, we replace it with the value of the argument passed in, the default value if there is no passed in argument, or $__UNSET if there is no default value.

This substitution function is pretty simple to describe even as a spec algorithm:

SubstituteFragmentArguments(fragment, arguments):

- Let {variablesToSubstitute} be initialized to an empty map.
- For each {variableDefinition} in {fragment}:
  - Let {variableName} be the name of {variableDefinition}.
  - If {variableName} is a key in {arguments}:
    - Let {argumentValue} be the value of {variableName} in {arguments}.
    - Add {argumentValue} to {variablesToSubstitute} at key {variableName}.
  - Otherwise if {variableDefinition} has a default value {defaultValue}:
    - Add {defaultValue} to {variablesToSubstitute} at key {variableName}.
  - Otherwise:
    - Set the key {variableName} in {variableToSubstitute} to a value indicating
      the variable is unset.
- Let {substitutedFragment} be a copy of {fragment} where:
  - For each {variable} in the selection set of {fragment}:
    - Let {variableUsageName} be the name of {variable}.
    - If {variableUsageName} is in {variablesToSubstitute}:
      - Replace {variable} with the value of {variableUsageName} in
        {variablesToSubstitute}.
- Return {substitutedFragment}.

Note we need to explicitly replace unset variables with some impossible-to-set-at-the-operation variable, so that they act the same as variables defined on the variable that are never set. If we just don't replace them, then we run into the bug where operation scoped variables can be used at execution for fragment-defined variable usages. Likewise, if we replace these instances with null, we run into the bug where a field with an argument with a default value would treat the value as an explicitly-set null rather than

Current choice: Option 2 (Replacement)

I like the mental model of the passed-in scope algorithm. However, if we consider changes to our existing algorithms, the second option is a much cleaner change. We end up only needing to update the execution algorithm in one location: when we encounter fragment spreads. With the scoped variables option, we need to update every part of our algorithms that check or coerce variable values to include the concept of first checking the local scope, then the operation scope. This affects basically every function under Execution, and we either need to add an extra argument on all of these functions for locally scoped variables, or we need to modify the type of variableValues.

Potential Additional Options

We could make it so that only variables with default values can be unset arguments. I actually prefer this mental model, but decided against it as this would mean understanding the difference in argument behavior between fields (where an unset argument becomes null unless there's a default value) vs. fragments (where it now is invalid to have an unset nullable argument without default value).

This option would eliminate the need for $__UNSET in Option 2, but at the cost of mental-model expansion. Given we could in the future eliminate $__UNSET by applying truly scoped algorithms, with no change in behavior, this benefit seems minor.

[mjmahone]

I don't think it matters too much if we start with 2: it won't be a breaking change in terms of server/client interaction to switch from 2 to 1.

2 is likely easier for most server implementations to start with, and if we find we really want scoping (for instance, if we wanted to have document scoped variables), then we could always shift to 1 incrementally.

[mjmahone]

Concern: Fields Can Merge Validation Complexity

This was brought up by @andimarek in the Jan 2023 APAC WG. Is the addition of fragment variables an attack vector for algorithmic complexity and explosion? The Fields Can Merge validation is already very expensive: does this PR increase the cost?

Belief

I believe the answer is generally no, or at least: the cost is no worse than creating N independent fragments, where N is the unique combination of arguments passed in. Note, this is how Relay handles fragment arguments today (they compile each fragment spread with unique arguments into a new fragment with a unique name). Given the way to "solve" for parameterized fragments today is by having uniquely-named fragments, the complexity seems, on the surface, to be isomorphic to today's field merging validation complexity.

The "cost" seems no different from adding fields with aliases and different arguments: whenever we do encounter two fragment spreads at the same level with the same name but different arguments, we are able to stop the validation then and there, rather than traversing deeper into the fragment.

TODO: Come up with degenerate examples, test implementations

There likely is a class of degenerate usage. The question we'll need to answer is if this degeneracy is worse than the degenerate field merging case. If it's not, I do not think the degeneracy should block adoption of fragment arguments. If it is worse, we may need to figure out algorithmic shortcuts (possibly updates to validation) that would reduce the attack vector.

[josephsavona]

Two cases I can imagine are:

• Fragment variables allow you to increase validation complexity disproportionately to the size of a query, such that simple query-size checks may fail to catch some attacks. However, that already seems like a very limited/weak form of protection, and in practice dynamic checks (max number of selections allowed during validation) are a more robust method that would work even with fragment variables.
• If we allowed recursion (which Relay does), then we'd want some mechanism to bound the recursion (like Relay does) to prevent infinite queries and stack overflow errors.

[mjmahone]

On the "validation complexity explosion": I think we already experience this today.

fragment Base on User {
  ...Nested
  self(x: "one") {
    ...Nested
    self(x: "two") {
      ...Nested
    }
  }
}

fragment Nested on User {
  self {
    name
  }
}

In this case, Nested needs to be independently traversed 3 times. The total size of the query is smaller than the complexity of the validation (the validation complexity is O(spreads*fields) not O(fields)). The danger with fragment spread arguments is that now we need to treat each spread with different arguments as unique: on the other hand, we can short-circuit the validation when we have overlapping, incompatible spreads, so we may in practice see validation costs go down.

For example, compare today's attempted solution for fragment arguments with what we'd have after this change:
before

fragment UserWithPhotos on User {
  ...UserWith50Photo
  ...UserWith100Photo
  ...UserWith400Photo
}

fragment UserWith50Photo on User {
  profile_picture { ...Photo50 }
}
fragment UserWith100Photo on User {
  profile_picture { ...Photo100 }
}
fragment UserWith400Photo on User {
  profile_picture { ...Photo400 }
}

fragment Photo50 on Photo {
  id
  url(size: 50)
}
fragment Photo100 on Photo {
  id
  url(size: 100)
}
fragment Photo400 on Photo {
  id
  url(size: 400)
}

In this case, to find the validation problems, the validation must traverse through every fragment spread and every field. In practice these nested potentially-problematic spreads can be much more complex than the above and non-intuitive on where the source of the problem is.

However, with fragment arguments, not only do we reduce the overall size of the query, but also the traversals needed to identify merging problems.
after

fragment UserWithPhotos on User {
  ...UserWithPhoto(size: 50)
  ...UserWithPhoto(size: 100)
  ...UserWithPhoto(size: 400)
}

fragment UserWithPhoto($size: Int) on User {
  profile_picture { ...Photo(size: $size) }
}

fragment Photo($size: Int) on Photo {
  id
  url(size: $size)
}

Here, we only need to traverse until the non-mergable spreads. This cuts off attempting to merge all child fields from those spreads, which in practice may cut out many of our field conflicts.

The question is basically whether we expect there to be more merge problems identified at the fragment spread location. II think for people using fragment arguments, this will be the case. Given the merge-complexity-explosion attack is already available with existing fragment spread syntax, and is exacerbated by naming fragments that do the same thing with different arguments differently, I think we can be cautiously optimistic about the average complexity impact, and shouldn't be overly concerned with the worst-case complexity because we already have the same worst-case.

[captbaritone]

Concern: Ecosystem support

In order for Relay users (an JavaScript users generally) to be able to adopt this syntax, there are a number of ecosystem updates that will need to be pushed through:

1. Prettier - Prettier must upgrade to a version of graphql-js that supports this syntax
2. ESLint rules - Relay's ESLint rules must to upgrade to a version of graphql-js that supports this syntax. This is also true for other GraphQL-aware lint rules in use in the project that wants to adopt
3. In editor syntax highlighting - Relay's VSCode extension depends upon the GraphQL: Syntax Highlighting package to provide syntax highlighting. Looks like this belongs to the foundation and lives here

None of these individually are complicated updates, but due to the dependency chains they may take time to roll out, especially if we don't have someone actively driving the work.

[mjmahone]

I think the crux of these issues is that:

• We need graphql-js AST support
• Then someone needs to upgrade the tools that people depend on (especially prettier and eslint rules)
• Then we could make this syntax the default in Relay instead of using directives

[captbaritone]

Yes, that sounds right, but syntax highlighting will probably be required as well.

[mjmahone]

VSCode PR: graphql/graphiql#3019

The remaining work is essentially "get the graphql-js PR in, do a version bump, and bump prettier".

[rikki-ts]

(@acao here from work) : we can add support for fragment arguments to graphql-language-service before it lands in graphql-js fortunately, thus we can add this to codemirror-graphql, monaco-graphql, graphiql, cm6-graphql, graphql-language-service-server and vscode-graphql and the other IDE extensions using our LSP, all in a single change to graphql-language-service

[JoviDeCroock]

That would be awesome @rikki-ts I have been picking up this work again in graphql/graphql-spec#1081 and graphql/graphql-js#4015