From 58f67cc7680f7371b5dd4260b9eef2ddcb403fcb Mon Sep 17 00:00:00 2001 From: Oryan Date: Fri, 23 Oct 2026 15:39:35 -0400 Subject: [PATCH 1/3] Reject enum values, interface fields and union members that a type declares twice A type that declared an enum value, interface field or union member twice, in the type and again in an extension, or twice in extensions, kept the first one and logged a warning. That hides mistakes: `interface Node { id: ID }` plus `extend interface Node { id: String }` built with `id: ID`, and only a log line said the extension's `id` was dropped. graphql-java's SchemaGenerator rejects these schemas. The build now throws a SchemaError that names the type, the repeated value, field or member, and where it is repeated. The test for extensions without a type no longer repeats anything, and a new test covers the errors. Fixes #863 --- .../graphql/kickstart/tools/SchemaParser.kt | 19 ++-- .../kickstart/tools/TypeExtensionTest.kt | 91 ++++++++++++++----- 2 files changed, 78 insertions(+), 32 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt index acfd9686..c2eedc95 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt @@ -249,7 +249,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) @@ -257,7 +257,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 @@ -308,7 +308,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) @@ -316,7 +316,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) } } } @@ -333,7 +333,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) @@ -375,11 +375,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.") - } + 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 9012e67f..02099afb 100644 --- a/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt @@ -14,6 +14,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 { @@ -342,13 +343,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 } @@ -356,41 +356,86 @@ 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") + assertEquals((schema.getType("FooBar") as GraphQLUnionType).types.map { it.name }, listOf("Foo", "Bar", "Baz")) + } - val node = schema.getType("Node") as GraphQLInterfaceType - assertEquals(node.fieldDefinitions.map { it.name }, listOf("id", "name")) - assertEquals(node.getFieldDefinition("id").description, "The base id") + @Test + 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 + ) + ) + + cases.forEach { repeat -> + val error = assertThrows(SchemaError::class.java) { + parser(repeat.schema.trimMargin()) { dictionary(*repeat.dictionary) }.makeExecutableSchema() + } - assertEquals((schema.getType("FooBar") as GraphQLUnionType).types.map { it.name }, listOf("Foo", "Bar", "Baz")) + assertEquals(error.message, repeat.message) + } } @Test @@ -473,6 +518,8 @@ class TypeExtensionTest { assertEquals(schema.getSchemaAppliedDirectives("link").map { it.getArgument("for")?.getValue() }, listOf("EXECUTION")) } + private class Repeat(val schema: String, val message: String, vararg val dictionary: KClass<*>) + private val fooBarBaz = """ type Foo { foo: String } type Bar { bar: String } From e858e425e64868d49e32eb49d3f11056f020e332 Mon Sep 17 00:00:00 2001 From: Oryan Date: Fri, 23 Oct 2026 15:39:58 -0400 Subject: [PATCH 2/3] Reject repeated fields of objects and input objects and list input fields like graphql-java An object type that declared a field in the type and again in an extension kept the last one, so `greeting` took the description of the extension's declaration. Input objects listed the fields of their extensions before their own, so `input Filter { term: String }` plus `extend input Filter { limit: Int }` gave `limit, term`. graphql-java rejects the repeated field and lists the fields of the type first, then those of each extension in order. Object and input fields now go through the same check as enum values, interface fields and union members. Input objects list the fields of the type first. Fixes #865 --- .../graphql/kickstart/tools/SchemaParser.kt | 32 ++++++++------- .../kickstart/tools/TypeExtensionTest.kt | 39 ++++++++++++++++++- 2 files changed, 55 insertions(+), 16 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt index c2eedc95..f3622dc1 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt @@ -171,6 +171,8 @@ class SchemaParser internal constructor( val name = objectDefinition.name val extensions = typeExtensions.extensionsOf(name) val directives = sdlDirectives.directivesOf(objectDefinition, extensions) + val fieldDefinitions = objectDefinition.withExtensions(typeExtensions).flatMap { it.fieldDefinitions } + rejectRepeated(name, "field", fieldDefinitions) val builder = GraphQLObjectType.newObject() .name(name) .definition(objectDefinition) @@ -185,7 +187,7 @@ class SchemaParser internal constructor( } } .apply { - objectDefinition.withExtensions(typeExtensions).flatMap { it.fieldDefinitions }.forEach { fieldDefinition -> + fieldDefinitions.forEach { fieldDefinition -> field { field -> createField(field, fieldDefinition, inputObjects) codeRegistryBuilder.dataFetcher( @@ -209,6 +211,8 @@ class SchemaParser internal constructor( inputObjectsInProgress: MutableSet): GraphQLInputObjectType { val extensionDefinitions = typeExtensions.extensionsOf(definition.name) val directives = sdlDirectives.directivesOf(definition, extensionDefinitions) + val inputValueDefinitions = definition.withExtensions(typeExtensions).flatMap { it.inputValueDefinitions } + rejectRepeated(definition.name, "field", inputValueDefinitions) inputObjectsInProgress.add(definition.name) @@ -219,20 +223,18 @@ class SchemaParser internal constructor( .description(getDocumentation(definition, options)) .withSdlDirectives(directives, INPUT_OBJECT) .apply { - (extensionDefinitions + definition).forEach { typeDefinition -> - typeDefinition.inputValueDefinitions.forEach { fieldDefinition -> - field( - GraphQLInputObjectField.newInputObjectField() - .name(fieldDefinition.name) - .definition(fieldDefinition) - .description(getDocumentation(fieldDefinition, options)) - .apply { fieldDefinition.defaultValue?.let { v -> defaultValueLiteral(v) } } - .apply { sdlDirectives.getDeprecated(fieldDefinition.directives)?.let { deprecate(it) } } - .type(determineInputType(fieldDefinition.type, inputObjects, inputObjectsInProgress)) - .withSdlDirectives(fieldDefinition.directives, INPUT_FIELD_DEFINITION) - .build() - ) - } + inputValueDefinitions.forEach { fieldDefinition -> + field( + GraphQLInputObjectField.newInputObjectField() + .name(fieldDefinition.name) + .definition(fieldDefinition) + .description(getDocumentation(fieldDefinition, options)) + .apply { fieldDefinition.defaultValue?.let { v -> defaultValueLiteral(v) } } + .apply { sdlDirectives.getDeprecated(fieldDefinition.directives)?.let { deprecate(it) } } + .type(determineInputType(fieldDefinition.type, inputObjects, inputObjectsInProgress)) + .withSdlDirectives(fieldDefinition.directives, INPUT_FIELD_DEFINITION) + .build() + ) } } diff --git a/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt b/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt index 02099afb..f37a91b6 100644 --- a/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt @@ -426,6 +426,28 @@ class TypeExtensionTest { """, "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." ) ) @@ -438,6 +460,21 @@ class TypeExtensionTest { } } + @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 fun `extensions of a different kind than the type are ignored`() { val (schema, warnings) = withWarnings { @@ -549,7 +586,7 @@ class TypeExtensionTest { class Item(val id: String, val name: String?, val fooBar: Foo? = null) : Node - class Filter(val id: String?) + class Filter(val id: String?, val limit: Int? = null, val offset: Int? = null) enum class Color { RED, GREEN } From 0dcf7df111e8f1441c0fd074c7fb58ec72aa13d1 Mon Sep 17 00:00:00 2001 From: Oryan Date: Fri, 23 Oct 2026 16:02:53 -0400 Subject: [PATCH 3/3] Test repeated object and input fields in one definition and between extensions --- .../kickstart/tools/TypeExtensionTest.kt | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt b/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt index f37a91b6..f04915ee 100644 --- a/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/TypeExtensionTest.kt @@ -448,6 +448,42 @@ class TypeExtensionTest { |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 { 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." ) )