GraphQlClient#retrieve does not raise FieldAccessException for failures at a path nested below the target field

[michielwittkampf]

I love that there is a Spring GraphQL library available. Thanks.

The javadoc for the retrieve method states:

throws FieldAccessException if the target field has any errors, including nested errors.

To me this suggests that a FieldAccessException is thrown if the results are partial. Yet if I test it, non is thrown. My suggestion would be to clarify this in the javadoc.

So the current only method to determine if a result is partial is probably to use the execute() method. Is that correct?

Background of my questions....

I retrieve a large data structure using graphql. When I receive a partial result in the GraphQlClient, I want to be able to easily determine they are partial, without checking all the data. In other words, check if any field errors are returned.

To make the retrieval of data easy I use the 'retrieve' method. But I see no way to check if the results are partial and what the returned errors are. Do I miss something?

[rstoyanchev]

To me this suggests that a FieldAccessException is thrown if the results are partial. Yet if I test it, non is thrown.

Do you mean that retrieve itself doesn't raise an exception? If so, this is expected. .retrieve.toEntity(..) returns a Mono and nothing happens until that Mono is subscribed to. If not, then this is not expected, in which case, please provide a reproducer.

To make the retrieval of data easy I use the 'retrieve' method. But I see no way to check if the results are partial and what the returned errors are. Do I miss something?

retrieve is just syntactic sugar that hides the entity decoding. Both execute and retrieve are deferred, so nothing happens yet when they're called, and that means there is no response to check until the output Mono is subscribed to.

We raise FieldAccessAcception for partial data with retrieve because we don't know if it's okay to decode. However, you can obtain the response from the exception, and start using it, as if you had called execute, checking errors, and decoding as needed, etc.

The only other option is for retrieve to return some container Object that exposes both errors and the decoded entity, but then execute().map(response -> ..) looks more convenient by comparison, vs retrieve(..).toEntityContainer(..).map(container -> ..).

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[michielwittkampf]

Thanks for the response.

I do understand that an exception can only be expected after the Mono is subscribed too.
So I will make a reproducer. Please give me two weeks to do so. I am currently sick.

[rstoyanchev]

No worries, take your time.

[michielwittkampf]

I reproduced the situation:
https://github.com/michielwittkampf/ReproduceFieldAccessExceptionNotThrown

If you run the tests in the class ClientTest, they show that a FieldAccessException is only thrown if the the Root data object is missing, and not if the Branch data object is missing.

[rstoyanchev]

Thanks for the repro.

It looks like DefaultResponseField#getError has an issue. It considers the field "failed" only if it is null in which case it returns any error, at any level (at, above or below). If the "failed" field is nested below, then value of the current field isn't null and we ignore the nested field error.

[michielwittkampf]

I agree.

[rstoyanchev]

On closer look, the Javaodc for ResponseField#getError does say explicitly that it returns an error only when the field has no value. That makes it inconsistent with and more opinionated than getErrors which returns all field errors. I would still like to align the two but as this would be a breaking change, I will make that in 1.1 with #524.

In the mean time, for 1.0.x, we can still correct the behavior for retrieve whose Javadoc does say that it will raise an exception for any field error. We can do that by checking if field.getErrors() is empty or not.

[rstoyanchev]

I ended up deprecating getError and hasValue both of which, without any extra logic, are very minor conveniences and can actually make things more confusing if anything.

While making changes I've also noticed one more related thing that needed a change, see #525.