Review GraphQL Query auto-configurations

[rstoyanchev]

The four auto config classes in autoconfigure/graphql/data -- Querydsl, Query by Example, reactive and non-reactive, all detect repositories auto-register them for GraphQL queries.

It's possible for more than of these to be able to auto-register for a query. For example see spring-projects/spring-graphql#661 where same repository is both Querydsl and QBE. If that happens the first registration wins, which as the four configuration classes can load in any order, it means that sometimes you may get Querydsl once, and QBE the next time.

It would be good to load these in a stable order so at least you get a consistent result. We've discussed this on the GraphQL team with @mp911de and @bclozel, and decided we could have Querydsl run ahread of QBE. Perhaps also reactive ahead of non-reactive.

[wilkinsona]

which as the four configuration classes can load in any order, it means that sometimes you may get Querydsl once, and QBE the next time

This shouldn't be the case. In the absence of any before and after constraints, auto-configurations are ordered alphabetically. This should result in the following ordering:

1. GraphQlQueryByExampleAutoConfiguration
2. GraphQlQuerydslAutoConfiguration
3. GraphQlReactiveQueryByExampleAutoConfiguration
4. GraphQlReactiveQuerydslAutoConfiguration

This isn't the desired ordering, but it should be consistent. We can of course modify the ordering but it would be a breaking change so I'd welcome your thoughts on the timing. Perhaps it should only be done in a new minor rather than in a maintenance release?

It sounds like the desired ordering is the following:

1. GraphQlReactiveQuerydslAutoConfiguration
2. GraphQlQuerydslAutoConfiguration
3. GraphQlReactiveQueryByExampleAutoConfiguration
4. GraphQlQueryByExampleAutoConfiguration

[rstoyanchev]

This is good to know that there is a consistent order at least. The main goal was to have consistency, but otherwise probably no ideal order. I think it would be fine to close this issue unless @mp911de disagrees.

It's probably more important how one would customize the order if needed, but at this point it's not a real issue, so only consider it if there is a reasonably straight forward change to make.

[wilkinsona]

Looking more closely at the code, I'm not sure that the ordering of the configuration classes matters here. Each configuration class defines a GraphQlSourceBuilderCustomizer bean. As far as I can tell, it's the ordering of these customisers that matters. The auto-configuration for GraphQlSource applies these customisers in order, but they don't express an order so they'll all have lowest precedence. As such, I think I was mistaken earlier and the ordering that matters isn't necessarily going to be consistent. It depends on the behavior of ObjectProvider.orderedStream() for unordered entries.

[rstoyanchev]

I see what you mean. Yes, it comes down to the order of GraphQlSourceBuilderCustomizer beans. Maybe we can leave this as is for the time being until we see an actual use case.

There might be a couple of small things to review and possibly correct. I think the 4 auto-configuration classes are each meant to create a customizer for one category of repositories. The reactive variants comply with that and detect only reactive repositories for either Querydsl or QBE. The non-reactive ones detect both reactive and non-reactive, but should probably detect non-reactive ones only. Also, GraphQlQuerydslSourceBuilderCustomizer should not have Querydsl in its name since it's not only for Querydsl. Perhaps DataGraphQlSourceBuilderCustomizer (or Query, AutoRegistration), or something similar.

[bclozel]

I see what you mean. Yes, it comes down to the order of GraphQlSourceBuilderCustomizer beans. Maybe we can leave this as is for the time being until we see an actual use case.

I've left the order as is for now, since we didn't get any feedback on that so far. I've turned this issue into a task to address Rossen's feedback on auto-configurations.

[wilkinsona]

These changes appear to be the cause of some build failures when Query DSL isn't on the classpath. I think we need to update the conditions on GraphQlQuerydslAutoConfiguration to back off in Query DSL's absence.

[bclozel]

Sorry @wilkinsona I've just noticed you've reopened this issue. I've just pushed changes in c951c4c to fix the missing classpath condition checks. I'm not sure how those were not surfaced before. There was probably some indirection before that was preventing the relevant classes to be loaded in other tests. Still those conditions were missing in the first place and we shouldn't contribute those customizers when QueryDsl is not on the classpath.

I'll close this issue when the build is green.

[bclozel]

We're good now, see this build.
Closing with c951c4c.

[wilkinsona]

No worries, Brian. Thanks for the changes. I'd written a couple of tests so I've just pushed those in 14a59a3. I think they're worth having as the failures were from tests in a different module and I think it's good to have coverage in the same module.