added mutation op type to examples 193, 194 #984
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Note the line above:
For example, given the following selection set to be executed serially:
We are talking about execution of selection sets here, not operations.
Previous comment: #957 (comment)
This PR should be closed in my opinion.
There was a problem hiding this comment.
That is correct and is stated in the section above.
When executing a mutation, the selections in the top most selection set will be
executed in serial order, starting with the first appearing field textually.
Note: clicked approve instead of reject. As stated above the example is correct when reading the section above the example.
|
@benji |
|
Just for reference (I also stumbled over this and thought of it as a mistake at first), I opened a PR some months back that proposes an additional note that makes it very explicit that these examples depict selection sets. #926 |
|
What I really do not understand is this RESISTANCE to this simple change. If we add 'mutation' - will it change the story you are telling here, about executing selection sets? I think NO, the message/text will be the same. What it will do is - make the example into a VALID GraphQL executable document, and will shut up the annoying guys like Roman, who keep asking a question like "Why we show invalid document in an example, but format/color it as a CORRECT doc"?! If you say it is not a complete doc, it's just part - then just give me an example of full valid executable doc with this part included. I can think about just one way to do it - same thing with 'mutation' before the first opening brace. |
|
@romanivantsov it would be inconsistent with the spec text that states:
It looks at the execution behavior for the selection set and not for the operation as a whole. @benjie I think an extra note can make this clearer, although the text is pretty clear on this. In general this is something people stumbled on more than once so I think lets add a note from #926 (merge it) and close this #984. |
|
@michaelstaib Clearly I am not the only one who finds it confusing |
|
There are not |
|
Or more clearly I would say its about the executor and how the executor will execute a given selection set. You would not have a mutation executor or a query executor but executor dealing with a certain execution behavior. graphql-dotnet for instance implemented these as strategies, we had the same in Hot Chocolate with version 10 before we moved to execution plans. in graphql-js it looks like this: function executeFieldsSerially(
exeContext: ExecutionContext,
parentType: GraphQLObjectType,
sourceValue: unknown,
path: Path | undefined,
fields: Map<string, ReadonlyArray<FieldNode>>,
): PromiseOrValue<ObjMap<unknown>> {
return promiseReduce(
fields.entries(),
(results, [responseName, fieldNodes]) => {
const fieldPath = addPath(path, responseName, parentType.name);
const result = executeField(
exeContext,
parentType,
sourceValue,
fieldNodes,
fieldPath,
);
if (result === undefined) {
return results;
}
if (isPromise(result)) {
return result.then((resolvedResult) => {
results[responseName] = resolvedResult;
return results;
});
}
results[responseName] = result;
return results;
},
Object.create(null),
);
}Where you get passed in a list of grouped fields. This section explains to the reader how such an executor shall do its work not how a mutation is executed. Further these executors can also be used in other cases. For instance, if we detect that we have a scoped ef core context in a selections set we will make sure to use serial execution since it is not thread safe. Just as an example that relates more to your world of thinking. |
|
We have mooted the idea of having GraphQL provide serial execution in more selection set locations in the past a few times (e.g. when talking about nested mutations if I recall correctly); so it's important to talk about serial execution of a selection set, rather than specifically execution of a mutation operation. |
|
This comment surprises me. If this has been mooted several times... presumably we do not expect this to change?... and there is only one case of serial execution by spec... root mutation fields, so wouldn't it be misleading to speak of serial execution generally in term of selection sets? |
|
It’s never been ruled out, it just doesn’t have a champion yet. GraphQL has many things like that, waiting for someone to pick up the torch and push them forward. |
|
Guys, I do not mind an example showing 'serial execution of selection sets', for whatever reason the SERIAL execution is selected. Service is free to choose any way. It is the names of the fields: changeBirthday, changeTheNumber - these are clearly mutating fields, which means the operation is mutation. The example GraphQL is 'internally inconsistent'. What if you change the names to getBirthday, getTheNumber? it would make the example internally consistent - it is a query, or part of the query subtree. And then, since the service can execute it 'normally' (either parallel or serial), you can say: "Assuming the service chooses to execute this selection set serially, then here's how it should do it etc". I would absolutely have no objections, everything is consistent now. |
|
Right, but I guess my point is that the spec as currently defined only has one serial execution and so it does NOT seem currently important to describe how that might work generally within the spec, as opposed to how it works in the context of mutations, which is the only selection set in which it is specified to work. Either way, I agree with @rivantsov that the current text is ambiguous and confusing, with a change helpful. I am ultimately agnostic, however, as to whether we should change the example to include the mutation operation or just add a note. |
|
@yaacovCR , not sure what you mean:
the spec dictates SERIAL for mutation, but for queries it leaves the freedom of choice to service implementors; it is called 'execute normally' meaning parallel or serial, service's choice |
|
Indeed, implementation choice, so as you say, the example is talking about a mutation. The only reason that serial execution should be described in the spec is because of mutation root fields. Otherwise it is no different than reverse serial order, which is an implementation choice, but not described. @benjie is correct that the example is referring to the selection set of a mutation rather than a query, and is correct as written, but as there is no way to distinguish within the example text between a default query operation and a selection set, I favor a change, either a note, or a change to reference the whole document. At this exact point, I favor a note, because I think in the long run it may be beneficial to remind spec readers that the spec may refer to a selection set, and get them used to that rather than have them always assume a leading curly brace is a query operation. It would be kind of limiting if we said the spec could never refer to selection sets in ANY example without including an operation... |
|
A mutation operation is not the only place in which a selection set must be executed serially even in the current GraphQL spec; both inline and named fragments may also be executed serially when referenced inside the mutation type: mutation {
... {
doTheThing
}
...Frag
}
fragment Frag on Mutation {
doTheThingAgain: doTheThing
} |
|
This is a duplicate of #926 |
The examples clearly show mutating fields (changeBirthday); since mutating fields are allowed only on top level, we clearly see the 'top' level of mutation request; however, there is no op type 'mutation' before the first opening brace. If op type is missing, it is assumed to be query.