Is it mandatory for servers to honor the defer/stream directive?

[robrichard]

Decision

The spec will state that servers SHOULD honor the @defer and @stream directives.

Consequences

• The ability to defer/stream parts of a response can have a potentially significant impact on application performance. Developers generally need clear, predictable control over their application's performance. It is highly recommended that the GraphQL server honor the @defer and @stream directives on each execution. However, the specification will allow advanced use-cases where the server can determine that it is more performant to not defer/stream. Therefore, GraphQL clients must be able to process a response that ignores the defer/stream directives. See discussion with josephsavona (Proposal: Deferrable and Streamable Directives Arguments graphql-spec#704 (comment))
• This also applies to the initialCount argument on the @stream directive. Clients must be able to process a streamed response that contains a different number of initial list items than what was specified in the initialCount argument.
• We will avoid using the term "hint" to describe this behavior, since it is highly recommended that servers do not ignore defer/stream directives.
• Servers that do not support defer/stream must not ignore these directives. They should error by failing the Directives Are Defined validation rule.

Most importantly, leaving open the possibility that defer/stream can be ignored ensures that clients remain flexible. This allows for spec changes in the future that are non-breaking. If this ends up being the wrong decision, a new RFC can be brought to the working group to change SHOULD to MUST without fear of breaking clients. If we go with MUST now, it would not be possible to change to SHOULD or MAY without breaking clients.

Status

• defined in spec
• implemented in graphql-js

[yaacovCR]

I think SHOULD could just be changed to MUST.

why does the server know better than the client what is more performant, and certainly will always take more bandwidth to send rather than defer.

perhaps an argument could be added ‘@defer unlessYouThinkNot=true’

i also don’t think we can say this is implemented in graphql js, maybe I missed it, is there a case where graphql js chooses not to honor the directive?

[maraisr]

Ya I'd definitely like that better, @defer(optional: true); which leaves the server complete freedom to adhere to or not. And for moments where the client needs it to behave in a way, just optimal: false.

[yaacovCR]

We're intentionally using imprecise terms like "should" and "advanced use-cases" to be flexible enough to allow experimentation. I think once there is wide adoption, different approaches can be compared and best practices will become more apparent.

I want to add that I appreciate the above point as a value, I just think that the current compromise in terms of specification language does not do enough to reflect the nuances of the underlying reasoning.

Taking a step back, my understanding of the goal of incremental delivery is something like the following: "overall most speedy result delivery (with client side suggestion of prioritization of the specified initial payload)."

According to the proposed edits, the @defer and @stream remain the client's best guess as to optimal performance, not a hint per se...but kind of a client-side (strongly-held?) belief? Servers are allowed to experiment to provide even better performance.

From the proposed edits:

the specification allows advanced use-cases where the server can determine that it is more performant to not defer and/or stream

Are servers encouraged to experiment? Are they discouraged from experimenting? Is the specification...neutral? This is imprecise. It also gives off the impression of complexity, where in truth none need exist.

The advantage of the argument approach is that it makes clear that server experimentation is encouraged when allowed. If this suggestion is considered premature, and we still wish to allow servers to experiment without additional arguments, I think the specification should state so explicitly.

Specifically, I would suggest that we drop the simple use of SHOULD to describe how the server should approach the directives. We should instead use language something like the following:

- `if: Boolean` - When `true`, fragment *should* be deferred. When `false`, fragment will not be deferred and data will be included in the initial response. If omitted, defaults to `true`. 
+ `if: Boolean` - When `true`, fragment *should* be deferred, unless the server considers it more performant to not defer. When `false`, fragment will not be deferred and data will be included in the initial response. If omitted, defaults to `true`. 

The comment at the end should instead point out that servers are allowed to remain agnostic as to what is more performant, and simply respect the client's belief that incremental delivery is the way to go.

These are small changes, but at least accurately reflect where we currently are.

I also find the parallel between the client vs server-controlled nullability and client vs server controlled incremental delivery very interesting. The server has the best understanding of the underlying implementation, but the client has the best understanding of its needs, in both cases. I think we should do our best to align the client vs server modifers in the two cases, although I am not yet sure how this would work.

[yaacovCR]

@robrichard any thoughts on slight adjustment in spec language above?

[robrichard]

We have almost this exact wording right below that section https://github.com/graphql/graphql-spec/pull/742/files#diff-30a69c5a5eded8e1aea52e53dad1181e6ec8f549ca2c50570b035153e2de1c43R2146

the server can determine that it is more performant to not defer and/or stream

I'd prefer to keep it there where the full context is explained.

[yaacovCR]

Maybe then what we need is some language that more generally specifies that client side “directives” may indicate mandatory server behavior in some cases, but also may indicate only recommended behavior, corresponding to the must/should keywords as used in the spec. That should probably be a separate spec pr that comes before these edits. What do you think?

[n1ru4l]

I think the following questions are also related to this: graphql/graphql-over-http#167

[mjmahone]

