Repository navigation
Unwrap a DataFetcherResult inside an Optional - #940
Merged
Merged
Conversation
Optional<DataFetcherResult<T>> bound T's type to the DataFetcherResult class, so graphql-java got the DataFetcherResult as the field value. DataFetcherResult and Optional are now peeled in either order, each once per value, and the Optional is unwrapped before graphql-java sees the DataFetcherResult. A DataFetcherResult or an Optional twice in one value, such as Optional<DataFetcherResult<Optional<T>>>, now fails the build.
|
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.



Description
Fixes #939
A resolver returning
Optional<DataFetcherResult<Payload>>gave graphql-java theDataFetcherResultitself as the field value. So{ payload { data } }returned"Payload@656f62dc"asdata, aStringfield got"DataFetcherResult{data=a, errors=[], ...}", and the result's errors were dropped, all without an error. A list inside it (Optional<DataFetcherResult<List<Payload>>>) failed the build, and so did any schema that also returnedPayloadsome other way ("Two different classes used for type Payload").TypeClassMatcherpeeled aDataFetcherResultand then anOptional, in that order only. So forOptional<DataFetcherResult<Payload>>it stopped at theDataFetcherResultand boundPayloadto that class. On top of that, graphql-java only reads aDataFetcherResultwhen it's the value the data fetcher returns, then unboxes anOptionalin its data. It never looks for one inside anOptional.Now:
DataFetcherResultandOptionalin whichever order they come, each at most once per value, with the same rules as before (DataFetcherResultonly in a return type,Optionalonly at the top level and not for a non-null type)DataFetcherResultsits right inside theOptional, the data fetcher unwraps theOptionaland hands graphql-java theDataFetcherResult, so graphql-java takes its data, errors, extensions and local context the usual way. An emptyOptionalresolves to nullOptional<X>still gets no extra step, so graphql-java (and your ownValueUnboxer) still get the resolver'sOptionalThese return types built on master and now fail the build, each for a nullable GraphQL type and any element type. All of them returned the wrong value on master, and they repeat a container the build already rejects on its own (
DataFetcherResult<DataFetcherResult<T>>,Optional<Optional<T>>):Optional<DataFetcherResult<DataFetcherResult<T>>>andDataFetcherResult<Optional<DataFetcherResult<T>>>:graphql.execution.DataFetcherResult cannot be nested within itselfOptional<DataFetcherResult<Optional<T>>>:java.util.Optional cannot be nested within itselfCLAUDE.md's rule is to keep invalid input building until the next major, and this goes out in 15.0.0, so I didn't add a WARN for them.
I first had the
Optionalalways unwrapped by the data fetcher. It's a bit less code, but then everyOptional<X>field does that work on each fetch, and a customValueUnboxernever sees theOptional.returns the resolver's own value when nothing is nestednow checks a plainOptionaltoo.resolves every return type the matcher acceptsdidn't catch this, because it skipped any match bound to a class other than the leaf. Now that counts as a failure, and the test only generates return types with as many lists as the GraphQL type. A return type with more lists binds a list to the leaf, which only the scanner rejects for an object type. Against master's main code every failure it reports is aDataFetcherResultinside anOptional("bound to graphql.execution.DataFetcherResult<...>"), and the new rejection test gets all three types accepted.Release note: a resolver can return⚠️ A return type that uses
Optional<DataFetcherResult<T>>. Before, graphql-java got theDataFetcherResultas the value.DataFetcherResultorOptionaltwice for one value, likeOptional<DataFetcherResult<Optional<T>>>orDataFetcherResult<Optional<DataFetcherResult<T>>>, now fails the build. These types built before but gave graphql-java the wrong value.Checklist
mvn clean verifypasses (436 tests). New tests inNestedGenericWrapperTestcover a present value keeping its error, an emptyOptional, wrappers in a list inside both, and the three rejections.Also checked through the installed jar in a separate consumer project. On master
{ optionalResult { data } }returns{"optionalResult": {"data": "scenarios.OptionalResultsPayload@656f62dc"}}with no errors, the list and mixed schemas fail to build, andOptional<DataFetcherResult<DataFetcherResult<Payload>>>returns the same kind of string. With this change the first returns{"data": "a"}with thepartialerror, the list returns both payloads, the mixed schema builds and resolves, and the nested one fails withDataFetcherResult cannot be nested within itself.The bounded check of every return type up to 3 containers deep (about 172k executions) no longer finds the
Optional<DataFetcherResult<...>>failures (35 return types on master), and finds nothing new. With theOptionalstep only emitted when something is nested below it, like before, it fails again on 18 return types.