original execute should throw if defer/stream directives are present
#3722
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
yaacovCR
changed the title
execute and subscribe should throw if defer/stream are used
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
|
I like the idea of being more consistent, but maybe this is consistent in the wrong direction and it would be more informative to take the choice that is more likely to push the error to the client? Not sure.
Just to be clear: this is a statement about execute specifically: such operations should still fail validation, no? |
|
@glasser correct, they should fail validation if not in schema. In terms of this behavior being more consistent, but not ideal, maybe it would make sense in v16 to try something along that line, like providing a temporary validation rule that outlaws the use of these directives that could be used if you're not using the experimental version. Maybe providing an experimental validate where only the original validate includes this rule? Otherwise, the original execute could just ignore the directives. I don't think execute should return a map with errors once execution has begun. It might sound a bit dogmatic, but I think that way of thinking also exposes that the "error" that is occurring is not actually a field error and should be handled earlier or differently. |
|
An alternative to this is #3727. |
|
It looks to me like this PR is essentially the behavior we were shooting for. Rebased and ready to review. |
yaacovCR
added
PR: breaking change 💥
yaacovCR
changed the title
execute and subscribe should throw if defer/stream are usedexecute and subscribe should throw if defer/stream directives are present
There was a problem hiding this comment.
This seems like it would cause confusion for any users who are using GraphQL execution indirectly via software like Apollo Server/GraphQL Yoga/graphql-helix/etc rather than calling execute directly. A user-level change (adding a line to their schema) tells them to change a function that they aren't actually calling directly.
I'm not saying I object to the basic concept of throwing an error in this case (though I'm a bit skeptical) but I do wonder if the error message could be more actionable in the common case where the person putting together the data (schema) is not the person calling the named function.
We cannot return a map with only errors after execution has begun. Instead, we must throw or return a rejected promise.
…ives are present in schema.
yaacovCR
changed the title
execute and subscribe should throw if defer/stream directives are presentexecute should throw if defer/stream directives are present
@glasser -- I made the error message a bit more generic, without reference to the exact method by which to enable experimental execution features. I am not sure if this is better, definitely open to suggestions for the exact language. |
|
@yaacovCR Thanks for iterating on this. |
|
Just want to note that this PR does not affect executeSync, which appears to be able to execute queries which don't contain defer/stream against schemas that do contain defer/stream. On the one hand this is a bit of a weird inconsistency for execute to be banned but executeSync to work, but breaking executeSync here would break introspectionFromSchema which would be quite bad. |
Currently, when the defer/stream directives are not present in the schema, defer/stream will not be triggered, and so the directives will be ignored even when used with the original function.
But -- if the directives are in the schema, the original functions will sometimes throw (if the first result from
executeis synchronous), but sometimes return maps with errors (when the first result fromexecuteis async or when any (sync or async)subscribepayload has incremental results.This PR just aligns the behavior in terms of throwing or returning a map with errors. We really shouldn't return a map with errors after execution has begun.
I think another viable alternative that potentially makes more sense is to have the original
executeandsubscribefunctions ignore the directives. To enable the experimental behavior, you would need to have the directives in the schema and use the experimental functions.@glasser @robrichard @IvanGoncharov @n1ru4l