Spec edits for incremental delivery, Section 3 & 7 only #1124
Conversation
robrichard
force-pushed
the
incremental-integration-response
branch
from
6734003
to
abafb76
Compare
There was a problem hiding this comment.
Thank you so much @robrichard for this amazingly written edits! What a great progress!! See my inline comments.
And let me know if you want me to raise the error handling as a separate PR or a separate topic in WG discussion.
spec/Section 3 -- Type System.md
Outdated
| 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. | ||
|
|
There was a problem hiding this comment.
Nits: let's move this after the @deprecated section because these are not required. Also should we mention that implementations not implementing @defer and @stream when facing a request with @defer and @stream will ignore these directives? (this might be specified somewhere else already as a general rule for unknown directive)
There was a problem hiding this comment.
Moved and added a sentence with a link to the Directives Are Defined validation rule which will fail if defer or stream is used on a service that does not implement them.
|
|
||
| - `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. |
spec/Section 3 -- Type System.md
Outdated
| 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`. |
There was a problem hiding this comment.
do you want to add a comment about validation error if this directive is applied on a non-list type?
There was a problem hiding this comment.
Added a sentence mentioning the "Stream Directives Are Used On List Fields" rule. It's a broken link for now, but we have this rule defined in the full PR
| 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 |
| last response of the stream. This entry must not be present for GraphQL | ||
| operations that return a single response map. |
There was a problem hiding this comment.
"This entry must not be present for GraphQL operations that return a single response map"
Why? Seems like an unnecessary restriction.
There was a problem hiding this comment.
We discussed this some time ago and landed on this: graphql/defer-stream-wg#57 (comment)
I think the main reason was a fear that clients may start to depend on hasNext: false being present in a single payload response, and this would break if they switch to a server that does not support defer/stream
There was a problem hiding this comment.
Ohhhhh yeah now I remember!
| Clients should expect the the GraphQL Service to incrementally deliver the | ||
| remainder of the fields contained in the deferred fragment. |
There was a problem hiding this comment.
there is some ambiguity with this statement. When I read this it seems like with a fragment X {a, b, c} which is deferred, the GraphQL service can incrementally deliver {a, b} and then deliver {c}
There was a problem hiding this comment.
I agree that it is ambiguous. I wrote this way because I was thinking that your example is technically possible. If a and b are in both a deferred and non-deferred fragment, a & b will be in the initial response and only c will be incrementally delivered. Or if a and b are in both deferred fragment X and deferred fragment Y, a & b might be delivered incrementally when fragment Y is complete, while c will be sent in a later payload.
Do you have a suggestion to clarify?
| The Incremental Object Result's `path` can be determined using the prior Pending | ||
| Result with the same `id` as this Incremental Result. The Incremental Object | ||
| Result may contain a `subPath` entry. If the `subPath` entry is present, The | ||
| Incremental Object Record's path can be determined by concatenating the Pending | ||
| Result's `path` with this `subPath`. If no `subPath` entry is present, the path | ||
| is the same as the Pending Result's `path`. | ||
|
|
There was a problem hiding this comment.
what about index? Do you want me to raise this as a separate PR?
There was a problem hiding this comment.
When we switched to the latest response format we decided not to put index in the incremental object. It's not strictly necessary since list items must be delivered in order. We were thinking it could be added in a future spec update if we want to support out of order or sparse responses. If this is an issue for you can you start a new discussion topic in https://github.com/graphql/defer-stream-wg/discussions?
robrichard
force-pushed
the
incremental-integration-response
branch
from
53cc60e
to
fcf898f
Compare
|
@Keweiqu thanks for your review! I have addressed your comments and also moved the examples into a new appendix as discussed during the working group meeting. |
spec/Section 3 -- Type System.md
Outdated
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@martinbonnin thanks for reviewing. I updated this to say services instead of implementations as I was referring to servers here
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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@oneOfand@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
@oneOfand@specifiedByand 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.
There was a problem hiding this comment.
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:
- RFC 2119
mustfor stuff that all services must support (@includefor an example) - RFC 2119
shouldto allow older services to still be compatible but newer services should support (@specifiedByfor an example) - explicit language like here for stuff that is completely optional (
@deferfor 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 @SpecifiedBySince @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!
Extracted from the full PR (#1110) and targeting an integration branch to aid in review.
Helpful reference material: