From 47443ade77a580df6ab5023fbaddd2b1625a81e3 Mon Sep 17 00:00:00 2001 From: Matt Krick Date: Wed, 7 Mar 2018 18:47:22 -0800 Subject: [PATCH 1/7] fix #1277 ensure interface has at least 1 concrete type --- src/type/schema.js | 3 +++ src/type/validate.js | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/type/schema.js b/src/type/schema.js index 1b2ea738d1..6365199a42 100644 --- a/src/type/schema.js +++ b/src/type/schema.js @@ -8,6 +8,7 @@ */ import { + isAbstractType, isObjectType, isInterfaceType, isUnionType, @@ -159,6 +160,8 @@ export class GraphQLSchema { this._implementations[iface.name] = [type]; } }); + } else if (isAbstractType(type) && !this._implementations[type.name]) { + this._implementations[type.name] = []; } }); } diff --git a/src/type/validate.js b/src/type/validate.js index d65b0dc87c..5c4cbf143b 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -257,6 +257,9 @@ function validateTypes(context: SchemaValidationContext): void { } else if (isInterfaceType(type)) { // Ensure fields are valid. validateFields(context, type); + + // Ensure Interfaces include at least 1 concrete type. + validateInterfaces(context, type); } else if (isUnionType(type)) { // Ensure Unions include valid member types. validateUnionMembers(context, type); @@ -355,6 +358,21 @@ function validateObjectInterfaces( }); } +function validateInterfaces( + context: SchemaValidationContext, + iface: GraphQLInterfaceType, +): void { + const possibleTypes = context.schema.getPossibleTypes(iface); + + if (possibleTypes.length === 0) { + context.reportError( + `No concrete types found for Interface type ${iface.name}. ` + + `If only referenced via abstraction, add concrete types to schema.types array`, + iface.astNode, + ); + } +} + function validateObjectImplementsInterface( context: SchemaValidationContext, object: GraphQLObjectType, From 1c1e089f49ac0f5841a24acdd922eb9030cd6508 Mon Sep 17 00:00:00 2001 From: Matt Krick Date: Wed, 7 Mar 2018 19:48:51 -0800 Subject: [PATCH 2/7] fix tests --- src/type/__tests__/validation-test.js | 21 ++++++++++++++++----- src/type/schema.js | 2 +- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/type/__tests__/validation-test.js b/src/type/__tests__/validation-test.js index 2105f1bda3..147bdb7ac5 100644 --- a/src/type/__tests__/validation-test.js +++ b/src/type/__tests__/validation-test.js @@ -31,9 +31,15 @@ const SomeScalarType = new GraphQLScalarType({ parseLiteral() {}, }); +const SomeInterfaceType = new GraphQLInterfaceType({ + name: 'SomeInterface', + fields: { f: { type: GraphQLString } }, +}); + const SomeObjectType = new GraphQLObjectType({ name: 'SomeObject', fields: { f: { type: GraphQLString } }, + interfaces: [SomeInterfaceType], }); const SomeUnionType = new GraphQLUnionType({ @@ -41,11 +47,6 @@ const SomeUnionType = new GraphQLUnionType({ types: [SomeObjectType], }); -const SomeInterfaceType = new GraphQLInterfaceType({ - name: 'SomeInterface', - fields: { f: { type: GraphQLString } }, -}); - const SomeEnumType = new GraphQLEnumType({ name: 'SomeEnum', values: { @@ -772,6 +773,7 @@ describe('Type System: Object fields must have output types', () => { f: { type: BadObjectType }, }, }), + types: [SomeObjectType], }); } @@ -1016,6 +1018,14 @@ describe('Type System: Interface fields must have output types', () => { }, }); + const BadImplementingType = new GraphQLObjectType({ + name: 'BadImplementing', + interfaces: [BadInterfaceType], + fields: { + badField: { type: fieldType }, + }, + }); + return new GraphQLSchema({ query: new GraphQLObjectType({ name: 'Query', @@ -1023,6 +1033,7 @@ describe('Type System: Interface fields must have output types', () => { f: { type: BadInterfaceType }, }, }), + types: [BadImplementingType, SomeObjectType], }); } diff --git a/src/type/schema.js b/src/type/schema.js index 6365199a42..9a3231c5d3 100644 --- a/src/type/schema.js +++ b/src/type/schema.js @@ -207,7 +207,7 @@ export class GraphQLSchema { if (!possibleTypeMap[abstractType.name]) { const possibleTypes = this.getPossibleTypes(abstractType); invariant( - Array.isArray(possibleTypes), + Array.isArray(possibleTypes) && possibleTypes.length > 0, `Could not find possible implementing types for ${abstractType.name} ` + 'in schema. Check that schema.types is defined and is an array of ' + 'all possible types in the schema.', From 40a22b402905fe91924521832800f2eea56aeb37 Mon Sep 17 00:00:00 2001 From: Matt Krick Date: Thu, 8 Mar 2018 16:49:21 -0800 Subject: [PATCH 3/7] update error message --- src/type/validate.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/type/validate.js b/src/type/validate.js index 5c4cbf143b..2b071c4d94 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -258,7 +258,7 @@ function validateTypes(context: SchemaValidationContext): void { // Ensure fields are valid. validateFields(context, type); - // Ensure Interfaces include at least 1 concrete type. + // Ensure Interfaces include at least 1 Object type. validateInterfaces(context, type); } else if (isUnionType(type)) { // Ensure Unions include valid member types. @@ -366,8 +366,7 @@ function validateInterfaces( if (possibleTypes.length === 0) { context.reportError( - `No concrete types found for Interface type ${iface.name}. ` + - `If only referenced via abstraction, add concrete types to schema.types array`, + `Interface ${iface.name} must be implemented by at least one object type.`, iface.astNode, ); } From bea481cabb0ef0515b6afec4b05d640d8efbd8b4 Mon Sep 17 00:00:00 2001 From: Matt Krick Date: Thu, 8 Mar 2018 17:03:32 -0800 Subject: [PATCH 4/7] fix lint --- src/type/validate.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/type/validate.js b/src/type/validate.js index 2b071c4d94..6420fd47c1 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -366,7 +366,8 @@ function validateInterfaces( if (possibleTypes.length === 0) { context.reportError( - `Interface ${iface.name} must be implemented by at least one object type.`, + `Interface ${iface.name} must be implemented` + + `by at least one Object type.`, iface.astNode, ); } From 87a5dcc0f57d40f8422eb511b5f3dcec6c3f9607 Mon Sep 17 00:00:00 2001 From: Matt Krick Date: Thu, 29 Mar 2018 18:49:20 -0700 Subject: [PATCH 5/7] remove impossible schema invariant and test --- src/type/__tests__/schema-test.js | 13 ------------- src/type/schema.js | 6 ------ 2 files changed, 19 deletions(-) diff --git a/src/type/__tests__/schema-test.js b/src/type/__tests__/schema-test.js index 1f8a5563e1..556776b071 100644 --- a/src/type/__tests__/schema-test.js +++ b/src/type/__tests__/schema-test.js @@ -76,19 +76,6 @@ const Schema = new GraphQLSchema({ }); describe('Type System: Schema', () => { - describe('Getting possible types', () => { - it('throws human-reable error if schema.types is not defined', () => { - const checkPossible = () => { - return Schema.isPossibleType(InterfaceType, ImplementingType); - }; - expect(checkPossible).to.throw( - 'Could not find possible implementing types for Interface in schema. ' + - 'Check that schema.types is defined and is an array of all possible ' + - 'types in the schema.', - ); - }); - }); - describe('Type Map', () => { it('includes input types only used in directives', () => { expect(Schema.getTypeMap()).to.include.key('DirInput'); diff --git a/src/type/schema.js b/src/type/schema.js index 9a3231c5d3..747f53959d 100644 --- a/src/type/schema.js +++ b/src/type/schema.js @@ -206,12 +206,6 @@ export class GraphQLSchema { if (!possibleTypeMap[abstractType.name]) { const possibleTypes = this.getPossibleTypes(abstractType); - invariant( - Array.isArray(possibleTypes) && possibleTypes.length > 0, - `Could not find possible implementing types for ${abstractType.name} ` + - 'in schema. Check that schema.types is defined and is an array of ' + - 'all possible types in the schema.', - ); possibleTypeMap[abstractType.name] = possibleTypes.reduce( (map, type) => ((map[type.name] = true), map), Object.create(null), From 3078b1b40bb346da20d6c896c40ec3d41b63acf9 Mon Sep 17 00:00:00 2001 From: Matt Krick Date: Thu, 29 Mar 2018 18:55:55 -0700 Subject: [PATCH 6/7] lint --- src/type/__tests__/schema-test.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/type/__tests__/schema-test.js b/src/type/__tests__/schema-test.js index 556776b071..e3f14ada41 100644 --- a/src/type/__tests__/schema-test.js +++ b/src/type/__tests__/schema-test.js @@ -23,12 +23,6 @@ const InterfaceType = new GraphQLInterfaceType({ fields: { fieldName: { type: GraphQLString } }, }); -const ImplementingType = new GraphQLObjectType({ - name: 'Object', - interfaces: [InterfaceType], - fields: { fieldName: { type: GraphQLString, resolve: () => '' } }, -}); - const DirectiveInputType = new GraphQLInputObjectType({ name: 'DirInput', fields: { From fb672ff574d3c9bf0c6b172e987b377201a2b123 Mon Sep 17 00:00:00 2001 From: Matt Krick Date: Tue, 3 Apr 2018 08:46:16 -0700 Subject: [PATCH 7/7] test for unimplemented interfaces --- src/type/__tests__/validation-test.js | 19 +++++++++++++++++++ src/type/validate.js | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/type/__tests__/validation-test.js b/src/type/__tests__/validation-test.js index 147bdb7ac5..aff0cc7085 100644 --- a/src/type/__tests__/validation-test.js +++ b/src/type/__tests__/validation-test.js @@ -1087,6 +1087,25 @@ describe('Type System: Interface fields must have output types', () => { }, ]); }); + + it('rejects an interface not implemented by at least one object', () => { + const schema = buildSchema(` + type Query { + test: SomeInterface + } + + interface SomeInterface { + foo: String + } + `); + expect(validateSchema(schema)).to.containSubset([ + { + message: + 'Interface SomeInterface must be implemented by at least one Object type.', + locations: [{ line: 6, column: 7 }], + }, + ]); + }); }); describe('Type System: Field arguments must have input types', () => { diff --git a/src/type/validate.js b/src/type/validate.js index 6420fd47c1..32093d8537 100644 --- a/src/type/validate.js +++ b/src/type/validate.js @@ -366,7 +366,7 @@ function validateInterfaces( if (possibleTypes.length === 0) { context.reportError( - `Interface ${iface.name} must be implemented` + + `Interface ${iface.name} must be implemented ` + `by at least one Object type.`, iface.astNode, );