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

Spec edits for incremental delivery, Section 3 & 7 only #1124

Open
wants to merge 8 commits into
base: incremental-integration
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions cspell.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ ignoreRegExpList:
- /[a-z]{2,}'s/
words:
# Terms of art
- deprioritization
- endianness
- interoperation
- monospace
Expand Down
191 changes: 191 additions & 0 deletions spec/Appendix C -- Examples.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
# C. Appendix: Examples

## Incremental Delivery Examples

### Example 1 - A query containing both defer and stream

```graphql example
query {
person(id: "cGVvcGxlOjE=") {
...HomeWorldFragment @defer(label: "homeWorldDefer")
name
films @stream(initialCount: 1, label: "filmsStream") {
title
}
}
}
fragment HomeWorldFragment on Person {
homeWorld {
name
}
}
```

The response stream might look like:

Payload 1, the initial response does not contain any deferred or streamed
results in the `data` entry. The initial response contains a `hasNext` entry,
indicating that subsequent payloads will be delivered. There are two Pending
Responses indicating that results for both the `@defer` and `@stream` in the
query will be delivered in the subsequent payloads.

```json example
{
"data": {
"person": {
"name": "Luke Skywalker",
"films": [{ "title": "A New Hope" }]
}
},
"pending": [
{ "id": "0", "path": ["person"], "label": "homeWorldDefer" },
{ "id": "1", "path": ["person", "films"], "label": "filmsStream" }
],
"hasNext": true
}
```

Payload 2, contains the deferred data and the first streamed list item. There is
one Completed Result, indicating that the deferred data has been completely
delivered.

```json example
{
"incremental": [
{
"id": "0",
"data": { "homeWorld": { "name": "Tatooine" } }
},
{
"id": "1",
"items": [{ "title": "The Empire Strikes Back" }]
}
],
"completed": [
{"id": "0"}
]
"hasNext": true
}
```

Payload 3, contains the final stream payload. In this example, the underlying
iterator does not close synchronously so {hasNext} is set to {true}. If this
iterator did close synchronously, {hasNext} would be set to {false} and this
would be the final response.

```json example
{
"incremental": [
{
"id": "1",
"items": [{ "title": "Return of the Jedi" }]
}
],
"hasNext": true
}
```

Payload 4, contains no incremental data. {hasNext} set to {false} indicates the
end of the response stream. This response is sent when the underlying iterator
of the `films` field closes.

```json example
{
"hasNext": false
}
```

### Example 2 - A query containing overlapping defers

```graphql example
query {
person(id: "cGVvcGxlOjE=") {
...HomeWorldFragment @defer(label: "homeWorldDefer")
...NameAndHomeWorldFragment @defer(label: "nameAndWorld")
firstName
}
}
fragment HomeWorldFragment on Person {
homeWorld {
name
terrain
}
}

fragment NameAndHomeWorldFragment on Person {
firstName
lastName
homeWorld {
name
}
}
```

The response stream might look like:

Payload 1, the initial response contains the results of the `firstName` field.
Even though it is also present in the `HomeWorldFragment`, it must be returned
in the initial payload because it is also defined outside of any fragments with
the `@defer` directive. Additionally, There are two Pending Responses indicating
that results for both `@defer`s in the query will be delivered in the subsequent
payloads.

```json example
{
"data": {
"person": {
"firstName": "Luke"
}
},
"pending": [
{ "id": "0", "path": ["person"], "label": "homeWorldDefer" },
{ "id": "1", "path": ["person"], "label": "nameAndWorld" }
],
"hasNext": true
}
```

Payload 2, contains the deferred data from `HomeWorldFragment`. There is one
Completed Result, indicating that `HomeWorldFragment` has been completely
delivered. Because the `homeWorld` field is present in two separate `@defer`s,
it is separated into its own Incremental Result.

The second Incremental Result contains the data for the `terrain` field. This
incremental result contains a `subPath` property to indicate to clients that the
path of this result can be determined by concatenating the path from the Pending
Result with id `"0"` and this `subPath` entry.

