search

leangen / graphql-spqr

Build a GraphQL service in seconds
Apache License 2.0
1.09k stars 181 forks source link

Fix bugs in ComplexityAnalyzer #447

Closed ysamsonov closed 1 year ago

ysamsonov commented 1 year ago

I tested ComplexityAnalyzer on my GraphQL schema and I founded some issues.

  1. ComplexityAnalyzer fails in case of fragment defined on Query
  2. ComplexityAnalyzer doesn't calculate complexity for Union type (I also found the same issue #415)

In this PR I fix all of these issues and write some tests for its.

kaqqao commented 1 year ago

As I'm sure you've noticed, the current complexity analyzer heavily depends on graphql-java's internal API and is a nightmare to maintain... It has been my intention to rewrite it completely using the newer and hopefully public API for the longest time now. And what prompted me to try it again was your request to make the analyzer public. I really didn't want to make it public in its current shape. On the bright side, graphql-java now has much richer API that can drastically simplify complexity analysis, and my new version is looking infinitely more maintainable. On the down side, graphql-java still doesn't make the needed info available early enough in the execution cycle, so I still have to make calls into internal classes, albeit much less invasively than before. I've opened a pull request to address this. Let's see where it goes.

In the meantime, I'll push my new version of the analyzer, but I still merged your PR to keep the tests you've added.

kaqqao commented 1 year ago

Hey, are you using SPQR with Spring or something else? I'm asking because Nashorn has been removed in JDK 15+, and the built-in ComplexityFunction is depending on it. For Spring, I intent to replace it with a SpEL-based evaluator, but if you need it outside of Spring, I'd have to come up with a new built-in implementation that is not dependent on SpEL.

ysamsonov commented 1 year ago

I'm using SPQR with Spring. In my project I have already implemented custom ComplexityFunction based on SpEL :) Also I added some custom logic to simplify setup custom weight of resolvers based on its return type (Mono / Future / my custom classes). Thats why I'm OK with using SPEL by default ComplexityFunction but not sure that is it OK for other users.

kaqqao commented 1 year ago

Ah, cool! Would you by any chance contribute that implementation to the Spring Starter project? Would be useful there.

ysamsonov commented 1 year ago

Yes, I'd like to contribute. I'll try to do it on the next week.