spring-projects / spring-graphql

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

Detect and report federated entities not covered by any `@EntityMapping` method #1088

Open bystam opened 1 week ago

bystam commented 1 week ago

Hello!

We just rewrote our entire platform from DGS to Spring GraphQL and generally we are very happy with what you have built here! We just stumbled upon one bug we had caused by misconfiguration that got kind of buried in the runtime errors where I think a louder crash upon launch maybe should be preferable.

The "problem"

In short: We had a copy paste error in one of our @EntityMapping declarations for a federated entity, and the result was that some of our federated queries failed without any loud exception simply with the error message No entity fetcher. We noticed eventually because some of our outer use cases failed, of course, but I wonder if the library could detect this problem earlier and complain super-loudly. I think DGS does this, but I am not 100% sure.

In detail, here's what happens (if I read the code correctly):

The suggestion

What if FederationSchemaFactory could (either in afterPropertiesSet or createSchemaTransformer or similar), double check that the list of provided @EntityMapping covers all the declared, resolvable, federated entities (types with @key(..., resolvable: true)), and if not crash with a user friendly exception?

Unless I am missing something, this should be the behaviour one would want, because if you didn't want an @EntityMapping, then you likely should mark the entity in the schema as resolvable: false. And my experience in general with Spring is that it is very good at complaining loudly with a clear explanation when there is a suspected misconfiguration.


More specifically in our case - the copy-paste error we had meant that we have DUPLICATE @EntityMapping declarations for the same time - which also didn't produce any kind of warning or error - which itself maybe should've been an error.

Let me know what you think, and whether you are open to me making an attempt at building this myself and sending a PR.

rstoyanchev commented 6 days ago

Thanks for the suggestion, and glad your experience has been good.

We will consider if we can integrate this into the SchemaMappingInspector. It should be possible but federation is a higher level package and there can't be a direct dependence.

Alternatively, it would be straightforward to build it into the FederationSchemaFactory which knows about entity mappings, abd can discover federated entity types similar to the way SchemaTransformer does.

bystam commented 6 days ago

Sounds nice! Thanks for the quick reply! Let me know if I can be of help in any way.