```json example
{
"incremental": [
{
"id": "0",
"data": { "homeWorld": { "name": "Tatooine" } }
},
{
"id": "0",
"subPath": ["homeWorld"],
"data": { "terrain": "desert" }
}
],
"completed": [{ "id": "0" }],
"hasNext": true
}
```

Payload 3, contains the remaining data from the `NameAndHomeWorldFragment`.
`lastName` is the only remaining field that has not been delivered in a previous
payload.

```json example
{
"incremental": [
{
"id": "1",
"data": { "lastName": "Skywalker" }
}
],
"completed": [{ "id": "1" }],
"hasNext": false
}
```
2 changes: 2 additions & 0 deletions spec/GraphQL.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,5 @@ Note: This is an example of a non-normative note.
# [Appendix: Notation Conventions](Appendix%20A%20--%20Notation%20Conventions.md)

# [Appendix: Grammar Summary](Appendix%20B%20--%20Grammar%20Summary.md)

# [Appendix: Examples](Appendix%20C%20--%20Examples.md)
111 changes: 109 additions & 2 deletions spec/Section 3 -- Type System.md
Original file line number Diff line number Diff line change
Expand Up @@ -794,8 +794,9 @@ And will yield the subset of each object type queried:
When querying an Object, the resulting mapping of fields are conceptually
ordered in the same order in which they were encountered during execution,
excluding fragments for which the type does not apply and fields or fragments
that are skipped via `@skip` or `@include` directives. This ordering is
correctly produced when using the {CollectFields()} algorithm.
that are skipped via `@skip` or `@include` directives or temporarily skipped via
`@defer`. This ordering is correctly produced when using the {CollectFields()}
algorithm.
robrichard marked this conversation as resolved.
Show resolved Hide resolved

Response serialization formats capable of representing ordered maps should
maintain this ordering. Serialization formats which can only represent unordered
Expand Down Expand Up @@ -2162,3 +2163,109 @@ to the relevant IETF specification.
```graphql example
scalar UUID @specifiedBy(url: "https://tools.ietf.org/html/rfc4122")
```

### @defer

GraphQL implementations are not required to implement the `@defer` and `@stream`
directives. If either or both of these directives are implemented, they must be
implemented according to this specification. GraphQL implementations that do not
support these directives must not make them available via introspection. The
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clarify what is meant by implementation here? My current understanding is that an "implementation" is typically a library, such as graphql-js or graphql-java while a "service" is typically a server or a collection of servers, such as the GitHub or Shopify APIs.

If that's the case, the "not required" part could be confusing. If I'm writing the graphql-cobol library, can I decide to leave @defer out and still claim compatibility with the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martinbonnin thanks for reviewing. I updated this to say services instead of implementations as I was referring to servers here

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification 🙏

Would it make sense to use language similar to @specifiedBy and @oneOf:

GraphQL implementations that support the type system definition language should provide the
@defer directive if representing custom scalar definitions.

This is the closest to feature discovery we have in GraphQL and using common language pattern would make the spec more palatable. Or is there something fundamentally different about @defer and @stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this applies to @defer and @stream, because they are directives that apply to "executable locations", whereas @specifiedBy and @oneOf only apply to "type system location".

I think a GraphQL server could not support the "type system definition language" but still support @defer and @stream. Clients could determine if @defer and @stream are supported by using the introspection query:

query {
  __schema {
    directives {
      name
    }
  }
}

I think the inverse could also be true. A GraphQL Server that supports the type system definition language may have no desire to support @defer/@stream and that's ok since we want this feature to be strictly optional.

It's possible I'm not thinking about this correctly, let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies in advance, a bunch of nitpicking on the vocabulary/glossary is coming below.

a GraphQL server could not support the "type system definition language" but still support @defer

I don't think it makes sense for a GraphQL server to support the "type system definition language". A server reads request in and writes responses out. SDL is an implementation detail for the server.

