Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Fragment Arguments (parameterized fragments) for GraphQL #865

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions spec/Appendix B -- Grammar Summary.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,15 +168,20 @@ Arguments[Const] : ( Argument[?Const]+ )

Argument[Const] : Name : Value[?Const]

FragmentSpread : ... FragmentName Directives?
FragmentSpread : ... FragmentName Arguments? Directives?

InlineFragment : ... TypeCondition? Directives? SelectionSet

FragmentDefinition : fragment FragmentName TypeCondition Directives?
SelectionSet
FragmentDefinition : fragment FragmentName FragmentArgumentsDefinition?
TypeCondition Directives? SelectionSet

FragmentName : Name but not `on`

FragmentArgumentsDefinition : ( FragmentArgumentDefinition+ )

FragmentArgumentDefinition : Description? Variable : Type DefaultValue?
Directives[Const]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we introduce FragmentArgumentDefinition instead of reusing VariableDefinition just because we want to add Description as well? How about simply adding Description to Variable definition - let's discuss it. If Description makes sense here then it makes sense in Operation variable definition (even more I think)

Copy link
Contributor Author

@mjmahone mjmahone Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried creating an implementation where we reuse VariableDefinition, and it turned out to be more clunky than this. Fragment argument definitions need to be described everywhere as being argument definitions, not as defining a variable. It causes confusion when referring to a fragment argument's variable definition, IMO. Furthermore, in the actual implementation in graphql-js, the AST refers to arguments in the same way that a field or directive has arguments. When those arguments are defined via variable definitions, you end up over-visiting the VariableDefinition values and needing to check the context of those definitions more than if you have a unique AST value for fragment arguments.

If I could, I would have reused InputValueDefinition: logically, fragment argument definitions produce the same kind of thing in the language as field argument definitions, directive argument definitions, and input object field definitions. Ideally, I would have had FragmentArgumentDefinition : $ InputValueDefinition. This doesn't work, though, as InputValueDefinition is defined as InputValueDefinition : Description? Name : Type DefaultValue? Directives[Const]?.

I could be swayed that it makes sense to reuse VariableDefinition, so long as we don't need to update the text to describe them as anything other than argument definitions in the same way that field arguments are argument definitions in the Spec description. However, another advantage to separating the two is that we probably do not want a directive that is allowed on VARIABLE_DEFINITION to automatically be allowed on FRAGMENT_ARGUMENT_DEFINITION and vice-versa.

On the Description front, agree it probably makes sense to add Description to Operation VariableDefinition, but even so I think FragmentArgumentDefinition makes sense to separate as it is defining an input as opposed to VariableDefinition, which defines a globally available value.

TypeCondition : on NamedType

Value[Const] :
Expand Down Expand Up @@ -396,6 +401,7 @@ ExecutableDirectiveLocation : one of
- `FRAGMENT_SPREAD`
- `INLINE_FRAGMENT`
- `VARIABLE_DEFINITION`
- `FRAGMENT_ARGUMENT_DEFINITION`

TypeSystemDirectiveLocation : one of

Expand Down
79 changes: 71 additions & 8 deletions spec/Section 2 -- Language.md
Original file line number Diff line number Diff line change
Expand Up @@ -516,10 +516,10 @@ which returns the result:

## Fragments

FragmentSpread : ... FragmentName Directives?
FragmentSpread : ... FragmentName Arguments? Directives?

FragmentDefinition : fragment FragmentName TypeCondition Directives?
SelectionSet
FragmentDefinition : fragment FragmentName FragmentArgumentsDefinition?
TypeCondition Directives? SelectionSet

FragmentName : Name but not `on`

Expand Down Expand Up @@ -1209,13 +1209,76 @@ size `60`:

**Variable Use Within Fragments**

Variables can be used within fragments. Variables have global scope with a given
operation, so a variable used within a fragment must be declared in any
top-level operation that transitively consumes that fragment. If a variable is
referenced in a fragment and is included by an operation that does not define
that variable, that operation is invalid (see
Variables can be used within fragments. Operation-defined variables have global
scope with a given operation, so a variable used within a fragment must either
Copy link
Contributor

@rivantsov rivantsov Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operation-defined variables ... ... within the operation

be declared in any top-level operation that transitively consumes that fragment,
or by that same fragment as a fragment argument. If a variable is referenced in
a fragment that does not define it as an argument and is included by an
operation that does not define that variable, that operation is invalid (see
[All Variable Uses Defined](#sec-All-Variable-Uses-Defined)).

## Fragment Arguments

FragmentArgumentsDefinition : ( FragmentArgumentDefinition+ )

FragmentArgumentDefinition : Description? Variable : Type DefaultValue?
Directives[Const]?

Fragments may define locally scoped arguments, which can be used in locations
that accept variables. This allows fragments to be reused while enabling the
caller to specify the fragment's behavior.

For example, the profile picture may need to be a different size depending on
the parent context:

```graphql example
query withFragmentArguments {
user(id: 4) {
...dynamicProfilePic(size: 100)
friends(first: 10) {
id
name
...dynamicProfilePic
}
}
}

fragment dynamicProfilePic($size: Int! = 50) on User {
profilePic(size: $size)
}
```

In this case the `user` will have a larger `profilePic` than those found in the
list of `friends`.

A fragment argument is scoped to the fragment that defines it. Fragment
arguments are allowed to shadow operation variables.

```graphql example
query withShadowedVariables($size: Int) {
user(id: 4) {
...variableProfilePic
}
secondUser: user(id: 5) {
...dynamicProfilePic(size: 10)
}
}

fragment variableProfilePic on User {
...dynamicProfilePic(size: $size)
}

fragment dynamicProfilePic($size: Int!) on User {
profilePic(size: $size)
}
```

The profilePic for `user` will be determined by the variables set by the
operation, while `secondUser` will always have a profilePic of size 10. In this
case, the fragment `variableProfilePic` uses the operation-defined variable,
while `dynamicProfilePic` uses the value passed in via the fragment spread's
argument `size`.

## Type References

Type :
Expand Down
1 change: 1 addition & 0 deletions spec/Section 3 -- Type System.md
Original file line number Diff line number Diff line change
Expand Up @@ -1876,6 +1876,7 @@ ExecutableDirectiveLocation : one of
- `FRAGMENT_SPREAD`
- `INLINE_FRAGMENT`
- `VARIABLE_DEFINITION`
- `FRAGMENT_ARGUMENT_DEFINITION`

TypeSystemDirectiveLocation : one of

Expand Down
2 changes: 2 additions & 0 deletions spec/Section 4 -- Introspection.md
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ enum __DirectiveLocation {
FRAGMENT_SPREAD
INLINE_FRAGMENT
VARIABLE_DEFINITION
FRAGMENT_ARGUMENT_DEFINITION
SCHEMA
SCALAR
OBJECT
Expand Down Expand Up @@ -475,6 +476,7 @@ supported. All possible locations are listed in the `__DirectiveLocation` enum:
- {"FRAGMENT_SPREAD"}
- {"INLINE_FRAGMENT"}
- {"VARIABLE_DEFINITION"}
- {"FRAGMENT_ARGUMENT_DEFINITION"}
- {"SCHEMA"}
- {"SCALAR"}
- {"OBJECT"}
Expand Down
160 changes: 145 additions & 15 deletions spec/Section 5 -- Validation.md
Original file line number Diff line number Diff line change
Expand Up @@ -418,8 +418,14 @@ fragment directFieldSelectionOnUnion on CatOrDog {

FieldsInSetCanMerge(set):

- Let {visitedSelections} be the selections in {set} including visiting
fragments and inline fragments an applying any supplied fragment arguments.
- Let {spreadsForName} be the set of fragment spreads with a given name in
{visitedSelections}.
- Given each pair of members {spreadA} and {spreadB} in {spreadsForName}:
- {spreadA} and {spreadB} must have identical sets of arguments.
- Let {fieldsForName} be the set of selections with a given response name in
{set} including visiting fragments and inline fragments.
{visitedSelections}.
- Given each pair of members {fieldA} and {fieldB} in {fieldsForName}:
- {SameResponseShape(fieldA, fieldB)} must be true.
- If the parent types of {fieldA} and {fieldB} are equal or if either is not
Expand Down Expand Up @@ -570,6 +576,50 @@ fragment conflictingDifferingResponses on Pet {
}
```

Fragment arguments can also cause fields to fail to merge.

While the following is valid:

```graphql example
fragment commandFragment($command: DogCommand!) on Dog {
doesKnowCommand(dogCommand: $command)
}

fragment potentiallyConflictingArguments(
$commandOne: DogCommand!
$commandTwo: DogCommand!
) on Dog {
...commandFragment(command: $commandOne)
...commandFragment(command: $commandTwo)
}

fragment safeFragmentArguments on Dog {
...potentiallyConflictingArguments(commandOne: SIT, commandTwo: SIT)
}
```

it is only valid because `safeFragmentArguments` uses
`potentiallyConflictingArguments` with the same value for `commandOne` and
`commandTwo`. Therefore `commandFragment` resolves `doesKnowCommand`'s
`dogCommand:` arg to `SIT` in both cases.

However, by changing the argument values:

```graphql counter-example
fragment conflictingFragmentArguments on Dog {
...potentiallyConflictingArguments(commandOne: SIT, commandTwo: DOWN)
}
```

the response will have two conflicting versions of the `doesKnowCommand`
fragment that cannot merge.

If two fragment spreads with the same name supply different argument values,
their fields will not be able to merge. In this case, validation fails because
the fragment spread `...commandFragment(command: SIT)` and
`...commandFragment(command: DOWN)` are part of the visited selections that will
be merged.

### Leaf Field Selections

**Formal Specification**
Expand Down Expand Up @@ -647,8 +697,8 @@ query directQueryOnObjectWithSubFields {

## Arguments

Arguments are provided to both fields and directives. The following validation
rules apply in both cases.
Arguments are provided to fields, fragment spreads and directives. The following
validation rules apply in each case.

### Argument Names

Expand All @@ -657,7 +707,7 @@ rules apply in both cases.
- For each {argument} in the document:
- Let {argumentName} be the Name of {argument}.
- Let {argumentDefinition} be the argument definition provided by the parent
field or definition named {argumentName}.
field, fragment definition or directive definition named {argumentName}.
- {argumentDefinition} must exist.

**Explanatory Text**
Expand All @@ -675,16 +725,34 @@ fragment argOnRequiredArg on Dog {
fragment argOnOptional on Dog {
isHouseTrained(atOtherHomes: true) @include(if: true)
}

fragment withFragmentArg($command: DogCommand) on Dog {
doesKnowCommand(dogCommand: $command)
}

fragment usesFragmentArg on Dog {
...withFragmentArg(command: DOWN)
}
```

the following is invalid since `command` is not defined on `DogCommand`.
The following is invalid since `command` is not defined on
`Dog.doesKnowCommand`.

```graphql counter-example
fragment invalidArgName on Dog {
doesKnowCommand(command: CLEAN_UP_HOUSE)
}
```

and this is also invalid as the argument `dogCommand` is not defined on fragment
`withFragmentArg`.

```graphql counter-example
fragment invalidFragmentArgName on Dog {
...withFragmentArg(dogCommand: SIT)
}
```

and this is also invalid as `unless` is not defined on `@include`.

```graphql counter-example
Expand Down Expand Up @@ -727,9 +795,9 @@ fragment multipleArgsReverseOrder on Arguments {

### Argument Uniqueness

Fields and directives treat arguments as a mapping of argument name to value.
More than one argument with the same name in an argument set is ambiguous and
invalid.
Fields, fragment spreads and directives treat arguments as a mapping of argument
name to value. More than one argument with the same name in an argument set is
ambiguous and invalid.

**Formal Specification**

Expand All @@ -741,10 +809,11 @@ invalid.

#### Required Arguments

- For each Field or Directive in the document:
- Let {arguments} be the arguments provided by the Field or Directive.
- Let {argumentDefinitions} be the set of argument definitions of that Field
or Directive.
- For each Field, Fragment Spread or Directive in the document:
- Let {arguments} be the arguments provided by the Field, Fragment Spread or
Directive.
- Let {argumentDefinitions} be the set of argument definitions of that Field,
Fragment Spread or Directive.
- For each {argumentDefinition} in {argumentDefinitions}:
- Let {type} be the expected type of {argumentDefinition}.
- Let {defaultValue} be the default value of {argumentDefinition}.
Expand Down Expand Up @@ -1776,7 +1845,7 @@ included in that operation.
- Let {variables} be the variables defined by that {operation}.
- Each {variable} in {variables} must be used at least once in either the
operation scope itself or any fragment transitively referenced by that
operation.
operation, excluding fragments that define the same name as an argument.

**Explanatory Text**

Expand Down Expand Up @@ -1828,6 +1897,29 @@ fragment isHouseTrainedWithoutVariableFragment on Dog {
}
```

Fragment arguments can shadow operation variables: fragments that use an
argument are not using the operation-defined variable of the same name.

Likewise, it would be invalid if the variable was shadowed by a fragment
argument:

```graphql counter-example
query variableNotUsedWithinFragment($atOtherHomes: Boolean) {
dog {
...shadowedVariableFragment
}
}

fragment shadowedVariableFragment($atOtherHomes: Boolean) on Dog {
isHouseTrained(atOtherHomes: $atOtherHomes)
}
```

because
{$atOtherHomes} is only referenced in a fragment that defines it as a
locally scoped argument, the operation-defined {$atOtherHomes}
variable is never used.

All operations in a document must use all of their variables.

As a result, the following document does not validate.
Expand All @@ -1853,6 +1945,40 @@ fragment isHouseTrainedFragment on Dog {
This document is not valid because {queryWithExtraVar} defines an extraneous
variable.

### All Fragment Arguments Used

**Formal Specification**

- For every {fragment} in the document:
- Let {arguments} be the arguments defined by that {fragment}.
- Each {argument} in {arguments} must be used at least once in the fragment's
scope.

**Explanatory Text**

All arguments defined by a fragment must be used in that same fragment. Because
fragment arguments are scoped to the fragment they are defined on, if the
fragment does not use the argument, then the argument is superfluous.

For example, the following is invalid:

```graphql counter-example
query queryWithFragmentArgUnused($atOtherHomes: Boolean) {
dog {
...fragmentArgUnused(atOtherHomes: $atOtherHomes)
}
}

fragment fragmentArgUnused($atOtherHomes: Boolean) on Dog {
isHouseTrained
}
```

This document is invalid because even though `fragmentArgUnused` is spread with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'is A spread' (currently 'spread' sounds like a verb)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under "Fragment Spread is Possible", spread is used as a verb:

Fragments are declared on a type and will only apply when the runtime object type matches the type condition. They also are spread within the context of a parent type.

I think we use spread as a verb explicitly: spreading is the action that you take when applying a fragment spread.

the argument `atOtherHomes`, and even though `$atOtherHomes` is defined as an
operation variable, there is never a variable `$atOtherHomes` used within the
scope of `fragmentArgUnused`.

### All Variable Usages Are Allowed

**Formal Specification**
Expand All @@ -1861,8 +1987,12 @@ variable.
- Let {variableUsages} be all usages transitively included in the {operation}.
- For each {variableUsage} in {variableUsages}:
- Let {variableName} be the name of {variableUsage}.
- Let {variableDefinition} be the {VariableDefinition} named {variableName}
defined within {operation}.
- If the usage is within a {fragment} that defines an argument of
{variableName}:
- Let {variableDefinition} be the {ArgumentDefinition} named
{variableName} defined within {fragment}.
- Otherwise, let {variableDefinition} be the {VariableDefinition} named
{variableName} defined within {operation}.
- {IsVariableUsageAllowed(variableDefinition, variableUsage)} must be
{true}.

Expand Down
Loading