From 5cb3dd98458ad7f50a42b2bae55fdb1384e87e63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eloy=20Dur=C3=A1n?= Date: Fri, 9 Feb 2018 00:07:39 +0100 Subject: [PATCH 01/10] Allow legacy names when building AST schema. --- src/utilities/__tests__/buildASTSchema-test.js | 11 +++++++++++ src/utilities/buildASTSchema.js | 11 +++++++++++ src/utilities/extendSchema.js | 10 ++++++++++ 3 files changed, 32 insertions(+) diff --git a/src/utilities/__tests__/buildASTSchema-test.js b/src/utilities/__tests__/buildASTSchema-test.js index c568cbded8..f8d52b636b 100644 --- a/src/utilities/__tests__/buildASTSchema-test.js +++ b/src/utilities/__tests__/buildASTSchema-test.js @@ -776,6 +776,17 @@ describe('Schema Builder', () => { const errors = validateSchema(schema); expect(errors.length).to.be.above(0); }); + + it('Accepts legacy names', () => { + const doc = parse(dedent` + type Query { + __badName: String + } + `); + const schema = buildASTSchema(doc, { allowedLegacyNames: ['__badName'] }); + const errors = validateSchema(schema); + expect(errors.length).to.equal(0); + }); }); describe('Failures', () => { diff --git a/src/utilities/buildASTSchema.js b/src/utilities/buildASTSchema.js index 8216a2e0f7..c143f06b64 100644 --- a/src/utilities/buildASTSchema.js +++ b/src/utilities/buildASTSchema.js @@ -89,6 +89,16 @@ type Options = {| * Default: false */ commentDescriptions?: boolean, + + /** + * If provided, the schema will consider fields or types with names included + * in this list valid, even if they do not adhere to the specification's + * schema validation rules. + * + * This option is provided to ease adoption and may be removed in a future + * major release. + */ + allowedLegacyNames?: ?Array, |}; function buildWrappedType( @@ -227,6 +237,7 @@ export function buildASTSchema( directives, astNode: schemaDef, assumeValid: options && options.assumeValid, + allowedLegacyNames: options && options.allowedLegacyNames, }); function getOperationTypes(schema: SchemaDefinitionNode) { diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index 860e067fde..dd6f3ef29a 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -55,6 +55,16 @@ type Options = {| * Default: false */ commentDescriptions?: boolean, + + /** + * If provided, the schema will consider fields or types with names included + * in this list valid, even if they do not adhere to the specification's + * schema validation rules. + * + * This option is provided to ease adoption and may be removed in a future + * major release. + */ + allowedLegacyNames?: ?Array, |}; /** From 5f9ff08657b64af89ed265ff5bca1c9a00c68a78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eloy=20Dur=C3=A1n?= Date: Fri, 9 Feb 2018 00:14:33 +0100 Subject: [PATCH 02/10] Always store allowed legacy names on schema. --- src/type/schema.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/type/schema.js b/src/type/schema.js index 9482a9d116..1f47c804cd 100644 --- a/src/type/schema.js +++ b/src/type/schema.js @@ -111,9 +111,9 @@ export class GraphQLSchema { '"allowedLegacyNames" must be Array if provided but got: ' + `${String(config.allowedLegacyNames)}.`, ); - this.__allowedLegacyNames = config.allowedLegacyNames; } + this.__allowedLegacyNames = config.allowedLegacyNames; this._queryType = config.query; this._mutationType = config.mutation; this._subscriptionType = config.subscription; From 3958eae4e98c2472c74e1e0fdd924c9e7a0eaab2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eloy=20Dur=C3=A1n?= Date: Fri, 9 Feb 2018 01:11:46 +0100 Subject: [PATCH 03/10] Allow additional legacy names when extending a schema. --- src/utilities/__tests__/extendSchema-test.js | 13 +++++++++---- src/utilities/extendSchema.js | 16 ++++++++++++---- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/utilities/__tests__/extendSchema-test.js b/src/utilities/__tests__/extendSchema-test.js index 15bc843f9d..12840de116 100644 --- a/src/utilities/__tests__/extendSchema-test.js +++ b/src/utilities/__tests__/extendSchema-test.js @@ -1225,18 +1225,23 @@ describe('extendSchema', () => { query: new GraphQLObjectType({ name: 'Query', fields: () => ({ - id: { type: GraphQLID }, + __badName: { type: GraphQLString }, }), }), allowedLegacyNames: ['__badName'], }); const ast = parse(` extend type Query { - __badName: String + __anotherBadName: String } `); - const schema = extendSchema(testSchemaWithLegacyNames, ast); - expect(schema.__allowedLegacyNames).to.deep.equal(['__badName']); + const schema = extendSchema(testSchemaWithLegacyNames, ast, { + allowedLegacyNames: ['__anotherBadName'], + }); + expect(schema.__allowedLegacyNames).to.deep.equal([ + '__badName', + '__anotherBadName', + ]); }); describe('does not allow extending a non-object type', () => { diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index dd6f3ef29a..563d226e72 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -58,8 +58,9 @@ type Options = {| /** * If provided, the schema will consider fields or types with names included - * in this list valid, even if they do not adhere to the specification's - * schema validation rules. + * in this list valid, in addition to those already allowed on the original + * schema, even if they do not adhere to the specification's schema validation + * rules. * * This option is provided to ease adoption and may be removed in a future * major release. @@ -255,6 +256,14 @@ export function extendSchema( types.push(definitionBuilder.buildType(typeName)); }); + let allowedLegacyNames = [ + ...(schema.__allowedLegacyNames || []), + ...((options && options.allowedLegacyNames) || []), + ]; + if (allowedLegacyNames.length === 0) { + allowedLegacyNames = null; + } + // Then produce and return a Schema with these types. return new GraphQLSchema({ query: queryType, @@ -263,8 +272,7 @@ export function extendSchema( types, directives: getMergedDirectives(), astNode: schema.astNode, - allowedLegacyNames: - schema.__allowedLegacyNames && schema.__allowedLegacyNames.slice(), + allowedLegacyNames, }); function appendExtensionToTypeExtensions( From 8f936da25d5d4beaf792e43b567ec090625767ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eloy=20Dur=C3=A1n?= Date: Fri, 9 Feb 2018 17:00:19 +0100 Subject: [PATCH 04/10] Standardise schema validation options. --- src/type/schema.js | 26 +++++++++++++++++++------- src/utilities/buildASTSchema.js | 20 ++------------------ src/utilities/extendSchema.js | 22 +++------------------- 3 files changed, 24 insertions(+), 44 deletions(-) diff --git a/src/type/schema.js b/src/type/schema.js index 1f47c804cd..981c9866bf 100644 --- a/src/type/schema.js +++ b/src/type/schema.js @@ -229,14 +229,16 @@ export class GraphQLSchema { type TypeMap = ObjMap; -export type GraphQLSchemaConfig = { - query?: ?GraphQLObjectType, - mutation?: ?GraphQLObjectType, - subscription?: ?GraphQLObjectType, - types?: ?Array, - directives?: ?Array, - astNode?: ?SchemaDefinitionNode, +export type GraphQLSchemaValidationOptions = {| + /** + * When building a schema from a GraphQL service's introspection result, it + * might be safe to assume the schema is valid. Set to true to assume the + * produced schema is valid. + * + * Default: false + */ assumeValid?: boolean, + /** * If provided, the schema will consider fields or types with names included * in this list valid, even if they do not adhere to the specification's @@ -246,6 +248,16 @@ export type GraphQLSchemaConfig = { * major release. */ allowedLegacyNames?: ?Array, +|}; + +export type GraphQLSchemaConfig = { + query?: ?GraphQLObjectType, + mutation?: ?GraphQLObjectType, + subscription?: ?GraphQLObjectType, + types?: ?Array, + directives?: ?Array, + astNode?: ?SchemaDefinitionNode, + ...GraphQLSchemaValidationOptions, }; function typeMapReducer(map: TypeMap, type: ?GraphQLType): TypeMap { diff --git a/src/utilities/buildASTSchema.js b/src/utilities/buildASTSchema.js index c143f06b64..e2ea8175d6 100644 --- a/src/utilities/buildASTSchema.js +++ b/src/utilities/buildASTSchema.js @@ -64,6 +64,7 @@ import { introspectionTypes } from '../type/introspection'; import { specifiedScalarTypes } from '../type/scalars'; import { GraphQLSchema } from '../type/schema'; +import type { GraphQLSchemaValidationOptions } from '../type/schema'; import type { GraphQLType, @@ -72,14 +73,7 @@ import type { } from '../type/definition'; type Options = {| - /** - * When building a schema from a GraphQL service's introspection result, it - * might be safe to assume the schema is valid. Set to true to assume the - * produced schema is valid. - * - * Default: false - */ - assumeValid?: boolean, + ...GraphQLSchemaValidationOptions, /** * Descriptions are defined as preceding string literals, however an older @@ -89,16 +83,6 @@ type Options = {| * Default: false */ commentDescriptions?: boolean, - - /** - * If provided, the schema will consider fields or types with names included - * in this list valid, even if they do not adhere to the specification's - * schema validation rules. - * - * This option is provided to ease adoption and may be removed in a future - * major release. - */ - allowedLegacyNames?: ?Array, |}; function buildWrappedType( diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index 563d226e72..c857e88d35 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -13,6 +13,8 @@ import { ASTDefinitionBuilder } from './buildASTSchema'; import { GraphQLError } from '../error/GraphQLError'; import { isSchema, GraphQLSchema } from '../type/schema'; +import type { GraphQLSchemaValidationOptions } from '../type/schema'; + import { isObjectType, isInterfaceType, @@ -38,14 +40,7 @@ import type { } from '../language/ast'; type Options = {| - /** - * When extending a schema with a known valid extension, it might be safe to - * assume the schema is valid. Set to true to assume the produced schema - * is valid. - * - * Default: false - */ - assumeValid?: boolean, + ...GraphQLSchemaValidationOptions, /** * Descriptions are defined as preceding string literals, however an older @@ -55,17 +50,6 @@ type Options = {| * Default: false */ commentDescriptions?: boolean, - - /** - * If provided, the schema will consider fields or types with names included - * in this list valid, in addition to those already allowed on the original - * schema, even if they do not adhere to the specification's schema validation - * rules. - * - * This option is provided to ease adoption and may be removed in a future - * major release. - */ - allowedLegacyNames?: ?Array, |}; /** From bb9c68c618474d19488db0279ed1a68a735ab9eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eloy=20Dur=C3=A1n?= Date: Fri, 9 Feb 2018 17:01:15 +0100 Subject: [PATCH 05/10] Allow legacy names in client schemas. --- .../__tests__/buildClientSchema-test.js | 28 +++++++++++++++++++ src/utilities/buildClientSchema.js | 12 +++----- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/utilities/__tests__/buildClientSchema-test.js b/src/utilities/__tests__/buildClientSchema-test.js index 04d2342440..6a93a96646 100644 --- a/src/utilities/__tests__/buildClientSchema-test.js +++ b/src/utilities/__tests__/buildClientSchema-test.js @@ -669,6 +669,34 @@ describe('Type System: build schema from introspection', () => { expect(secondIntrospection.data).to.containSubset(newIntrospection); }); + it('builds a schema with legacy names', () => { + const introspection = { + __schema: { + queryType: { + name: 'Query', + }, + types: [ + { + name: 'Query', + kind: 'OBJECT', + fields: [ + { + name: '__badName', + args: [], + type: { name: 'String' }, + }, + ], + interfaces: [], + }, + ], + }, + }; + const schema = buildClientSchema(introspection, { + allowedLegacyNames: ['__badName'], + }); + expect(schema.__allowedLegacyNames).to.deep.equal(['__badName']); + }); + it('builds a schema aware of deprecation', async () => { const schema = new GraphQLSchema({ query: new GraphQLObjectType({ diff --git a/src/utilities/buildClientSchema.js b/src/utilities/buildClientSchema.js index cc883b3a88..7d0ff73648 100644 --- a/src/utilities/buildClientSchema.js +++ b/src/utilities/buildClientSchema.js @@ -60,15 +60,10 @@ import type { IntrospectionNamedTypeRef, } from './introspectionQuery'; +import type { GraphQLSchemaValidationOptions } from '../type/schema'; + type Options = {| - /** - * When building a schema from a GraphQL service's introspection result, it - * might be safe to assume the schema is valid. Set to true to assume the - * produced schema is valid. - * - * Default: false - */ - assumeValid?: boolean, + ...GraphQLSchemaValidationOptions, |}; /** @@ -414,5 +409,6 @@ export function buildClientSchema( types, directives, assumeValid: options && options.assumeValid, + allowedLegacyNames: options && options.allowedLegacyNames, }); } From 3beb35503b863c3bbfd60a0d541f967c9c310c68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eloy=20Dur=C3=A1n?= Date: Fri, 9 Feb 2018 23:18:13 +0100 Subject: [PATCH 06/10] Add test coverage for schema validation options. --- src/type/__tests__/schema-test.js | 62 +++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/src/type/__tests__/schema-test.js b/src/type/__tests__/schema-test.js index ab48895d12..6e37a63301 100644 --- a/src/type/__tests__/schema-test.js +++ b/src/type/__tests__/schema-test.js @@ -95,4 +95,66 @@ describe('Type System: Schema', () => { expect(Schema.getTypeMap()).to.include.key('WrappedDirInput'); }); }); + + describe('Validity', () => { + describe('when not assumed valid', () => { + it('configures the schema to still needing validation', () => { + expect( + new GraphQLSchema({ + assumeValid: false, + }).__validationErrors, + ).to.equal(undefined); + }); + + it('configures the schema for allowed legacy names', () => { + expect( + new GraphQLSchema({ + allowedLegacyNames: ['__badName'], + }).__allowedLegacyNames, + ).to.deep.equal(['__badName']); + }); + + it('checks the configuration for mistakes', () => { + expect(() => new GraphQLSchema(() => null)).to.throw(); + expect(() => new GraphQLSchema({ types: {} })).to.throw(); + expect(() => new GraphQLSchema({ directives: {} })).to.throw(); + expect(() => new GraphQLSchema({ allowedLegacyNames: {} })).to.throw(); + }); + }); + + describe('when assumed valid', () => { + it('configures the schema to have no errors', () => { + expect( + new GraphQLSchema({ + assumeValid: true, + }).__validationErrors, + ).to.deep.equal([]); + }); + + it('still configures the schema for allowed legacy names', () => { + expect( + new GraphQLSchema({ + assumeValid: true, + allowedLegacyNames: ['__badName'], + }).__allowedLegacyNames, + ).to.deep.equal(['__badName']); + }); + + it('does not check the configuration for mistakes', () => { + expect(() => { + const config = () => null; + config.assumeValid = true; + return new GraphQLSchema(config); + }).to.not.throw(); + expect(() => { + return new GraphQLSchema({ + assumeValid: true, + types: {}, + directives: { reduce: () => [] }, + allowedLegacyNames: {}, + }); + }).to.not.throw(); + }); + }); + }); }); From 50be8c60a7eea6410fa6d77be783ae32c5a984b6 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Wed, 14 Feb 2018 15:31:25 -0500 Subject: [PATCH 07/10] Simplify value merge --- src/utilities/extendSchema.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index c857e88d35..90a4e659b7 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -240,13 +240,13 @@ export function extendSchema( types.push(definitionBuilder.buildType(typeName)); }); - let allowedLegacyNames = [ - ...(schema.__allowedLegacyNames || []), - ...((options && options.allowedLegacyNames) || []), - ]; - if (allowedLegacyNames.length === 0) { - allowedLegacyNames = null; - } + // Support both original legacy names and extended legacy names. + const schemaAllowedLegacyNames = schema.__allowedLegacyNames; + const extendAllowedLegacyNames = options && options.allowedLegacyNames; + const allowedLegacyNames = + schemaAllowedLegacyNames && extendAllowedLegacyNames + ? schemaAllowedLegacyNames.concat(extendAllowedLegacyNames) + : schemaAllowedLegacyNames || extendAllowedLegacyNames; // Then produce and return a Schema with these types. return new GraphQLSchema({ From e58be0f05b13dfd10f2eee5d04cd29e9b2f4faf5 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Wed, 14 Feb 2018 15:33:57 -0500 Subject: [PATCH 08/10] Add test --- src/utilities/__tests__/extendSchema-test.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/utilities/__tests__/extendSchema-test.js b/src/utilities/__tests__/extendSchema-test.js index 12840de116..85eff6a971 100644 --- a/src/utilities/__tests__/extendSchema-test.js +++ b/src/utilities/__tests__/extendSchema-test.js @@ -1221,6 +1221,25 @@ describe('extendSchema', () => { }); it('maintains configuration of the original schema object', () => { + const testSchemaWithLegacyNames = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: () => ({ + id: { type: GraphQLID }, + }), + }), + allowedLegacyNames: ['__badName'], + }); + const ast = parse(` + extend type Query { + __badName: String + } + `); + const schema = extendSchema(testSchemaWithLegacyNames, ast); + expect(schema.__allowedLegacyNames).to.deep.equal(['__badName']); + }); + + it('adds to the configuration of the original schema object', () => { const testSchemaWithLegacyNames = new GraphQLSchema({ query: new GraphQLObjectType({ name: 'Query', From ae1ab1dea3ce7e86ad7c0a7bb811a4ea77ab8cf9 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Wed, 14 Feb 2018 15:35:10 -0500 Subject: [PATCH 09/10] Trailing space --- src/utilities/extendSchema.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utilities/extendSchema.js b/src/utilities/extendSchema.js index 90a4e659b7..e0c812d06c 100644 --- a/src/utilities/extendSchema.js +++ b/src/utilities/extendSchema.js @@ -243,7 +243,7 @@ export function extendSchema( // Support both original legacy names and extended legacy names. const schemaAllowedLegacyNames = schema.__allowedLegacyNames; const extendAllowedLegacyNames = options && options.allowedLegacyNames; - const allowedLegacyNames = + const allowedLegacyNames = schemaAllowedLegacyNames && extendAllowedLegacyNames ? schemaAllowedLegacyNames.concat(extendAllowedLegacyNames) : schemaAllowedLegacyNames || extendAllowedLegacyNames; From e8b626240bb464de240aa3651d9893bb02ca2b18 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Wed, 14 Feb 2018 15:40:25 -0500 Subject: [PATCH 10/10] Recycling this value means we need to be read-only strict --- src/type/schema.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/type/schema.js b/src/type/schema.js index 981c9866bf..a83a0a2043 100644 --- a/src/type/schema.js +++ b/src/type/schema.js @@ -247,7 +247,7 @@ export type GraphQLSchemaValidationOptions = {| * This option is provided to ease adoption and may be removed in a future * major release. */ - allowedLegacyNames?: ?Array, + allowedLegacyNames?: ?$ReadOnlyArray, |}; export type GraphQLSchemaConfig = {