Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forbid @skip and @include directives in subscription root selection #3974

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 63 additions & 14 deletions src/execution/collectFields.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { AccumulatorMap } from '../jsutils/AccumulatorMap.js';
import type { ObjMap } from '../jsutils/ObjMap.js';

import type {
DirectiveNode,
FieldNode,
FragmentDefinitionNode,
FragmentSpreadNode,
Expand All @@ -23,7 +24,11 @@ import { typeFromAST } from '../utilities/typeFromAST.js';

import type { GraphQLVariableSignature } from './getVariableSignature.js';
import type { VariableValues } from './values.js';
import { getDirectiveValues, getFragmentVariableValues } from './values.js';
import {
experimentalGetArgumentValues,
getDirectiveValues,
getFragmentVariableValues,
} from './values.js';

export interface DeferUsage {
label: string | undefined;
Expand Down Expand Up @@ -52,6 +57,8 @@ interface CollectFieldsContext {
runtimeType: GraphQLObjectType;
visitedFragmentNames: Set<string>;
hideSuggestions: boolean;
forbiddenDirectiveInstances: Array<DirectiveNode>;
forbidSkipAndInclude: boolean;
}

/**
Expand All @@ -71,9 +78,11 @@ export function collectFields(
runtimeType: GraphQLObjectType,
selectionSet: SelectionSetNode,
hideSuggestions: boolean,
forbidSkipAndInclude = false,
): {
groupedFieldSet: GroupedFieldSet;
newDeferUsages: ReadonlyArray<DeferUsage>;
forbiddenDirectiveInstances: ReadonlyArray<DirectiveNode>;
} {
const groupedFieldSet = new AccumulatorMap<string, FieldDetails>();
const newDeferUsages: Array<DeferUsage> = [];
Expand All @@ -84,10 +93,16 @@ export function collectFields(
runtimeType,
visitedFragmentNames: new Set(),
hideSuggestions,
forbiddenDirectiveInstances: [],
forbidSkipAndInclude,
};

collectFieldsImpl(context, selectionSet, groupedFieldSet, newDeferUsages);
return { groupedFieldSet, newDeferUsages };
return {
groupedFieldSet,
newDeferUsages,
forbiddenDirectiveInstances: context.forbiddenDirectiveInstances,
};
}

/**
Expand Down Expand Up @@ -119,6 +134,8 @@ export function collectSubfields(
runtimeType: returnType,
visitedFragmentNames: new Set(),
hideSuggestions,
forbiddenDirectiveInstances: [],
forbidSkipAndInclude: false,
};
const subGroupedFieldSet = new AccumulatorMap<string, FieldDetails>();
const newDeferUsages: Array<DeferUsage> = [];
Expand Down Expand Up @@ -166,7 +183,12 @@ function collectFieldsImpl(
switch (selection.kind) {
case Kind.FIELD: {
if (
!shouldIncludeNode(selection, variableValues, fragmentVariableValues)
!shouldIncludeNode(
context,
selection,
variableValues,
fragmentVariableValues,
)
) {
continue;
}
Expand All @@ -180,6 +202,7 @@ function collectFieldsImpl(
case Kind.INLINE_FRAGMENT: {
if (
!shouldIncludeNode(
context,
selection,
variableValues,
fragmentVariableValues,
Expand Down Expand Up @@ -224,7 +247,12 @@ function collectFieldsImpl(

if (
visitedFragmentNames.has(fragName) ||
!shouldIncludeNode(selection, variableValues, fragmentVariableValues)
!shouldIncludeNode(
context,
selection,
variableValues,
fragmentVariableValues,
)
) {
continue;
}
Expand Down Expand Up @@ -320,26 +348,47 @@ function getDeferUsage(
* directives, where `@skip` has higher precedence than `@include`.
*/
function shouldIncludeNode(
context: CollectFieldsContext,
node: FragmentSpreadNode | FieldNode | InlineFragmentNode,
variableValues: VariableValues,
fragmentVariableValues: VariableValues | undefined,
): boolean {
const skip = getDirectiveValues(
GraphQLSkipDirective,
node,
variableValues,
fragmentVariableValues,
const skipDirectiveNode = node.directives?.find(
(directive) => directive.name.value === GraphQLSkipDirective.name,
);
if (skipDirectiveNode && context.forbidSkipAndInclude) {
context.forbiddenDirectiveInstances.push(skipDirectiveNode);
return false;
}
const skip = skipDirectiveNode
? experimentalGetArgumentValues(
skipDirectiveNode,
GraphQLSkipDirective.args,
variableValues,
fragmentVariableValues,
context.hideSuggestions,
)
: undefined;
if (skip?.if === true) {
return false;
}

const include = getDirectiveValues(
GraphQLIncludeDirective,
node,
variableValues,
fragmentVariableValues,
const includeDirectiveNode = node.directives?.find(
(directive) => directive.name.value === GraphQLIncludeDirective.name,
);
if (includeDirectiveNode && context.forbidSkipAndInclude) {
context.forbiddenDirectiveInstances.push(includeDirectiveNode);
return false;
}
const include = includeDirectiveNode
? experimentalGetArgumentValues(
includeDirectiveNode,
GraphQLIncludeDirective.args,
variableValues,
fragmentVariableValues,
context.hideSuggestions,
)
: undefined;
if (include?.if === false) {
return false;
}
Expand Down
42 changes: 42 additions & 0 deletions src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,48 @@ describe('Validate: Subscriptions with single field', () => {
]);
});

it('fails with @skip or @include directive', () => {
expectErrors(`
subscription RequiredRuntimeValidation($bool: Boolean!) {
newMessage @include(if: $bool) {
body
sender
}
disallowedSecondRootField @skip(if: $bool)
}
`).toDeepEqual([
{
message:
'Subscription "RequiredRuntimeValidation" must not use `@skip` or `@include` directives in the top level selection.',
locations: [
{ line: 3, column: 20 },
{ line: 7, column: 35 },
],
},
]);
});

it('fails with @skip or @include directive in anonymous subscription', () => {
expectErrors(`
subscription ($bool: Boolean!) {
newMessage @include(if: $bool) {
body
sender
}
disallowedSecondRootField @skip(if: $bool)
}
`).toDeepEqual([
{
message:
'Anonymous Subscription must not use `@skip` or `@include` directives in the top level selection.',
locations: [
{ line: 3, column: 20 },
{ line: 7, column: 35 },
],
},
]);
});

it('skips if not subscription type', () => {
const emptySchema = buildSchema(`
type Query {
Expand Down
32 changes: 23 additions & 9 deletions src/validation/rules/SingleFieldSubscriptionsRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ function toNodes(fieldDetailsList: FieldDetailsList): ReadonlyArray<FieldNode> {
* Subscriptions must only include a non-introspection field.
*
* A GraphQL subscription is valid only if it contains a single root field and
* that root field is not an introspection field.
* that root field is not an introspection field. `@skip` and `@include`
* directives are forbidden.
*
* See https://spec.graphql.org/draft/#sec-Single-root-field
*/
Expand All @@ -45,14 +46,27 @@ export function SingleFieldSubscriptionsRule(
fragments[definition.name.value] = { definition };
}
}
const { groupedFieldSet } = collectFields(
schema,
fragments,
variableValues,
subscriptionType,
node.selectionSet,
context.hideSuggestions,
);
const { groupedFieldSet, forbiddenDirectiveInstances } =
collectFields(
schema,
fragments,
variableValues,
subscriptionType,
node.selectionSet,
context.hideSuggestions,
true,
);
if (forbiddenDirectiveInstances.length > 0) {
context.reportError(
new GraphQLError(
operationName != null
? `Subscription "${operationName}" must not use \`@skip\` or \`@include\` directives in the top level selection.`
: 'Anonymous Subscription must not use `@skip` or `@include` directives in the top level selection.',
{ nodes: forbiddenDirectiveInstances },
),
);
return;
}
if (groupedFieldSet.size > 1) {
const fieldDetailsLists = [...groupedFieldSet.values()];
const extraFieldDetailsLists = fieldDetailsLists.slice(1);
Expand Down
Loading