spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.54k stars 306 forks source link

Lenient handling of empty representations list in EntitiesDataFetcher #1057

Closed roookeee closed 2 months ago

roookeee commented 2 months ago

The EntitiesDataFetcher erronously returns null when an empty list is passed in representations. This is caused by a wrong usage of Mono.zip which coerces empty lists to an empty mono, see here. The empty monoList should be preserved and passed to EntitiesDataFetcher::toDataFetcherResult.

While this will probably never occur in production (why would federation send an empty list?) it's just not spec conforming, see here. The _entities endpoints returns a non-nullable result and the current implementation returns null in the outlined scenario, which yields the following error in our tests:

org.springframework.graphql.client.FieldAccessException: Invalid field '_entities', errors: [{message=The field at path '/_entities' was declared as a non null type, but the code involved in retrieving data has wrongly returned a null value.  The graphql specification requires that the parent field be set to null, or if that is non nullable that it bubble up null to its parent and so on. The non-nullable type is '[_Entity]' within parent type 'Query', path=[_entities], extensions={classification=NullValueInNonNullableField}}]

We noticed this issue while trying to migrate from another GraphQL framework to Spring GraphQL. Some previously green tests are red because of this.

An easy fix would be to add an .defaultIfEmpty(emptyList()) after the Mono.zip call, but it seems a bit hacky to me - I am not that familiar with the Mono APIs, better solutions might exist.

roookeee commented 2 months ago

Thank you for the quick fix!