graphql-spec

graphql-spec copied to clipboard

Corresponding graphql-js pull request: https://github.com/graphql/graphql-js/pull/2319

Please keep discussion limited to specifics on how the spec text is written. There is a dedicated discussion forum for questions, feedback, and comments on defer & stream: https://github.com/robrichard/defer-stream-wg/discussions

Following up on: https://github.com/graphql/graphql-js/pull/2319#issuecomment-660164331

I am still not quite sure on use case for label with stream if different labels are not allowed. probably just missing something, but if it could be elaborated upon with an example of a conflict, it would be most appreciated.

Thanks!

@yaacovCR There's a possibility for conflict when you combine defer and stream. I posted an example here: https://gist.github.com/robrichard/0a5e9fb7b0e615545198183c2b9c95a8

Well, defer needs a label in that example, but does stream?

Thanks so much for your quick reply!!!

My unsolicited two cents from the feedback so far based on the different ways fragments are processed and the implementing code is that the directive should be named patch instead of defer, as in theory, this is not a regular fragment of fields at all, but an indication to split off a new branch of execution from the existing operation, and would be better off with a new keyword in fact but for the need to preserve backwards compatibility.

@yaacovCR in this proposal, label is an optional argument to both @defer and @stream, so you will not be required to pass it unless your client library requires it. While it's possible that there aren't any conflicts that require a label on @stream, I don't think the proposal gains any clarity by forbidding it.

For @defer I think it is important that deferred fragments are regular fragments. It is only at the location the fragment is spread that we indicate that it should be deferred. I think it's an important use case that a single fragment can be deferred in one part of the query, and used normally in another part.

Wow, you have clarified everything! Thanks so much for your time.

I do still disagree as to the name of the defer directive. It seems the first conceptual point is that a new branch of execution is being created via a patch with a response that is possibly deferred. It is up to the server in fact to indicate whether it is indeed delayed at all. Perhaps defer means deferred to another payload, rather than another timepoint, but that is somewhat unclear.

It seems like a patch is a more appropriate, more general term, as defer more aptly describes how the payload is to be delivered, which might be better off in an argument to the proposed patch directive, with more granular options even than defer.

Other than that, again, this conversation has been tremendously clarifying for me, and very helpful to me at least in organizing some initial thoughts in terms of integrating schema stitching with these changes.

I very much appreciate your time and all the work that has gone into this spec change by @lilianammmatos, yourself and all others involved, apologies to all I am missing.

Although I suppose patch is not quite right for defer either as a patch might be a stream payload...

Maybe directive @branch(deliver="defer")

@robrichard Can you please separate changes to the RFC document so I can merge them as is?

@IvanGoncharov the RFC changes were already merged here: https://github.com/graphql/graphql-spec/pull/745

I rebased this PR so that change no longer is shown in the diff.

Sorry if this has been discussed elsewhere; but what if you don't know if the current payload is going to be the final payload? Is sending a payload with just {hasNext: false} (and the path/label/etc) with no items from the iterator acceptable? I couldn't quite figure this out from a quick scan of the algorithm.

Let me back this question up with a concrete example: price comparison websites.

Imagine we visit a price comparison website, PCW, and search for home insurance. PCW's back-end goes off to it's panel of 500 home insurance providers and asks them each for a quote with your details. Some of the providers offer a quote, others decline, and they take varying amounts of time to do so up to a maximum cut-off time of 2 minutes. Lets say 90 providers give a quote.

In GraphQL without @stream this might look like a 2 minute request that results in an array of these 90 quotes. Not a very good experience, since the user sees nothing for 2 minutes.

In an ideal @stream-enabled collection, you'd get the result from each provider as it comes through, allowing you to populate the page and show the prices coming in, giving the user feedback during the 2 minute wait. Once either all providers have responded or the 2 minute timeout elapses, the stream can be ended with no additional data.

In the currently proposed @stream, if I'm reading it right, I think it's saying you'd need to keep back one result such that once the 500 providers have finished responding you can return it with hasNext: false also in the same payload. This means that the result displayed to the user would always have to be one item behind unless you know exactly how many items you're expecting.

@benjie thanks for taking a look. It is our intention for the spec to support a final payload of {hasNext: false} for exactly this type of use case. I will update the spec to make sure this is reflected. We have this covered in test cases in graphql-js https://github.com/graphql/graphql-js/pull/2319/files#diff-a370dc23e95e90e2b04fe67a3a933e6eb09296c64b4eae7509cd3ae93edaf2aeR334

Awesome :raised_hands:

@benjie I made some updates to these sections, let me know if there's anything else that's unclear.

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: robrichard / name: Rob Richard (42bd98aeb90b0cd58f8d367f99d5c8f288546eca, 583acb616b83f1e9275b0fe688059982295a8484, a4bbad68adf9c769ce071e1ad00be8722840e51c, b4fb65c378ab8691c3e88566dcb707519661a255, 3257f192bfb8c4a72043f880d96b742f144280b6, 3cce3fa41c5dcb4793fe6df15145650fe8e5a2ea, f81d74546960d8aa11c783b5fb26492316da7b5c, b0cae7f4368f84e8affa3fc90a7ace670e47605d, 9449853523deb86ca8e0fc1e60a6255df6dd82d2, c81289029f6e57541721d9fa3c848c10bb44ee11, c1fefe4870b0a08528d31ee04cf96527f3605ba4, 6dd9b79e9ff300b31fe36982652ed961ba23fa20, ebfefb6e00cbfd044dfb9b82b0300b0c6fd3b941, 30674c8434963ea5dab295f8d31921ea3c6a80b0, 101516b7d7b53900f785f4dcf339543b2b771c88, 80ff450997e75751c3f37da8c624d0cda814a170, b73af22d3c54da1c88e4c6d6890b99b37dd3da48, f634192c098589e6dc955b1529fe337538762d0d, 7eafac873417eecaf0a7cf2d34c93bf1f94a8dce, 83e058d945a96d619efe35b4db1cefced6d06f5f, 5aa915b7c910ad41e44e01096d38b13dbb573442, d7fe43aa0a754aea52a17cf681d31df97df1ad5f, 379f10caf01ac8734a0326ff257613a0121703fb, 1ffad84f9594a6b350caad061855fc643aeff146, 21dd0bc62b2d57ce48173d440e7675f17cd2baa1, 3a569b6d79dfb57ccd251f9ea98cb953b844dbee, a596eadb76b1521bf53d7c6f1e8d4fbd4ce24229, 4543a479c3e7b1414f63985701878b7eb90a3b54, 5a093cda76c550fb1167dea0d6117c5f6e45b373, 8fa426eda1f2bf3d7a16d1534311ad1dc0154b0b, fcc63936ee1603a06e7bb033a78681a65c3fb884, aa22c5ff0b78eb3edf9384b41c59aebf01815267, 089d06c75fd8b498cd49fbf8fd7667ea5a9d6fb1, a154bda6aa3942750fd2bc83598792eb47f61659, 67689c5a8bb1864c47c8f461a4537869f05fa202, 9230085f180891c43b8298c0e506c6862657bdce, 4bf09d0306496029cfddc1f47e1c56602ecb4327, 1499e640582dd8e5cfdfe43d24b7a993d784514d, 722de48877321ea65e524774284687b4ce48661c, c4bdfaff92a16fdde62931d4521d0578463fad89, d84dd0b1b6e9e369a3b987b80fa44a6034f2104e, 2248f35bfe78fe028383435ebba4d63f7782c316, 2fb409c0e1ea28be6ecd67b05ab5af62355f20cf, 66c3f7bc48b267cc1fe38807cf2799347a553eae, 9287b6291bd436bb4ea4ad0fd3b034a62c0c4180, e0c0ad9be267eebf94c6df8609f174332d2d6b05, 99baf54e52238f7da9d661d1cceca22f5a956765, 4931f7a0785d90c3fc5786a9b5174d63cd9115bf, da96e9830321e6be4e5c8d5774d0432061358d19, aff1113ebf758c5874c8195d6ef605de937838c1, fb3e05c03e9c5e76168dcf39e5857cd62e41484c, 979e581f3a1e18800ace355cf12d2eb6e00b2c9a, 120712104102e49c847b7f798df3ddea79f74d22, 6545a5587bb52ce84d191f37598895fe3fe87bb0, e4ee3eb9a9a545e0418274245ead20ff7b1bd9da, 22de5de5b892eebe8bbf4a201a2cb35301d96e4a)
• :white_check_mark: login: yaacovCR / name: Yaacov Rydzinski (039e4ebd70321a6d8eda55fb1420491ec7d5972e, 777c21b031262f6fbeef36d2383744c97d3f1c53, 7626800dd3e94094ca2c6fc56f4694ea96a2b3ca, d16a4322c082ca8e37cb2ced6a37c7a89c7b9d1b, 4c2e3f9cfe630588b326f47c13ab2a029d88cacc, c630301560d9819d33255d3ba00f548e8abbcdc6)

The committers are authorized under a signed CLA.

• :white_check_mark: Rob Richard (ab27b91fbdd99e073794789cc4e991b5772f0dac, 2c63436c3211969598725148c24225f9fc9923bd, 9b54d705034d8124a8715e7f25af75e6ae8440bd, 13a5a06743d9b68ec21188213cd3c9210cf10870, 26fd78c4a89a79552dcc0c7e0140a975ce654400)

The committers are authorized under a signed CLA.

• :white_check_mark: Rob Richard (3c6eab1fc63fd19efd93ebf256cda547a7b0ef9a, 5b79a4a570a7831018e4dd4516be2d7fb003124f, 09cba54af8d8de90ca48d8d7e2c7f8ca7724d22b, 6e4c4cca2f225f27c55416e988b0807b375f6ce7, d2e947f609d7026e097a0695598debaf1fbac6ad)

The committers are authorized under a signed CLA.

