Repository navigation
Resolve type variables in resolver return types - #883
Merged
Merged
Conversation
|
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.



Checklist
Description
Same problem as #880, but for return types.
MethodFieldResolverused the return type of a method inherited from a generic class without resolving the class's type variables, so it sawTwhere the scanner sawFuture<Any>orEventStream. Two cases broke because of it:CompletionStagefuture behind a type variable got handed to graphql-java as the value, with no error. I found this while reviewing Resolve type variables in resolver parameter types #882.{ name }withname: Stringreturned"java.util.concurrent.FutureTask@4554de02[Completed normally]". As a union it failed in the type resolver instead. The check that rejects these futures only treats the value as a wrapper when the method's return type isFuture, and it readT. Now it fails with the same error as a method declared to returnFuture<Any>:Publishersubtype failed the build:(or "No method or field found" depending on the schema).
asPublisher, which turnsEventStreamintoPublisher<Event>, ran before the type variables were resolved, so it gotPand did nothing. Now it builds and streams the event.The fix:
getReturnType()is now a lazyreturnTypethat resolves the type variables withGenericTypeResolver#resolveTypeVariables, same asgetMethodParameterTypesince #882.getResolvedReturnType(incl.asPublisher) and theFuturecheck both start from it. It's lazy becauseFieldResolverScanner#findFieldResolverbuilds a resolver per search and may only use the first one.This is the first of a few PRs that put the scanner and the data fetchers on one code path for a resolver's Java types. Nothing that builds and works today should change. No public API change, only private members of the internal
MethodFieldResolver.Release note: Resolver methods inherited from a generic class now resolve the class's type variables in their return type too. A non-
CompletionStageFuturebehind a type variable now gets the same error as a declared one instead of returning itstoString(), and subscription methods returning aPublishersubtype through a type variable now build.Testing
Folded both cases into existing tests:
inheritedSearchinReactiveTestandonInheritedPublisherinSchemaParserTest's publisher subtypes test. Both failed before the change.mvn clean verifypasses on JDK 25, on JDK 25 against graphql-java 25.1, and on JDK 11.Also checked through the public API with the classes above, on graphql-java 26.1 and 25.1, before and after. Before:
namewas theFutureTaskstring and the subscription schema didn't build. After: the error above, and{"onEvent":{"name":"inherited"}}. ACompletableFuturebehind a type variable resolves the same in both.Schema build benchmark (JMH, graphql-java 25.1) didn't get slower: large schema 37.5 ± 1.0 ms vs 39.3 ± 1.1 on master, same allocation per op.