Allow empty selection sets - flag version - #4852
martinbonnin wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Co-authored-by: Benjie <benjie@jemjie.com>
|
With the parser now accepting The spec's Single Root Field rule says the collected fields map "must have exactly one entry", and this PR doesn't change that rule. if (groupedFieldSet.size > 1) {That was enough while the grammar guaranteed at least one selection. Now it lets an empty subscription through, and the same goes for Suggestion: report when For reference, Hot Chocolate's implementation of this change (ChilliCream/graphql-platform#10337) allows empty selection sets on query and mutation roots and on composite fields, but keeps rejecting an empty subscription root. Drafted by Claude (Anthropic AI assistant). |
| if (groupedFieldSet.size > 1) { | ||
| if (node.selectionSet.selections.length === 0) { | ||
| context.reportError( | ||
| new GraphQLError( | ||
| 'Subscription must select exactly one top level field.', | ||
| { nodes: node.selectionSet }, | ||
| ), | ||
| ); | ||
| } else if (groupedFieldSet.size > 1) { |
There was a problem hiding this comment.
I would just do if (groupedFieldSet.size !== 1) {, and update the error message to "must select exactly one".
There was a problem hiding this comment.
Agreed, I like the "exactly one" too. I would also always put the selection set in the GraphQLErrorOptions
export interface GraphQLErrorOptions {
/** AST node or nodes associated with this error. */
nodes?: ReadonlyArray<ASTNode> | ASTNode | null | undefined;
(compared to now the list of extra fields).
Would that be an OK thing to change @yaacovCR @benjie ? Or do we prefer to keep things as is?
There was a problem hiding this comment.
The spec says "exactly one entry, which must not be an introspection field", so I'd just echo that, personally. 👍
There was a problem hiding this comment.
Ah... the difference is the error reporting. If >1 then the additional fields are the error, if <1 then the selection set itself is the error.
Scratch what I said before; I'd keep it as-is.
|
@glen-84 sorry I forgot to reply here but the case where a subscription has no root field is now handled in this PR. Thank you and Claude for catching it. |
See graphql/graphql-spec#1227
There is a new (and already deprecated!) parser/validation option
allowEmptySelectionSets. It is true by default (new behaviour). Set it tofalseto switch back to the old behaviour.