One reason why should is better for advanced use cases (maybe not a great reason but a reason):

If you have a server that composes many child servers (so each of those child servers' schemas compose to form the parent schema): some of those child servers may implement @defer/@stream, but if even one child server responsible for even a single schema type does not support it, you either need to walk the entire tree in the parent server in order to split up the already-stitched-together response tree (if all servers that support @defer/@stream must batch every time), or we need to allow ignoring the defer/stream directives when the server chooses.

[michaelstaib]

The problem with the allow optimizations is that the client is at this point deployed and maybe does not receive further updates, but the server might change and the perf characteristics of the server might change so that at one point there might now be an optimization where defer makes no sense anymore.

[yaacovCR]

What I am suggesting is that this behavior SHOULD vs MUST is different enough to warrant supporting both. If you think clients would want SHOULD to allow optimizations then we should expect them to say so.

It sounds like you are saying that client developers may forget they can say so, and then regret it later when they realize they no longer need defer?

[michaelstaib]

No, I am saying it defeats the purpose if we introduced such a control.

[yaacovCR]

Could be. I am not understanding.

[yaacovCR]

One reason why should is better for advanced use cases (maybe not a great reason but a reason):

If you have a server that composes many child servers (so each of those child servers' schemas compose to form the parent schema): some of those child servers may implement @defer/@stream, but if even one child server responsible for even a single schema type does not support it, you either need to walk the entire tree in the parent server in order to split up the already-stitched-together response tree (if all servers that support @defer/@stream must batch every time), or we need to allow ignoring the defer/stream directives when the server chooses.

@mjmahone I just hit this example, and so understand your comment a bit better. I will flesh it out below with additional comments given the interim proposal changes.

Let's say I have a parent server with types:

type Query {
  nodes: [SomeType]
}

type SomeType {
  someField: String
  anotherField: String
}

where fulfillment of the operation is via child servers A and B with types:

Child Server A:

type SomeType {
  someField: String
}

and

Child Server B:

type SomeType {
  anotherField: String
}

with query:

query {
  nodes: {
    ... @defer {
      someField
      anotherField
    }
  }
}

What if child server A supports defer/stream, but child server B does not? Child server A may return:

{
  "data": {
    "nodes": [
      N x
      {},
    ],
  },
}

N x {
  "incremental":[
    {
      "path": ["nodes", n],
      "data": {
        "someField": "someData",
      } ,
    },
  ],
  "hasNext": usuallyTrueExceptLastN
}

and child server B may return:

{
  "data": {
    "nodes": [
      N x
      {
        "anotherField": "otherData",
      },
    ],
  },
}

We want to enable a situation where we can just forward the merged results, rather than transforming results unnecessarily. If inlining is allowed, we can merge the initial results and send the following:

{
  "data": {
    "nodes": [
      N x
      {
        "anotherField": "otherData",
      },
    ],
  },
}

N x {
  "incremental":[
    {
      "path": ["nodes", n],
      "data": {
        "someField": "someData",
      } ,
    },
  ],
  "hasNext": usuallyTrueExceptLastN
}

But the above response would not be valid if we change SHOULD to MUST. If inlining is not allowed for the parent server, we have to "walk the initial result" from child server "B" and strip out the deferred bits and reconstruct the incremental response ¹².
Note, that if inlining is allowed as according to the current proposal, then even if both child servers A and B support defer/stream, they could at any time decide to inline data, and the parent server would have to send the response-shape above, such that this would not be an uncommon scenario.

We do need to update this discussion in light of more recent discussions by the incremental delivery sub-WG. In particular, at recent meetings, it has been suggest that if defer/stream directives are to be ignored by advanced servers, they should either be honored or ignored for all of the fields within the fragment => that would render the above example response by the parent server invalid.

However, at even more recent meetings suggested that we allow incremental delivery of data in any format, but add pending/completed portions to the incremental map to indicate which paths are not complete. We could then augment the payload above with something TBD perhaps like:

{
  "data": {
    "nodes": [
      N x
      {
        "anotherField": "otherData",
      },
    ],
  },
  "pending": [
    N x
    { "path": ["nodes", n] },
  ],
  ### or maybe a more concise one-liner:
  "pending": [{ "path": ["nodes", "*"] },],
  "hasNext": true,
}

N x {
  "incremental": [
    {
      "path": ["nodes", n],
      "data": {
        "someField": "someData",
      } ,
      "completed": [{ "path": ["nodes", n] } ],
    },
  ],
  "hasNext": usuallyTrueExceptLastN
}

Footnotes

1. Alternatively, what I suggested above is that we strip the deferred bits out from the get-go in terms of what we send to B, and get a second result in that fashion, hoping that the server appropriately used dataloaders so that the second response is not too delayed. ↩

2. Note that it's not exactly the "walking the initial result" that is the extra work, as we have to walk it anyway to perform the deep merge. It's the checking of the fields within that result as to whether they need to be deferred and then the creation of the objects representing the deferred patches that presumably does add some overhead. ↩