From dbec446f2fef5d4ab41c185a86e0bb46aee33873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernd=20St=C3=BCbinger?= <41049452+stuebingerb@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:17:30 +0200 Subject: [PATCH] refactor!: refactor union properties handling Refactors handling of union properties and processes them just like regular fragments, thereby benefitting from all recent bugfixes and simplifying code. Fixes #697 BREAKING CHANGE: removed `Type.Union`, `Field.Union`, and `Execution.Union`. BREAKING CHANGE: invalid fragment conditions in union properties were previously simply ignored and now cause the query to fail. --- kgraphql/api/kgraphql.api | 30 ------ .../kgraphql/schema/execution/Execution.kt | 39 -------- .../execution/ParallelRequestExecutor.kt | 59 +---------- .../kgraphql/schema/structure/Field.kt | 19 ---- .../schema/structure/RequestInterpreter.kt | 51 ---------- .../schema/structure/SchemaCompilation.kt | 2 +- .../kgraphql/schema/structure/Validation.kt | 19 ---- .../kgraphql/schema/SchemaBuilderTest.kt | 2 +- .../language/FragmentsSpecificationTest.kt | 99 +++++++++++++++++++ .../typesystem/UnionsSpecificationTest.kt | 4 +- 10 files changed, 104 insertions(+), 220 deletions(-) diff --git a/kgraphql/api/kgraphql.api b/kgraphql/api/kgraphql.api index 987661e0..f1a292d5 100644 --- a/kgraphql/api/kgraphql.api +++ b/kgraphql/api/kgraphql.api @@ -826,15 +826,6 @@ public final class de/stuebingerb/kgraphql/schema/execution/Execution$Remote : d public synthetic fun withParent$de_stuebingerb_kgraphql (Lde/stuebingerb/kgraphql/schema/execution/Execution;)Lde/stuebingerb/kgraphql/schema/execution/Execution; } -public final class de/stuebingerb/kgraphql/schema/execution/Execution$Union : de/stuebingerb/kgraphql/schema/execution/Execution$Node { - public fun (Lde/stuebingerb/kgraphql/schema/model/ast/SelectionNode$FieldNode;Lde/stuebingerb/kgraphql/schema/structure/Field$Union;Ljava/util/Map;Ljava/util/Map;Lde/stuebingerb/kgraphql/schema/execution/Execution;)V - public final fun getMemberChildren ()Ljava/util/Map; - public final fun getUnionField ()Lde/stuebingerb/kgraphql/schema/structure/Field$Union; - public final fun memberExecution (Lde/stuebingerb/kgraphql/schema/structure/Type;)Lde/stuebingerb/kgraphql/schema/execution/Execution$Node; - public synthetic fun withParent$de_stuebingerb_kgraphql (Lde/stuebingerb/kgraphql/schema/execution/Execution;)Lde/stuebingerb/kgraphql/schema/execution/Execution$Node; - public synthetic fun withParent$de_stuebingerb_kgraphql (Lde/stuebingerb/kgraphql/schema/execution/Execution;)Lde/stuebingerb/kgraphql/schema/execution/Execution; -} - public final class de/stuebingerb/kgraphql/schema/execution/ExecutionMode : java/lang/Enum { public static final field Normal Lde/stuebingerb/kgraphql/schema/execution/ExecutionMode; public static final field Serial Lde/stuebingerb/kgraphql/schema/execution/ExecutionMode; @@ -2089,27 +2080,6 @@ public final class de/stuebingerb/kgraphql/schema/structure/Field$RemoteOperatio public fun isDeprecated ()Z } -public final class de/stuebingerb/kgraphql/schema/structure/Field$Union : de/stuebingerb/kgraphql/schema/structure/Field, de/stuebingerb/kgraphql/schema/model/FunctionWrapper { - public fun (Lde/stuebingerb/kgraphql/schema/model/PropertyDef$Union;Lde/stuebingerb/kgraphql/schema/structure/Type;Ljava/util/List;)V - public fun arity ()I - public fun checkAccess (Ljava/lang/Object;Lde/stuebingerb/kgraphql/Context;)V - public fun getArguments ()Ljava/util/List; - public fun getArgumentsDescriptor ()Ljava/util/Map; - public fun getDeprecationReason ()Ljava/lang/String; - public fun getDescription ()Ljava/lang/String; - public fun getHasReceiver ()Z - public fun getKFunction ()Lkotlin/reflect/KFunction; - public fun getName ()Ljava/lang/String; - public fun getReturnType ()Lde/stuebingerb/kgraphql/schema/structure/Type; - public fun hasReturnType ()Z - public fun invoke (Ljava/util/List;Ljava/util/List;Lcom/fasterxml/jackson/databind/ObjectWriter;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; - public fun invoke ([Ljava/lang/Object;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; - public fun isDeprecated ()Z - public fun subscribe (Ljava/lang/String;Lde/stuebingerb/kgraphql/schema/Subscriber;)V - public fun unsubscribe (Ljava/lang/String;)V - public fun valueParameters ()Ljava/util/List; -} - public final class de/stuebingerb/kgraphql/schema/structure/InputValue : de/stuebingerb/kgraphql/schema/introspection/__InputValue { public fun (Lde/stuebingerb/kgraphql/schema/model/InputValueDef;Lde/stuebingerb/kgraphql/schema/structure/Type;Ljava/lang/String;)V public synthetic fun (Lde/stuebingerb/kgraphql/schema/model/InputValueDef;Lde/stuebingerb/kgraphql/schema/structure/Type;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V diff --git a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/Execution.kt b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/Execution.kt index 574cbd3d..880e314f 100644 --- a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/Execution.kt +++ b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/Execution.kt @@ -7,7 +7,6 @@ import de.stuebingerb.kgraphql.schema.model.ast.OperationTypeNode import de.stuebingerb.kgraphql.schema.model.ast.SelectionNode import de.stuebingerb.kgraphql.schema.model.ast.VariableDefinitionNode import de.stuebingerb.kgraphql.schema.structure.Field -import de.stuebingerb.kgraphql.schema.structure.Type sealed class Execution { abstract val selectionNode: SelectionNode @@ -74,44 +73,6 @@ sealed class Execution { ) } - class Union( - node: SelectionNode.FieldNode, - val unionField: Field.Union<*>, - val memberChildren: Map>, - directives: Map?, - parent: Execution? - ) : Node( - selectionNode = node, - field = unionField, - children = emptyList(), - arguments = null, - directives = directives, - variables = null, - arrayIndex = null, - parent = parent - ) { - fun memberExecution(type: Type): Node = Node( - selectionNode = selectionNode, - field = field, - children = requireNotNull(memberChildren[type]) { - "Union ${unionField.name} has no member $type" - }, - arguments = arguments, - directives = directives, - variables = variables, - arrayIndex = arrayIndex, - parent = parent - ) - - override fun withParent(parent: Execution): Union = Union( - selectionNode, - unionField, - memberChildren, - directives, - parent - ) - } - class Remote( selectionNode: SelectionNode.FieldNode, field: Field, diff --git a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.kt b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.kt index a908fd6e..f2872d26 100644 --- a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.kt +++ b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/execution/ParallelRequestExecutor.kt @@ -46,7 +46,6 @@ internal class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecu forEach { execution -> when (execution) { is Execution.Fragment -> execution.elements.inspect() - is Execution.Union -> execution.memberChildren.values.flatten().inspect() is Execution.Node -> { execution.children.inspect() if (execution.field is Field.DataLoader<*, *, *>) { @@ -126,40 +125,6 @@ internal class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecu jsonNodeFactory.nullNode() } - private suspend fun createUnionOperationNode( - ctx: ExecutionContext, - parent: T, - node: Execution.Union, - unionProperty: Field.Union - ): Deferred { - try { - node.field.checkAccess(parent, ctx.requestContext) - } catch (e: Throwable) { - return handleException(ctx, node, node.field.returnType, e) - } - val operationResult: Any? = unionProperty.invoke( - funName = unionProperty.name, - receiver = parent, - inputValues = node.field.arguments, - args = node.arguments, - executionNode = node, - ctx = ctx - ).await() - - val possibleTypes = (unionProperty.returnType.unwrapped() as Type.Union).possibleTypes - val returnType = possibleTypes.find { it.isInstance(operationResult) } - - if (returnType == null && unionProperty.returnType.isNotNullable()) { - val expectedOneOf = possibleTypes.joinToString { it.name.toString() } - throw ExecutionException( - "Unexpected type of union property value, expected one of [$expectedOneOf] but was '$operationResult'", - node - ) - } - - return createNode(ctx, operationResult, node, returnType ?: unionProperty.returnType) - } - private suspend fun createNode( ctx: ExecutionContext, value: T?, @@ -224,13 +189,6 @@ internal class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecu node.children.isNotEmpty() -> createObjectNode(ctx, value, node, returnType) - node is Execution.Union -> createObjectNode( - ctx, - value, - node.memberExecution(returnType), - returnType - ) - // TODO: do we have to consider more? more validation e.g.? value is JsonNode -> CompletableDeferred(value) @@ -291,21 +249,6 @@ internal class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecu type: Type ): Pair>? { when (child) { - // Union is subclass of Node so check it first - is Execution.Union -> { - val field = checkNotNull(type.unwrapped()[child.key]) { - "Execution unit '${child.key}' is not contained by operation return type '${type.unwrapped().name}'" - } - if (field is Field.Union<*>) { - return child.aliasOrKey to createUnionOperationNode(ctx, value, child, field as Field.Union) - } else { - throw ExecutionException( - "Unexpected non-union field for union execution node '${child.aliasOrKey}'", - child - ) - } - } - is Execution.Remote -> { return child.aliasOrKey to handleFunctionProperty( ctx, @@ -407,7 +350,7 @@ internal class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecu return handleDataProperty(ctx, parentValue, node, field) } - else -> error("Unexpected field type '$field', should be Field.Kotlin, Field.Function or Field.DataLoader") + else -> error("Unexpected field type '$field', should be Field.Kotlin, Field.Function, or Field.DataLoader") } } else { return null diff --git a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Field.kt b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Field.kt index eee5196b..70fd1f40 100644 --- a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Field.kt +++ b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Field.kt @@ -84,25 +84,6 @@ sealed class Field : __Field { } } - class Union( - private val kql: PropertyDef.Union, - override val returnType: Type, - override val arguments: List> - ) : Field(), FunctionWrapper by kql { - - override val name: String = kql.name - - override val description: String? = kql.description - - override val isDeprecated: Boolean = kql.isDeprecated - - override val deprecationReason: String? = kql.deprecationReason - - override fun checkAccess(parent: Any?, ctx: Context) { - kql.accessRule?.invoke(parent as T?, ctx)?.let { throw it } - } - } - class RemoteOperation( private val kql: BaseOperationDef, val field: Delegated, diff --git a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt index 5f0d3455..7362fba8 100644 --- a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt +++ b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/RequestInterpreter.kt @@ -43,9 +43,6 @@ internal class RequestInterpreter(private val schemaModel: SchemaModel) { // prevent stack overflow private val fragmentsStack = Stack() - fun getFragment(name: String) = - checkNotNull(fragments[name]) { "Fragment '$name' not found" }.also { usedFragments.add(name) } - fun get(node: FragmentSpreadNode): Execution.Fragment? { if (fragmentsStack.contains(node.name.value)) { throw ValidationException("Fragment spread circular references are not allowed", node) @@ -176,8 +173,6 @@ internal class RequestInterpreter(private val schemaModel: SchemaModel) { node ) - is Field.Union<*> -> handleUnion(field, node, ctx) - is Field.RemoteOperation<*, *> -> handleRemoteOperation(field, node, ctx) else -> { @@ -251,52 +246,6 @@ internal class RequestInterpreter(private val schemaModel: SchemaModel) { ) } - private fun handleUnion( - field: Field.Union, - selectionNode: FieldNode, - ctx: InterpreterContext - ): Execution.Union { - validateUnionRequest(field, selectionNode) - - // https://spec.graphql.org/October2021/#sec-Unions - // "With interfaces and objects, only those fields defined on the type can be queried directly; to query - // other fields on an interface, typed fragments must be used. This is the same as for unions, but unions - // do not define any fields, so *no* fields may be queried on this type without the use of type refining - // fragments or inline fragments (with the exception of the meta-field `__typename`)." - val unionMembersChildren: Map> = - (field.returnType.unwrapped() as Type.Union).possibleTypes.associateWith { possibleType -> - val mergedSelectionsForType = selectionNode.selectionSet?.selections?.flatMap { - when { - // Only __typename is allowed as field selection - it is FieldNode && it.name.value == "__typename" - -> listOf(it) - - it is FragmentSpreadNode && ctx.getFragment(it.name.value).first.name == possibleType.name - -> ctx.getFragment(it.name.value).second.selections - - it is InlineFragmentNode && possibleType.name == it.typeCondition?.name?.value - -> it.selectionSet.selections - - else -> emptyList() - } - } - - if (!mergedSelectionsForType.isNullOrEmpty()) { - handleReturnType(ctx, possibleType, SelectionSetNode(null, mergedSelectionsForType)) - } else { - emptyList() - } - } - - return Execution.Union( - node = selectionNode, - unionField = field, - memberChildren = unionMembersChildren, - directives = selectionNode.directives?.lookup(), - parent = null - ) - } - private fun Type?.validateForFragment(fragment: ASTNode, enclosingType: Type? = null): Type { val (typeName, fragmentName) = when (fragment) { is InlineFragmentNode -> fragment.typeCondition?.name?.value to "inline fragment" 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 e27fbf05..8dc41d82 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 @@ -230,7 +230,7 @@ open class SchemaCompilation( private suspend fun handleUnionProperty(unionProperty: PropertyDef.Union<*>): Field { val inputValues = handleInputValues(unionProperty, unionProperty.inputValues) val type = applyNullability(unionProperty.nullable, handleUnionType(unionProperty.union)) - return Field.Union(unionProperty, type, inputValues) + return Field.Function(unionProperty, type, inputValues) } private suspend fun handlePossiblyWrappedType(kType: KType, typeCategory: TypeCategory): Type = when { diff --git a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Validation.kt b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Validation.kt index 5d4cea0c..956a58e9 100644 --- a/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Validation.kt +++ b/kgraphql/src/main/kotlin/de/stuebingerb/kgraphql/schema/structure/Validation.kt @@ -57,25 +57,6 @@ private fun Field.validateArguments(requestNode: FieldNode, parentTypeName: Stri return exceptions } -/** - * validate that only typed fragments or __typename are present - */ -internal fun validateUnionRequest(field: Field.Union<*>, selectionNode: FieldNode) { - val illegalChildren = - selectionNode.selectionSet?.selections?.filterIsInstance()?.filter { it.name.value != "__typename" } - - if (!illegalChildren.isNullOrEmpty()) { - throw ValidationException( - message = "Invalid selection set with properties: ${ - illegalChildren.joinToString(prefix = "[", postfix = "]") { - it.aliasOrName.value - } - } on union type property ${field.name} : ${(field.returnType.unwrapped() as Type.Union).possibleTypes.map { it.name }}", - node = selectionNode - ) - } -} - fun validateName(name: String) { if (name.startsWith("__")) { throw SchemaException( diff --git a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/schema/SchemaBuilderTest.kt b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/schema/SchemaBuilderTest.kt index 8c8bdae0..6b0173b4 100644 --- a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/schema/SchemaBuilderTest.kt +++ b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/schema/SchemaBuilderTest.kt @@ -203,7 +203,7 @@ class SchemaBuilderTest { val unionField = scenarioType["pdf"] unionField shouldNotBe null - unionField shouldBeInstanceOf Field.Union::class + unionField shouldBeInstanceOf Field.Function::class } @Test diff --git a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.kt b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.kt index 014c6948..8af17b46 100644 --- a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.kt +++ b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/language/FragmentsSpecificationTest.kt @@ -1,6 +1,8 @@ package de.stuebingerb.kgraphql.specification.language import de.stuebingerb.kgraphql.Actor +import de.stuebingerb.kgraphql.Film +import de.stuebingerb.kgraphql.FilmType import de.stuebingerb.kgraphql.KGraphQL import de.stuebingerb.kgraphql.Specification import de.stuebingerb.kgraphql.ValidationException @@ -602,6 +604,103 @@ class FragmentsSpecificationTest : BaseSchemaTest() { } } + @Test + fun `fragments on impossible union properties should be denied`() { + val schema = KGraphQL.schema { + val movieUnion = unionType("MovieUnion") { + type() + type() + } + + enum() + + type { + unionProperty("favourite") { + returnType = movieUnion + resolver { actor -> + actor + } + } + } + + query("actors") { + resolver { -> + listOf(Actor("John Smith", 42), Actor("Foo Bar", 24)) + } + } + } + + expectRequestError("Invalid type 'Director' in type condition on fragment 'directorFragment' of type 'MovieUnion'; must be one of '[Actor, Film]'") { + schema.executeBlocking( + """ + { + actors { + name + favourite { + ...actorFragment + ...directorFragment + } + } + } + fragment actorFragment on Actor { + name + } + fragment directorFragment on Director { + name + } + """.trimIndent() + ) + } + + expectRequestError("Fragments can only be specified on object types, interfaces, and unions but 'FilmType' is ENUM on inline fragment") { + schema.executeBlocking( + """ + { + actors { + name + favourite { + ... on Actor { name } + ... on FilmType { illegal } + } + } + } + """.trimIndent() + ) + } + + expectRequestError("Fragments can only be specified on object types, interfaces, and unions but 'String' is SCALAR on inline fragment") { + schema.executeBlocking( + """ + { + actors { + name + favourite { + ... on Actor { name } + ... on String { illegal } + } + } + } + """.trimIndent() + ) + } + + expectRequestError("Unknown type 'MissingType' in type condition on inline fragment") { + schema.executeBlocking( + """ + { + actors { + name + favourite { + ... on Actor { name } + ... on MissingType { illegal } + } + } + } + """.trimIndent() + ) + } + } + @Test fun `fragments on impossible interface types should be denied`() { val schema = KGraphQL.schema { diff --git a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/UnionsSpecificationTest.kt b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/UnionsSpecificationTest.kt index e6aeccb1..b55471b2 100644 --- a/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/UnionsSpecificationTest.kt +++ b/kgraphql/src/test/kotlin/de/stuebingerb/kgraphql/specification/typesystem/UnionsSpecificationTest.kt @@ -65,7 +65,7 @@ class UnionsSpecificationTest : BaseSchemaTest() { @Test fun `query union property with invalid selection set`() { - expectRequestError("Invalid selection set with properties: [name] on union type property favourite : [Actor, Scenario, Director]") { + expectRequestError("Property 'name' on 'Favourite' does not exist") { testedSchema.executeBlocking("{actors{name, favourite{ name }}}") } } @@ -189,7 +189,7 @@ class UnionsSpecificationTest : BaseSchemaTest() { @Test fun `non-nullable union types should fail`() { - expectExecutionError("Unexpected type of union property value, expected one of [Actor, Scenario, Director] but was 'null'") { + expectExecutionError("Null result for non-nullable operation 'favourite'") { testedSchema.executeBlocking( """{ actors(all: true) {