From 63c1fa6ef757b5e3cd85fe27659bc3eed4344356 Mon Sep 17 00:00:00 2001 From: Oryan Date: Thu, 5 Nov 2026 19:50:22 -0500 Subject: [PATCH 1/4] Unwrap property and Map field values like method values Public fields and typed Map values now go through the same TypeShape.Unwrapper as method return values, so nested futures are awaited, generic wrappers run their transformers and a DataFetcherResult inside an Optional is read. A Map whose value type is Object still hands its values to graphql-java as they are. A FutureTask behind a declared Future names the field or Map value in its error. The bounded check and ResolverCompatibilityTest have no known findings left. --- .../tools/resolver/MapFieldResolver.kt | 22 +++++--- .../tools/resolver/MethodFieldResolver.kt | 2 +- .../tools/resolver/PropertyFieldResolver.kt | 25 ++++----- .../kickstart/tools/resolver/TypeShape.kt | 34 ++++++------ .../tools/NestedGenericWrapperTest.kt | 2 +- .../tools/ResolverCompatibilityTest.kt | 52 ++++++++++--------- src/test/resources/returnvalue/Check.java | 21 ++++---- .../resources/returnvalue/known-findings.txt | 5 -- 8 files changed, 85 insertions(+), 78 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt index c13f67f4..f758c831 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt @@ -4,6 +4,7 @@ import com.fasterxml.classmate.TypeResolver import graphql.kickstart.tools.ScanDictionary import graphql.kickstart.tools.SchemaParserOptions import graphql.kickstart.tools.util.JavaType +import graphql.kickstart.tools.util.unwrap import graphql.language.FieldDefinition import graphql.schema.DataFetcher import graphql.schema.DataFetchingEnvironment @@ -23,7 +24,7 @@ internal class MapFieldResolver( mapClass: JavaType ) : FieldResolver(field, search, options) { - private var mapGenericValue: JavaType = getMapGenericType(mapClass) + private val mapGenericValue: JavaType = getMapGenericType(mapClass) /** * Takes a type which implements Map and tries to find the @@ -38,25 +39,32 @@ internal class MapFieldResolver( return typeParameters.elementAtOrElse(1) { Any::class.java } } - override fun createDataFetcher(): DataFetcher<*> { - return MapFieldResolverDataFetcher(createSourceResolver(), field.name) + private val returnShape by lazy { + TypeShape.returnType(field.type, mapGenericValue, search.isSubscription, search.type, options, ScanDictionary.FieldTypeReference(field.name)) } - override fun scanForMatches(): Sequence { - return sequenceOf(TypeShape.returnType(field.type, mapGenericValue, isSubscription = false, search.type, options, ScanDictionary.FieldTypeReference(field.name))) + private val hasTypedValues get() = resolveTypeVariables(mapGenericValue, search.type) != Any::class.java + + override fun createDataFetcher(): DataFetcher<*> { + val unwrapper = if (hasTypedValues) TypeShape.Unwrapper(returnShape, "map value", "${search.type.unwrap().name}.${field.name}") else null + return MapFieldResolverDataFetcher(createSourceResolver(), field.name, unwrapper) } + override fun scanForMatches(): Sequence = sequenceOf(returnShape) + override fun toString() = "MapFieldResolver{key=${field.name}}" } internal class MapFieldResolverDataFetcher( private val sourceResolver: SourceResolver, - private val key: String + private val key: String, + private val unwrapper: TypeShape.Unwrapper? ) : LightDataFetcher { override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any?, environmentSupplier: Supplier): Any? { if (sourceObject is Map<*, *>) { - return sourceObject[key] + val value = sourceObject[key] + return if (value == null || unwrapper == null) value else unwrapper.unwrap(value, environmentSupplier) } else { throw RuntimeException("MapFieldResolver attempt to fetch a field from an object instance that was not a map") } diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index 63fc38e2..b86d5e0e 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -114,7 +114,7 @@ internal class MethodFieldResolver( } } - val unwrapper = TypeShape.Unwrapper(returnShape, method, isSuspendFunction) + val unwrapper = TypeShape.Unwrapper(returnShape, "method", "${method.declaringClass.name}.${method.name}", isSuspendFunction) return if (numberOfParameters > 0 || isSuspendFunction) { // requires arguments and environment or is a suspend function diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt index 7f93c648..ee97d553 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt @@ -20,33 +20,28 @@ internal class PropertyFieldResolver( val property: Field ) : FieldResolver(field, search, options) { - override fun createDataFetcher(): DataFetcher<*> { - return PropertyFieldResolverDataFetcher(createSourceResolver(), property) + private val returnShape by lazy { + TypeShape.returnType(field.type, property.genericType, search.isSubscription, search.type, options, ScanDictionary.FieldTypeReference(property.toString())) } - override fun scanForMatches(): Sequence { - return sequenceOf( - TypeShape.returnType( - field.type, - property.genericType, - isSubscription = false, - search.type, - options, - ScanDictionary.FieldTypeReference(property.toString()) - ) - ) + override fun createDataFetcher(): DataFetcher<*> { + val unwrapper = TypeShape.Unwrapper(returnShape, "field", "${property.declaringClass.name}.${property.name}") + return PropertyFieldResolverDataFetcher(createSourceResolver(), property, unwrapper) } + override fun scanForMatches(): Sequence = sequenceOf(returnShape) + override fun toString() = "PropertyFieldResolver{property=$property}" } internal class PropertyFieldResolverDataFetcher( private val sourceResolver: SourceResolver, - private val field: Field + private val field: Field, + private val unwrapper: TypeShape.Unwrapper ) : LightDataFetcher { override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any?, environmentSupplier: Supplier): Any? { - return field.get(sourceResolver.resolve(null, sourceObject)) + return field.get(sourceResolver.resolve(null, sourceObject))?.let { unwrapper.unwrap(it, environmentSupplier) } } override fun get(environment: DataFetchingEnvironment): Any? { diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt index 1696a7ed..7d853a6e 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt @@ -21,7 +21,6 @@ import graphql.schema.idl.TypeUtil import kotlinx.coroutines.future.asDeferred import org.apache.commons.lang3.reflect.TypeUtils import org.reactivestreams.Publisher -import java.lang.reflect.Method import java.lang.reflect.ParameterizedType import java.lang.reflect.TypeVariable import java.lang.reflect.WildcardType @@ -47,23 +46,23 @@ internal class TypeShape private constructor( fun error(msg: String) = matchError(graphQLType, reference, javaType, msg) - class Unwrapper(shape: TypeShape, method: Method, isSuspendFunction: Boolean) { + class Unwrapper(shape: TypeShape, resolverKind: String, resolverName: String, isSuspendFunction: Boolean = false) { private val wrappers = shape.wrappers private val awaitable: Awaitable private val isOutermostFuture: Boolean private val nested: Step? init { - // the class the return type unwraps to, so a value that happens to implement Future isn't mistaken for a wrapper. - // Object (e.g. for a union) can't tell them apart, so then every future of a method whose return type is Future is checked + // the class the declared type unwraps to, so a value that happens to implement Future isn't mistaken for a wrapper. + // Object (e.g. for a union) can't tell them apart, so then every future of a value declared as Future is checked val valueClass = (TypeUtils.getRawType(shape.unwrappedRootType, null) ?: Any::class.java) .takeUnless { it == Any::class.java && TypeUtils.getRawType(shape.declaredType, null) == Future::class.java } - awaitable = Awaitable(method, valueClass, isSuspendFunction) + awaitable = Awaitable(resolverKind, resolverName, valueClass, isSuspendFunction) val outermost = shape.layers.firstOrNull() as? Layer.Wrapper val belowOutermost = if (outermost == null) shape.layers else shape.layers.drop(1) val leafClass = TypeUtils.getRawType(shape.leafJavaType, null) - val awaits = Awaitable(method, leafClass?.takeUnless { it == Any::class.java }, isSuspendFunction) - val peels = Awaitable(method, leafClass, isSuspendFunction) + val awaits = Awaitable(resolverKind, resolverName, leafClass?.takeUnless { it == Any::class.java }, isSuspendFunction) + val peels = Awaitable(resolverKind, resolverName, leafClass, isSuspendFunction) isOutermostFuture = outermost?.isFuture == true // graphql-java completes a subscription's events one at a time nested = if (shape.isSubscription) null else steps(belowOutermost, awaits, peels) @@ -102,7 +101,10 @@ internal class TypeShape private constructor( val javaType = if (isSubscription) declaredType.asPublisher() else declaredType val walk = Walk(graphQLType, javaType, containingType, GenericWrappers(options.genericWrappers), reference, Position.RETURN_TYPE) val (leafGraphQLType, leafJavaType) = walk.leaf() - return TypeShape(graphQLType, javaType, reference, leafGraphQLType, leafJavaType, walk.layers, declaredType, walk.unwrappedRootType, isSubscription, walk.wrappers) + return TypeShape( + graphQLType, javaType, reference, leafGraphQLType, leafJavaType, walk.layers, resolveTypeVariables(declaredType, containingType), + walk.unwrappedRootType, isSubscription, walk.wrappers + ) } fun parameterType( @@ -391,7 +393,12 @@ private fun Any?.asCompletionStage(): CompletionStage = // graphql-java only waits on a CompletionStage returned by a data fetcher, so any other future, or a future // that a suspend function completes with, would reach it as the field value -private class Awaitable(private val method: Method, private val valueClass: Class<*>?, private val isSuspendFunction: Boolean) { +private class Awaitable( + private val resolverKind: String, + private val resolverName: String, + private val valueClass: Class<*>?, + private val isSuspendFunction: Boolean +) { fun requireOutermost(value: Any?): Any? = if (value !is Future<*> || valueClass?.isInstance(value) == true || value is CompletionStage<*> && !isSuspendFunction) value else fail(value) @@ -401,13 +408,10 @@ private class Awaitable(private val method: Method, private val valueClass: Clas private fun fail(value: Any): Nothing = throw ResolverError( if (isSuspendFunction) { - "Suspend function '${method.resolverName}' resolved to a ${value.javaClass.name}, which graphql-java can't wait on. Return its value instead." + "Suspend function '$resolverName' resolved to a ${value.javaClass.name}, which graphql-java can't wait on. Return its value instead." } else { - "Resolver method '${method.resolverName}' resolved to a ${value.javaClass.name}, which graphql-java can't wait on. " + - "Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead." + "Resolver $resolverKind '$resolverName' resolved to a ${value.javaClass.name}, which graphql-java can't wait on. " + + "Return a CompletionStage, such as a CompletableFuture, from the $resolverKind or its generic wrapper transformer instead." } ) } - -private val Method.resolverName - get() = "${declaringClass.name}.$name" diff --git a/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt b/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt index da1a934b..ce343178 100644 --- a/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt @@ -250,7 +250,7 @@ class NestedGenericWrapperTest { if (typeShape.leafJavaType != leaf(graphQLType).javaClass) { return@mapNotNull "${TypeUtils.toString(javaType)} as ${AstPrinter.printAst(graphQLType)}: bound to ${TypeUtils.toString(typeShape.leafJavaType)}" } - val unwrapper = TypeShape.Unwrapper(typeShape, method, isSuspendFunction = false) + val unwrapper = TypeShape.Unwrapper(typeShape, "method", method.name) val value = value(shape, leaf(graphQLType)) val result = GraphQL.newGraphQL(fieldSchema(graphQLType) { environment -> unwrapper.unwrap(value) { environment } }).build() .execute(if (leaf(graphQLType) is Payload) "query { field { data } }" else "query { field }") diff --git a/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt b/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt index 40b0d1d4..a957dacd 100644 --- a/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt @@ -24,17 +24,12 @@ import java.util.concurrent.FutureTask class ResolverCompatibilityTest { @Test - fun `@JvmField and typed Map fields return what a method returns apart from the known findings, and untyped Map fields hand graphql-java the value as it is`() { + fun `@JvmField and typed Map fields return what a method returns, and untyped Map fields hand graphql-java the value as it is`() { val wrong = Kind.entries.flatMap { kind -> shapes.mapNotNull { shape -> val got = holderField(kind, shape) val expected = if (kind == Kind.UNTYPED_MAP) rawValueResults.getValue(shape.name) else shape.expected(kind) - val known = knownFindings[kind to shape.name] - when { - got == (known ?: expected) -> null - known != null && got == expected -> "$kind ${shape.name} now returns what a method returns. Delete its known finding." - else -> "$kind ${shape.name}\n expected: ${known ?: expected}\n got: $got" - } + "$kind ${shape.name}\n expected: $expected\n got: $got".takeUnless { got == expected } } } if (wrong.isNotEmpty()) { @@ -207,9 +202,9 @@ class ResolverCompatibilityTest { } @Test - fun `subscription events reach graphql-java as they are, so a future event prints as a string`() { + fun `subscription events of a method or a field reach graphql-java as they are, so a future or wrapped event prints as a string`() { val schema = SchemaParser.newParser() - .schemaString("type Query { unused: String } type Subscription { events: String futureEvents: String }") + .schemaString("type Query { unused: String } type Subscription { events: String futureEvents: String boxEvents: String }") .resolvers( object : GraphQLQueryResolver { fun unused(): String? = null @@ -223,7 +218,9 @@ class ResolverCompatibilityTest { fun futureEvents(): Publisher> = publish { send(completedFuture("a")) } - }) + }, + SubscriptionFields()) + .options(SchemaParserOptions.newOptions().genericWrappers(boxWrapper).build()) .build() .makeExecutableSchema() val gql = GraphQL.newGraphQL(schema).build() @@ -232,6 +229,7 @@ class ResolverCompatibilityTest { assertEquals(events("subscription { events }"), listOf("{events=a}", "{events=b}")) assertEquals(events("subscription { futureEvents }"), listOf("{futureEvents=CompletableFuture@...[Completed normally]}")) + assertEquals(events("subscription { boxEvents }"), listOf("{boxEvents=Box@...}")) } private fun execute( @@ -273,11 +271,11 @@ class ResolverCompatibilityTest { .replace("${ResolverCompatibilityTest::class.java.name}$", "") .replace("java.util.concurrent.", "") - enum class Kind(val holder: (Shape) -> Any) { - METHOD({ MethodHolder() }), - JVM_FIELD({ JvmFieldHolder() }), - TYPED_MAP({ shape -> shape.typedMap(JvmFieldHolder()) }), - UNTYPED_MAP({ shape -> hashMapOf(shape.name to shape.value()) }) + enum class Kind(val resolverKind: String, val holder: (Shape) -> Any) { + METHOD("method", { MethodHolder() }), + JVM_FIELD("field", { JvmFieldHolder() }), + TYPED_MAP("map value", { shape -> shape.typedMap(JvmFieldHolder()) }), + UNTYPED_MAP("map value", { shape -> hashMapOf(shape.name to shape.value()) }) } class Shape(val name: String, val type: String, private val expected: String, val typedMap: (JvmFieldHolder) -> Map) { @@ -285,7 +283,7 @@ class ResolverCompatibilityTest { fun value(): Any? = JvmFieldHolder::class.java.getField(name).get(JvmFieldHolder()) - fun expected(kind: Kind) = expected.replace("HOLDER", kind.holder(this).javaClass.simpleName) + fun expected(kind: Kind) = expected.replace("HOLDER", kind.holder(this).javaClass.simpleName).replace("KIND", kind.resolverKind) } class Foo(val name: String) @@ -302,6 +300,7 @@ class ResolverCompatibilityTest { @JvmField val listOfStringFutures: List> = listOf(completedFuture("a"), completedFuture("b")) @JvmField val boxOfString: Box = Box("a") @JvmField val listOfOptionals: List> = listOf(Optional.of("a"), Optional.empty()) + @JvmField val futureOfAny: CompletableFuture = completedFuture("a") } class MethodHolder { @@ -318,6 +317,7 @@ class ResolverCompatibilityTest { fun listOfStringFutures() = values.listOfStringFutures fun boxOfString() = values.boxOfString fun listOfOptionals() = values.listOfOptionals + fun futureOfAny() = values.futureOfAny } class FutureMap : HashMap>() @@ -331,6 +331,7 @@ class ResolverCompatibilityTest { class ListOfStringFuturesMap : HashMap>>() class BoxOfStringMap : HashMap>() class ListOfOptionalsMap : HashMap>>() + class FutureOfAnyMap : HashMap>() open class Box(val content: T) @@ -370,6 +371,10 @@ class ResolverCompatibilityTest { suspend fun future(): CompletableFuture = completedFuture("a") } + class SubscriptionFields : GraphQLSubscriptionResolver { + @JvmField val boxEvents: Publisher> = publish { send(Box("a")) } + } + class Holder(val name: String) { @JvmField val tag = "t" } @@ -389,15 +394,16 @@ class ResolverCompatibilityTest { Shape("result", "Foo", "{name=a}") { ResultMap().apply { put("result", it.result) } }, Shape( "plainFuture", "Foo", - "null\nerror: Exception while fetching data (/holder/plainFuture) : Resolver method 'HOLDER.plainFuture' resolved to a FutureTask, " + - "which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead." + "null\nerror: Exception while fetching data (/holder/plainFuture) : Resolver KIND 'HOLDER.plainFuture' resolved to a FutureTask, " + + "which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the KIND or its generic wrapper transformer instead." ) { PlainFutureMap().apply { put("plainFuture", it.plainFuture) } }, Shape("box", "Foo", "{name=a}") { BoxMap().apply { put("box", it.box) } }, Shape("optionalResult", "Foo", "{name=a}") { OptionalResultMap().apply { put("optionalResult", it.optionalResult) } }, Shape("listOfFutures", "[Foo]", "[{name=a}, {name=b}]") { ListOfFuturesMap().apply { put("listOfFutures", it.listOfFutures) } }, Shape("listOfStringFutures", "[String]", "[a, b]") { ListOfStringFuturesMap().apply { put("listOfStringFutures", it.listOfStringFutures) } }, Shape("boxOfString", "String", "a") { BoxOfStringMap().apply { put("boxOfString", it.boxOfString) } }, - Shape("listOfOptionals", "[String]", "rejected: java.util.Optional can only be used at the top level of a return type") { ListOfOptionalsMap().apply { put("listOfOptionals", it.listOfOptionals) } } + Shape("listOfOptionals", "[String]", "rejected: java.util.Optional can only be used at the top level of a return type") { ListOfOptionalsMap().apply { put("listOfOptionals", it.listOfOptionals) } }, + Shape("futureOfAny", "String", "a") { FutureOfAnyMap().apply { put("futureOfAny", it.futureOfAny) } } ) private fun notFoo(path: String, got: String) = @@ -414,12 +420,8 @@ class ResolverCompatibilityTest { "listOfFutures" to "[{name=null}, {name=null}]\n${notFoo("listOfFutures[0]", "CompletableFuture")}\n${notFoo("listOfFutures[1]", "CompletableFuture")}", "listOfStringFutures" to "[CompletableFuture@...[Completed normally], CompletableFuture@...[Completed normally]]", "boxOfString" to "Box@...", - "listOfOptionals" to "[a, null]" + "listOfOptionals" to "[a, null]", + "futureOfAny" to "a" ) - - val knownFindings = listOf(Kind.JVM_FIELD, Kind.TYPED_MAP).flatMap { kind -> - listOf("futureOfListOfFutures", "plainFuture", "box", "optionalResult", "listOfFutures", "listOfStringFutures", "boxOfString") - .map { (kind to it) to rawValueResults.getValue(it) } - }.toMap() } } diff --git a/src/test/resources/returnvalue/Check.java b/src/test/resources/returnvalue/Check.java index d445ef3a..650faf6f 100644 --- a/src/test/resources/returnvalue/Check.java +++ b/src/test/resources/returnvalue/Check.java @@ -101,16 +101,18 @@ enum Leaf { } enum Kind { - METHOD("method", List.of(Nullability.values()), List.of(Timing.values())), - PROPERTY("property", List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)), - MAP("map", List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)); + METHOD("method", "method", List.of(Nullability.values()), List.of(Timing.values())), + PROPERTY("property", "field", List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)), + MAP("map", "map value", List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)); final String label; + final String resolverKind; final List nullabilities; final List timings; - Kind(String label, List nullabilities, List timings) { + Kind(String label, String resolverKind, List nullabilities, List timings) { this.label = label; + this.resolverKind = resolverKind; this.nullabilities = nullabilities; this.timings = timings; } @@ -220,14 +222,14 @@ static final class Built { static final class Context { final Timing timing; - final String method; + final String resolverName; final List completions = new ArrayList<>(); final Set failures = new LinkedHashSet<>(); int nextId = 1; - Context(Timing timing, String method) { + Context(Timing timing, String resolverName) { this.timing = timing; - this.method = method; + this.resolverName = resolverName; } CompletableFuture future(Object value, String failure) { @@ -590,8 +592,9 @@ static Built build(Case testCase, Plan plan, int level, Context context) { } } case FUTURE_TASK: { - context.failures.add("Resolver method '" + context.method + "' resolved to a " + FutureTask.class.getName() + ", which graphql-java can't wait on. " - + "Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead."); + String kind = testCase.kind.resolverKind; + context.failures.add("Resolver " + kind + " '" + context.resolverName + "' resolved to a " + FutureTask.class.getName() + ", which graphql-java can't wait on. " + + "Return a CompletionStage, such as a CompletableFuture, from the " + kind + " or its generic wrapper transformer instead."); FutureTask task = new FutureTask<>(() -> null); task.run(); return new Built(task, null, "FutureTask"); diff --git a/src/test/resources/returnvalue/known-findings.txt b/src/test/resources/returnvalue/known-findings.txt index ca0a5c60..e69de29b 100644 --- a/src/test/resources/returnvalue/known-findings.txt +++ b/src/test/resources/returnvalue/known-findings.txt @@ -1,5 +0,0 @@ -448 ^(property|map) \w+<.*Future<.*String>+$ # properties and Maps don't await a future below the top level, so a String field returns the future's toString with no error -896 ^(property|map) \w+<.*Future<.*(Payload|Object)>+$ # properties and Maps don't await a future below the top level, so an object or union field fails -234 ^(property|map) Future< # properties and Maps don't check a declared Future, so a FutureTask fails with "Expected source object" or returns its toString, not the "can't wait on" error -1254 ^(property|map) .*\b(Page|Later|Try)< # properties and Maps don't run generic wrapper transformers, so the field fails or a String field returns the wrapper's toString -18 ^(property|map) (CompletableFuture<)?Optional Date: Thu, 5 Nov 2026 19:51:50 -0500 Subject: [PATCH 2/4] Describe property and Map unwrapping in the rule table and the bounded check skill The skill's sync commit moves to the unwrapping change, its known findings section says the file is empty, and it says how to catch a return type the build stops accepting now that no count pins them. --- .../verify-return-value-unwrapping/SKILL.md | 20 +++++++++---------- AGENTS.md | 4 ++-- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/.claude/skills/verify-return-value-unwrapping/SKILL.md b/.claude/skills/verify-return-value-unwrapping/SKILL.md index dba6d4b3..ac8f6f36 100644 --- a/.claude/skills/verify-return-value-unwrapping/SKILL.md +++ b/.claude/skills/verify-return-value-unwrapping/SKILL.md @@ -107,8 +107,8 @@ A real bug versus a modeling error: - **Modeling error.** The check's own idea of the value is wrong. For example, a new container variant builds a value that its declared type doesn't allow, or the error message the check expects for a `FutureTask` (in `build`, case `FUTURE_TASK`) differs from a message the library changed on purpose. Confirm against the library's own tests in - `src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt` before you edit the check. Properties and Maps - are expected to fail a `FutureTask` with the method's wording, naming the generated class and `field`. + `src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt` before you edit the check. A property fails a + `FutureTask` as a `Resolver field` and a Map as a `Resolver map value`, each naming the generated class and `field`. - **`MODEL ERROR` about rejected return types.** A change made the build reject a return type in `mustAccept` in `Check.java`. That's either a regression in the build, which you report, or an intended change, after which you swap in another accepted return type of the same kind. @@ -126,12 +126,8 @@ regression in a fixed return type is a `FAIL`. A count that grows makes it `NEW` reject now fails under the line. What stays hidden is a known return type that starts failing in a different way, so after a change to the code a known finding covers, read its counterexample. -At the sync commit, methods have none. Properties and Maps hand their value to graphql-java as it is. graphql-java awaits -only a top-level `CompletionStage`, reads a `DataFetcherResult` at the top, inside that future and as a list element, and -unboxes any `Optional`, but not a `DataFetcherResult` inside one. So every nested future, every generic wrapper, a -`DataFetcherResult` inside an `Optional`, and a `FutureTask` behind a declared `Future` fail for them. The five lines -match exactly those 2,850 return types, and their counts add up to it. A String field gets the container's `toString`, -such as `CompletableFuture@5a7cb161[Completed normally]`, with no error. +At the sync commit the file is empty. Methods, properties and typed Maps unwrap their values through the same +`TypeShape.Unwrapper`, and every return type the build accepts passes. The check found two method bugs when it was written, and both are fixed. One was a `DataFetcherResult` inside an `Optional` (#939). The other was a wrapper whose transformer returns a future, directly around a declared future (#938). A @@ -151,13 +147,15 @@ or delete it when none fail, so that a regression shows up as `FAIL`. and DataLoader futures. - Lists of more than 2 elements, more than 2 non-default choices per value, more than 3 containers, and completion orders other than the two listed. A pass says nothing beyond those bounds. -- Which return types the schema build should reject. The check only exercises what the build accepts. +- Which return types the schema build should reject. The check only exercises what the build accepts. With no known + findings, a return type the build stops accepting fails the check only when it's in `mustAccept`, so after a change + to the build or to a resolver's `createDataFetcher`, diff `--rejected` output against the last commit. - Untyped Maps, whose value type is `Object`, properties with a getter (those run as methods), and properties and Maps on a type below the root other than `Holder`. `ResolverCompatibilityTest` pins untyped Maps and Kotlin `@JvmField`s. ## Coverage and sync -Last confirmed to describe the code at commit `6d41328d` (2026-11-05). It covers: +Last confirmed to describe the code at commit `63c1fa6e` (2026-11-05). It covers: | File | What | |---|---| @@ -170,7 +168,7 @@ Last confirmed to describe the code at commit `6d41328d` (2026-11-05). It covers To see whether a change needs another look: ```bash -git log --oneline 6d41328d..HEAD -- src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt +git log --oneline 63c1fa6e..HEAD -- src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt ``` Move the sync commit forward only after you've read those changes and confirmed that the containers, variants and diff --git a/AGENTS.md b/AGENTS.md index 900d03a3..34dd67e4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -91,9 +91,9 @@ happening. "docs only" means nothing does yet, so the rule depends on you readin | Rule | Enforced by | |---|---| -| A resolver method's return value is unwrapped by one rule that the schema build and the data fetcher share | `TypeShape`, whose one walk yields both the class the schema build matches and the layers the data fetcher unwraps. A container kind can't be added without its placement rule and its value step, since both are abstract on the enum, and `ReturnValueUnwrappingTest` fails until the bounded check builds values for it | +| The value of a resolver method, a public field or a typed Map is unwrapped by one rule that the schema build and the data fetcher share | `TypeShape`, whose one walk yields both the class the schema build matches and the layers that `TypeShape.Unwrapper` unwraps in the method, field and Map data fetchers. An untyped Map's values reach graphql-java as they are. A container kind can't be added without its placement rule and its value step, since both are abstract on the enum, and `ReturnValueUnwrappingTest` fails until the bounded check builds values for it | | A resolver method's parameter layout, type variables and source are decided once, at schema build | docs only | -| A change to how resolvers are found, bound or unwrapped keeps what a method, a property and a Map field return | `ResolverCompatibilityTest` and the bounded check in `ReturnValueUnwrappingTest`. Each lists exactly today's property and Map gaps as known findings, and fails when one is fixed until it's removed, or when a new failure would hide under one | +| A change to how resolvers are found, bound or unwrapped keeps what a method, a property and a Map field return | `ResolverCompatibilityTest`, which compares field and Map values with a method's, and the bounded check in `ReturnValueUnwrappingTest`, which compares all three with the plain value. Neither has known findings, so any value that differs fails the build | | One output has one builder. Don't add a second path that builds the same thing | docs only | | Read an SDL type's members merged with its extensions, never from the raw definition | `MergedType` in `MergedDefinitions.kt` and `DefinitionsBoundaryTest`, which fails on a raw member, directive or child-node read, an extension class, an SDL node walk or a `TypeDefinitionRegistry` call outside it. A read by reflection gets past it | | Every kind of SDL type starts its graphql-java type from the same recipe | `MergedType.prepare`, `DefinitionsBoundaryTest` and `TypeKindRecipeTest` | From 5ce3d1dbf1cbf436c6b531f3ca708293d8c634eb Mon Sep 17 00:00:00 2001 From: Oryan Date: Thu, 5 Nov 2026 20:11:03 -0500 Subject: [PATCH 3/4] Pin the bounded check's coverage and say what to change in a field's FutureTask error With no known findings, a change that made the build reject more return types, generated fewer shapes or ran fewer values still printed PASS. The check now pins the return type count and, per kind, the rejected count and the executions at the default bounds, and a difference is STALE. A field or Map value holding a FutureTask now says to assign a CompletionStage to the field or put one in the map, instead of returning one from it. --- .../tools/resolver/MapFieldResolver.kt | 2 +- .../tools/resolver/MethodFieldResolver.kt | 2 +- .../tools/resolver/PropertyFieldResolver.kt | 2 +- .../kickstart/tools/resolver/TypeShape.kt | 14 +++-- .../tools/NestedGenericWrapperTest.kt | 3 +- .../tools/ResolverCompatibilityTest.kt | 14 ++--- src/test/resources/returnvalue/Check.java | 58 +++++++++++++++---- 7 files changed, 69 insertions(+), 26 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt index f758c831..fa33e15e 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt @@ -46,7 +46,7 @@ internal class MapFieldResolver( private val hasTypedValues get() = resolveTypeVariables(mapGenericValue, search.type) != Any::class.java override fun createDataFetcher(): DataFetcher<*> { - val unwrapper = if (hasTypedValues) TypeShape.Unwrapper(returnShape, "map value", "${search.type.unwrap().name}.${field.name}") else null + val unwrapper = if (hasTypedValues) TypeShape.Unwrapper(returnShape, ResolverKind.MAP_VALUE, "${search.type.unwrap().name}.${field.name}") else null return MapFieldResolverDataFetcher(createSourceResolver(), field.name, unwrapper) } diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index b86d5e0e..24ec4c19 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -114,7 +114,7 @@ internal class MethodFieldResolver( } } - val unwrapper = TypeShape.Unwrapper(returnShape, "method", "${method.declaringClass.name}.${method.name}", isSuspendFunction) + val unwrapper = TypeShape.Unwrapper(returnShape, ResolverKind.METHOD, "${method.declaringClass.name}.${method.name}", isSuspendFunction) return if (numberOfParameters > 0 || isSuspendFunction) { // requires arguments and environment or is a suspend function diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt index ee97d553..112ead6c 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt @@ -25,7 +25,7 @@ internal class PropertyFieldResolver( } override fun createDataFetcher(): DataFetcher<*> { - val unwrapper = TypeShape.Unwrapper(returnShape, "field", "${property.declaringClass.name}.${property.name}") + val unwrapper = TypeShape.Unwrapper(returnShape, ResolverKind.FIELD, "${property.declaringClass.name}.${property.name}") return PropertyFieldResolverDataFetcher(createSourceResolver(), property, unwrapper) } diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt index 7d853a6e..609d7ed4 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt @@ -46,7 +46,7 @@ internal class TypeShape private constructor( fun error(msg: String) = matchError(graphQLType, reference, javaType, msg) - class Unwrapper(shape: TypeShape, resolverKind: String, resolverName: String, isSuspendFunction: Boolean = false) { + class Unwrapper(shape: TypeShape, resolverKind: ResolverKind, resolverName: String, isSuspendFunction: Boolean = false) { private val wrappers = shape.wrappers private val awaitable: Awaitable private val isOutermostFuture: Boolean @@ -391,10 +391,16 @@ private fun List.awaitAll(): Any { private fun Any?.asCompletionStage(): CompletionStage = this as? CompletionStage ?: CompletableFuture.completedFuture(this) +internal enum class ResolverKind(val noun: String, val holdCompletionStage: String) { + METHOD("method", "Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead."), + FIELD("field", "Assign a CompletionStage, such as a CompletableFuture, to the field, or return one from its generic wrapper transformer instead."), + MAP_VALUE("map value", "Put a CompletionStage, such as a CompletableFuture, in the map, or return one from its generic wrapper transformer instead.") +} + // graphql-java only waits on a CompletionStage returned by a data fetcher, so any other future, or a future // that a suspend function completes with, would reach it as the field value private class Awaitable( - private val resolverKind: String, + private val resolverKind: ResolverKind, private val resolverName: String, private val valueClass: Class<*>?, private val isSuspendFunction: Boolean @@ -410,8 +416,8 @@ private class Awaitable( if (isSuspendFunction) { "Suspend function '$resolverName' resolved to a ${value.javaClass.name}, which graphql-java can't wait on. Return its value instead." } else { - "Resolver $resolverKind '$resolverName' resolved to a ${value.javaClass.name}, which graphql-java can't wait on. " + - "Return a CompletionStage, such as a CompletableFuture, from the $resolverKind or its generic wrapper transformer instead." + "Resolver ${resolverKind.noun} '$resolverName' resolved to a ${value.javaClass.name}, which graphql-java can't wait on. " + + resolverKind.holdCompletionStage } ) } diff --git a/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt b/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt index ce343178..213b25c5 100644 --- a/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt @@ -9,6 +9,7 @@ import graphql.execution.DataFetcherResult import graphql.Scalars import graphql.execution.DefaultValueUnboxer import graphql.kickstart.tools.SchemaParserOptions.GenericWrapper +import graphql.kickstart.tools.resolver.ResolverKind import graphql.kickstart.tools.resolver.TypeShape import graphql.kickstart.tools.util.ParameterizedTypeImpl import graphql.kickstart.tools.util.unwrap @@ -250,7 +251,7 @@ class NestedGenericWrapperTest { if (typeShape.leafJavaType != leaf(graphQLType).javaClass) { return@mapNotNull "${TypeUtils.toString(javaType)} as ${AstPrinter.printAst(graphQLType)}: bound to ${TypeUtils.toString(typeShape.leafJavaType)}" } - val unwrapper = TypeShape.Unwrapper(typeShape, "method", method.name) + val unwrapper = TypeShape.Unwrapper(typeShape, ResolverKind.METHOD, method.name) val value = value(shape, leaf(graphQLType)) val result = GraphQL.newGraphQL(fieldSchema(graphQLType) { environment -> unwrapper.unwrap(value) { environment } }).build() .execute(if (leaf(graphQLType) is Payload) "query { field { data } }" else "query { field }") diff --git a/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt b/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt index a957dacd..ddf81e21 100644 --- a/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/ResolverCompatibilityTest.kt @@ -271,11 +271,11 @@ class ResolverCompatibilityTest { .replace("${ResolverCompatibilityTest::class.java.name}$", "") .replace("java.util.concurrent.", "") - enum class Kind(val resolverKind: String, val holder: (Shape) -> Any) { - METHOD("method", { MethodHolder() }), - JVM_FIELD("field", { JvmFieldHolder() }), - TYPED_MAP("map value", { shape -> shape.typedMap(JvmFieldHolder()) }), - UNTYPED_MAP("map value", { shape -> hashMapOf(shape.name to shape.value()) }) + enum class Kind(val resolverKind: String, val holdCompletionStage: String, val holder: (Shape) -> Any) { + METHOD("method", "Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead.", { MethodHolder() }), + JVM_FIELD("field", "Assign a CompletionStage, such as a CompletableFuture, to the field, or return one from its generic wrapper transformer instead.", { JvmFieldHolder() }), + TYPED_MAP("map value", "Put a CompletionStage, such as a CompletableFuture, in the map, or return one from its generic wrapper transformer instead.", { shape -> shape.typedMap(JvmFieldHolder()) }), + UNTYPED_MAP("map value", "Put a CompletionStage, such as a CompletableFuture, in the map, or return one from its generic wrapper transformer instead.", { shape -> hashMapOf(shape.name to shape.value()) }) } class Shape(val name: String, val type: String, private val expected: String, val typedMap: (JvmFieldHolder) -> Map) { @@ -283,7 +283,7 @@ class ResolverCompatibilityTest { fun value(): Any? = JvmFieldHolder::class.java.getField(name).get(JvmFieldHolder()) - fun expected(kind: Kind) = expected.replace("HOLDER", kind.holder(this).javaClass.simpleName).replace("KIND", kind.resolverKind) + fun expected(kind: Kind) = expected.replace("HOLDER", kind.holder(this).javaClass.simpleName).replace("KIND", kind.resolverKind).replace("HOLD_COMPLETION_STAGE", kind.holdCompletionStage) } class Foo(val name: String) @@ -395,7 +395,7 @@ class ResolverCompatibilityTest { Shape( "plainFuture", "Foo", "null\nerror: Exception while fetching data (/holder/plainFuture) : Resolver KIND 'HOLDER.plainFuture' resolved to a FutureTask, " + - "which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the KIND or its generic wrapper transformer instead." + "which graphql-java can't wait on. HOLD_COMPLETION_STAGE" ) { PlainFutureMap().apply { put("plainFuture", it.plainFuture) } }, Shape("box", "Foo", "{name=a}") { BoxMap().apply { put("box", it.box) } }, Shape("optionalResult", "Foo", "{name=a}") { OptionalResultMap().apply { put("optionalResult", it.optionalResult) } }, diff --git a/src/test/resources/returnvalue/Check.java b/src/test/resources/returnvalue/Check.java index 650faf6f..815dce54 100644 --- a/src/test/resources/returnvalue/Check.java +++ b/src/test/resources/returnvalue/Check.java @@ -37,6 +37,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.EnumMap; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -101,18 +102,23 @@ enum Leaf { } enum Kind { - METHOD("method", "method", List.of(Nullability.values()), List.of(Timing.values())), - PROPERTY("property", "field", List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)), - MAP("map", "map value", List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)); + METHOD("method", "method", "Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead.", + List.of(Nullability.values()), List.of(Timing.values())), + PROPERTY("property", "field", "Assign a CompletionStage, such as a CompletableFuture, to the field, or return one from its generic wrapper transformer instead.", + List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)), + MAP("map", "map value", "Put a CompletionStage, such as a CompletableFuture, in the map, or return one from its generic wrapper transformer instead.", + List.of(Nullability.NULLABLE), List.of(Timing.DONE, Timing.PENDING_IN_ORDER)); final String label; final String resolverKind; + final String holdCompletionStage; final List nullabilities; final List timings; - Kind(String label, String resolverKind, List nullabilities, List timings) { + Kind(String label, String resolverKind, String holdCompletionStage, List nullabilities, List timings) { this.label = label; this.resolverKind = resolverKind; + this.holdCompletionStage = holdCompletionStage; this.nullabilities = nullabilities; this.timings = timings; } @@ -307,6 +313,13 @@ static final class KnownFinding { "java.util.List>> [Item!]!" ); + private static final List pinnedCoverage = List.of( + "7020 return types", + "method: 861 rejected, 171693 executions", + "property: 252 rejected, 27292 executions", + "map: 252 rejected, 27292 executions" + ); + public static boolean check(String[] args, String classpath, PrintStream out) throws Exception { int depth = intArg(args, "--depth", DEPTH); int budget = intArg(args, "--budget", BUDGET); @@ -328,8 +341,8 @@ public static boolean check(String[] args, String classpath, PrintStream out) th List cases = cases(depth); Map> resolvers = compile(cases, classpath); - int rejected = 0; - int runs = 0; + Map rejectedByKind = new EnumMap<>(Kind.class); + Map runsByKind = new EnumMap<>(Kind.class); boolean blocked = false; Set accepted = new LinkedHashSet<>(); Set acceptedReturnTypes = new LinkedHashSet<>(); @@ -341,7 +354,7 @@ public static boolean check(String[] args, String classpath, PrintStream out) th try { schema = schema(testCase, instance); } catch (RuntimeException e) { - rejected++; + rejectedByKind.merge(testCase.kind, 1, Integer::sum); if (flag(args, "--rejected")) { out.println("rejected " + testCase.describe() + ": " + e.getMessage().lines().findFirst().orElse("")); } @@ -356,7 +369,7 @@ public static boolean check(String[] args, String classpath, PrintStream out) th plans: for (Plan plan : plansWithinBudget(testCase, 0, testCase.kind == Kind.METHOD ? budget : propertyBudget)) { for (Timing timing : timings) { - runs++; + runsByKind.merge(testCase.kind, 1, Integer::sum); Violation violation = run(testCase, plan, timing, actual, oracle, returning); if (violation != null) { violations.computeIfAbsent(testCase, c -> new ArrayList<>()).add(violation); @@ -375,6 +388,8 @@ public static boolean check(String[] args, String classpath, PrintStream out) th .map(it -> kind.label + " " + it)) .filter(it -> !accepted.contains(it)) .collect(Collectors.toList()); + int rejected = rejectedByKind.values().stream().mapToInt(Integer::intValue).sum(); + int runs = runsByKind.values().stream().mapToInt(Integer::intValue).sum(); out.printf("depth %d, budget %d (%d for properties and Maps): %d return types, %d rejected by the schema build, %d accepted, %d executions, %.1fs%n", depth, budget, propertyBudget, cases.size(), rejected, accepted.size(), runs, (System.nanoTime() - started) / 1e9); if (blocked) { @@ -415,6 +430,11 @@ public static boolean check(String[] args, String classpath, PrintStream out) th passed = false; } if (depth == DEPTH && budget == BUDGET && propertyBudget == PROPERTY_BUDGET) { + List coverage = new ArrayList<>(List.of(cases.size() + " return types")); + for (Kind kind : Kind.values()) { + coverage.add(kind.label + ": " + rejectedByKind.getOrDefault(kind, 0) + " rejected, " + runsByKind.getOrDefault(kind, 0) + " executions"); + } + passed &= coverageIsPinned(coverage, out); passed &= knownFindingsAreExact(known, failing.keySet(), show, out); } if (passed) { @@ -423,6 +443,23 @@ public static boolean check(String[] args, String classpath, PrintStream out) th return passed; } + private static boolean coverageIsPinned(List coverage, PrintStream out) { + if (coverage.equals(pinnedCoverage)) { + return true; + } + out.println("\nSTALE: this run's coverage differs from pinnedCoverage in Check.java. More rejected return types or fewer executions " + + "means the check stopped exercising something, which no other line of this report fails on:"); + for (int i = 0; i < coverage.size(); i++) { + if (!coverage.get(i).equals(pinnedCoverage.get(i))) { + out.println(" pinned " + pinnedCoverage.get(i) + ", got " + coverage.get(i)); + } + } + out.println("Diff --rejected output against the last commit to see which return types the build now rejects or accepts, " + + "and confirm the change is intended. Then set pinnedCoverage to these counts in the same diff:"); + coverage.forEach(it -> out.println(" \"" + it + "\"")); + return false; + } + private static boolean knownFindingsAreExact(List known, Set failing, int show, PrintStream out) { List stale = new ArrayList<>(); List added = new ArrayList<>(); @@ -592,9 +629,8 @@ static Built build(Case testCase, Plan plan, int level, Context context) { } } case FUTURE_TASK: { - String kind = testCase.kind.resolverKind; - context.failures.add("Resolver " + kind + " '" + context.resolverName + "' resolved to a " + FutureTask.class.getName() + ", which graphql-java can't wait on. " - + "Return a CompletionStage, such as a CompletableFuture, from the " + kind + " or its generic wrapper transformer instead."); + context.failures.add("Resolver " + testCase.kind.resolverKind + " '" + context.resolverName + "' resolved to a " + FutureTask.class.getName() + + ", which graphql-java can't wait on. " + testCase.kind.holdCompletionStage); FutureTask task = new FutureTask<>(() -> null); task.run(); return new Built(task, null, "FutureTask"); From db5afdc1300f1542414f0e328b7f7220dbed2d6d Mon Sep 17 00:00:00 2001 From: Oryan Date: Thu, 5 Nov 2026 20:11:16 -0500 Subject: [PATCH 4/4] Describe the pinned coverage and every field without a getter in the rule table and the bounded check skill --- .../verify-return-value-unwrapping/SKILL.md | 36 +++++++++++++------ AGENTS.md | 4 +-- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/.claude/skills/verify-return-value-unwrapping/SKILL.md b/.claude/skills/verify-return-value-unwrapping/SKILL.md index ac8f6f36..f0a32b40 100644 --- a/.claude/skills/verify-return-value-unwrapping/SKILL.md +++ b/.claude/skills/verify-return-value-unwrapping/SKILL.md @@ -1,13 +1,13 @@ --- name: verify-return-value-unwrapping -description: Exhaustively check, up to a bound, that the value of a resolver method, public field or typed Map reaches graphql-java fully unwrapped. Futures are awaited, generic wrappers are transformed, Optional and DataFetcherResult are handled, and list order, nulls and failures are kept, for every return type nested up to 3 containers deep. Use it before pushing any change to TypeShape (the type walk, ContainerKind, the generic wrapper rules or the Unwrapper), the data fetchers in MethodFieldResolver, PropertyFieldResolver or MapFieldResolver, or the GenericWrapper defaults, and whenever a bug report says graphql-java got a CompletableFuture, Optional or wrapper object as a field value. +description: Exhaustively check, up to a bound, that the value of a resolver method, a field with no matching getter or a typed Map reaches graphql-java fully unwrapped. Futures are awaited, generic wrappers are transformed, Optional and DataFetcherResult are handled, and list order, nulls and failures are kept, for every return type nested up to 3 containers deep. Use it before pushing any change to TypeShape (the type walk, ContainerKind, the generic wrapper rules or the Unwrapper), the data fetchers in MethodFieldResolver, PropertyFieldResolver or MapFieldResolver, or the GenericWrapper defaults, and whenever a bug report says graphql-java got a CompletableFuture, Optional or wrapper object as a field value. --- # Verify return value unwrapping ## The property -A user writes a resolver method, a public field or a `Map` whose type nests containers, such as +A user writes a resolver method, a field with no matching getter (of any visibility) or a `Map` whose type nests containers, such as `CompletableFuture>>`. If the schema build accepts it, then graphql-java must get exactly the value it returned, with every container removed. Futures are waited on and generic wrappers go through their transformers. List order and nulls are kept. A failed future or a throwing transformer fails the field. @@ -74,12 +74,26 @@ as `-Dunwrapping.args`, and prints the report. On a failure it also prints the p | 0 | `PASS`, or `PASS apart from the known findings above` | | 1 | `FAIL`: a return type that isn't a known finding breaks the property. `NEW`: a known finding matches more accepted return types than its line records | | 2 | `MODEL ERROR`: the checkout or the check didn't compile, the build rejected a return type the check must exercise, or a `ContainerKind` in `TypeShape.kt` has no `Container` in `Check.java`, so a pass would prove nothing | -| 3 | `STALE`: a known finding matches a return type that no longer fails, or fewer accepted return types than its line records | +| 3 | `STALE`: a known finding matches a return type that no longer fails, or fewer accepted return types than its line records. Or the run's coverage differs from `pinnedCoverage` in `Check.java` | Options: `--depth N`, `--budget N`, `--property-budget N`, `--show N` (counterexamples and stale return types printed, default 15), and `--rejected`, which lists every combination the schema build rejected, with the build's error. A depth below 3 can't build the `mustAccept` return types, so it ends in `MODEL ERROR`. `NEW` and `STALE` apply only at the default depth and budgets, because the -counts in `known-findings.txt` are for those. +counts in `known-findings.txt` and `pinnedCoverage` are for those. A run with other bounds checks values only. + +## Pinned coverage + +`pinnedCoverage` in `Check.java` records how many return types the check generates, and for each kind how many of them +the schema build rejects and how many executions run. A run at the default bounds whose counts differ is `STALE`, and the +report prints the pinned and the new counts. Without the pin, a change that makes the build reject more return types, +skips generating some shapes, or runs fewer values per return type would still print `PASS`, because nothing it stopped +running can fail. + +When the counts move, diff `--rejected` output against the last commit to see which return types the build now rejects +or accepts, and confirm the change is intended. A change to `Check.java` itself moves the counts with no change in +`--rejected`. Then copy the counts the report prints into `pinnedCoverage` in the same diff, so a reviewer sees what the +check stopped or started covering. A change that rejects one return type and accepts another of the same kind, with the +same number of executions, keeps the counts and gets past the pin. ## Reading a counterexample @@ -147,15 +161,15 @@ or delete it when none fail, so that a regression shows up as `FAIL`. and DataLoader futures. - Lists of more than 2 elements, more than 2 non-default choices per value, more than 3 containers, and completion orders other than the two listed. A pass says nothing beyond those bounds. -- Which return types the schema build should reject. The check only exercises what the build accepts. With no known - findings, a return type the build stops accepting fails the check only when it's in `mustAccept`, so after a change - to the build or to a resolver's `createDataFetcher`, diff `--rejected` output against the last commit. -- Untyped Maps, whose value type is `Object`, properties with a getter (those run as methods), and properties and Maps on - a type below the root other than `Holder`. `ResolverCompatibilityTest` pins untyped Maps and Kotlin `@JvmField`s. +- Which return types the schema build should reject. The check only exercises what the build accepts. A change in how + many it rejects fails through `pinnedCoverage`, but the check doesn't judge whether the new count is right. +- Untyped Maps, whose value type is `Object`, properties with a getter (those run as methods), non-public fields (the + generated ones are public, and the same data fetcher reads private ones), and properties and Maps on a type below the + root other than `Holder`. `ResolverCompatibilityTest` pins untyped Maps and Kotlin `@JvmField`s. ## Coverage and sync -Last confirmed to describe the code at commit `63c1fa6e` (2026-11-05). It covers: +Last confirmed to describe the code at commit `5ce3d1db` (2026-11-05). It covers: | File | What | |---|---| @@ -168,7 +182,7 @@ Last confirmed to describe the code at commit `63c1fa6e` (2026-11-05). It covers To see whether a change needs another look: ```bash -git log --oneline 63c1fa6e..HEAD -- src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt +git log --oneline 5ce3d1db..HEAD -- src/main/kotlin/graphql/kickstart/tools/resolver/TypeShape.kt src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/SchemaParserOptions.kt src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt ``` Move the sync commit forward only after you've read those changes and confirmed that the containers, variants and diff --git a/AGENTS.md b/AGENTS.md index 34dd67e4..508baf22 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -91,9 +91,9 @@ happening. "docs only" means nothing does yet, so the rule depends on you readin | Rule | Enforced by | |---|---| -| The value of a resolver method, a public field or a typed Map is unwrapped by one rule that the schema build and the data fetcher share | `TypeShape`, whose one walk yields both the class the schema build matches and the layers that `TypeShape.Unwrapper` unwraps in the method, field and Map data fetchers. An untyped Map's values reach graphql-java as they are. A container kind can't be added without its placement rule and its value step, since both are abstract on the enum, and `ReturnValueUnwrappingTest` fails until the bounded check builds values for it | +| The value of a resolver method, a field with no matching getter or a typed Map is unwrapped by one rule that the schema build and the data fetcher share | `TypeShape`, whose one walk yields both the class the schema build matches and the layers that `TypeShape.Unwrapper` unwraps in the method, field and Map data fetchers. An untyped Map's values reach graphql-java as they are. A container kind can't be added without its placement rule and its value step, since both are abstract on the enum, and `ReturnValueUnwrappingTest` fails until the bounded check builds values for it | | A resolver method's parameter layout, type variables and source are decided once, at schema build | docs only | -| A change to how resolvers are found, bound or unwrapped keeps what a method, a property and a Map field return | `ResolverCompatibilityTest`, which compares field and Map values with a method's, and the bounded check in `ReturnValueUnwrappingTest`, which compares all three with the plain value. Neither has known findings, so any value that differs fails the build | +| A change to how resolvers are found, bound or unwrapped keeps what a method, a property and a Map field return | `ResolverCompatibilityTest`, which compares field and Map values with a method's, and the bounded check in `ReturnValueUnwrappingTest`, which compares all three with the plain value. Neither has known findings, so a value that differs on a return type they run fails the build. The bounded check also pins how many return types it generates, and for each kind how many the build rejects and how many executions run, so a change that stops running some fails until the same diff updates `pinnedCoverage`. It doesn't judge whether a newly rejected return type should build | | One output has one builder. Don't add a second path that builds the same thing | docs only | | Read an SDL type's members merged with its extensions, never from the raw definition | `MergedType` in `MergedDefinitions.kt` and `DefinitionsBoundaryTest`, which fails on a raw member, directive or child-node read, an extension class, an SDL node walk or a `TypeDefinitionRegistry` call outside it. A read by reflection gets past it | | Every kind of SDL type starts its graphql-java type from the same recipe | `MergedType.prepare`, `DefinitionsBoundaryTest` and `TypeKindRecipeTest` |