Nested @defer in selection sets against same field

[benjie]

If you had this operation:

query Q {
  usersList {
    ... @defer {
      ...UserFrag
      ... @defer {
        ...UserFrag
        avatarUrl
        ... @defer {
          ...UserFrag
          bio
        }
      }
    }
  }
}

fragment UserFrag on User {
  id
  name
}

Am I right in thinking that nothing special prevents this? I.e. you'd end up with:

• a deferred payload of id, name
  • a nested deferred payload of id, name, avatarUrl
    • a nested deferred payload of id, name, bio

i.e. three nested (chained) deferred payloads from a single field.

I think we have to allow this (due to fragment isolation) but also... it does feel like another attack vector. I guess a server is free to obey the first defer and ignore the others, resulting in a deferred list of id, name, avatarUrl, bio.

Whilst pondering this, I was wondering if we could/should instead do flattening and make this equivalent to:

query Q {
  usersList {
    ... @defer {
      ...UserFrag
    }
    ... @defer {
      ...UserFrag
      avatarUrl
    }
    ... @defer {
      ...UserFrag
      bio
    }
  }
}

fragment UserFrag on User {
  id
  name
}

One advantage of this approach is the deferreds are no longer chained; but a disadvantage is that we can't do the solution I suggested above whereby we honour the top one only and yet still result in all fields being deferred.

If you've had discussions about this previously, I'd love to read through them - could you point me to them? I'm somewhat tempted in my own servers to limit the number of defers in the document, or potentially to always disobey nested stream/defers.

[robrichard]

@benjie am I understanding correctly that this is about the order which the payloads from these deferred fragments are returned? The prior discussion on this is here: #17. I think this case is similar to example B in the linked discussion. In both of your examples, all of the deferred fragments should begin their execution when the recursive CollectFields runs on the list item from usersList. The fact that they are nested doesn't matter because CollectFields should flatten the selection.

Looking again, my graphql-js pr implements this correctly (unit test), but I don't think it is defined correctly in the spec. I'll update the spec PR to make sure this is clear.

[benjie]

am I understanding correctly that this is about the order which the payloads from these deferred fragments are returned?

No, I'm more concerned about clients being able to command the server to do expensive operations without the client putting much effort in.

In particular I'm concerned:

1. that nesting of defers at the same "CollectFields" level doesn't seem like it has a common practical use, but is potentially an attack vector.
2. the redundant execution of the same selection set over and over, executed asynchronously which may affect some implementation's batching logic.

The fact that they are nested doesn't matter because CollectFields should flatten the selection.

I don't think they are merged in that way according to the current algorithm; the @defer existing on a fragment causes that selection set to be executed via ExecuteDeferredFragment, so they each create an "asynchronous future value" before then "running" their child selection set (which does it again). This is clearer if we give them different labels:

query Q {
  usersList {
    ... @defer(label: "A") {
      ...UserFrag
      ... @defer(label: "B") {
        ...UserFrag
        avatarUrl
        ... @defer(label: "C") {
          ...UserFrag
          bio
        }
      }
    }
  }
}

fragment UserFrag on User {
  id
  name
}

This query, which without defer would be equivalent to { usersList { id name avatarUrl bio } }, now creates three separate promises and executes the UserFrag fragment three times for each entry in usersList. Imagine that usersList itself is @stream'd too, the amount of work the server would have to put in just generating the payload for this request is significant (if it chooses to honour the @stream/@defer directives). This would require a lot of runloop ticks in the GraphQL.js Promise-based solution currently proposed, I think.

I'm wondering right now if only the root-most @stream or @defer should be honoured, and any nested ones simply ignored. Technically this is permitted by the spec in its current form, but I wonder if this is what we should actually encourage? Do we have actual use cases for nested deferreds?

[robrichard]

@benjie I will update the spec so that CollectFields does flatten the selection. That's what we agreed on in #17 and what the graphql-js implementation does, but I agree that it is not what the spec currently says. (Edit: updated now)

That said, it is definitely possible for clients to construct queries with many @defer and @stream directives. Does this fall into the same category of problems that query complexity analysis and validation attempt to solve?

[glasser]

To make sure I follow here: this means that the messages that are actually sent don't correspond one-to-one with the structure of individual @defer'd fragments, right?

I do wonder what this means for codegen...

[robrichard]

Do we have actual use cases for nested deferreds?

@mjmahone can you speak to this based on your experience at Meta? It seems to me like it would be easy to create nested defers from Relays fragment composition

[benjie]

(For context: I’m not concerned about them existing (they obviously have to because fragments), more about whether we should actually honour them or not. Do they need to be honoured, or is one level of honoured defer/stream enough for the vast majority of use cases?)

