Added fallback mechanism for QueryEnhancer.

[DiegoKrupitza]

In #2564 we discovered that sometimes queries contain content that cannot be parsed by special implementation such as JSqlParserQueryEnhancer. Therefore, we implement a fallback mechanism that tries to use the first possible suitable implementation for a given query, that does not fail with an exception on creation. If the first one fails we fallback to the next one until we have reached the DefaultQueryEnhancer.

Closes #2564

NOTE: as mentioned in the comment section of the issue. Members should decide if we should even bother for such special cases that lead to a fail of a special implementation of QueryEnhancer.

• You have read the Spring Data contribution guidelines.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.
• You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

[gregturn]

@DiegoKrupitza I'm pondering if we need some annotation-drive solution to Choose Your Own QueryEnhancer.

Either...

public interface PetRepository extends CrudRepository<Pet, Long> {

    @Query("....")
    @QueryEnhancer(MyOwnQueryEnhancer.class)
    public Pet customQuery(String name);

}

Either a custom annotation, or an optional attribute added to @Query.

It would offer people one additional "escape hatch" to tune and adjust things themselves. Otherwise, it seems people run into situations where JSqlParserQueryEnhancer is applied all over the place, but they want to override a single query with an alternative. Or the DefaultQueryEnhancer.

[DiegoKrupitza]

@DiegoKrupitza I'm pondering if we need some annotation-drive solution to Choose Your Own QueryEnhancer.

Either...

public interface PetRepository extends CrudRepository<Pet, Long> {

    @Query("....")
    @QueryEnhancer(MyOwnQueryEnhancer.class)
    public Pet customQuery(String name);

}

Either a custom annotation, or an optional attribute added to @Query.

It would offer people one additional "escape hatch" to tune and adjust things themselves. Otherwise, it seems people run into situations where JSqlParserQueryEnhancer is applied all over the place, but they want to override a single query with an alternative. Or the DefaultQueryEnhancer.

@gregturn I think your idea is better. By default we use the "best" predefined solution and if devs want to use a special QueryEnhancer for a given Query, the just use the @QueryEnhancer(....). I would not add another value to @Query(...) because this would just make the Annotation way to overloaded.

I would design it the following way:

public interface PetRepository extends CrudRepository<Pet, Long> {

    @Query("....")
    @QueryEnhancer(MyOwnQueryEnhancer.class) // custom impl
    public Pet customQueryUsesMyOwnQueryEnhancer(String name);
    
    @Query("....")
    @QueryEnhancer // using DefaultQueryEnhancer
    public Pet customQueryUsesDefaultQueryEnhancer(String name);

    @Query("....") // use spring-data-jpa predefined logic for QueryEnhancer
    public Pet customQueryUsingOldWayOfDecision(String name);
}

• For the first method customQueryUsesMyOwnQueryEnhancer we will use the class that is provided as param to the Annotation. Either an impl by us or a totally custom implementation by the dev (just has to impl the interface)
• For the second method customQueryUsesDefaultQueryEnhancer we will use the DefaultQueryEnhancer because the default value for @QueryEnhancer is DefaultQueryEnhancer.
• For the third method customQueryUsingOldWayOfDecision we will fallback to the already existing code which means a pre-definied QueryEnhancer impl based on the query properties.

[DiegoKrupitza]

I just realised that the Word QueryEnhancer is already used for the interface (org.springframework.data.jpa.repository.query.QueryEnhancer) so we need a different name for the Annotation, since bought would be in the same package.

[DiegoKrupitza]

@gregturn I introduced a new annotation for this problem called QueryEnhancerChoice (better names are welcome). Unfortunately I had to convert the QueryEnhancer interface to an abstract class, so I can make sure there will be always a constructor (with one param) that I can use to create an instance using reflection.

[gregturn]

At first glance, this looks quite nice @DiegoKrupitza! I think I'll wait until January to dig in further and try to process it. It does carry some key changes, so I'm not sure we can backport it.

[DiegoKrupitza]

At first glance, this looks quite nice @DiegoKrupitza! I think I'll wait until January to dig in further and try to process it. It does carry some key changes, so I'm not sure we can backport it.

Yes I agree. It would make sense to include this only in newer versions since there is a fundamental change happening with they "QueryEnhancer".

