Repository navigation
Unwrap property and Map field values like method values - #953
Open
oryan-block wants to merge 4 commits into
Open
oryan-block wants to merge 4 commits into
oryan-block wants to merge 4 commits into
Conversation
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.
…rule table and the bounded check skill
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



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 aList<CompletableFuture<Foo>>field failed every element withExpected source object to be an instance of '...Foo' but instead got 'java.util.concurrent.CompletableFuture'.Checklist
Description
PropertyFieldResolverandMapFieldResolvernow pass their value through the sameTypeShape.UnwrapperasMethodFieldResolver. So a field with no matching getter or a typed Map value gets nested futures awaited, generic wrappers transformed and aDataFetcherResultinside anOptionalread, 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 Kotlinprivate vals that have no getter (FieldResolverScannerreads them withgetAllFields+setAccessible).Mapsubclass that fixes the value type, e.g.class FooMap : HashMap<String, Box<Foo>>(). A resolver method declared to returnMap<String, Box<Foo>>is still untyped. Unwrapping untyped values would have to go by each value's class, which nobody asked forFutureTaskin a field or Map value declaredFuturenow fails with the "can't wait on" error instead ofExpected source object .... The error starts withResolver field '<class>.<field>'orResolver map value '<map class>.<key>'and says to assign aCompletionStageto the field or put one in the map. The method message is unchangedTypeShapelike methods do. Otherwise the steps below a subscription field'sPublisherwould run on thePublisheritself, and a@JvmField val boxEvents: Publisher<Box<String>>subscription failed instead of sending its eventsTypeShapekeeps the declared type with its type variables resolved. A typed Map's value type comes from Classmate as aResolvedType, whichTypeUtils.getRawTypethrows on, so without this aHashMap<String, CompletableFuture<Any>>value stops building (unknown type)known-findings.txtis empty andResolverCompatibilityTesthas noknownFindingsleft. It gets aCompletableFuture<Any>shape for the case above and a subscription field caseCompletableFuture<Optional<...>>properties, stopped generating Map shapes whose outermost container isFuture, or ran Map values with completed futures only still printedPASS. SoCheck.javanow has apinnedCoveragenext tomustAccept: the return type count and, per kind, how many the build rejects and how many executions run. At the default bounds a different count isSTALE(exit 3), printing the pinned and new counts and asking you to diff--rejectedand update the pin in the same diff. Other bounds (e.g.--budget 1) skip it, same as the known findings countsverify-return-value-unwrappingskill describes the pin instead of the manual "diff--rejected" step, its sync commit moves to this change, and the twoAGENTS.mdrule table rows about unwrapping now cover every field without a getter and say what the pin does and doesn't catchRelease note for the next minor:
Testing:
mise exec java@temurin-17.0.21+9 -- mvn -B -ntp clean verifypasses at this head, 480 tests, japicmp printsNo changes.against 15.0.0.mvn -B -ntp clean verifyon JDK 25 passes tooPASSwith no known findings: 7,020 return types, 1,365 rejected, 5,655 accepted, 226,277 executions, the same counts asagent-friendly.--budget 1printsPASSwith 125,766 executions, so the pin stays out of the way off the default boundscheck.sh. Each exits 3 withSTALEnaming the kind that moved:TypeShapewithout the declared type resolve:pinned map: 252 rejected, 27292 executions, got map: 336 rejected, 25897 executions(ResolverCompatibilityTestalso fails on theCompletableFuture<Any>Map)PropertyFieldResolverrejectingCompletableFuture<Optional<...>>:pinned property: 252 rejected, 27292 executions, got property: 276 rejected, 26936 executionsCheck.javaskipping Map shapes that start withFuture:pinned 7020 return types, got 6801 return types+ the map lineCheck.javarunning Map values with completed futures only:pinned map: 252 rejected, 27292 executions, got map: 252 rejected, 15610 executionsisSubscription = falsefor fields fails the subscription testa7f6457aand atagent-friendly. Head:@JvmFieldList<CompletableFuture<String>>->["a", "b"],List<CompletableFuture<Foo>>-> both names,Box<String>->"e", typed MapBox<Foo>->{"name": "g"},FutureTask-> the "can't wait on" error naming the field, untyped MapBox->"...PropertyMapBox@36aa52d2".agent-friendly: the futures'toString(), 3Expected source objecterrors and the box'stoString()