Unwrap property and Map field values like method values

.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<List<Try<Payload>>>`. 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

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` |

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<TypeShape> {
-        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<TypeShape> = 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<Any> {
 
     override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any?, environmentSupplier: Supplier<DataFetchingEnvironment>): 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")
         }

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

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<TypeShape> {
-        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<TypeShape> = 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<Any> {
 
     override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any?, environmentSupplier: Supplier<DataFetchingEnvironment>): 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? {