[yaacovCR]

I think this issue was wrongly decided. Although I agree with @benjie that there is potential an attack vector with multiple deferred fragments, but I am not sure that the problem is exacerbated too much with honoring nesting versus flattening.

Nesting does imply that the spread fragments have to respect a certain order, i.e. that there is a second level of "please deliver these fields with respect to other fields" going on. More formally: nested @defer fragments are an elegant way to express the desired order of several fragments, much more desirable (in my opinion) than adding arguments to @defer with priority levels. Most importantly, honoring nesting of deferred fragments respects the eventual strong goal of fragment modularity.

[leebyron]

Based on live discussion today, I'm also a little concerned. But not sure which is the right call.

One thing to specifically consider is that in larger codebases fragment references can sprawl out across many files. Local applications of defer may find themselves within nested other applications of defer.

That means sometimes nested defer are intentional statements of phasing, others are accidents of composition of modular fragments

[yaacovCR]

That means sometimes nested defer are intentional statements of phasing, others are accidents of composition of modular fragments

I think it might help to discuss this accidental composition with an example, perhaps a more realistic one than above?

suppose we have:

query Q {
  usersList {
    ...UserFrag
  }
}

fragment UserFrag on User {
  ...BasicUserFrag
  ...ExtendedUserFrag @defer
}
fragment BasicUserFrag on User {
  id
  name
}
fragment ExtendedUserFrag on User {
  bio
  ...FriendsFrag @defer
}
fragment FriendsFrag on User {
   friends {
     ...BasicUserFrag
   }
}

The idea behind this operation is that the client may which to display basic info about the user (id / name) before rendering more extensive info, perhaps info about their friends. I guess the concern is that UserFrag might end up being used in other fragments, and it encodes an order that those fragments might not need.

I think the response essentially would be to:

1. facilitate ways to colocate fragments with the code that uses them, i.e. fight fragment sprawl
2. educate developers that with this change, fragments encode not just which data is needed, but when it is needed, i.e. fragments encode which data is needed first + which data is needed second, etc.

Moreover, even if we take the current behavior -- where a deeply nested defer is only deferred with respect to the original non-deferred data but not with respect to all of the intervening deferred parent fragments -- we should remember that nested defer is still deferred. So with fragment sprawl (and fragments that perhaps use names that only describe the data they contain and not the order in which payloads are delivered) can still cause the exact same problem.

That means sometimes nested defer are intentional statements of phasing, others are accidents of composition of modular fragments

Specifically: this argument against respecting the ordering implied by the use of nested defer is also an argument against allowing defer within any fragments at all!

We could do that, but I think the better answer is colocation, education, and, of course, better naming of fragments.

[yaacovCR]

There is a possible solution that would formalize the "education" / "better naming" suggestions that would avoid footguns in all cases where developers reference a fragment that unexpectedly has a defer component:

We could introduce a new keyword to "plan" the ordering of fields/fragments, and restrict the use use of the @defer keyword to those planning groups, rather than the data fragments with which we are already familiar. This would transform my above example into the below:

query Q {
  usersList {
    ***UserPlan
  }
}

plan UserPlan on User {
  ...BasicUserFrag
  ***ExtendedUserPlan @defer
}
fragment BasicUserFrag on User {
  id
  name
}
plan ExtendedUserPlan on User {
  bio
  ...FriendsFrag @defer
}
fragment FriendsFrag on User {
   friends {
     ...BasicUserFrag
   }
}

Developers can reference "fragments" without fear of deferring anything because that kind of change in ordering is restricted to "plans". In the above example, the syntax for spreading a plan has also been changed to make it obvious to developers that they may encounter deferring behavior.

Once we are separating delivery syntax from data syntax, additional options open up:

The above query could also be syntaxed as per below:

plan {
  query Q {
    &ref: usersList {
      ...BasicUserFrag
    }
  }
  step with ref {
    bio
  }
  step with ref {
    ...FriendsFrag
  }
}

[yaacovCR]

Based on live discussion today, I'm also a little concerned. But not sure which is the right call.

One thing to specifically consider is that in larger codebases fragment references can sprawl out across many files. Local applications of defer may find themselves within nested other applications of defer.

That means sometimes nested defer are intentional statements of phasing, others are accidents of composition of modular fragments

great pick up from @benjie in today's incremental delivery wg that we should revisit this in a non-branching context!

[yaacovCR]

clarification @leebyron -- your educational/organization concern is still an issue but original issue of attack vector is less of a concern....

[robrichard]

This has been addressed by the new response format described in #69

Deferred fields are no longer duplicated in this response format

[yaacovCR]

Continuing related issue in #80