From 68a0818b44f96e2ff18eb188eab815084ad8f211 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 29 Jan 2020 12:55:11 +0800 Subject: [PATCH] Schema: keep order of user provided types (#2410) Motivation: #2362 --- src/__tests__/starWarsIntrospection-test.js | 8 +- .../__tests__/union-interface-test.js | 4 +- src/type/__tests__/introspection-test.js | 18 +- src/type/__tests__/schema-test.js | 51 ++++ src/type/__tests__/validation-test.js | 16 +- src/type/schema.js | 224 +++++++++--------- 6 files changed, 185 insertions(+), 136 deletions(-) diff --git a/src/__tests__/starWarsIntrospection-test.js b/src/__tests__/starWarsIntrospection-test.js index 7bcfcf9b81..8db0c324c6 100644 --- a/src/__tests__/starWarsIntrospection-test.js +++ b/src/__tests__/starWarsIntrospection-test.js @@ -32,16 +32,16 @@ describe('Star Wars Introspection Tests', () => { expect(data).to.deep.equal({ __schema: { types: [ - { name: 'Query' }, - { name: 'Episode' }, + { name: 'Human' }, { name: 'Character' }, { name: 'String' }, - { name: 'Human' }, + { name: 'Episode' }, { name: 'Droid' }, + { name: 'Query' }, + { name: 'Boolean' }, { name: '__Schema' }, { name: '__Type' }, { name: '__TypeKind' }, - { name: 'Boolean' }, { name: '__Field' }, { name: '__InputValue' }, { name: '__EnumValue' }, diff --git a/src/execution/__tests__/union-interface-test.js b/src/execution/__tests__/union-interface-test.js index d92befc813..b85987f42d 100644 --- a/src/execution/__tests__/union-interface-test.js +++ b/src/execution/__tests__/union-interface-test.js @@ -199,7 +199,7 @@ describe('Execute: Union and intersection types', () => { name: 'Named', fields: [{ name: 'name' }], interfaces: [], - possibleTypes: [{ name: 'Person' }, { name: 'Dog' }, { name: 'Cat' }], + possibleTypes: [{ name: 'Dog' }, { name: 'Cat' }, { name: 'Person' }], enumValues: null, inputFields: null, }, @@ -208,7 +208,7 @@ describe('Execute: Union and intersection types', () => { name: 'Mammal', fields: [{ name: 'progeny' }, { name: 'mother' }, { name: 'father' }], interfaces: [{ name: 'Life' }], - possibleTypes: [{ name: 'Person' }, { name: 'Dog' }, { name: 'Cat' }], + possibleTypes: [{ name: 'Dog' }, { name: 'Cat' }, { name: 'Person' }], enumValues: null, inputFields: null, }, diff --git a/src/type/__tests__/introspection-test.js b/src/type/__tests__/introspection-test.js index 7744ee976d..5aea9aa837 100644 --- a/src/type/__tests__/introspection-test.js +++ b/src/type/__tests__/introspection-test.js @@ -69,6 +69,15 @@ describe('Introspection', () => { enumValues: null, possibleTypes: null, }, + { + kind: 'SCALAR', + name: 'Boolean', + fields: null, + inputFields: null, + interfaces: null, + enumValues: null, + possibleTypes: null, + }, { kind: 'OBJECT', name: '__Schema', @@ -385,15 +394,6 @@ describe('Introspection', () => { ], possibleTypes: null, }, - { - kind: 'SCALAR', - name: 'Boolean', - fields: null, - inputFields: null, - interfaces: null, - enumValues: null, - possibleTypes: null, - }, { kind: 'OBJECT', name: '__Field', diff --git a/src/type/__tests__/schema-test.js b/src/type/__tests__/schema-test.js index c8646a9080..ec98c576dd 100644 --- a/src/type/__tests__/schema-test.js +++ b/src/type/__tests__/schema-test.js @@ -253,6 +253,57 @@ describe('Type System: Schema', () => { }); }); + it('preserve order of use provided types', () => { + const aType = new GraphQLObjectType({ + name: 'A', + fields: { + sub: { type: new GraphQLScalarType({ name: 'ASub' }) }, + }, + }); + const zType = new GraphQLObjectType({ + name: 'Z', + fields: { + sub: { type: new GraphQLScalarType({ name: 'ZSub' }) }, + }, + }); + const queryType = new GraphQLObjectType({ + name: 'Query', + fields: { + a: { type: aType }, + z: { type: zType }, + sub: { type: new GraphQLScalarType({ name: 'QuerySub' }) }, + }, + }); + const schema = new GraphQLSchema({ + types: [zType, queryType, aType], + query: queryType, + }); + + const typeNames = Object.keys(schema.getTypeMap()); + expect(typeNames).to.deep.equal([ + 'Z', + 'ZSub', + 'Query', + 'QuerySub', + 'A', + 'ASub', + 'Boolean', + 'String', + '__Schema', + '__Type', + '__TypeKind', + '__Field', + '__InputValue', + '__EnumValue', + '__Directive', + '__DirectiveLocation', + ]); + + // Also check that this order is stable + const copySchema = new GraphQLSchema(schema.toConfig()); + expect(Object.keys(copySchema.getTypeMap())).to.deep.equal(typeNames); + }); + it('can be Object.toStringified', () => { const schema = new GraphQLSchema({}); diff --git a/src/type/__tests__/validation-test.js b/src/type/__tests__/validation-test.js index 7dc138f29d..b17245812f 100644 --- a/src/type/__tests__/validation-test.js +++ b/src/type/__tests__/validation-test.js @@ -1330,11 +1330,11 @@ describe('Type System: Interface fields must have output types', () => { expect(validateSchema(schema)).to.deep.equal([ { message: - 'The type of BadInterface.badField must be Output Type but got: undefined.', + 'The type of BadImplementing.badField must be Output Type but got: undefined.', }, { message: - 'The type of BadImplementing.badField must be Output Type but got: undefined.', + 'The type of BadInterface.badField must be Output Type but got: undefined.', }, ]); }); @@ -1346,10 +1346,10 @@ describe('Type System: Interface fields must have output types', () => { const schema = schemaWithInterfaceFieldOfType(type); expect(validateSchema(schema)).to.deep.equal([ { - message: `The type of BadInterface.badField must be Output Type but got: ${typeStr}.`, + message: `The type of BadImplementing.badField must be Output Type but got: ${typeStr}.`, }, { - message: `The type of BadImplementing.badField must be Output Type but got: ${typeStr}.`, + message: `The type of BadInterface.badField must be Output Type but got: ${typeStr}.`, }, ]); }); @@ -1361,14 +1361,14 @@ describe('Type System: Interface fields must have output types', () => { expect(validateSchema(schema)).to.deep.equal([ { message: - 'The type of BadInterface.badField must be Output Type but got: [function Number].', + 'The type of BadImplementing.badField must be Output Type but got: [function Number].', }, { - message: 'Expected GraphQL named type but got: [function Number].', + message: + 'The type of BadInterface.badField must be Output Type but got: [function Number].', }, { - message: - 'The type of BadImplementing.badField must be Output Type but got: [function Number].', + message: 'Expected GraphQL named type but got: [function Number].', }, ]); }); diff --git a/src/type/schema.js b/src/type/schema.js index 1950ef022f..891f942bf7 100644 --- a/src/type/schema.js +++ b/src/type/schema.js @@ -1,6 +1,7 @@ // @flow strict import find from '../polyfills/find'; +import arrayFrom from '../polyfills/arrayFrom'; import objectValues from '../polyfills/objectValues'; import { SYMBOL_TO_STRING_TAG } from '../polyfills/symbols'; @@ -130,8 +131,12 @@ export class GraphQLSchema { _subscriptionType: ?GraphQLObjectType; _directives: $ReadOnlyArray; _typeMap: TypeMap; - _implementations: ObjMap; _subTypeMap: ObjMap>; + _implementationsMap: ObjMap<{| + objects: Array, + interfaces: Array, + |}>; + // Used as a cache for validateSchema(). __validationErrors: ?$ReadOnlyArray; @@ -162,30 +167,89 @@ export class GraphQLSchema { // Provide specified directives (e.g. @include and @skip) by default. this._directives = config.directives || specifiedDirectives; - // Build type map now to detect any errors within this schema. - const initialTypes: Array = [ - this._queryType, - this._mutationType, - this._subscriptionType, - __Schema, - ].concat(config.types); - - // Keep track of all types referenced within the schema. - let typeMap: TypeMap = Object.create(null); + // To preserve order of user-provided types, we add first to add them to + // the set of "collected" types, so `collectReferencedTypes` ignore them. + const allReferencedTypes: Set = new Set(config.types); + if (config.types != null) { + for (const type of config.types) { + // When we ready to process this type, we remove it from "collected" types + // and then add it together with all dependent types in the correct position. + allReferencedTypes.delete(type); + collectReferencedTypes(type, allReferencedTypes); + } + } - // First by deeply visiting all initial types. - typeMap = initialTypes.reduce(typeMapReducer, typeMap); + if (this._queryType != null) { + collectReferencedTypes(this._queryType, allReferencedTypes); + } + if (this._mutationType != null) { + collectReferencedTypes(this._mutationType, allReferencedTypes); + } + if (this._subscriptionType != null) { + collectReferencedTypes(this._subscriptionType, allReferencedTypes); + } - // Then by deeply visiting all directive types. - typeMap = this._directives.reduce(typeMapDirectiveReducer, typeMap); + for (const directive of this._directives) { + // Directives are not validated until validateSchema() is called. + if (isDirective(directive)) { + for (const arg of directive.args) { + collectReferencedTypes(arg.type, allReferencedTypes); + } + } + } + collectReferencedTypes(__Schema, allReferencedTypes); // Storing the resulting map for reference by the schema. - this._typeMap = typeMap; - + this._typeMap = Object.create(null); this._subTypeMap = Object.create(null); - // Keep track of all implementations by interface name. - this._implementations = collectImplementations(objectValues(typeMap)); + this._implementationsMap = Object.create(null); + + for (const namedType of arrayFrom(allReferencedTypes)) { + if (namedType == null) { + continue; + } + + const typeName = namedType.name; + if (this._typeMap[typeName] !== undefined) { + throw new Error( + `Schema must contain uniquely named types but contains multiple types named "${typeName}".`, + ); + } + this._typeMap[typeName] = namedType; + + if (isInterfaceType(namedType)) { + // Store implementations by interface. + for (const iface of namedType.getInterfaces()) { + if (isInterfaceType(iface)) { + let implementations = this._implementationsMap[iface.name]; + if (implementations === undefined) { + implementations = this._implementationsMap[iface.name] = { + objects: [], + interfaces: [], + }; + } + + implementations.interfaces.push(namedType); + } + } + } else if (isObjectType(namedType)) { + // Store implementations by objects. + for (const iface of namedType.getInterfaces()) { + if (isInterfaceType(iface)) { + let implementations = this._implementationsMap[iface.name]; + if (implementations === undefined) { + implementations = this._implementationsMap[iface.name] = { + objects: [], + interfaces: [], + }; + } + + implementations.objects.push(namedType); + } + } + } + } } getQueryType(): ?GraphQLObjectType { @@ -218,8 +282,12 @@ export class GraphQLSchema { getImplementations( interfaceType: GraphQLInterfaceType, - ): InterfaceImplementations { - return this._implementations[interfaceType.name]; + ): {| + objects: /* $ReadOnly */ Array, + interfaces: /* $ReadOnly */ Array, + |} { + const implementations = this._implementationsMap[interfaceType.name]; + return implementations || { objects: [], interfaces: [] }; } // @deprecated: use isSubType instead - will be removed in v16. @@ -287,11 +355,6 @@ export class GraphQLSchema { type TypeMap = ObjMap; -type InterfaceImplementations = {| - objects: $ReadOnlyArray, - interfaces: $ReadOnlyArray, -|}; - export type GraphQLSchemaValidationOptions = {| /** * When building a schema from a GraphQL service's introspection result, it @@ -327,100 +390,35 @@ export type GraphQLSchemaNormalizedConfig = {| assumeValid: boolean, |}; -function collectImplementations( - types: $ReadOnlyArray, -): ObjMap { - const implementationsMap = Object.create(null); +function collectReferencedTypes( + type: GraphQLType, + typeSet: Set, +): Set { + const namedType = getNamedType(type); - for (const type of types) { - if (isInterfaceType(type)) { - if (implementationsMap[type.name] === undefined) { - implementationsMap[type.name] = { objects: [], interfaces: [] }; + if (!typeSet.has(namedType)) { + typeSet.add(namedType); + if (isUnionType(namedType)) { + for (const memberType of namedType.getTypes()) { + collectReferencedTypes(memberType, typeSet); + } + } else if (isObjectType(namedType) || isInterfaceType(namedType)) { + for (const interfaceType of namedType.getInterfaces()) { + collectReferencedTypes(interfaceType, typeSet); } - // Store implementations by interface. - for (const iface of type.getInterfaces()) { - if (isInterfaceType(iface)) { - const implementations = implementationsMap[iface.name]; - if (implementations === undefined) { - implementationsMap[iface.name] = { - objects: [], - interfaces: [type], - }; - } else { - implementations.interfaces.push(type); - } + for (const field of objectValues(namedType.getFields())) { + collectReferencedTypes(field.type, typeSet); + for (const arg of field.args) { + collectReferencedTypes(arg.type, typeSet); } } - } else if (isObjectType(type)) { - // Store implementations by objects. - for (const iface of type.getInterfaces()) { - if (isInterfaceType(iface)) { - const implementations = implementationsMap[iface.name]; - if (implementations === undefined) { - implementationsMap[iface.name] = { - objects: [type], - interfaces: [], - }; - } else { - implementations.objects.push(type); - } - } + } else if (isInputObjectType(namedType)) { + for (const field of objectValues(namedType.getFields())) { + collectReferencedTypes(field.type, typeSet); } } } - return implementationsMap; -} - -function typeMapReducer(map: TypeMap, type: ?GraphQLType): TypeMap { - if (!type) { - return map; - } - - const namedType = getNamedType(type); - const seenType = map[namedType.name]; - if (seenType) { - if (seenType !== namedType) { - throw new Error( - `Schema must contain uniquely named types but contains multiple types named "${namedType.name}".`, - ); - } - return map; - } - map[namedType.name] = namedType; - - let reducedMap = map; - - if (isUnionType(namedType)) { - reducedMap = namedType.getTypes().reduce(typeMapReducer, reducedMap); - } else if (isObjectType(namedType) || isInterfaceType(namedType)) { - reducedMap = namedType.getInterfaces().reduce(typeMapReducer, reducedMap); - - for (const field of objectValues(namedType.getFields())) { - const fieldArgTypes = field.args.map(arg => arg.type); - reducedMap = fieldArgTypes.reduce(typeMapReducer, reducedMap); - reducedMap = typeMapReducer(reducedMap, field.type); - } - } else if (isInputObjectType(namedType)) { - for (const field of objectValues(namedType.getFields())) { - reducedMap = typeMapReducer(reducedMap, field.type); - } - } - - return reducedMap; -} - -function typeMapDirectiveReducer( - map: TypeMap, - directive: ?GraphQLDirective, -): TypeMap { - // Directives are not validated until validateSchema() is called. - if (!isDirective(directive)) { - return map; - } - return directive.args.reduce( - (_map, arg) => typeMapReducer(_map, arg.type), - map, - ); + return typeSet; }