Skip to content

Unwrap property and Map field values like method values - #953

Open
oryan-block wants to merge 4 commits into
agent-friendlyfrom
agent-friendly-p11-property-map-unwrap
Open

oryan-block wants to merge 4 commits into
agent-friendlyfrom
agent-friendly-p11-property-map-unwrap

Conversation

@oryan-block

@oryan-block oryan-block commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

Why

Agents working in graphql-java-tools keep repeating the same mistakes, and the repo should change so a change that looks right from one file is right for the whole repo. After #952 the schema build matched fields and typed Map values through TypeShape, but their data fetchers still handed the raw value to graphql-java. So the same type unwrapped on a method and not on a field. E.g. a @JvmField val strings: List<CompletableFuture<String>> returned ["java.util.concurrent.CompletableFuture@3c3820bb[Completed normally]", ...] with no error, and a List<CompletableFuture<Foo>> field failed every element with Expected source object to be an instance of '...Foo' but instead got 'java.util.concurrent.CompletableFuture'.

Checklist

  • Pull requests follows the contribution guide
  • New or modified functionality is covered by tests

Description

PropertyFieldResolver and MapFieldResolver now pass their value through the same TypeShape.Unwrapper as MethodFieldResolver. So a field with no matching getter or a typed Map value gets nested futures awaited, generic wrappers transformed and a DataFetcherResult inside an Optional read, same as a method returning that type. "Field" here means any field the scanner reads directly, whatever its visibility: public fields, Kotlin @JvmFields, and also private Java fields and Kotlin private vals that have no getter (FieldResolverScanner reads them with getAllFields + setAccessible).

  • untyped Maps still hand their values to graphql-java as they are. "Typed" means a Map subclass that fixes the value type, e.g. class FooMap : HashMap<String, Box<Foo>>(). A resolver method declared to return Map<String, Box<Foo>> is still untyped. Unwrapping untyped values would have to go by each value's class, which nobody asked for
  • a FutureTask in a field or Map value declared Future now fails with the "can't wait on" error instead of Expected source object .... The error starts with Resolver field '<class>.<field>' or Resolver map value '<map class>.<key>' and says to assign a CompletionStage to the field or put one in the map. The method message is unchanged
  • fields and Map values pass the subscription flag to TypeShape like methods do. Otherwise the steps below a subscription field's Publisher would run on the Publisher itself, and a @JvmField val boxEvents: Publisher<Box<String>> subscription failed instead of sending its events
  • TypeShape keeps the declared type with its type variables resolved. A typed Map's value type comes from Classmate as a ResolvedType, which TypeUtils.getRawType throws on, so without this a HashMap<String, CompletableFuture<Any>> value stops building (unknown type)
  • known-findings.txt is empty and ResolverCompatibilityTest has no knownFindings left. It gets a CompletableFuture<Any> shape for the case above and a subscription field case
  • with no known findings left, nothing pinned what the bounded check covers. A change that made the build reject CompletableFuture<Optional<...>> properties, stopped generating Map shapes whose outermost container is Future, or ran Map values with completed futures only still printed PASS. So Check.java now has a pinnedCoverage next to mustAccept: the return type count and, per kind, how many the build rejects and how many executions run. At the default bounds a different count is STALE (exit 3), printing the pinned and new counts and asking you to diff --rejected and update the pin in the same diff. Other bounds (e.g. --budget 1) skip it, same as the known findings counts
  • the verify-return-value-unwrapping skill describes the pin instead of the manual "diff --rejected" step, its sync commit moves to this change, and the two AGENTS.md rule table rows about unwrapping now cover every field without a getter and say what the pin does and doesn't catch

Release note for the next minor:

Fields read without a getter (public fields, Kotlin @JvmField properties, and private fields or Kotlin private vals with no getter) and values of typed Map subclasses are now unwrapped like resolver method return values. Futures nested in lists or wrappers are awaited, generic wrapper transformers run and a DataFetcherResult inside an Optional is read. Before, a String field returned the future's or wrapper's toString() with no error, and an object field failed with "Expected source object to be an instance of ...".

  • A "typed" Map is a Map subclass that fixes the value type, e.g. class FooMap : HashMap<String, Box<Foo>>(). A resolver method declared to return Map<String, Box<Foo>> is still untyped, and Maps whose value type is Object are unchanged.
  • A field declared Future holding a future that isn't a CompletionStage now fails with an error saying graphql-java can't wait on it.
  • A field whose value is an instance of a registered generic wrapper type now goes through that wrapper's transformer.
  • A Subscription field backed by a Kotlin ReceiveChannel now goes through the default transformer to a Publisher, and a Flux<Foo> Subscription field now builds.
  • ⚠️ The one way existing code could break: a custom scalar whose Coercing unwrapped a registered wrapper itself, because the field handed it the wrapper. It now gets the inner value.

Testing:

  • mise exec java@temurin-17.0.21+9 -- mvn -B -ntp clean verify passes at this head, 480 tests, japicmp prints No changes. against 15.0.0. mvn -B -ntp clean verify on JDK 25 passes too
  • the bounded check prints PASS with no known findings: 7,020 return types, 1,365 rejected, 5,655 accepted, 226,277 executions, the same counts as agent-friendly. --budget 1 prints PASS with 125,766 executions, so the pin stays out of the way off the default bounds
  • broke things on purpose, one at a time, and ran check.sh. Each exits 3 with STALE naming the kind that moved:
    • TypeShape without the declared type resolve: pinned map: 252 rejected, 27292 executions, got map: 336 rejected, 25897 executions (ResolverCompatibilityTest also fails on the CompletableFuture<Any> Map)
    • PropertyFieldResolver rejecting CompletableFuture<Optional<...>>: pinned property: 252 rejected, 27292 executions, got property: 276 rejected, 26936 executions
    • Check.java skipping Map shapes that start with Future: pinned 7020 return types, got 6801 return types + the map line
    • Check.java running Map values with completed futures only: pinned map: 252 rejected, 27292 executions, got map: 252 rejected, 15610 executions
  • passing isSubscription = false for fields fails the subscription test
  • verify-tools probe with a one-off scenario (not committed) at a7f6457a and at agent-friendly. Head: @JvmField List<CompletableFuture<String>> -> ["a", "b"], List<CompletableFuture<Foo>> -> both names, Box<String> -> "e", typed Map Box<Foo> -> {"name": "g"}, FutureTask -> the "can't wait on" error naming the field, untyped Map Box -> "...PropertyMapBox@36aa52d2". agent-friendly: the futures' toString(), 3 Expected source object errors and the box's toString()

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.
…d 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.
…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.
@sonarqubecloud

sonarqubecloud Bot commented Nov 6, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant