Support returning list of entities from `@EntityMapping` methods

[srdjan-bestic-ananas]

Hi there, let me start by saying thank you for providing spring-graphql in the first place, and especially for delving into federation support as of recently. In the company I work for, we have recently adapted spring-graphql and the biggest missing piece was exactly that, no out of the box federation support that we had to implement on our own.

During the process, we came to discover that __referenceResolver queries can be fairly pricey if left to deal with singular entities. This is something that apollo team seems to warn about as can be seen in the note of the paragraph here https://www.apollographql.com/docs/federation/entities/#2-define-a-reference-resolver

ⓘ NOTE

A particular reference resolver might be called many times to resolve a single query. It's crucial that reference resolvers account for "N+1" issues (typically via data loaders). For details, see Handling the N+1 problem.

For us, due to custom implementation, we have easily adapted to the advice.

I am curious to know your thoughts on the topic of mixing batch mapping with entities fetching and if there is a plan to support this in spring-graphql in the near future?

[rstoyanchev]

@srdjan-bestic-ananas, thanks for taking a look at our milestone and for the feedback.

At the moment, @EntityMapping methods don't support a DataLoader argument, but that's more of an oversight that we can easily fix, and it makes sense to allow the graph below federated entities to be batch loaded.

I've scheduled this to make that change, but do let me know if this is what you had in mind?

[rstoyanchev]

I should mention, it should be possible still get access to DataLoaders from the DataFetchingEnvironment by injecting it into the controller method, but we'll make that easier.

[srdjan-bestic-ananas]

Thanks for the response @rstoyanchev

I'm aware of accessing DataLoader through DataFetchingEnviornement, as well as using it as an argument, since we used that exact approach when custom mapping _entities query to one of the controller methods.

The 'issue' with that approach is it leaves room for improvements as you still need to register custom data loaders and load data into them, approach which amounts to boilerplate code that BatchMapping annotation was designed to mitigate. And that leads to the actual question, do you see an easy way and a need to mix BatchMapping and EntityMapping logic, such that registering data loaders and loading them manually can be avoided?

From my point of view and with limited time in studying spring-graphql code (1.3 milestone), it might prove difficult to add batch loading of entities elegantly at a later phase.

[rstoyanchev]

I experimented, see this test with a Book federated type, and a related Author. I replaced the author @SchemaMapping method with a @BatchMapping method and it works.

Basically, the support for nested fields is the same. It doesn't matter if the parent was fetched through a @QueryMapping or an @EntityMapping controller method.

[srdjan-bestic-ananas]

My concern is regarding the root fields, not nested fields.

From the example in the test, imagine that book entity is retrieved through IO, e.g. database call. You have an N+1 problem that can be solved only by manually registering batch data loader that is then invoked/loaded through method annotated with EntityMapping. The original question is essentially, have you accounted for maybe allowing List to be passed as a parameter to EntityMapping method, instead of a single id?

[rstoyanchev]

That is a more clear description.

I think batch mapping is maybe not be the best fit. In regular queries, data loading applies to nested fields, and it's not necessary at the top where the query simply returns a list. The use of the data loader library is also driven by GraphQL Java, while here we would need to do that ourselves. I'm not sure this is worth it.

We could consider allowing an @EntityMapping method to return a list of entities:

@EntityMapping
public List<Book> book(@Argument List<Integer> id) {
	// ..
}

We'd have to group representations by type, and remember their position in the original list, so when results are ready, we can put them back in their original place on the list.

[srdjan-bestic-ananas]

That seems reasonable.

My mind automatically shifts to batch loading logic due to, as you say a need to reorder, which is a problem solved in mapped vs non mapped batch loading, but it makes sense to skip the data loading all together as this is a root query essentially that expects a list back.

I'd even argue that @EntityMapping method should only be able to return a list of entities, for a list of ids as it mimics the purpose of the query better.