From 947f88dd7b82d84af4a754fa61a2acee042204b1 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Wed, 25 Nov 2020 17:33:48 +0000 Subject: [PATCH 01/15] Assert subscription field is not introspection. --- .../SingleFieldSubscriptionsRule-test.ts | 28 +++++++++++++++++++ .../rules/SingleFieldSubscriptionsRule.ts | 21 ++++++++++++-- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index c8ddc5add3..18155fbde8 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -83,4 +83,32 @@ describe('Validate: Subscriptions with single field', () => { }, ]); }); + + it('fails with introspection field', () => { + expectErrors(` + subscription ImportantEmails { + __typename + } + `).to.deep.equal([ + { + message: + 'Subscription "ImportantEmails" must not select an introspection top level field.', + locations: [{ line: 3, column: 9 }], + }, + ]); + }); + + it('fails with introspection field in anonymous subscription', () => { + expectErrors(` + subscription { + __typename + } + `).to.deep.equal([ + { + message: + 'Anonymous Subscription must not select an introspection top level field.', + locations: [{ line: 3, column: 9 }], + }, + ]); + }); }); diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 760fe3c144..867bc7ceae 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -6,9 +6,10 @@ import type { OperationDefinitionNode } from '../../language/ast'; import type { ASTValidationContext } from '../ValidationContext'; /** - * Subscriptions must only include one field. + * Subscriptions must only include a non-introspection field. * - * A GraphQL subscription is valid only if it contains a single root field. + * A GraphQL subscription is valid only if it contains a single root field and + * that root field is not an introspection field. */ export function SingleFieldSubscriptionsRule( context: ASTValidationContext, @@ -25,6 +26,22 @@ export function SingleFieldSubscriptionsRule( node.selectionSet.selections.slice(1), ), ); + } else { + const selection = node.selectionSet.selections[0]; + if (selection.kind === 'Field') { + const fieldName = selection.name.value; + // fieldName represents an introspection field if it starts with `__` + if (fieldName[0] === '_' && fieldName[1] === '_') { + context.reportError( + new GraphQLError( + node.name + ? `Subscription "${node.name.value}" must not select an introspection top level field.` + : 'Anonymous Subscription must not select an introspection top level field.', + node.selectionSet.selections, + ), + ); + } + } } } }, From fe4e7048bfaf29025f07fcb15009fd3625fd9d4a Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 5 Dec 2020 12:47:14 +0000 Subject: [PATCH 02/15] Walk selection set fully --- .../SingleFieldSubscriptionsRule-test.ts | 2 +- .../rules/SingleFieldSubscriptionsRule.ts | 129 +++++++++++++++--- 2 files changed, 109 insertions(+), 22 deletions(-) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index 18155fbde8..2fa8d93dec 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -45,7 +45,7 @@ describe('Validate: Subscriptions with single field', () => { `).to.deep.equal([ { message: - 'Subscription "ImportantEmails" must select only one top level field.', + 'Subscription "ImportantEmails" must not select an introspection top level field.', locations: [{ line: 4, column: 9 }], }, ]); diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 867bc7ceae..3e3f089331 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -1,10 +1,80 @@ import { GraphQLError } from '../../error/GraphQLError'; import type { ASTVisitor } from '../../language/visitor'; -import type { OperationDefinitionNode } from '../../language/ast'; +import type { + OperationDefinitionNode, + SelectionSetNode, +} from '../../language/ast'; +import { Kind } from '../../language/kinds'; import type { ASTValidationContext } from '../ValidationContext'; +/** + * Walks the selection set and returns a list of selections where extra fields + * were selected, and selections where introspection fields were selected. + */ +function walkSubscriptionSelectionSet( + context: ASTValidationContext, + selectionSet: SelectionSetNode, + responseKeys, + fieldNames = new Set(), + visitedFragmentNames = new Set(), + extraFieldSelections = [], + introspectionFieldSelections = [], +) { + for (const selection of selectionSet.selections) { + switch (selection.kind) { + case Kind.FIELD: { + const fieldName = selection.name.value; + fieldNames.add(fieldName); + const responseName = selection.alias + ? selection.alias.value + : fieldName; + responseKeys.add(responseName); + if (fieldName[0] === '_' && fieldName[1] === '_') { + // fieldName represents an introspection field if it starts with `__` + introspectionFieldSelections.push(selection); + } else if (fieldNames.size > 1 || responseKeys.size > 1) { + extraFieldSelections.push(selection); + } + break; + } + case Kind.FRAGMENT_SPREAD: { + const fragmentName = selection.name.value; + if (!visitedFragmentNames.has(fragmentName)) { + visitedFragmentNames.add(fragmentName); + const fragment = context.getFragment(fragmentName); + if (fragment) { + walkSubscriptionSelectionSet( + context, + fragment.selectionSet, + responseKeys, + fieldNames, + visitedFragmentNames, + extraFieldSelections, + introspectionFieldSelections, + ); + } + } + break; + } + case Kind.INLINE_FRAGMENT: { + walkSubscriptionSelectionSet( + context, + selection.selectionSet, + responseKeys, + fieldNames, + visitedFragmentNames, + extraFieldSelections, + introspectionFieldSelections, + ); + break; + } + } + } + return [extraFieldSelections, introspectionFieldSelections]; +} + /** * Subscriptions must only include a non-introspection field. * @@ -17,31 +87,48 @@ export function SingleFieldSubscriptionsRule( return { OperationDefinition(node: OperationDefinitionNode) { if (node.operation === 'subscription') { - if (node.selectionSet.selections.length !== 1) { + const responseKeys = new Set(); + const operationName = node.name ? node.name.value : null; + const [ + extraFieldSelections, + introspectionFieldSelections, + ] = walkSubscriptionSelectionSet( + context, + node.selectionSet, + responseKeys, + ); + if (extraFieldSelections.length > 0) { context.reportError( new GraphQLError( - node.name - ? `Subscription "${node.name.value}" must select only one top level field.` + operationName != null + ? `Subscription "${operationName}" must select only one top level field.` : 'Anonymous Subscription must select only one top level field.', - node.selectionSet.selections.slice(1), + extraFieldSelections, + ), + ); + } + if (introspectionFieldSelections.length > 0) { + context.reportError( + new GraphQLError( + operationName != null + ? `Subscription "${operationName}" must not select an introspection top level field.` + : 'Anonymous Subscription must not select an introspection top level field.', + introspectionFieldSelections, + ), + ); + } + + // Currently an empty selection set cannot be parsed so this should + // never happen; however that may change in future. + if (responseKeys.size === 0) { + context.reportError( + new GraphQLError( + operationName != null + ? `Subscription "${operationName}" must select a top level field.` + : 'Anonymous Subscription must select a top level field.', + node.selectionSet, ), ); - } else { - const selection = node.selectionSet.selections[0]; - if (selection.kind === 'Field') { - const fieldName = selection.name.value; - // fieldName represents an introspection field if it starts with `__` - if (fieldName[0] === '_' && fieldName[1] === '_') { - context.reportError( - new GraphQLError( - node.name - ? `Subscription "${node.name.value}" must not select an introspection top level field.` - : 'Anonymous Subscription must not select an introspection top level field.', - node.selectionSet.selections, - ), - ); - } - } } } }, From 14e1450f89bcbf0807afcf52fdcf57e6b903549a Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 5 Dec 2020 12:54:38 +0000 Subject: [PATCH 03/15] More tests --- .../SingleFieldSubscriptionsRule-test.ts | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index 2fa8d93dec..c415936953 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -21,6 +21,41 @@ describe('Validate: Subscriptions with single field', () => { `); }); + it('valid subscription with fragment', () => { + // From https://spec.graphql.org/draft/#example-13061 + expectValid(` + subscription sub { + ...newMessageFields + } + + fragment newMessageFields on Subscription { + newMessage { + body + sender + } + } + `); + }); + + it('valid subscription with fragment and field', () => { + // From https://spec.graphql.org/draft/#example-13061 + expectValid(` + subscription sub { + newMessage { + body + } + ...newMessageFields + } + + fragment newMessageFields on Subscription { + newMessage { + body + sender + } + } + `); + }); + it('fails with more than one root field', () => { expectErrors(` subscription ImportantEmails { @@ -70,6 +105,35 @@ describe('Validate: Subscriptions with single field', () => { ]); }); + it('fails with many more than one root field via fragments', () => { + expectErrors(` + subscription ImportantEmails { + importantEmails + ... { + moreImportantEmails + } + ...NotImportantEmails + } + fragment NotImportantEmails on Subscription { + notImportantEmails + ...SpamEmails + } + fragment SpamEmails on Subscription { + spamEmails + } + `).to.deep.equal([ + { + message: + 'Subscription "ImportantEmails" must select only one top level field.', + locations: [ + { line: 5, column: 11 }, + { line: 10, column: 9 }, + { line: 14, column: 9 }, + ], + }, + ]); + }); + it('fails with more than one root field in anonymous subscriptions', () => { expectErrors(` subscription { From 41178856830f9ffefae6e66db83c03fd478c6e0b Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 5 Dec 2020 12:57:31 +0000 Subject: [PATCH 04/15] Remove responseKeys.size > 0 assertion to appease code coverage --- .../rules/SingleFieldSubscriptionsRule.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 3e3f089331..50239834bb 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -117,19 +117,6 @@ export function SingleFieldSubscriptionsRule( ), ); } - - // Currently an empty selection set cannot be parsed so this should - // never happen; however that may change in future. - if (responseKeys.size === 0) { - context.reportError( - new GraphQLError( - operationName != null - ? `Subscription "${operationName}" must select a top level field.` - : 'Anonymous Subscription must select a top level field.', - node.selectionSet, - ), - ); - } } }, }; From 7263b835e697d5b09b9393dd55c3715cd1165416 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 5 Dec 2020 13:11:50 +0000 Subject: [PATCH 05/15] Spam more tests to attempt to appease code coverage --- .../SingleFieldSubscriptionsRule-test.ts | 54 ++++++++++++++++++- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index c415936953..f7c8cc005a 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -86,6 +86,24 @@ describe('Validate: Subscriptions with single field', () => { ]); }); + it('fails with more than one root field including aliased introspection via fragment', () => { + expectErrors(` + subscription ImportantEmails { + importantEmails + ...Introspection + } + fragment Introspection on Subscription { + typename: __typename + } + `).to.deep.equal([ + { + message: + 'Subscription "ImportantEmails" must not select an introspection top level field.', + locations: [{ line: 7, column: 9 }], + }, + ]); + }); + it('fails with many more than one root field', () => { expectErrors(` subscription ImportantEmails { @@ -110,12 +128,13 @@ describe('Validate: Subscriptions with single field', () => { subscription ImportantEmails { importantEmails ... { - moreImportantEmails + more: moreImportantEmails } ...NotImportantEmails } fragment NotImportantEmails on Subscription { notImportantEmails + deleted: deletedEmails ...SpamEmails } fragment SpamEmails on Subscription { @@ -128,7 +147,38 @@ describe('Validate: Subscriptions with single field', () => { locations: [ { line: 5, column: 11 }, { line: 10, column: 9 }, - { line: 14, column: 9 }, + { line: 11, column: 9 }, + { line: 15, column: 9 }, + ], + }, + ]); + }); + + it('fails with many more than one root field via fragments (anonymous)', () => { + expectErrors(` + subscription { + importantEmails + ... { + more: moreImportantEmails + } + ...NotImportantEmails + } + fragment NotImportantEmails on Subscription { + notImportantEmails + deleted: deletedEmails + ...SpamEmails + } + fragment SpamEmails on Subscription { + spamEmails + } + `).to.deep.equal([ + { + message: 'Anonymous Subscription must select only one top level field.', + locations: [ + { line: 5, column: 11 }, + { line: 10, column: 9 }, + { line: 11, column: 9 }, + { line: 15, column: 9 }, ], }, ]); From 7ca980590aed07183e1a1ad7ab0a58f836b0481f Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 5 Dec 2020 13:15:42 +0000 Subject: [PATCH 06/15] Finally appease codecov --- .../__tests__/SingleFieldSubscriptionsRule-test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index f7c8cc005a..f76f61e81d 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -160,25 +160,33 @@ describe('Validate: Subscriptions with single field', () => { importantEmails ... { more: moreImportantEmails + ...NotImportantEmails } ...NotImportantEmails } fragment NotImportantEmails on Subscription { notImportantEmails deleted: deletedEmails + ... { + ... { + archivedEmails + } + } ...SpamEmails } fragment SpamEmails on Subscription { spamEmails + ...NonExistantFragment } `).to.deep.equal([ { message: 'Anonymous Subscription must select only one top level field.', locations: [ { line: 5, column: 11 }, - { line: 10, column: 9 }, { line: 11, column: 9 }, - { line: 15, column: 9 }, + { line: 12, column: 9 }, + { line: 15, column: 13 }, + { line: 21, column: 9 }, ], }, ]); From a846b2ba9ea7d0d0131fbd08637efbc06188717f Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sat, 5 Dec 2020 13:31:04 +0000 Subject: [PATCH 07/15] Typo --- src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index f76f61e81d..6e5aa996df 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -176,7 +176,7 @@ describe('Validate: Subscriptions with single field', () => { } fragment SpamEmails on Subscription { spamEmails - ...NonExistantFragment + ...NonExistentFragment } `).to.deep.equal([ { From 2e8c6356635d84b21dca22781bef6f99509b29c1 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 7 Jan 2021 11:32:16 +0000 Subject: [PATCH 08/15] Add test to demonstrate that recursive fragments don't cause infinite loop --- .../__tests__/SingleFieldSubscriptionsRule-test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index 6e5aa996df..a36b9e9b0e 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -154,6 +154,17 @@ describe('Validate: Subscriptions with single field', () => { ]); }); + it('does not infinite loop on recursive fragments', () => { + expectErrors(` + subscription NoInfiniteLoop { + ...A + } + fragment A on Subscription { + ...A + } + `).to.deep.equal([]); + }); + it('fails with many more than one root field via fragments (anonymous)', () => { expectErrors(` subscription { From 9a69bb12242167e5df687dcbb3cfe4164f21b228 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sun, 25 Apr 2021 13:43:26 +0100 Subject: [PATCH 09/15] Reflect @leebyron's PR feedback --- .../SingleFieldSubscriptionsRule-test.ts | 26 ++- src/validation/__tests__/harness.ts | 15 ++ .../rules/SingleFieldSubscriptionsRule.ts | 166 ++++++++---------- 3 files changed, 104 insertions(+), 103 deletions(-) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index a36b9e9b0e..bb04f87237 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -28,7 +28,7 @@ describe('Validate: Subscriptions with single field', () => { ...newMessageFields } - fragment newMessageFields on Subscription { + fragment newMessageFields on SubscriptionRoot { newMessage { body sender @@ -47,7 +47,7 @@ describe('Validate: Subscriptions with single field', () => { ...newMessageFields } - fragment newMessageFields on Subscription { + fragment newMessageFields on SubscriptionRoot { newMessage { body sender @@ -78,6 +78,11 @@ describe('Validate: Subscriptions with single field', () => { __typename } `).to.deep.equal([ + { + message: + 'Subscription "ImportantEmails" must select only one top level field.', + locations: [{ line: 4, column: 9 }], + }, { message: 'Subscription "ImportantEmails" must not select an introspection top level field.', @@ -92,10 +97,15 @@ describe('Validate: Subscriptions with single field', () => { importantEmails ...Introspection } - fragment Introspection on Subscription { + fragment Introspection on SubscriptionRoot { typename: __typename } `).to.deep.equal([ + { + message: + 'Subscription "ImportantEmails" must select only one top level field.', + locations: [{ line: 7, column: 9 }], + }, { message: 'Subscription "ImportantEmails" must not select an introspection top level field.', @@ -132,12 +142,12 @@ describe('Validate: Subscriptions with single field', () => { } ...NotImportantEmails } - fragment NotImportantEmails on Subscription { + fragment NotImportantEmails on SubscriptionRoot { notImportantEmails deleted: deletedEmails ...SpamEmails } - fragment SpamEmails on Subscription { + fragment SpamEmails on SubscriptionRoot { spamEmails } `).to.deep.equal([ @@ -159,7 +169,7 @@ describe('Validate: Subscriptions with single field', () => { subscription NoInfiniteLoop { ...A } - fragment A on Subscription { + fragment A on SubscriptionRoot { ...A } `).to.deep.equal([]); @@ -175,7 +185,7 @@ describe('Validate: Subscriptions with single field', () => { } ...NotImportantEmails } - fragment NotImportantEmails on Subscription { + fragment NotImportantEmails on SubscriptionRoot { notImportantEmails deleted: deletedEmails ... { @@ -185,7 +195,7 @@ describe('Validate: Subscriptions with single field', () => { } ...SpamEmails } - fragment SpamEmails on Subscription { + fragment SpamEmails on SubscriptionRoot { spamEmails ...NonExistentFragment } diff --git a/src/validation/__tests__/harness.ts b/src/validation/__tests__/harness.ts index 711ea91a6e..5de9f29f06 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -130,8 +130,23 @@ export const testSchema: GraphQLSchema = buildSchema(` complicatedArgs: ComplicatedArgs } + type Message { + body: String + sender: String + } + + type SubscriptionRoot { + importantEmails: [String] + notImportantEmails: [String] + moreImportantEmails: [String] + spamEmails: [String] + deletedEmails: [String] + newMessage: Message + } + schema { query: QueryRoot + subscription: SubscriptionRoot } directive @onQuery on QUERY diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 50239834bb..94956932eb 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -1,78 +1,19 @@ +import type { ObjMap } from '../../jsutils/ObjMap'; import { GraphQLError } from '../../error/GraphQLError'; import type { ASTVisitor } from '../../language/visitor'; import type { OperationDefinitionNode, - SelectionSetNode, + FragmentDefinitionNode, } from '../../language/ast'; import { Kind } from '../../language/kinds'; -import type { ASTValidationContext } from '../ValidationContext'; +import type { ValidationContext } from '../ValidationContext'; +import type { ExecutionContext } from '../../execution/execute'; +import { collectFields } from '../../execution/execute'; -/** - * Walks the selection set and returns a list of selections where extra fields - * were selected, and selections where introspection fields were selected. - */ -function walkSubscriptionSelectionSet( - context: ASTValidationContext, - selectionSet: SelectionSetNode, - responseKeys, - fieldNames = new Set(), - visitedFragmentNames = new Set(), - extraFieldSelections = [], - introspectionFieldSelections = [], -) { - for (const selection of selectionSet.selections) { - switch (selection.kind) { - case Kind.FIELD: { - const fieldName = selection.name.value; - fieldNames.add(fieldName); - const responseName = selection.alias - ? selection.alias.value - : fieldName; - responseKeys.add(responseName); - if (fieldName[0] === '_' && fieldName[1] === '_') { - // fieldName represents an introspection field if it starts with `__` - introspectionFieldSelections.push(selection); - } else if (fieldNames.size > 1 || responseKeys.size > 1) { - extraFieldSelections.push(selection); - } - break; - } - case Kind.FRAGMENT_SPREAD: { - const fragmentName = selection.name.value; - if (!visitedFragmentNames.has(fragmentName)) { - visitedFragmentNames.add(fragmentName); - const fragment = context.getFragment(fragmentName); - if (fragment) { - walkSubscriptionSelectionSet( - context, - fragment.selectionSet, - responseKeys, - fieldNames, - visitedFragmentNames, - extraFieldSelections, - introspectionFieldSelections, - ); - } - } - break; - } - case Kind.INLINE_FRAGMENT: { - walkSubscriptionSelectionSet( - context, - selection.selectionSet, - responseKeys, - fieldNames, - visitedFragmentNames, - extraFieldSelections, - introspectionFieldSelections, - ); - break; - } - } - } - return [extraFieldSelections, introspectionFieldSelections]; +function fakeResolver() { + /* noop */ } /** @@ -82,40 +23,75 @@ function walkSubscriptionSelectionSet( * that root field is not an introspection field. */ export function SingleFieldSubscriptionsRule( - context: ASTValidationContext, + context: ValidationContext, ): ASTVisitor { return { OperationDefinition(node: OperationDefinitionNode) { if (node.operation === 'subscription') { - const responseKeys = new Set(); - const operationName = node.name ? node.name.value : null; - const [ - extraFieldSelections, - introspectionFieldSelections, - ] = walkSubscriptionSelectionSet( - context, - node.selectionSet, - responseKeys, - ); - if (extraFieldSelections.length > 0) { - context.reportError( - new GraphQLError( - operationName != null - ? `Subscription "${operationName}" must select only one top level field.` - : 'Anonymous Subscription must select only one top level field.', - extraFieldSelections, - ), - ); - } - if (introspectionFieldSelections.length > 0) { - context.reportError( - new GraphQLError( - operationName != null - ? `Subscription "${operationName}" must not select an introspection top level field.` - : 'Anonymous Subscription must not select an introspection top level field.', - introspectionFieldSelections, - ), + const schema = context.getSchema(); + const subscriptionType = schema.getSubscriptionType(); + if (subscriptionType) { + const operationName = node.name ? node.name.value : null; + const variableValues: { + [variable: string]: mixed, + ... + } = {}; + const document = context.getDocument(); + const fragments: ObjMap = Object.create(null); + for (const definition of document.definitions) { + if (definition.kind === Kind.FRAGMENT_DEFINITION) { + fragments[definition.name.value] = definition; + } + } + const fakeExecutionContext: ExecutionContext = { + schema, + fragments, + rootValue: undefined, + contextValue: undefined, + operation: node, + variableValues, + fieldResolver: fakeResolver, + typeResolver: fakeResolver, + errors: [], + }; + const fields = collectFields( + fakeExecutionContext, + subscriptionType, + node.selectionSet, + {}, + {}, ); + const responseKeys = Object.keys(fields); + if (responseKeys.length > 1) { + const extraResponseKeys = responseKeys.slice(1); + const extraFieldSelections = extraResponseKeys.flatMap( + (key) => fields[key], + ); + context.reportError( + new GraphQLError( + operationName != null + ? `Subscription "${operationName}" must select only one top level field.` + : 'Anonymous Subscription must select only one top level field.', + extraFieldSelections, + ), + ); + } + for (const responseKey of Object.keys(fields)) { + const field = fields[responseKey][0]; + if (field) { + const fieldName = field.name.value; + if (fieldName[0] === '_' && fieldName[1] === '_') { + context.reportError( + new GraphQLError( + operationName != null + ? `Subscription "${operationName}" must not select an introspection top level field.` + : 'Anonymous Subscription must not select an introspection top level field.', + fields[responseKey], + ), + ); + } + } + } } } }, From 9f718b848bcd92c264937ccec61709271d406f48 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Sun, 25 Apr 2021 14:04:51 +0100 Subject: [PATCH 10/15] Appease codecoverage --- .../SingleFieldSubscriptionsRule-test.ts | 18 +++++++++- src/validation/__tests__/harness.ts | 10 ++++++ .../rules/SingleFieldSubscriptionsRule.ts | 36 +++++++++---------- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index bb04f87237..e8d9d1c8c3 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -2,7 +2,11 @@ import { describe, it } from 'mocha'; import { SingleFieldSubscriptionsRule } from '../rules/SingleFieldSubscriptionsRule'; -import { expectValidationErrors } from './harness'; +import { + expectValidationErrors, + expectValidationErrorsWithSchema, + emptySchema, +} from './harness'; function expectErrors(queryStr: string) { return expectValidationErrors(SingleFieldSubscriptionsRule, queryStr); @@ -254,4 +258,16 @@ describe('Validate: Subscriptions with single field', () => { }, ]); }); + + it('skips if not subscription type', () => { + expectValidationErrorsWithSchema( + emptySchema, + SingleFieldSubscriptionsRule, + ` + subscription { + __typename + } + `, + ).to.deep.equal([]); + }); }); diff --git a/src/validation/__tests__/harness.ts b/src/validation/__tests__/harness.ts index 5de9f29f06..0e29384035 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -159,6 +159,16 @@ export const testSchema: GraphQLSchema = buildSchema(` directive @onVariableDefinition on VARIABLE_DEFINITION `); +export const emptySchema: GraphQLSchema = buildSchema(` + type QueryRoot { + empty: Boolean + } + + schema { + query: QueryRoot + } +`); + export function expectValidationErrorsWithSchema( schema: GraphQLSchema, rule: ValidationRule, diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 94956932eb..62d4b8a69f 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -10,11 +10,11 @@ import { Kind } from '../../language/kinds'; import type { ValidationContext } from '../ValidationContext'; import type { ExecutionContext } from '../../execution/execute'; -import { collectFields } from '../../execution/execute'; - -function fakeResolver() { - /* noop */ -} +import { + collectFields, + defaultFieldResolver, + defaultTypeResolver, +} from '../../execution/execute'; /** * Subscriptions must only include a non-introspection field. @@ -50,8 +50,8 @@ export function SingleFieldSubscriptionsRule( contextValue: undefined, operation: node, variableValues, - fieldResolver: fakeResolver, - typeResolver: fakeResolver, + fieldResolver: defaultFieldResolver, + typeResolver: defaultTypeResolver, errors: [], }; const fields = collectFields( @@ -78,18 +78,16 @@ export function SingleFieldSubscriptionsRule( } for (const responseKey of Object.keys(fields)) { const field = fields[responseKey][0]; - if (field) { - const fieldName = field.name.value; - if (fieldName[0] === '_' && fieldName[1] === '_') { - context.reportError( - new GraphQLError( - operationName != null - ? `Subscription "${operationName}" must not select an introspection top level field.` - : 'Anonymous Subscription must not select an introspection top level field.', - fields[responseKey], - ), - ); - } + const fieldName = field.name.value; + if (fieldName[0] === '_' && fieldName[1] === '_') { + context.reportError( + new GraphQLError( + operationName != null + ? `Subscription "${operationName}" must not select an introspection top level field.` + : 'Anonymous Subscription must not select an introspection top level field.', + fields[responseKey], + ), + ); } } } From ad2f93f371deaf1a3f35bd25f8067cb4704a3718 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 27 May 2021 13:56:30 +0100 Subject: [PATCH 11/15] Apply changes Ivan requested --- src/validation/rules/SingleFieldSubscriptionsRule.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 62d4b8a69f..aafc9a24d8 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -35,7 +35,7 @@ export function SingleFieldSubscriptionsRule( const variableValues: { [variable: string]: mixed, ... - } = {}; + } = Object.create(null); const document = context.getDocument(); const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { @@ -58,8 +58,8 @@ export function SingleFieldSubscriptionsRule( fakeExecutionContext, subscriptionType, node.selectionSet, - {}, - {}, + new Map(), + new Set(), ); const responseKeys = Object.keys(fields); if (responseKeys.length > 1) { From ce36b47f789fb741cd5697852825a5d8cb80517b Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 27 May 2021 13:58:16 +0100 Subject: [PATCH 12/15] It's TypeScript now --- src/validation/rules/SingleFieldSubscriptionsRule.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index aafc9a24d8..d6a6530fe8 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -33,8 +33,7 @@ export function SingleFieldSubscriptionsRule( if (subscriptionType) { const operationName = node.name ? node.name.value : null; const variableValues: { - [variable: string]: mixed, - ... + [variable: string]: any; } = Object.create(null); const document = context.getDocument(); const fragments: ObjMap = Object.create(null); From 8d9822da8a1ec040c940ef5eb2d7014e38123262 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 27 May 2021 14:02:04 +0100 Subject: [PATCH 13/15] collectFields returns a Map now --- .../rules/SingleFieldSubscriptionsRule.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index d6a6530fe8..ded4dd738f 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -60,11 +60,11 @@ export function SingleFieldSubscriptionsRule( new Map(), new Set(), ); - const responseKeys = Object.keys(fields); - if (responseKeys.length > 1) { + if (fields.size > 1) { + const responseKeys = [...fields.keys()]; const extraResponseKeys = responseKeys.slice(1); - const extraFieldSelections = extraResponseKeys.flatMap( - (key) => fields[key], + const extraFieldSelections = extraResponseKeys.flatMap((key) => + fields.get(key), ); context.reportError( new GraphQLError( @@ -75,8 +75,8 @@ export function SingleFieldSubscriptionsRule( ), ); } - for (const responseKey of Object.keys(fields)) { - const field = fields[responseKey][0]; + for (const fieldNodes of fields.values()) { + const field = fieldNodes[0]; const fieldName = field.name.value; if (fieldName[0] === '_' && fieldName[1] === '_') { context.reportError( @@ -84,7 +84,7 @@ export function SingleFieldSubscriptionsRule( operationName != null ? `Subscription "${operationName}" must not select an introspection top level field.` : 'Anonymous Subscription must not select an introspection top level field.', - fields[responseKey], + fieldNodes, ), ); } From 06b75878bbd3a0f1ae6524037b6eaebaf8bf9a97 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Thu, 27 May 2021 14:08:15 +0100 Subject: [PATCH 14/15] Slightly more efficient code --- src/validation/rules/SingleFieldSubscriptionsRule.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index ded4dd738f..c8094dab79 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -61,11 +61,9 @@ export function SingleFieldSubscriptionsRule( new Set(), ); if (fields.size > 1) { - const responseKeys = [...fields.keys()]; - const extraResponseKeys = responseKeys.slice(1); - const extraFieldSelections = extraResponseKeys.flatMap((key) => - fields.get(key), - ); + const fieldSelectionLists = [...fields.values()]; + const extraFieldSelectionLists = fieldSelectionLists.slice(1); + const extraFieldSelections = extraFieldSelectionLists.flat(); context.reportError( new GraphQLError( operationName != null From 5ba89bb8ea8bfafe6c7f33c2f56252dfce8ca6c0 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Thu, 3 Jun 2021 20:46:37 +0300 Subject: [PATCH 15/15] Apply suggestions from code review --- src/validation/rules/SingleFieldSubscriptionsRule.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index c8094dab79..06a63a8c6a 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -42,6 +42,7 @@ export function SingleFieldSubscriptionsRule( fragments[definition.name.value] = definition; } } + // FIXME: refactor out `collectFields` into utility function that doesn't need fake context. const fakeExecutionContext: ExecutionContext = { schema, fragments,