But maybe not for its implementation (==lib?). A lib takes SDL in and generates a compliant service out of it. I always thought of implementation in the spec as being the "lib" but re-reading it, I'm not sure any more.

Regardless, if we use this service vs implementation distinction, I would rephrase like so:

A GraphQL implementation that does not support the "type system definition language"  can
still support @defer and @stream

Clients can determine if @defer and @stream are supported by using the introspection 
query against a service.

A GraphQL service may have no desire to support @defer/@stream and that's ok since we 
want this feature to be strictly optional. 

I think this is the same for @oneOf and @specifiedBy:

A GraphQL implementation that does not support the "type system definition language"  can
still support @oneOf and @specifiedBy

Clients can determine if @oneOf and @specifiedBy are supported by using the introspection 
query against a service.

A GraphQL service may have no desire to support @oneOf and @specifiedBy and that's ok since we 
want this feature to be strictly optional. 

That's why I'm tempted to put them in the same bucket of features even if @defer is obviously quite important for clients to be aware of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martinbonnin I am definitely open to changing this, just want to talk it through to make sure I fully understand.

Is this statement true for @oneOf and @specifiedBy?

A GraphQL implementation that does not support the "type system definition language" can
still support @oneOf and @specifiedBy

Since @oneOf and @specifiedBy are directives that can only be applied to grammar that is part of the "type system definition language", how can an implementation support this directive without supporting the grammar that is required to place the directive? I agree that specifiedByURL introspection field may be supported but this is not the same as the directive.

The same is not true for @defer and @stream because these directives are applied to the execution grammar.

A GraphQL service may have no desire to support @oneOf and @specifiedBy and that's ok since we
want this feature to be strictly optional.

Again, I could be wrong about this but my understand for @specifiedBy is that ideally all GraphQL servers would support it, but that's not possible for servers written before they were added to the spec, so RFC 2119 should is used. Additionally, we further restrict it to "implementations that support the type system definition language" since @specifiedBy can only be placed in the type system grammar.

For @defer and @stream we don't want to use RFC 2119 should/recommended language since it is optional and we expect some servers will choose not to implement it for reasons other than being behind on the latest spec changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

my understand for @SpecifiedBy is that ideally all GraphQL servers would support it, but that's not possible for servers written before they were added to the spec, so RFC 2119 should is used.
[...]
For @defer and @stream we don't want to use RFC 2119 should/recommended language since it is optional and we expect some servers will choose not to implement it for reasons other than being behind on the latest spec changes.

Gotcha, thanks! So we've got:

  1. RFC 2119 must for stuff that all services must support (@include for an example)
  2. RFC 2119 should to allow older services to still be compatible but newer services should support (@specifiedBy for an example)
  3. explicit language like here for stuff that is completely optional (@defer for an example)

Did I get that right?

If yes, I was missing the 2. vs 3. distinction, thanks for pointing that out 🙏

I'm still not 100% sold on it though. I kind of like that other directives all use the same sentence and feels a bit weird that @defer is not following the pattern (or maybe it's just my OCD...).

What about using RFC 2119 may to denote that it's "more" optional than @specifiedBy? Was that ever mentioned?

:: A _built-in directive_ is any directive defined within this specification.

GraphQL implementations should provide the `@skip` and `@include` directives.

GraphQL implementations that support the type system definition language must
provide the `@deprecated` directive if representing deprecated portions of the
schema.

GraphQL implementations that support the type system definition language should
provide the `@specifiedBy` directive if representing custom scalar definitions.

GraphQL implementations that support the type system definition language may
provide the `@defer` directive if they support returning response streams.

GraphQL implementations that support the type system definition language may
provide the `@stream` directive if they support incrementally delivered results.

I don't want to bikeshed this too much especially if it was discussed already. Is there any reading material and/or pointers I could catch up on?

A GraphQL implementation that does not support the "type system definition language" can
still support @OneOf and @SpecifiedBy