• :white_check_mark: Rob Richard (efd5e6bdd790c663389c15516551e37ecda49573, f23f8588b0e2cdfb26b23a46fa264c111ccbd3ff, 265dc4f32ca63986817beeab9e4d08cfbae4c6d4, c923a62f820871e6499ab6d3e3dbab66b33d84e7, 4ba4e6a43b60ec2e8842b70f02d02d329335b522)

The committers are authorized under a signed CLA.

• :white_check_mark: Rob Richard (427cc508732e72718e6b4d2369b4eb7d60cab958, 2ba855fce7047daa6d5af0e879a035b49a1c5cef, ef2be06ec2815ef6533641713d084148789d7f00, 8771871db24702545e15a42b54cd2fbd61b4cccc, d17f370f742139d877e3f7c277653685be7d874e)

@benjie thanks for reviewing this. I think I've either addressed or left notes on all your comments. Please let me know if there's anything I can improve.

In implementing support for schema stitching with defer, hit a bump on the road, I want to depending on the fields requested in all fragments, deferred or otherwise, want to include additional fields that may be required to stitch to another subschema. Those additional fields should not be deferred, because we want to immediately assemble all of the required key fields for the entire sub schema set.

It is not so straightforward to annotate a query with additional fields on deferred fragments where those additional fields are not deferred, because we only allow specification of defer on the fragment level, not the field level. There are other solutions possible, but one solution I thought of would be to allow specification of an "undefer" directive on the field level

This way, we could have our cake and eat it too.

loading states between the different fields that are deferred within a fragment would be coordinated, but we could also specify that some fields are not meant to be deferred.

As an example, for a query like this:

query {
  ...ProductFragment @defer
}
fragment ProductFragment on Product {
    price
    weight
}

could have an a upc field that is non-deferred easily added to it as follows:

query {
  ...ProductFragment @defer
}
fragment ProductFragment on Product {
    upc @undefer
    price
    weight
}

which would be equivalent to:

query {
  ...UndeferredProductFragment
  ...ProductFragment @defer
}
fragment UndeferredProductFragment on Product {
    upc
}
fragment ProductFragment on Product {
    price
    weight
}

Why?

Because currently we cannot specify deferred status on a per field basis, we basically have to jump through a few hoops to transform an operation AST just to add a set of non-deferred fields based on the presence of certain deferred fields. Before defer, we could just update any selectionSet anywhere in the AST with the new fields we need based on the current fields -- now we have to create new fragments/selectionSets.

Undefer should hoist up to only one field collection level, so in theory you can nest defer and undefer, examples to follow in separate PR hopefully.

@eapache thanks for your review! I've corrected the typos and incorporated your suggestions.

@robrichard @michaelstaib I’m not seeing the part which specifies field error paths as discussed in the WG meeting :sweat:

@brainkim you mean regarding the path or that errors can be in subsequent results. I will reread the spec today and catch up on the latest edits. We might just want to add a note.

What are the rules regarding duplicate defer fields with the same label. E.g.:

{
  ...GroupAdminFragment @defer(label: "groupAdminDefer")
  ...GroupAdminFragment @defer(label: "groupAdminDefer")
}

fragment GroupAdminFragment {
   managed_groups {
      id
   }
}

In https://github.com/graphql/graphql-spec/pull/742/files#diff-30a69c5a5eded8e1aea52e53dad1181e6ec8f549ca2c50570b035153e2de1c43R1964 it states that the label must be unique. This would mean the query above would be invalid, correct? Is there an additional validation needed to ensure that?

Also: can the label value be a variable? What are the use cased for labels be variables?

Question: seems implicit within spec that deferred fragments on root mutation type do not have their fields executed serially, with that special behavior just for the initial grouped field set.

That seems to match current implementation to my eye.

Is that what we want? I would think not, because then you cannot cleanly use defer to batch multiple requests of mutations (where the order of individual requests might not matter but the order of their individual root fields do).

@yaacovCR Please can you clarify your comment with some examples? Many of the terms you're using are ambiguous, for example when you say "the order of individual mutations might not matter but the order of their individual fields do" it's not 100% clear to me what you mean. At the moment I think you might mean batching mutations between multiple operations (i.e. multiple calls to ExecuteRequest()) which is outside of the scope of the GraphQL spec as it currently stands but still something we can talk about.

Edited slightly above, that’s indeed what I’m referring to. I know that request batching is not currently referenced in the spec, but I would naively expect from semantics of fragments and defer that it would be possible to achieve that.

Put batching aside, though, a request with a deferred fragment on a root mutation type would currently have different semantics than the same request sent to a server not supporting defer, where the server supporting defer would execute the fragment’s fields in parallel while the non/defer supporting server would execute in series, potentially giving different results. This just seems undesirable, I think it should be clarified at the least. :)

I personally don't think that @defer or @stream should be available within the root selection set of a Mutation or Subscription operation at this time; we should be restrictive to start with and we can lift restrictions later if there's the impetus to do so.