RFC: Remove fragmentSpreadName from visitedFragments when visit is complete

[benjie]

@robrichard raised an issue for @defer whereby under the current draft specification if you have this query:

query HeroNameQuery {
  hero {
    ...NameFragment
    ...NameFragment @defer
  }
}
fragment NameFragment on Hero {
  name
}

then the @defer will never be visited; but if you move the @defer earlier:

query HeroNameQuery {
  hero {
    ...NameFragment @defer
    ...NameFragment
  }
}
fragment NameFragment on Hero {
  name
}

then the non-deferred version will never be visited.

---

@mjmahone has an RFC open about fragment arguments which suffers a similar issue: visits to a fragment aren't equivalent depending on the variables.

query HeroNameQuery {
  hero {
    ...ComponentOne
    ...ComponentTwo
  }
}
fragment ComponentOne on Hero {
  id
  ...HeroDetails(includeBio: true, includeAvatar: false)
}
fragment ComponentTwo on Hero {
  ...HeroDetails(includeBio: false, includeAvatar: true)
}
fragment HeroDetails($includeBio: Boolean!, $includeAvatar: Boolean!) on Hero {
  superheroName
  realName
  initials
  ... @include(if: $includeAvatar) {
    avatarLarge 
    avatarSmall
  }
  ... @include(if: $includeBio) {
    bioTitle
    bioSubtitle
    bioEntryParagraph
    bioQuote
    bioMainText
  }
}

Matt's solution takes a Relay-esque approach and generates a key for the fragments based on the values of their arguments.

---

Another consideration is where there are user-defined directives; e.g. { id ...Profile name ...Profile @live } might benefit from walking Profile again now that it has @live attached.

A simple solution to this is rather than each of these problems having their own solution, to simply navigate the fragment spread again - it's only a single layer that we need to worry about, so the cost is likely to be marginal. This RFC proposes this with a tiny change to the spec - after adding the fragmentSpreadName to visitedFragments, and calling CollectFields, we then remove it again.

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	0d2a3bb
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/64ff2a34db270c00087c6299
😎 Deploy Preview	https://deploy-preview-1045--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.

[martinbonnin]

Interesing!

Another consideration is where there are user-defined directives; e.g. { id ...Profile name ...Profile @live } might benefit from walking Profile again now that it has @live attached.

Do we have the same issue for fields? Assuming @live can be applied to fields, am I correct assuming the second name will not reach the resolver here?

{
  id
  name 
  # This is not passed to the resolver
  name @live
}

Because of this:

ExecuteField
1. Let field be the first entry in fields.

So the second name is still visited by CollectFields but not ExecuteField. If @live is important to the resolver, it is lost forever?

[benjie]

@martinbonnin Speaking from GraphQL.js' POV the entire of fields is made available to the resolver (via resolveInfo), so the resolver can determine that @live is present on some of the selections that represent the currently executing field. GraphQL spec does not pass any directives to resolvers, independent of order, so @live is never indicated to a resolver - that's something for the execution engine to take care of.

[benjie]

Counterpoint to my proposal:

query Malicious {
  listOfStuff {
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
    ...ManyFields
  }    
}
fragment ManyFields on ListItem {
  a
  b
  c
  # ...
  zx
  zy
  zz
}

This could multiply up expensiveness easily. But then again, assuming it's memoized it'd be negligible.

[yaacovCR]

Really interesting!

Additional potential counterpoint

Below example is attempt to explore how nested (but non-cyclic!) fragments might be handled.

Nested fragment depth is linear with respect to overall query length, but server field collection work would be non-linear with respect to the query length, potentially opening up an attack vector.

query Malicious {
  object {
    ...F1
    ...<10 more times>...
    ...F1
  }    
}

fragment F1 on SomeObject {
  ...F2
    ...<10 more times>...
  ...F2
}

fragment F2 on SomeObject {
  ...F3
    ...<10 more times>...
  ...F3
}

fragment F3 on SomeObject {
  ...F4
  ...<10 more times>...
  ...F4
}

fragment F4 on SomeObject {
  ...F5
  ...<10 more times>...
  ...F5
}

fragment F5 on SomeObject {
  ...FewFields
  ...<10 more times>...
  ...FewFields
}

fragment FewFields on SomeObject {
  a
  b
  c
}

Definitely appreciate the idea of a common solution.

One approach would be to adopt the fragment argument Relay-esque approach to include execution-level directives on fragments like @defer (and potentially @live?). This works completely with regard to fragment arguments, where every new combination of arguments is meaningful, but doesn't quite make sense with regard to @defer (and presumably @live) in that a fragment spread without @defer supersedes a spread with @defer (and presumably the opposite in the case of @live.

For differing directive usage on sibling spreads, I see 3 different scenarios:

1. Fragment spreads should fail validation if they inconsistently use a directive (akin to how we have a validation rule requiring stream directive usage to be identical for overlapping fields).
2. Fragment spreads that only sometimes use the directive should be considered AS NOT using the directive => desired behavior for @defer.
3. Fragment spreads that only sometimes use the directive should be considered AS IF using the directive => desired behavior for @live.

Self-referencing fragments with differing arguments/directives

Another point to consider is that presumably self-referencing fragment spreads with different arguments should also be allowed, as they do not form a true cycle. So we may require the Relay-esque style behavior anyway. Modifying your example above:

fragment ComponentOne on Hero {
  id
  ...HeroDetails(includeBio: true, includeAvatar: false)
  ...SubComponentA
}
fragment SubComponentA on Hero {
  ...HeroDetails(includeBio: false, includeAvatar: true)
}

For @defer, currently nesting a deferred fragment at the same level under a deferred or vice a versa would be meaningless. For @live, it could be used to ensure that wherever the fragment is used, it would also get the directive. Not sure if that should be encouraged!

[benjie]

I don't think we'll allow a fragment to reference itself even if it has different arguments.

I think we should allow fragments to be specified multiple times with different directives, since different components may use the same data in different ways (e.g. the logged in user's avatar may be rendered in multiple places on a web page, in some it may be deferrable, and others not - that's up to the components themselves to decide). The issue of having to align ! and ? across components is one that bothers me with CCN, but that's a different topic 😉

Assuming field collection is memoized (where the implementation knows to key it by the arguments and directives applied) then your non-linear example wouldn't be much of an issue. I considered explicitly defining this in the spec like Matt does for fragment arguments, but the risk is that someone would use a Base64-encoded avatar image as an argument to a fragment, and suddenly in your malicious query example but extended to use fragment arguments we'd have to generate that large key many times!

A more subtle option could be to track "fragments with no arguments and no directives" separately from those with either or both.

[yaacovCR]

I think we should allow fragments to be specified multiple times with different directives, since different components may use the same data in different ways (e.g. the logged in user's avatar may be rendered in multiple places on a web page, in some it may be deferrable, and others not - that's up to the components themselves to decide).

We should allow fragments to be specified by different components some with defer, some not, but once the named fragment is non-deferred by any particular component, the deferred version is superfluous and can/should be subsumed. In other words, I think this particular situation should not fail validation, but the deferred spread should ideally be ignored.

[benjie]

Yep, exact same page on that! (From an observable behavior point of view. Not necessarily from the algorithm's point of view.)