More allowed legacy names (#1235)

* Allow legacy names when building AST schema.

* Always store allowed legacy names on schema.

* Allow additional legacy names when extending a schema.

* Standardise schema validation options.

* Allow legacy names in client schemas.

* Add test coverage for schema validation options.

* Simplify value merge

* Add test

* Trailing space

* Recycling this value means we need to be read-only strict

Author: alloy

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();
+      });
+    });
+  });
 });

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;
@@ -229,14 +229,16 @@ export class GraphQLSchema {
 
 type TypeMap = ObjMap<GraphQLNamedType>;
 
-export type GraphQLSchemaConfig = {
-  query?: ?GraphQLObjectType,
-  mutation?: ?GraphQLObjectType,
-  subscription?: ?GraphQLObjectType,
-  types?: ?Array<GraphQLNamedType>,
-  directives?: ?Array<GraphQLDirective>,
-  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
@@ -245,7 +247,17 @@ export type GraphQLSchemaConfig = {
    * This option is provided to ease adoption and may be removed in a future
    * major release.
    */
-  allowedLegacyNames?: ?Array<string>,
+  allowedLegacyNames?: ?$ReadOnlyArray<string>,
+|};
+
+export type GraphQLSchemaConfig = {
+  query?: ?GraphQLObjectType,
+  mutation?: ?GraphQLObjectType,
+  subscription?: ?GraphQLObjectType,
+  types?: ?Array<GraphQLNamedType>,
+  directives?: ?Array<GraphQLDirective>,
+  astNode?: ?SchemaDefinitionNode,
+  ...GraphQLSchemaValidationOptions,
 };
 
 function typeMapReducer(map: TypeMap, type: ?GraphQLType): TypeMap {

src/utilities/__tests__/buildASTSchema-test.js

@@ -764,6 +764,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', () => {

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({

src/utilities/__tests__/extendSchema-test.js

@@ -818,6 +818,30 @@ describe('extendSchema', () => {
     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',
+        fields: () => ({
+          __badName: { type: GraphQLString },
+        }),
+      }),
+      allowedLegacyNames: ['__badName'],
+    });
+    const ast = parse(`
+      extend type Query {
+        __anotherBadName: String
+      }
+    `);
+    const schema = extendSchema(testSchemaWithLegacyNames, ast, {
+      allowedLegacyNames: ['__anotherBadName'],
+    });
+    expect(schema.__allowedLegacyNames).to.deep.equal([
+      '__badName',
+      '__anotherBadName',
+    ]);
+  });
+
   describe('does not allow extending a non-object type', () => {
     it('not an object', () => {
       const ast = parse(`

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
@@ -227,6 +221,7 @@ export function buildASTSchema(
     directives,
     astNode: schemaDef,
     assumeValid: options && options.assumeValid,
+    allowedLegacyNames: options && options.allowedLegacyNames,
   });
 
   function getOperationTypes(schema: SchemaDefinitionNode) {

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,
   });
 }

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
@@ -245,6 +240,14 @@ export function extendSchema(
     types.push(definitionBuilder.buildType(typeName));
   });
 
+  // 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({
     query: queryType,
@@ -253,8 +256,7 @@ export function extendSchema(
     types,
     directives: getMergedDirectives(),
     astNode: schema.astNode,
-    allowedLegacyNames:
-      schema.__allowedLegacyNames && schema.__allowedLegacyNames.slice(),
+    allowedLegacyNames,
   });
 
   function appendExtensionToTypeExtensions(