[gregturn]

@DiegoKrupitza I'm working on merging this PR to main branch. I have honed in on @QueryEnhancerOverride. It seems to convey the intent, which is to override whatever else the system picked with the developer's deliberate choice.

[gregturn]

@DiegoKrupitza I've sifted through all your code, made some tweaks here and there, and done a little polishing.

I'm mostly good with it. The one thing I'm uncertain about it, is altering the visibility of a method in QueryUtils in order to support a test case. I'm wondering if we can achieve such a test without that. Frankly, I'd like to keep as much of QueryUtils sealed off as possible to minimize risk.

[gregturn]

@DiegoKrupitza I invite you to visit https://github.com/spring-projects/spring-data-jpa/commits/issue/gh-2564, where I've taken all your work, applied tweaks and polishings, added material for the ref docs, and squashed it into a single commit.

I'm consulting with my colleagues about the final outcome of QueryEnhancer vs. DefaultQueryEnhancer vs. JSqlParserQueryEnhancer. Should DefaultQueryEnhancer perhaps be collapsed into QueryEnhancer since that is now a class? Should QueryEnhancer.hasConstructorExpression() be made abstract and its contents pushed down into the concrete instances? Something about this arrangement is a little blurry, and I'd like to sharpen the lines.

[gregturn]

Resolved via ca6b650.

Thanks @DiegoKrupitza!

[mp911de]

After revisiting the entire thread, two things became apparent:

1. QueryEnhancer as abstract class isn't ideal, because it is associated with a DeclaredQuery already. Also, we cannot easily switch to an interface because it would break existing code.
2. @QueryEnhancer as standalone-annotation isn't ideal, because derived query methods could be annotated as well. A better place would be really the @Query annotation because enhancers can be only applied with custom string (or named) queries. In both cases, we back off from query derivation.

There's conflict potential in the way things are arranged, queryEnhancer and queryEnhancerOverride. A query should have exactly one of these and should not distinguish which one to use. Such a decision should be made in a single place that creates those queries.

We should take a step back and revisit the design to not ship something with apparent issues in the design as we cannot make changes once the code has been released in a GA release.

[DiegoKrupitza]

After revisiting the entire thread, two things became apparent:

1. QueryEnhancer as abstract class isn't ideal, because it is associated with a DeclaredQuery already. Also, we cannot easily switch to an interface because it would break existing code.
2. @QueryEnhancer as standalone-annotation isn't ideal, because derived query methods could be annotated as well. A better place would be really the @Query annotation because enhancers can be only applied with custom string (or named) queries. In both cases, we back off from query derivation.

There's conflict potential in the way things are arranged, queryEnhancer and queryEnhancerOverride. A query should have exactly one of these and should not distinguish which one to use. Such a decision should be made in a single place that creates those queries.

We should take a step back and revisit the design to not ship something with apparent issues in the design as we cannot make changes once the code has been released in a GA release.

Thank you for the feedback. I see the problems you are pointing out and yes we should definitely revisit the design. I saw that #2846 introduces a new Parser for JPQL. In my opinion it would make the most sense to wait until this is completed, so we have a "clean canvas" and changes on QueryEnhancer does not the great work of the PR.

I cant promise when I find time to spend on redesigning, since I am currently quite busy with my master thesis. But in case someone else wants to work on it in the meantime, I am happy to assist with answering questions etc.

[mp911de]

Thanks for your update. Once the JPQL parser is done, let us know how you want to continue.

[daromnik]

Hello, is there any information when this will be released?

[gregturn]

@daromnik Based upon the recent addition of the query parser, we're not certain what the path forward is at this time. We are waiting @DiegoKrupitza to consult on that direction.

[DiegoKrupitza]

So I revisited the entire thread again since its been some time when I last worked on this issue. The main goal why we wanted to introduce something like @QueryEnhancerChoice was to allow users to select what concrete implementation of the QueryEnhancer interface should be used. A use case is shown in #2564 (aka JSQLParser cannot parse hibernate placeholders).

At first sight (and this is what I implemented in the original PR), it would make sense to give the users full control (aka let them select what QueryEnhancer one wants to use). This although introduces many problems:

