diff --git a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/SchemaCompilation.kt b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/SchemaCompilation.kt index daed9018..b7e60e09 100644 --- a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/SchemaCompilation.kt +++ b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/SchemaCompilation.kt @@ -14,7 +14,6 @@ import de.stuebingerb.kgraphql.schema.SchemaException import de.stuebingerb.kgraphql.schema.builtin.BuiltInScalars import de.stuebingerb.kgraphql.schema.directive.Directive import de.stuebingerb.kgraphql.schema.execution.Execution -import de.stuebingerb.kgraphql.schema.introspection.NotIntrospected import de.stuebingerb.kgraphql.schema.introspection.SchemaProxy import de.stuebingerb.kgraphql.schema.introspection.TypeKind import de.stuebingerb.kgraphql.schema.introspection.__Schema @@ -34,7 +33,6 @@ import kotlin.reflect.KProperty1 import kotlin.reflect.KType import kotlin.reflect.KVisibility import kotlin.reflect.full.createType -import kotlin.reflect.full.findAnnotation import kotlin.reflect.full.isSubclassOf import kotlin.reflect.full.isSuperclassOf import kotlin.reflect.full.memberProperties @@ -356,7 +354,8 @@ open class SchemaCompilation( acc + def.transformations.mapKeys { (property) -> property.name } } - val kotlinFields = kClass.memberProperties + val declaredFields = mutableListOf() + kClass.memberProperties .filter { field -> field.visibility == KVisibility.PUBLIC } .filterNot { field -> objectDefs.any { it.isIgnored(field.name) } } .map { property -> @@ -365,6 +364,8 @@ open class SchemaCompilation( kqlProperty = allKotlinProperties[property.name], transformation = allTransformations[property.name] ) + }.forEach { + declaredFields.addField(it) } val extensionFields = objectDefs @@ -386,32 +387,20 @@ open class SchemaCompilation( val __typenameField = typenameField(FunctionWrapper.on(typenameResolver, true)) // https://spec.graphql.org/October2021/#sec-Objects.Type-Validation - val declaredFields = kotlinFields.toMutableList() extensionFields.forEach { - if (declaredFields.any { field -> field.name == it.name }) { - throw SchemaException("Cannot add extension field with duplicated name '${it.name}'") - } - declaredFields.add(it) + declaredFields.addField(it, "extension") } unionFields.forEach { - if (declaredFields.any { field -> field.name == it.name }) { - throw SchemaException("Cannot add union field with duplicated name '${it.name}'") - } - declaredFields.add(it) + declaredFields.addField(it, "union") } dataloadExtensionFields.forEach { - if (declaredFields.any { field -> field.name == it.name }) { - throw SchemaException("Cannot add dataloaded field with duplicated name '${it.name}'") - } - declaredFields.add(it) + declaredFields.addField(it, "dataloaded") } if (declaredFields.isEmpty()) { throw SchemaException("An object type must define one or more fields. Found none on type '${objectDef.name}'") } - declaredFields.forEach { validateName(it.name) } - val allFields = declaredFields + __typenameField typeProxy.proxied = if (kind == TypeKind.OBJECT) { Type.Object(objectDef, allFields) @@ -421,6 +410,23 @@ open class SchemaCompilation( return typeProxy } + private fun MutableList.addField(field: Field, fieldType: String? = null) { + validateName(field.name) + if (any { it.name == field.name }) { + val fieldTypeString = fieldType?.let { "$it " } ?: "" + throw SchemaException("Cannot add ${fieldTypeString}field with duplicated name '${field.name}'") + } + add(field) + } + + private fun MutableList>.addInputValue(value: InputValue<*>) { + validateName(value.name) + if (any { it.name == value.name }) { + throw SchemaException("Cannot add field with duplicated name '${value.name}'") + } + add(value) + } + private suspend fun handleInputType(kClass: KClass<*>): Type { assertValidInputObjectType(kClass) @@ -445,66 +451,60 @@ open class SchemaCompilation( val typeProxy = TypeProxy(objectType) inputTypeProxies[kClass] = typeProxy - val fieldNames = mutableSetOf() - val fields = if (kClass.findAnnotation() == null) { - if (kClass.isSealed) { - // OneOf input types have one field per allowed sub type - val sealedSubclasses = kClass.sealedSubclasses.toSet() - if (sealedSubclasses.isEmpty()) { - val type = if (kClass.java.isInterface) { - "interface" - } else { - "class" - } - throw SchemaException("The sealed $type '${kClass.simpleName}' does not have known subclasses") - } - sealedSubclasses.sortedBy { it.simpleName }.mapNotNull { clazz -> - val kProperty = inputObjectDef.oneOfProperties[clazz] - - if (kProperty?.isIgnored == true) { - return@mapNotNull null - } - - // All properties must be nullable - val type = handlePossiblyWrappedType(clazz.createType(nullable = true), TypeCategory.INPUT) - val parameterName = kProperty?.name ?: clazz.defaultOneOfPropertyName() - if (parameterName in fieldNames) { - throw SchemaException("Cannot add oneOf field with duplicated name '${parameterName}'") - } - fieldNames.add(parameterName) - InputValue( - InputValueDef( - kClass = clazz, - name = parameterName, - description = kProperty?.description, - isDeprecated = kProperty?.isDeprecated ?: false, - deprecationReason = kProperty?.deprecationReason, - parameterName = parameterName - ), - type - ) + val fields = mutableListOf>() + if (kClass.isSealed) { + // OneOf input types have one field per allowed sub type + val sealedSubclasses = kClass.sealedSubclasses.toSet() + if (sealedSubclasses.isEmpty()) { + val type = if (kClass.java.isInterface) { + "interface" + } else { + "class" } - } else { - val primaryConstructor = kClass.primaryConstructor - ?: throw SchemaException("Java class '${kClass.simpleName}' as input type is not supported") - val memberPropertiesByName = kClass.memberProperties.associateBy { it.name } - // Input types are created using their primary constructor. Therefore, it makes sense to (only) use the - // parameters of this constructor for the fields (cf. https://github.com/stuebingerb/KGraphQL/issues/235). - // Member properties are sorted by name (https://youtrack.jetbrains.com/issue/KT-41042), and SDL is - // sorting by name, so we'll also sort constructor parameters to be consistent. - primaryConstructor.parameters.sortedBy { it.name }.map { parameter -> - // kProperty is used to configure deprecation and description in the DSL, so we use it - // if available - val kProperty = memberPropertiesByName[parameter.name] - handleKotlinInputProperty( - parameter = parameter, - kProperty = kProperty, - kqlProperty = kProperty?.let { inputObjectDef.kotlinProperties[it] } - ) + throw SchemaException("The sealed $type '${kClass.simpleName}' does not have known subclasses") + } + sealedSubclasses.sortedBy { it.simpleName }.mapNotNull { clazz -> + val kProperty = inputObjectDef.oneOfProperties[clazz] + + if (kProperty?.isIgnored == true) { + return@mapNotNull null } + + // All properties must be nullable + val type = handlePossiblyWrappedType(clazz.createType(nullable = true), TypeCategory.INPUT) + val parameterName = kProperty?.name ?: clazz.defaultOneOfPropertyName() + val inputValue = InputValue( + InputValueDef( + kClass = clazz, + name = parameterName, + description = kProperty?.description, + isDeprecated = kProperty?.isDeprecated ?: false, + deprecationReason = kProperty?.deprecationReason, + parameterName = parameterName + ), + type + ) + fields.addInputValue(inputValue) } } else { - listOf() + val primaryConstructor = kClass.primaryConstructor + ?: throw SchemaException("Java class '${kClass.simpleName}' as input type is not supported") + val memberPropertiesByName = kClass.memberProperties.associateBy { it.name } + // Input types are created using their primary constructor. Therefore, it makes sense to (only) use the + // parameters of this constructor for the fields (cf. https://github.com/stuebingerb/KGraphQL/issues/235). + // Member properties are sorted by name (https://youtrack.jetbrains.com/issue/KT-41042), and SDL is + // sorting by name, so we'll also sort constructor parameters to be consistent. + primaryConstructor.parameters.sortedBy { it.name }.map { parameter -> + // kProperty is used to configure deprecation and description in the DSL, so we use it + // if available + val kProperty = memberPropertiesByName[parameter.name] + val inputValue = handleKotlinInputProperty( + parameter = parameter, + kProperty = kProperty, + kqlProperty = kProperty?.let { inputObjectDef.kotlinProperties[it] } + ) + fields.addInputValue(inputValue) + } } if (fields.isEmpty()) { @@ -515,9 +515,6 @@ open class SchemaCompilation( // OneOf input types require that exactly one field is set, which means that all others must be nullable throw SchemaException("OneOf input types must only have nullable fields. Fields ${notNullableFields.map { it.name }} are non-nullable on type '${inputObjectDef.name}'") } - - fields.forEach { validateName(it.name) } - typeProxy.proxied = Type.Input(inputObjectDef, fields) return typeProxy } diff --git a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/InputObjectTest.kt b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/InputObjectTest.kt index acfe0a35..e658b386 100644 --- a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/InputObjectTest.kt +++ b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/InputObjectTest.kt @@ -165,9 +165,11 @@ class InputObjectTest { @Test fun `property name must not start with __ when configured`() { - expect("Unable to handle input type 'Person': Illegal name '__name'. Names starting with '__' are reserved for introspection system") { + expect("Unable to handle input type 'PersonInput': Illegal name '__name'. Names starting with '__' are reserved for introspection system") { KGraphQL.schema { inputType { + name = "PersonInput" + property(Person::name) { name = "__name" } @@ -1200,4 +1202,51 @@ class InputObjectTest { } """.trimIndent() } + + @Test + fun `property names must be unique when configured`() { + data class MyInputObject(val foo: String?, val bar: String?) + expect("Unable to handle input type 'MyInputObject': Cannot add field with duplicated name 'foo'") { + KGraphQL.schema { + query("test") { + resolver { input: MyInputObject -> input.foo } + } + inputType { + MyInputObject::foo.configure { + deprecate("Deprecated old input value") + } + MyInputObject::bar.configure { + name = "foo" + } + } + } + } + + val schema = KGraphQL.schema { + query("test") { + resolver { input: MyInputObject -> input.foo } + } + inputType { + // Although highly confusing, this should work + MyInputObject::foo.configure { + name = "bar" + } + MyInputObject::bar.configure { + name = "foo" + } + } + } + + SchemaPrinter().print(schema) shouldBe """ + type Query { + test(input: MyInputObject!): String + } + + input MyInputObject { + bar: String + foo: String + } + + """.trimIndent() + } } diff --git a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/ObjectTest.kt b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/ObjectTest.kt index fa789339..bcbeb0b7 100644 --- a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/ObjectTest.kt +++ b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/integration/ObjectTest.kt @@ -3,6 +3,7 @@ package de.stuebingerb.kgraphql.integration import de.stuebingerb.kgraphql.KGraphQL import de.stuebingerb.kgraphql.expect import de.stuebingerb.kgraphql.schema.SchemaException +import de.stuebingerb.kgraphql.schema.SchemaPrinter import io.kotest.matchers.shouldBe import org.junit.jupiter.api.Test @@ -98,4 +99,51 @@ class ObjectTest { } } } + + @Test + fun `property names must be unique when configured`() { + data class MyObject(val foo: String?, val bar: String?) + expect("Unable to handle object type 'MyObject': Cannot add field with duplicated name 'foo'") { + KGraphQL.schema { + query("test") { + resolver { foo: String, bar: String -> MyObject(foo, bar) } + } + type { + MyObject::foo.configure { + deprecate("Deprecated old input value") + } + MyObject::bar.configure { + name = "foo" + } + } + } + } + + val schema = KGraphQL.schema { + query("test") { + resolver { foo: String, bar: String -> MyObject(foo, bar) } + } + type { + // Although highly confusing, this should work + MyObject::foo.configure { + name = "bar" + } + MyObject::bar.configure { + name = "foo" + } + } + } + + SchemaPrinter().print(schema) shouldBe """ + type MyObject { + bar: String + foo: String + } + + type Query { + test(bar: String!, foo: String!): MyObject! + } + + """.trimIndent() + } } diff --git a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/InputObjectsSpecificationTest.kt b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/InputObjectsSpecificationTest.kt index b22685d5..322cc904 100644 --- a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/InputObjectsSpecificationTest.kt +++ b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/InputObjectsSpecificationTest.kt @@ -804,7 +804,7 @@ class InputObjectsSpecificationTest { @Test fun `oneOf input objects out of sealed classes must have unique field names`() { - expect("Unable to handle input type 'InvalidInputSealed': Cannot add oneOf field with duplicated name 'inputSealedSub1'") { + expect("Unable to handle input type 'InvalidInputSealed': Cannot add field with duplicated name 'inputSealedSub1'") { KGraphQL.schema { query("test") { resolver { input: InputSealed? -> input }