The schema build matches a return type with its generic wrappers unwrapped at
every level, so List<Try<Payload>> builds against [Payload!]!, but the data
fetcher only unwrapped the outermost value. Every query then failed, or a
[String] field returned the wrapper's toString().
The data fetcher now unwraps the value at the positions where the build
unwrapped its type. Lists are mapped, and futures below the outermost value
are combined into one, so one failed element fails the whole field. Fields
with no nested wrapper, subscriptions and custom scalars keep today's
behaviour.
Fixes #254
Fixes #254
Checklist
Description
The schema build looks through generic wrappers at every level of a method's return type, but the data fetcher only unwrapped the outermost value. So with
GenericWrapper.withTransformer(Try::class, 0, { it.get() }):Same for
List<CompletableFuture<Item>>(graphql-java only waits on a future the data fetcher itself returns) andCompletableFuture<CompletableFuture<Item>>. On a[String]field,List<Try<String>>andList<CompletableFuture<String>>returned the wrapper'stoString()instead of failing.Now the data fetcher unwraps the value at the positions where the build unwrapped its type. The new
ReturnValueUnwrappergets them from the sameGenericTypeResolver.resolve()the matcher calls (through a newonUnwrapcallback), plus a copy of howTypeClassMatcherwalks the GraphQL type (DataFetcherResult,Optional, non-null, list elements). Lists are mapped and the futures below the outermost value are combined withallOf, so graphql-java gets one future. Nested wrappers are picked by the value's class with the same code as the outermost one, which moved fromMethodFieldResolverinto the new file as is.One failed element fails the whole field. A failed future or a transformer that throws anywhere in the list gives one error at the field's path and a null field, not a null element.
What stays the same:
[JSON]field returningList<Try<Map>>still hands theTryto the scalar, which also means[DateTime]fromList<CompletableFuture<OffsetDateTime>>is still brokenFutureTask) is passed through instead of rejected, so an app whoseValueUnboxercallsget()on it keeps workingTry<CompletableFuture<Item>>still returns the resolver's own futureApps that already unwrap these with their own
ValueUnboxerget the same data, their unboxer just sees unwrapped values now. Three cases that worked can change, all where the registered transformer disagrees with what handled the value before:Tryor a failed future into null while the transformer throws or the future fails. The element was null, now the whole field failsOptionalregistered as a generic wrapper with a transformer that throws when empty, used in a list. graphql-java used to turn the empty one into nullIterableclass registered withlistCollectionWithTransformer, nested in a list. graphql-java used to iterate it, now the transformer's list is used. Only different if the two disagreeDataLoaders: the unwrapper never calls
load()itself, it only combines the futures it gets. But with graphql-java's default dispatch, aload()made after the field's level was dispatched is never dispatched, so the field hangs. That happens when the load waits on an async step that's still running at dispatch time, and it's already the case today for e.g.supplyAsync { slowCall() }.thenCompose { loader.load(it) }. It's timing dependent, a fast async step finishes before the dispatch and works. Two shapes that used to fail with "Expected source object to be an instance of ..." can now hang the same way instead:supplyAsync { ids.map { loader.load(it) } })load(), behind a future that completes later (CompletableFuture<List<Ref<Item>>>). The transformer now runs when that future completes, which can be after the dispatchBoth resolve with
DataLoaderDispatchingContextKeys.setEnableDataLoaderChaining(or exhausted dispatching), checked on graphql-java 26.1 and 25.1. A transformer that doesn't load, or loads from a value that isn't behind a future, is fine either way.Suspend functions: Kotlin reflection types
suspend fun items(): List<CompletableFuture<Item>>asList<? extends CompletableFuture<Item>>(the bytecode has no generic return type, andCompletableFutureis open).GenericTypeResolver.resolve()returned a wildcard's upper bound as is, so[Item]failed the build with "Two different classes used for type Item" ([Item!]only worked because non-null resolves again). Now a parameterized bound is resolved too. A type variable bound stays as is, sosuspend fun <T> strings(): List<T>for[String]keeps building.Release note: a generic wrapper or future nested in a list or in another wrapper (
List<Try<Payload>>,List<CompletableFuture<Item>>,CompletableFuture<CompletableFuture<Item>>) is now unwrapped instead of failing every query. If one element fails, the whole field fails. A transformer or nested future that calls a DataLoader after an async step needs graphql-java's DataLoader chaining (25+) enabled, otherwise the field never completes.NestedGenericWrapperTesthas 26 cases, 22 of them fail on master, plus 2 for the suspend case above. The other 4 pin what stays the same (custom scalar, plain futures in a list, the resolver's own value,Optional<CompletableFuture<Item>>).Since the walk is a copy, one more test keeps it in step with
TypeClassMatcher. It builds every return type up to 4 deep (with and without? extendslist elements) out ofList,CompletableFuture, a generic wrapper, a list collection wrapper,DataFetcherResultandOptional, against 5 GraphQL types. Every combination the matcher accepts (2224) has to resolve through graphql-java with the real unwrapper. With the nested unwrapping turned off, 2022 of them fail.mvn clean verifypasses on JDK 25 against graphql-java 26.1 and 25.1, and on JDK 11 (385 tests).Also checked it through the installed jar in a separate consumer project, with graphql-java 26.1 and 25.1 at runtime: the issue's example, lists of futures (also from a DataLoader), nested futures, and a failed element in a list of
Trys and of futures. On master every one fails or returnstoString(), on this branch they resolve and a failed element gives one field error. With an appValueUnboxerthat unwrapsTryand joins futures, every result is identical to master except the failed elements, which threw out ofexecute()on master and are a field error now. The suspend functions above fail the build on master and resolve on this branch, on both graphql-java versions.javaponly shows internal classes changing (GenericTypeResolver.resolveand the data fetchers' constructors).JMH on graphql-java 25.1 (gjt-bench, 3 forks x 5 iterations, master and branch alternated):
edealerSchema62.7 / 60.5 ms on master vs 62.4 / 60.9 on the branch,largeSchema33.8 / 33.2 vs 34.4 / 32.6. None of those schemas has a nested wrapper, so this is only the check that skips the walkList<CompletableFuture<Item>>takes 257-268 µs andList<Box<Item>>238-243 µs, vs 211 µs forCompletableFuture<List<Item>>and 216-223 µs for a plain list. So roughly 100 ns per future and 40 ns per wrapper