Since @OneOf and @SpecifiedBy are directives that can only be applied to grammar that is part of the "type system definition language", how can an implementation support this directive without supporting the grammar that is required to place the directive?

It can't but it's fine?

My reading of A GraphQL implementation that does not support the "type system definition language" is a code-first GraphQL implementation like graphql-kotlin for an example. It has nothing to do with supporting the grammar or anything like this. A code-first implementation can implement @specifiedBy.

Supporting the "type system definition language" is a feature of some GraphQL libs that has nothing to do with how the grammar is internally implemented.

I read all of these sentences as: "if a lib reads a SDL written by a user, then the lib must/should/may provide the built in directive definitions":

user SDL + built-in directives == final schema

The same is not true for @defer and @stream because these directives are applied to the execution grammar.

The way I see it the directive location (executable or type system) is irrelevant. These sentences are about the directive definitions that must/should/may be added to the resulting schema.

But again this is my very personal understanding of it and maybe PTSD from the full schemas discussion. Thanks for diving into this with me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martinbonnin I agree that we should probably rewrite this to use RFC 2119 may or optional to clearly state that it's an optional feature instead of recommended.

I still don't think we should include that support the type system definition language for anything related to @defer or @stream because they are features that are unrelated to the SDL.

I think a GraphQL server that creates everything programmatically fits the definition of an implementation that does not support the type system definition. My interpretation is that the spec is not providing any sort of guidance for the @specifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ....

But for @defer and @stream it is important that this type of implementation follow this guidance regardless of SDL support so we should not include the same phrase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't think we should include that support the type system definition language for anything related to @defer or @stream because they are features that are unrelated to the SDL.

I see it now! Thanks for explaining 🙏

My interpretation is that the spec is not providing any sort of guidance for the @SpecifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ...

Agreed here as well 👍 But I also think this may be a problem?

Do we have somewhere in the spec where we say what built-in directives are required in a valid full/final schema and which ones are not? I initially thought this was the place but since it excludes code-first implementations it can't reasonnably be it?

But for @defer and @stream it is important that this type of implementation follow this guidance regardless of SDL support so we should not include the same phrase.

Right 👍 I actually like the sentence as it is now because it applies to services. I think using service here is correct since SDL support is irrelevant for a service.

Finally, I'd say there should be a similar sentence for other directives? @include/@skip come to mind for an example because they are used in executable locations. Can a service ignore them? But I would probably do it for all directives for symmetry and consistency reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@include/@skip come to mind for an example because they are used in executable locations.

For @include/@skip, the spec does have the following sentence at the beginning of this section

GraphQL implementations should provide the `@skip` and `@include` directives.

Do we have somewhere in the spec where we say what built-in directives are required in a valid full/final schema and which ones are not? I initially thought this was the place but since it excludes code-first implementations it can't reasonnably be it?

I do think this is the correct place, and with the above sentence for @skip and @include we are covered for all SDL and non-SDL directives. But even @skip and @include are not required since the spec is using RFC 2119 should.

My interpretation is that the spec is not providing any sort of guidance for the @specifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ...

Agreed here as well 👍 But I also think this may be a problem?

Perhaps! Maybe it's worth a WG discussion to clarify?

