Skip to content

Unwrap a DataFetcherResult inside an Optional - #940

Merged
oryan-block merged 2 commits into
masterfrom
bugfix/optional-data-fetcher-result
Oct 29, 2026
Merged

oryan-block merged 2 commits into
masterfrom
bugfix/optional-data-fetcher-result

Conversation

@oryan-block

Copy link
Copy Markdown
Collaborator

Description

Fixes #939

A resolver returning Optional<DataFetcherResult<Payload>> gave graphql-java the DataFetcherResult itself as the field value. So { payload { data } } returned "Payload@656f62dc" as data, a String field 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 returned Payload some other way ("Two different classes used for type Payload").

TypeClassMatcher peeled a DataFetcherResult and then an Optional, in that order only. So for Optional<DataFetcherResult<Payload>> it stopped at the DataFetcherResult and bound Payload to that class. On top of that, graphql-java only reads a DataFetcherResult when it's the value the data fetcher returns, then unboxes an Optional in its data. It never looks for one inside an Optional.

Now:

  • the matcher peels DataFetcherResult and Optional in whichever order they come, each at most once per value, with the same rules as before (DataFetcherResult only in a return type, Optional only at the top level and not for a non-null type)
  • when a DataFetcherResult sits right inside the Optional, the data fetcher unwraps the Optional and hands graphql-java the DataFetcherResult, so graphql-java takes its data, errors, extensions and local context the usual way. An empty Optional resolves to null
  • a plain Optional<X> still gets no extra step, so graphql-java (and your own ValueUnboxer) still get the resolver's Optional

These 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>>> and DataFetcherResult<Optional<DataFetcherResult<T>>>: graphql.execution.DataFetcherResult cannot be nested within itself
  • Optional<DataFetcherResult<Optional<T>>>: java.util.Optional cannot be nested within itself

CLAUDE.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 Optional always unwrapped by the data fetcher. It's a bit less code, but then every Optional<X> field does that work on each fetch, and a custom ValueUnboxer never sees the Optional. returns the resolver's own value when nothing is nested now checks a plain Optional too.

resolves every return type the matcher accepts didn'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 a DataFetcherResult inside an Optional ("bound to graphql.execution.DataFetcherResult<...>"), and the new rejection test gets all three types accepted.

Release note: a resolver can return Optional<DataFetcherResult<T>>. Before, graphql-java got the DataFetcherResult as the value. ⚠️ A return type that uses DataFetcherResult or Optional twice for one value, like Optional<DataFetcherResult<Optional<T>>> or DataFetcherResult<Optional<DataFetcherResult<T>>>, now fails the build. These types built before but gave graphql-java the wrong value.

Checklist

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

mvn clean verify passes (436 tests). New tests in NestedGenericWrapperTest cover a present value keeping its error, an empty Optional, 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, and Optional<DataFetcherResult<DataFetcherResult<Payload>>> returns the same kind of string. With this change the first returns {"data": "a"} with the partial error, the list returns both payloads, the mixed schema builds and resolves, and the nested one fails with DataFetcherResult 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 the Optional step only emitted when something is nested below it, like before, it fails again on 18 return types.

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

Copy link
Copy Markdown

@oryan-block
oryan-block merged commit 1b314a3 into master Oct 29, 2026
5 checks passed
@oryan-block
oryan-block deleted the bugfix/optional-data-fetcher-result branch October 29, 2026 23:05
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.

A DataFetcherResult inside an Optional is never unwrapped

1 participant