From 1c4477c50d0ce4bded3654015060ca00335b9f57 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Thu, 25 May 2017 19:26:20 -0700 Subject: [PATCH] Spec compliance: Validation error on multi-field subscription (#882) As per the spec, a subscription should simply use the first field defined, and not make any other assertions during the Subscribe operation. Instead, a validation rule should detect this and report it. --- src/index.js | 1 + src/subscription/__tests__/subscribe-test.js | 36 ++++++--- src/subscription/subscribe.js | 4 - .../SingleFieldSubscriptions-test.js | 81 +++++++++++++++++++ src/validation/index.js | 5 ++ .../rules/SingleFieldSubscriptions.js | 39 +++++++++ src/validation/specifiedRules.js | 4 + 7 files changed, 156 insertions(+), 14 deletions(-) create mode 100644 src/validation/__tests__/SingleFieldSubscriptions-test.js create mode 100644 src/validation/rules/SingleFieldSubscriptions.js diff --git a/src/index.js b/src/index.js index 3b3e3ae834..78892c326c 100644 --- a/src/index.js +++ b/src/index.js @@ -271,6 +271,7 @@ export { PossibleFragmentSpreadsRule, ProvidedNonNullArgumentsRule, ScalarLeafsRule, + SingleFieldSubscriptionsRule, UniqueArgumentNamesRule, UniqueDirectivesPerLocationRule, UniqueFragmentNamesRule, diff --git a/src/subscription/__tests__/subscribe-test.js b/src/subscription/__tests__/subscribe-test.js index 349d4ecc39..1fb98282da 100644 --- a/src/subscription/__tests__/subscribe-test.js +++ b/src/subscription/__tests__/subscribe-test.js @@ -211,12 +211,27 @@ describe('Subscribe', () => { }).not.to.throw(); }); - it('should throw when querying for multiple fields', async () => { + it('should only resolve the first field of invalid multi-field', async () => { + let didResolveImportantEmail = false; + let didResolveNonImportantEmail = false; + const SubscriptionTypeMultiple = new GraphQLObjectType({ name: 'Subscription', fields: { - importantEmail: { type: EmailEventType }, - nonImportantEmail: { type: EmailEventType }, + importantEmail: { + type: EmailEventType, + subscribe() { + didResolveImportantEmail = true; + return eventEmitterAsyncIterator(new EventEmitter(), 'event'); + } + }, + nonImportantEmail: { + type: EmailEventType, + subscribe() { + didResolveNonImportantEmail = true; + return eventEmitterAsyncIterator(new EventEmitter(), 'event'); + } + }, } }); @@ -232,13 +247,14 @@ describe('Subscribe', () => { } `); - expect(() => { - subscribe( - testSchema, - ast - ); - }).to.throw( - 'A subscription operation must contain exactly one root field.'); + const subscription = subscribe(testSchema, ast); + subscription.next(); // Ask for a result, but ignore it. + + expect(didResolveImportantEmail).to.equal(true); + expect(didResolveNonImportantEmail).to.equal(false); + + // Close subscription + subscription.return(); }); it('produces payload for multiple subscribe in same subscription', diff --git a/src/subscription/subscribe.js b/src/subscription/subscribe.js index c4f952eb31..6d40cb8fa4 100644 --- a/src/subscription/subscribe.js +++ b/src/subscription/subscribe.js @@ -158,10 +158,6 @@ export function createSourceEventStream( Object.create(null) ); const responseNames = Object.keys(fields); - invariant( - responseNames.length === 1, - 'A subscription operation must contain exactly one root field.' - ); const responseName = responseNames[0]; const fieldNodes = fields[responseName]; const fieldNode = fieldNodes[0]; diff --git a/src/validation/__tests__/SingleFieldSubscriptions-test.js b/src/validation/__tests__/SingleFieldSubscriptions-test.js new file mode 100644 index 0000000000..28dba961c2 --- /dev/null +++ b/src/validation/__tests__/SingleFieldSubscriptions-test.js @@ -0,0 +1,81 @@ +/** + * Copyright (c) 2015, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +import { describe, it } from 'mocha'; +import { expectPassesRule, expectFailsRule } from './harness'; +import { + SingleFieldSubscriptions, + singleFieldOnlyMessage, +} from '../rules/SingleFieldSubscriptions'; + + +describe('Validate: Subscriptions with single field', () => { + + it('valid subscription', () => { + expectPassesRule(SingleFieldSubscriptions, ` + subscription ImportantEmails { + importantEmails + } + `); + }); + + it('fails with more than one root field', () => { + expectFailsRule(SingleFieldSubscriptions, ` + subscription ImportantEmails { + importantEmails + notImportantEmails + } + `, [ { + message: singleFieldOnlyMessage('ImportantEmails'), + locations: [ { line: 4, column: 9 } ], + path: undefined, + } ]); + }); + + it('fails with more than one root field including introspection', () => { + expectFailsRule(SingleFieldSubscriptions, ` + subscription ImportantEmails { + importantEmails + __typename + } + `, [ { + message: singleFieldOnlyMessage('ImportantEmails'), + locations: [ { line: 4, column: 9 } ], + path: undefined, + } ]); + }); + + it('fails with many more than one root field', () => { + expectFailsRule(SingleFieldSubscriptions, ` + subscription ImportantEmails { + importantEmails + notImportantEmails + spamEmails + } + `, [ { + message: singleFieldOnlyMessage('ImportantEmails'), + locations: [ { line: 4, column: 9 }, { line: 5, column: 9 } ], + path: undefined, + } ]); + }); + + it('fails with more than one root field in anonymous subscriptions', () => { + expectFailsRule(SingleFieldSubscriptions, ` + subscription { + importantEmails + notImportantEmails + } + `, [ { + message: singleFieldOnlyMessage(null), + locations: [ { line: 4, column: 9 } ], + path: undefined, + } ]); + }); + +}); diff --git a/src/validation/index.js b/src/validation/index.js index 4970a27d70..c2190f4bbd 100644 --- a/src/validation/index.js +++ b/src/validation/index.js @@ -96,6 +96,11 @@ export { ScalarLeafs as ScalarLeafsRule } from './rules/ScalarLeafs'; +// Spec Section: "Subscriptions with Single Root Field" +export { + SingleFieldSubscriptions as SingleFieldSubscriptionsRule +} from './rules/SingleFieldSubscriptions'; + // Spec Section: "Argument Uniqueness" export { UniqueArgumentNames as UniqueArgumentNamesRule diff --git a/src/validation/rules/SingleFieldSubscriptions.js b/src/validation/rules/SingleFieldSubscriptions.js new file mode 100644 index 0000000000..1361fe6d06 --- /dev/null +++ b/src/validation/rules/SingleFieldSubscriptions.js @@ -0,0 +1,39 @@ +/* @flow */ +/** + * Copyright (c) 2015, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +import type { ValidationContext } from '../index'; +import { GraphQLError } from '../../error'; +import type { OperationDefinitionNode } from '../../language/ast'; + + +export function singleFieldOnlyMessage(name: ?string): string { + return (name ? `Subscription "${name}" ` : 'Anonymous Subscription ') + + 'must select only one top level field.'; +} + +/** + * Subscriptions must only include one field. + * + * A GraphQL subscription is valid only if it contains a single root field. + */ +export function SingleFieldSubscriptions(context: ValidationContext): any { + return { + OperationDefinition(node: OperationDefinitionNode) { + if (node.operation === 'subscription') { + if (node.selectionSet.selections.length !== 1) { + context.reportError(new GraphQLError( + singleFieldOnlyMessage(node.name && node.name.value), + node.selectionSet.selections.slice(1) + )); + } + } + } + }; +} diff --git a/src/validation/specifiedRules.js b/src/validation/specifiedRules.js index 981ccebb56..3ac89285fa 100644 --- a/src/validation/specifiedRules.js +++ b/src/validation/specifiedRules.js @@ -14,6 +14,9 @@ import { UniqueOperationNames } from './rules/UniqueOperationNames'; // Spec Section: "Lone Anonymous Operation" import { LoneAnonymousOperation } from './rules/LoneAnonymousOperation'; +// Spec Section: "Subscriptions with Single Root Field" +import { SingleFieldSubscriptions } from './rules/SingleFieldSubscriptions'; + // Spec Section: "Fragment Spread Type Existence" import { KnownTypeNames } from './rules/KnownTypeNames'; @@ -98,6 +101,7 @@ import type { ValidationContext } from './index'; export const specifiedRules: Array<(context: ValidationContext) => any> = [ UniqueOperationNames, LoneAnonymousOperation, + SingleFieldSubscriptions, KnownTypeNames, FragmentsOnCompositeTypes, VariablesAreInputTypes,