diff --git a/.claude/skills/verify-return-value-unwrapping/SKILL.md b/.claude/skills/verify-return-value-unwrapping/SKILL.md index dba6d4b3..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 @@ -107,8 +121,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 +140,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 +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. -- 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 `6d41328d` (2026-11-05). It covers: +Last confirmed to describe the code at commit `5ce3d1db` (2026-11-05). It covers: | File | What | |---|---| @@ -170,7 +182,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 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 900d03a3..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 | |---|---| -| 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 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` 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 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` | diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt index c13f67f4..fa33e15e 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, ResolverKind.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..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, 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 7f93c648..112ead6c 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, ResolverKind.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..609d7ed4 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: ResolverKind, 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( @@ -389,9 +391,20 @@ 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 method: Method, private val valueClass: Class<*>?, private val isSuspendFunction: Boolean) { +private class Awaitable( + private val resolverKind: ResolverKind, + 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 +414,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.noun} '$resolverName' resolved to a ${value.javaClass.name}, which graphql-java can't wait on. " + + resolverKind.holdCompletionStage } ) } - -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..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, isSuspendFunction = false) + 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 40b0d1d4..ddf81e21 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 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) { @@ -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).replace("HOLD_COMPLETION_STAGE", kind.holdCompletionStage) } 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. 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) } }, 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..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,16 +102,23 @@ 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", "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, 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; } @@ -220,14 +228,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) { @@ -305,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); @@ -326,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<>(); @@ -339,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("")); } @@ -354,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); @@ -373,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) { @@ -413,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) { @@ -421,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<>(); @@ -590,8 +629,8 @@ 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."); + 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"); 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