diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt index fc210651..6c574229 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt @@ -173,7 +173,7 @@ class SchemaParser internal constructor( val extensions = typeExtensions.extensionsOf(name) val directives = sdlDirectives.directivesOf(objectDefinition, extensions) val fieldDefinitions = objectDefinition.withExtensions(typeExtensions).flatMap { it.fieldDefinitions } - warnRepeatedFields(name, fieldDefinitions) + rejectRepeated(name, "field", fieldDefinitions) val builder = GraphQLObjectType.newObject() .name(name) .definition(objectDefinition) @@ -212,8 +212,8 @@ class SchemaParser internal constructor( inputObjectsInProgress: MutableSet): GraphQLInputObjectType { val extensionDefinitions = typeExtensions.extensionsOf(definition.name) val directives = sdlDirectives.directivesOf(definition, extensionDefinitions) - val fieldDefinitions = (extensionDefinitions + definition).flatMap { it.inputValueDefinitions } - warnRepeatedFields(definition.name, fieldDefinitions) + val inputValueDefinitions = definition.withExtensions(typeExtensions).flatMap { it.inputValueDefinitions } + rejectRepeated(definition.name, "field", inputValueDefinitions) inputObjectsInProgress.add(definition.name) @@ -224,7 +224,7 @@ class SchemaParser internal constructor( .description(getDocumentation(definition, options)) .withSdlDirectives(directives, INPUT_OBJECT) .apply { - fieldDefinitions.forEach { fieldDefinition -> + inputValueDefinitions.forEach { fieldDefinition -> field( GraphQLInputObjectField.newInputObjectField() .name(fieldDefinition.name) @@ -252,7 +252,7 @@ class SchemaParser internal constructor( val extensions = typeExtensions.extensionsOf(name) val directives = sdlDirectives.directivesOf(definition, extensions) val values = definition.withExtensions(typeExtensions).flatMap { it.enumValueDefinitions } - warnRepeated(name, "value", values.map { it.name }) + rejectRepeated(name, "value", values) val builder = GraphQLEnumType.newEnum() .name(name) .definition(definition) @@ -260,7 +260,7 @@ class SchemaParser internal constructor( .description(getDocumentation(definition, options)) .withSdlDirectives(directives, ENUM) .apply { - values.distinctBy { it.name }.forEach { valueDefinition -> + values.forEach { valueDefinition -> val enumName = valueDefinition.name val enumValue = if (enumClass == null) { enumName @@ -306,7 +306,7 @@ class SchemaParser internal constructor( val extensions = typeExtensions.extensionsOf(interfaceDefinition.name) val directives = sdlDirectives.directivesOf(interfaceDefinition, extensions) val fieldDefinitions = interfaceDefinition.withExtensions(typeExtensions).flatMap { it.fieldDefinitions } - warnRepeated(interfaceDefinition.name, "field", fieldDefinitions.map { it.name }) + rejectRepeated(interfaceDefinition.name, "field", fieldDefinitions) val builder = GraphQLInterfaceType.newInterface() .name(interfaceDefinition.name) .definition(interfaceDefinition) @@ -314,7 +314,7 @@ class SchemaParser internal constructor( .description(getDocumentation(interfaceDefinition, options)) .withSdlDirectives(directives, INTERFACE) .apply { - fieldDefinitions.distinctBy { it.name }.forEach { fieldDefinition -> + fieldDefinitions.forEach { fieldDefinition -> field { field -> createField(field, fieldDefinition, inputObjects) } } } @@ -331,7 +331,7 @@ class SchemaParser internal constructor( private fun createUnionObject(definition: UnionTypeDefinition, types: List): GraphQLUnionType { val extensions = typeExtensions.extensionsOf(definition.name) val directives = sdlDirectives.directivesOf(definition, extensions) - warnRepeated(definition.name, "member", definition.withExtensions(typeExtensions).flatMap { it.memberTypes }.map { (it as TypeName).name }) + rejectRepeated(definition.name, "member", definition.withExtensions(typeExtensions).flatMap { it.memberTypes }.map { it as TypeName }) val builder = GraphQLUnionType.newUnionType() .name(definition.name) .definition(definition) @@ -373,21 +373,10 @@ class SchemaParser internal constructor( return leafObjects } - // TODO throw a SchemaError instead in 15.0.0 (#863), like graphql-java does - private fun warnRepeated(typeName: String, kind: String, names: List) { - names.groupingBy { it }.eachCount().filterValues { it > 1 }.keys.forEach { - log.warn("Type '$typeName' declares $kind '$it' more than once, only the first one is kept.") - } - } - - // TODO throw a SchemaError instead in 15.0.0 (#865), like graphql-java does - private fun warnRepeatedFields(typeName: String, fieldDefinitions: List>) { - fieldDefinitions.groupBy { it.name }.values.filter { it.size > 1 }.forEach { declarations -> - val kept = declarations.last() - val location = kept.sourceLocation.describe() - val which = if (location.isEmpty()) "one of them" else "the one$location" - log.warn("Type '$typeName' declares field '${kept.name}' more than once, only $which is kept.") - } + private fun rejectRepeated(typeName: String, kind: String, declarations: List>) { + val names = mutableSetOf() + val repeat = declarations.firstOrNull { !names.add(it.name) } ?: return + throw SchemaError("Type '$typeName' declares $kind '${repeat.name}' more than once${repeat.sourceLocation.describe()}.") } private fun createField(field: GraphQLFieldDefinition.Builder, fieldDefinition: FieldDefinition, inputObjects: List): GraphQLFieldDefinition.Builder { diff --git a/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt b/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt index 680835aa..d1b05bab 100644 --- a/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt @@ -17,6 +17,7 @@ import graphql.schema.idl.SchemaPrinter import graphql.schema.validation.InvalidSchemaException import org.junit.Assert.assertThrows import org.junit.Test +import kotlin.reflect.KClass class TypeExtensionTest { @@ -345,13 +346,12 @@ class TypeExtensionTest { } @Test - fun `repeats and extensions without a type keep building`() { + fun `enums and unions that only have extensions are built from them`() { val (schema, warnings) = withWarnings { parser( """ type Query { color: Color - node: Node fooBar: FooBar } @@ -359,153 +359,156 @@ class TypeExtensionTest { "The first red" RED } - extend enum Color { RED GREEN } - - interface Node { - "The base id" - id: ID! - } - extend interface Node { id: ID! name: String } - - type Item implements Node { id: ID! name: String } + extend enum Color { GREEN } extend union FooBar = Foo | Bar - extend union FooBar = Bar | Baz + extend union FooBar = Baz $fooBarBaz """) { - dictionary(Item::class, Foo::class, Bar::class, Baz::class) + dictionary(Foo::class, Bar::class, Baz::class) }.makeExecutableSchema() } schema.assertEveryFieldResolves() - assertEquals(warnings, listOf( - "Type 'Node' declares field 'id' more than once, only the first one is kept.", - "Type 'FooBar' declares member 'Bar' more than once, only the first one is kept.", - "Type 'Color' declares value 'RED' more than once, only the first one is kept." - )) + assertEquals(warnings, listOf()) val color = schema.getType("Color") as GraphQLEnumType assertEquals(color.values.map { it.name }, listOf("RED", "GREEN")) assertEquals(color.getValue("RED")?.description, "The first red") - - val node = schema.getType("Node") as GraphQLInterfaceType - assertEquals(node.fieldDefinitions.map { it.name }, listOf("id", "name")) - assertEquals(node.getFieldDefinition("id").description, "The base id") - assertEquals((schema.getType("FooBar") as GraphQLUnionType).types.map { it.name }, listOf("Foo", "Bar", "Baz")) } @Test - fun `repeated fields of objects and inputs keep building and keep the last one`() { - assertRepeatedObjectField( - """ - type Item implements Node { - id: ID! - "From the type" - name: String - } - extend type Item { - "From the extension" - name: String - } - """, - location = "at line 9, column 5", - kept = "From the extension" - ) - assertRepeatedObjectField( - """ - type Item implements Node { id: ID! } - extend type Item { - "From the first extension" - name: String - } - extend type Item { - "From the second extension" - name: String - } - """, - location = "at line 9, column 5", - kept = "From the second extension" - ) - assertRepeatedObjectField( - """ - type Item implements Node { - id: ID! - "The first" - name: String - "The second" - name: String - } - """, - location = "at line 7, column 5", - kept = "The second" + fun `a type that declares a value, field or member twice is rejected`() { + val cases = listOf( + Repeat( + """ + |type Query { color: Color } + |enum Color { + | RED + |} + |extend enum Color { + | RED + | GREEN + |} + """, + "Type 'Color' declares value 'RED' more than once at line 6, column 5." + ), + Repeat( + """ + |type Query { color: Color } + |extend enum Color { RED } + |extend enum Color { RED GREEN } + """, + "Type 'Color' declares value 'RED' more than once at line 3, column 21." + ), + Repeat( + """ + |type Query { color: Color } + |enum Color { RED GREEN RED } + """, + "Type 'Color' declares value 'RED' more than once at line 2, column 24." + ), + Repeat( + """ + |type Query { node: Node } + |interface Node { id: ID } + |extend interface Node { id: String } + |type Item implements Node { id: ID } + """, + "Type 'Node' declares field 'id' more than once at line 3, column 25.", + Item::class + ), + Repeat( + """ + |type Query { fooBar: FooBar } + |union FooBar = Foo | Bar + |extend union FooBar = Bar | Baz + |$fooBarBaz + """, + "Type 'FooBar' declares member 'Bar' more than once at line 3, column 23.", + Foo::class, Bar::class, Baz::class + ), + Repeat( + """ + |type Query { node: Node } + |interface Node { id: ID } + |type Item implements Node { + | id: ID + |} + |extend type Item { + | id: ID + |} + """, + "Type 'Item' declares field 'id' more than once at line 7, column 5.", + Item::class + ), + Repeat( + """ + |type Query { search(filter: Filter): String } + |input Filter { id: ID } + |extend input Filter { id: ID } + """, + "Type 'Filter' declares field 'id' more than once at line 3, column 23." + ), + Repeat( + """ + |type Query { node: Node } + |interface Node { id: ID } + |type Item implements Node { id: ID name: String name: String } + """, + "Type 'Item' declares field 'name' more than once at line 3, column 49.", + Item::class + ), + Repeat( + """ + |type Query { search(filter: Filter): String } + |input Filter { id: ID id: ID } + """, + "Type 'Filter' declares field 'id' more than once at line 2, column 23." + ), + Repeat( + """ + |type Query { search(filter: Filter): String } + |input Filter { inner: Inner } + |input Inner { id: ID id: ID } + """, + "Type 'Inner' declares field 'id' more than once at line 3, column 22." + ), + Repeat( + """ + |type Query { node: Node } + |interface Node { id: ID } + |type Item implements Node { id: ID } + |extend type Item { name: String } + |extend type Item { name: String } + """, + "Type 'Item' declares field 'name' more than once at line 5, column 20.", + Item::class + ), + Repeat( + """ + |type Query { search(filter: Filter): String } + |input Filter { id: ID } + |extend input Filter { limit: Int } + |extend input Filter { limit: Int } + """, + "Type 'Filter' declares field 'limit' more than once at line 4, column 23." + ) ) - assertRepeatedInputField( - "Filter", - """ - input Filter { - "From the type" - id: ID - } - extend input Filter { - "From the extension" - id: ID - } - """, - location = "at line 3, column 5", - kept = "From the type" - ) - assertRepeatedInputField( - "Filter", - """ - input Filter { term: String } - extend input Filter { - "From the first extension" - id: ID - } - extend input Filter { - "From the second extension" - id: ID - } - """, - location = "at line 8, column 5", - kept = "From the second extension" - ) - assertRepeatedInputField( - "Filter", - """ - input Filter { - "The first" - id: ID - "The second" - id: ID - } - """, - location = "at line 5, column 5", - kept = "The second" - ) - assertRepeatedInputField( - "Inner", - """ - input Filter { - inner: Inner - } - input Inner { - "The first" - id: ID - "The second" - id: ID + cases.forEach { repeat -> + val error = assertThrows(SchemaError::class.java) { + parser(repeat.schema.trimMargin()) { dictionary(*repeat.dictionary) }.makeExecutableSchema() } - """, - location = "at line 8, column 5", - kept = "The second" - ) + + assertEquals(error.message, repeat.message) + } } @Test - fun `a repeated field without a source location says only that one is kept`() { + fun `a repeat without a source location is rejected without one`() { fun field(name: String) = FieldDefinition.newFieldDefinition().name(name).type(TypeName("String")).build() val item = ObjectTypeDefinition.newObjectTypeDefinition() .name("Item") @@ -513,7 +516,7 @@ class TypeExtensionTest { .fieldDefinitions(listOf(field("id"), field("name"), field("name"))) .build() - val (_, warnings) = withWarnings { + val error = assertThrows(SchemaError::class.java) { parser("type Query { node: Node }\ninterface Node { id: String }") { dictionary(Item::class) options(SchemaParserOptions.newOptions().typeDefinitionFactory(object : TypeDefinitionFactory { @@ -522,7 +525,22 @@ class TypeExtensionTest { }.makeExecutableSchema() } - assertEquals(warnings, listOf("Type 'Item' declares field 'name' more than once, only one of them is kept.")) + assertEquals(error.message, "Type 'Item' declares field 'name' more than once.") + } + + @Test + fun `input object fields list those of the type before those of its extensions`() { + val schema = parser( + """ + type Query { search(filter: Filter): String } + + extend input Filter { limit: Int } + input Filter { id: ID } + extend input Filter { offset: Int } + """).makeExecutableSchema() + .assertEveryFieldResolves() + + assertEquals((schema.getType("Filter") as GraphQLInputObjectType).fieldDefinitions.map { it.name }, listOf("id", "limit", "offset")) } @Test @@ -605,25 +623,7 @@ class TypeExtensionTest { assertEquals(schema.getSchemaAppliedDirectives("link").map { it.getArgument("for")?.getValue() }, listOf("EXECUTION")) } - private fun assertRepeatedObjectField(item: String, location: String, kept: String) { - val (schema, warnings) = withWarnings { - parser("type Query { node: Node }\ninterface Node { id: ID! }\n${item.trimIndent()}") { - dictionary(Item::class) - }.makeExecutableSchema() - } - - assertEquals(warnings, listOf("Type 'Item' declares field 'name' more than once, only the one $location is kept.")) - assertEquals((schema.getType("Item") as GraphQLObjectType).getFieldDefinition("name").description, kept) - } - - private fun assertRepeatedInputField(repeating: String, filter: String, location: String, kept: String) { - val (schema, warnings) = withWarnings { - parser("type Query { search(filter: Filter): String }\n${filter.trimIndent()}").makeExecutableSchema() - } - - assertEquals(warnings, listOf("Type '$repeating' declares field 'id' more than once, only the one $location is kept.")) - assertEquals((schema.getType(repeating) as GraphQLInputObjectType).getField("id").description, kept) - } + private class Repeat(val schema: String, val message: String, vararg val dictionary: KClass<*>) private val fooBarBaz = """ type Foo { foo: String } @@ -654,7 +654,7 @@ class TypeExtensionTest { class Item(val id: String, val name: String?, val fooBar: Foo? = null) : Node - class Filter(val id: String?, val term: String? = null, val inner: Inner? = null) + class Filter(val id: String?, val limit: Int? = null, val offset: Int? = null, val inner: Inner? = null) class Inner(val id: String?)