1. Users can select a QueryEnhancer implementation that does not work with the given type of query (aka select JSqlParser for HQL Queries)
2. QueryEnhancer had to become an abstract class making it an own entity although it is strongly coupled with DeclaredQuery
3. In case a user wants to have custom implementation of QueryEnhancer only to override detectAlias() for some specific reason, all the other methods had to be implemented too. Although QueryEnhancer only has a small set of methods there are still quite a lot methods that have rather drastic implications when implemented wrongly (aka createCountQueryFor)

So now that we identified that there are (at least 3) problems, what do we do know?

I suggest to not move forward with the current design and goal. My proposal is to go into a different direction. Users should not be able to select/provide a custom QueryEnhancer, instead there should be only an option to "fallback" to the DefaultQueryEnhancer.

As @mp911de already proposed, this would mean that @Query would receive a new boolean flag useDefaultEnhancer. If this flag is set the implementation DefaultQueryEnhancer will be used. If the flag is not set then we proceed as currently implemented in QueryEnhancerFactory#forQuery(...) (in the main branch currently in GA).

// native
@Query(value = "select * from SD_User u where u.emailAddress = ?", nativeQuery = true)
User nativeWillUseWhatIsDefinedByFactory(String emailAddress); // will use what QueryEnhancerFactory#forQuery(...) intended 

@Query(value = "select * from SD_User u where u.emailAddress = ?", nativeQuery = true, useDefaultEnhancer=true)
User nativeWillUseDefaultEnhancer(String emailAddress); // will use DefaultQueryEnhancer intended 

// JPQL / HQL
@Query(value="....")
User willUseWhatIsDefinedByFactory(String emailAddress); // will use what QueryEnhancerFactory#forQuery(...) intended 

@Query(value = "....", useDefaultEnhancer=true)
User willUseDefaultEnhancer(String emailAddress); // will use DefaultQueryEnhancer intended 

Why should we follow this proposal? Although we do not satisfy the main goal (mentioned in the first paragraph of this comment), we do allow users to fallback to the "trusted" QueryEnhancer implementation that is used for a loooong time, in case the new QueryEnhancer methods do not work as intended or have issues.
We also do not have the the 3 problems listed above, because

1. Users cannot manually select wrong implementations.
2. No need to change QueryEnhancer.
3. No possible to provide custom implementations.

I know there might be super rare edge cases where it would be really beneficial to have the possibility to have a custom QueryEnhancer, but this would add a lot of maintenance overhead and complexity to spring jpa that is not desired.

Furthermore, as highlighted by @gregturn (here #2564 (comment) ) it is often possible to achieve a goal with way simpler means.

[gregturn]

Thanks @DiegoKrupitza.

At this junction, I frankly don't see a huge value in proceeding with this. I'd rather close this issue and let people continue to either:

• Migrate toward using the HQL/JPQL parsers
• Use what support we DO have for native queries
• Migrate toward a custom implementation for queries that are simply too complex for the other options

We are essentially trying to say that QueryUtils in a frozen state, so there will be no more enhancements to support native queries. And we have a much more powerful solution in place for JPQL and HQL queries.

Therefore, I don't think QueryEnhancer itself needs any improvements we can't get through the options. Final comments?

[DiegoKrupitza]

Thanks @DiegoKrupitza.

At this junction, I frankly don't see a huge value in proceeding with this. I'd rather close this issue and let people continue to either:

• Migrate toward using the HQL/JPQL parsers
• Use what support we DO have for native queries
• Migrate toward a custom implementation for queries that are simply too complex for the other options

We are essentially trying to say that QueryUtils in a frozen state, so there will be no more enhancements to support native queries. And we have a much more powerful solution in place for JPQL and HQL queries.

Therefore, I don't think QueryEnhancer itself needs any improvements we can't get through the options. Final comments?

Yes, I agree with you that this will not add a lot of value to the project, especially when considering that the main goal is to slowly move away from QueryUtils.

What I would suggest is that we somewhere mention in the documentation, that when someone encounters problems with the query processing that there are 3 possible options one can go for (aka the 3 bullet points you mentioned).

[gregturn]

I'm closing this request with a link to #3005 to track updates to the ref docs about giving readers a jumping off point when queries are too complex for the builtin support.