[Directives Are Defined](#sec-Directives-Are-Defined) validation rule will
prevent GraphQL Operations containing the `@defer` or `@stream` directive from
being executed by a GraphQL service that does not implement these directives.

```graphql
directive @defer(
label: String
if: Boolean! = true
) on FRAGMENT_SPREAD | INLINE_FRAGMENT
```

The `@defer` directive may be provided for fragment spreads and inline fragments
to inform the executor to delay the execution of the current fragment to
indicate deprioritization of the current fragment. A query with `@defer`
directive will cause the request to potentially return multiple responses, where
deferred data is delivered in subsequent responses. `@include` and `@skip` take
precedence over `@defer`.
robrichard marked this conversation as resolved.
Show resolved Hide resolved

```graphql example
query myQuery($shouldDefer: Boolean) {
robrichard marked this conversation as resolved.
Show resolved Hide resolved
user {
name
...someFragment @defer(label: "someLabel", if: $shouldDefer)
}
}
fragment someFragment on User {
id
profile_picture {
uri
}
}
```

#### @defer Arguments

- `if: Boolean! = true` - When `true`, fragment _should_ be deferred (see
related note below). When `false`, fragment will not be deferred and data will
be included in the initial response. Defaults to `true` when omitted.
robrichard marked this conversation as resolved.
Show resolved Hide resolved
- `label: String` - May be used by GraphQL clients to identify the data from
responses and associate it with the corresponding defer directive. If
provided, the GraphQL service must add it to the corresponding pending object
in the response. `label` must be unique label across all `@defer` and
`@stream` directives in a document. `label` must not be provided as a
variable.
robrichard marked this conversation as resolved.
Show resolved Hide resolved

### @stream

```graphql
directive @stream(
label: String
if: Boolean! = true
initialCount: Int = 0
) on FIELD
```

The `@stream` directive may be provided for a field of `List` type so that the
backend can leverage technology such as asynchronous iterators to provide a
partial list in the initial response, and additional list items in subsequent
responses. `@include` and `@skip` take precedence over `@stream`. The
[Stream Directives Are Used On List Fields](#sec-Stream-Directives-Are-Used-On-List-Fields)
validation rule is used to prevent the `@stream` directive from being applied to
a field that is not a `List` type.
robrichard marked this conversation as resolved.
Show resolved Hide resolved

```graphql example
query myQuery($shouldStream: Boolean) {
robrichard marked this conversation as resolved.
Show resolved Hide resolved
user {
friends(first: 10) {
nodes @stream(label: "friendsStream", initialCount: 5, if: $shouldStream)
benjie marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
```

#### @stream Arguments

- `if: Boolean! = true` - When `true`, field _should_ be streamed (see related
note below). When `false`, the field will not be streamed and all list items
will be included in the initial response. Defaults to `true` when omitted.
- `label: String` - May be used by GraphQL clients to identify the data from
responses and associate it with the corresponding stream directive. If
provided, the GraphQL service must add it to the corresponding pending object
in the response. `label` must be unique label across all `@defer` and
`@stream` directives in a document. `label` must not be provided as a
variable.
- `initialCount: Int` - The number of list items the service should return as
part of the initial response. If omitted, defaults to `0`. A field error will
be raised if the value of this argument is less than `0`.

Note: The ability to defer and/or stream parts of a response can have a
robrichard marked this conversation as resolved.
Show resolved Hide resolved
potentially significant impact on application performance. Developers generally
need clear, predictable control over their application's performance. It is
highly recommended that GraphQL services honor the `@defer` and `@stream`
directives on each execution. However, the specification allows advanced use
cases where the service can determine that it is more performant to not defer
and/or stream. Therefore, GraphQL clients _must_ be able to process a response
that ignores the `@defer` and/or `@stream` directives. 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.
Comment on lines +2274 to +2277
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I didn't realise we had applied this to initialCount - I thought it was either non-deferred (i.e. give you the whole list) or deferred with initialCount supplied.

I think we should revisit this discussion, it's quite different to the general "don't defer" and "don't stream" optimizations in my mind - specifically if you specify initialCount: 2 I'd argue that at least 2 results should be supplied, or the entire thing should not be deferred (e.g. if there are fewer than 2 results). Skipping @defer / @include result in the client getting more data up front, but ignoring initialCount allows for the client to get less data up front, and that's a problem to my mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to revisit this discussion. If I am remembering correctly, the arguments for this version were:

  • We are still "highly recommending" servers send the correct number of list items earlier in this paragraph
  • This allows the most flexibility, clients shouldn't make any assumptions about the length of the initial list
  • From a type perspective clients should already not be making assumptions about the length of the list

Loading
Loading