QueryEnhancerFactory with JSqlParser on classpath prevent using an {h-schema} placeholder in native queries

[valkuc]

Hello.
Using Spring Data 2.7.0 (from spring-data-bom version 2021.2.0) and having com.github.jsqlparser in the classpath prevent application to start if repositories use native queries with hibernate placeholders like h-schema. It failing early with net.sf.jsqlparser.JSQLParserException: Encountered unexpected token: "{" "{".
I did not find ability to turn off that feature. The decision to use JSqlParserQueryEnhancer instead of DefaultQueryEnhancer is made in static way in QueryEnhancerFactory.

My project is based on Spring Boot 2.6.8 and in order to mitigate this issue I had switched from shipped spring-data-bom version 2021.1.4 to 2021.2.0. This solved mentioned critical issue but introduced new one :(

Here is test project to help reproduce that issue. Just run CountryRepositoryTest.
Thanks in advance.

[DiegoKrupitza]

Ok as far as I can see the main problem is that com.github.jsqlparser does not allow to use hibernate placeholders such as h-schema because this is not part of the SQL standard.

I dug a bit deeper to find a solution to that problem and it turns out your query SELECT c.* FROM {h-schema}countries c will not even work correctly with QueryUtils which is basically what DefaultQueryEnhancer uses.
For example the QueryUtils.createCountQueryFor("SELECT c.* FROM {h-schema}countries c",null);, which will be performed if you would call DefaultQueryEnhancer#createCountQueryFor(), does not return a correct count query (see screenshot 1).

If I replace {h-schema} with, let's say, public or delete it, the count query creation works perfectly fine (see screenshot 2&3).

As far as I can see using DefaultQueryEnhancer here can lead to other issues (because wrong count query is generated).

I identified two things that need to be changed/fixed:

1. QueryUtils should support hibernate placeholders (if we even support this? @gregturn )
2. Add some kind of configuration (maybe a property) in case the user wants to use DefaultQueryEnhancer over JSQLParserQueryEnhancer for native queries in case they have JSQLParser in classpath for other tasks.

I think Point 2 needs a decision from the members.

[valkuc]

Thanks for advanced research! Regarding p.1 - yes it's definitely supported at least by sprint-data-jpa supplied with Spring Boot 2.5.x. Actually this issue happened when I decided to perform migration from SB 2.5 to 2.6

[DiegoKrupitza]

Thanks for advanced research! Regarding p.1 - yes it's definitely supported at least by sprint-data-jpa supplied with Spring Boot 2.5.x.

Yeah as far as I can see it does not fail because the count query is not used anywhere (yet).

When I checkout spring-projects/spring-data-jpa/2.5.x and add the following test case:

@Test
void hibernatePlaceHolderStringCountQuery() {
   assertCountQuery("SELECT c.* FROM {h-schema}countries c", "SELECT count(c.*) FROM {h-schema}countries c");
}

the count query is wrong. (Test case fails). So I think this should be fixed.

[vyemialyanchyk]

I have the same issue and want to use DefaultQueryEnhancer without dig publicity into unnecessary details....

this is resolution for Kotlin, expect you'll fix the issue to avoid such workarounds ASAP:

    ByteBuddyAgent.install();
    ByteBuddy()
        .redefine(QueryEnhancerFactory::class.java)
        .method(named("isJSqlParserInClassPath"))
        .intercept(FixedValue.value(false))
        .make()
        .load(
            QueryEnhancerFactory::class.java.getClassLoader(),
            ClassReloadingStrategy.fromInstalledAgent()
        )

[valkuc]

@DiegoKrupitza Hi. The issue is still reproducing in SB 2.7.3. Any ETA to fix it? It would be nice to have some configuration property to tell application either to use JSQL parser for query enhancer, or no.

@vyemialyanchyk Thanks for the snippet. Currently I solved it by placing my own implementation of QueryEnhancerFactory in the same package which return false in isJSqlParserInClassPath method.

[DiegoKrupitza]

@DiegoKrupitza Hi. The issue is still reproducing in SB 2.7.3. Any ETA to fix it? It would be nice to have some configuration property to tell application either to use JSQL parser for query enhancer, or no.

Hey @valkuc,
in #2578 (comment) @gregturn mentioned:

But if this is going to involve "options", then I'm inclined to say this moves beyond the scope of anything we want to maintain directly inside Spring Data JPA.

I understood this as "a flag in the applications.properties is a no-go and if you can detect this without this its ok". The easiest implementation for this would be to introduce a flag but this is not the path to got.

In my opinion there are two ways on how to deal with this problem without introducing a flag:

1. parse the string for special chars and fall back to DefaultImpl.
2. pass the string to every "suitable and realistic" implementation and choose the best result.

(1.) is of course not a maintainable solutions and is obviously a bad decision.
(2.) is a better solution but the problem here is to decided which "suitable and realistic" to choose. If there are too many options, the performance will suffer, if there are not enough options we still have the problem that we do not find a good result.

Right now I can't say when I have time to work on this because I am really busy with uni and other stuff... But maybe you could try to come up with a solution based on my thoughts and create a PR ( one more contributor 🤩). I would be happy to look over it :)

[valkuc]

@DiegoKrupitza Let me disagree with you :) I don't see anything bad in allowing developer to choose which query parser he want to use independently of what is located in the classpath. Let's take spring.jpa.database-platform for example. If this property is not set the actual value will be detected automatically. Why just not introduce something like spring.jpa.query-enhancer - if developer will not set it - then enhancer will be detected automatically (as it currently works). In my opinion this is much faster, performant and error-prone then doing analysis of every query for some specific chars/placeholders. I can work in this direction if you don't mind.

Just for info: we added JSQL parser in our project just to perform syntax analysis of sql queries entered by user in UI and we definitely do not want for it to be used in spring internals, but... there is no way to turn that off.

[valkuc]

@DiegoKrupitza So, what is you opinion? Just want to be sure that if I will start working on my approach there will be a chance that I will not waste time :)

[DiegoKrupitza]

@DiegoKrupitza So, what is you opinion? Just want to be sure that if I will start working on my approach there will be a chance that I will not waste time :)

@valkuc I see your point and It makes sense but unfortunately I am not the one who decides this. I am just a normal contributer as you. Members of spring data jpa have to decide this.

Regarding disabling certain parsers with properties.. I have a counter example. Let's say we have multiple QueryEnhancer implementations (JSQLParser for SQL, Hibernate Parser for JPQL, EclipseLink for JPQL and the DefaultImpl using QueryUtils). Your approach must have configuration for all of them and ideally should not be hardcoded. Because maybe I want to use for JPQL the recommended Impl which is decided based on whats in classpath and only the DefaultImpl for native SQL queries. There has to be 2 configuration properties: One for SQL and one for JPQL. But now, what if I want to use EclipseLink over Hibernate for JPQL (and have both in classpath) how do you configure this? I think with the comment from @gregturn it was meant to avoid such complicated configuration.

[valkuc]

@DiegoKrupitza Interesting... I got your point. Well, maybe this is a bit of "off-topic" but could you please guide me where I can find information about why JSQLParser support was introduced. Just want to understand, because for me current situation looks like: "hey, that functionality worked for years and now it's broken" (I'm about {h-schema} placeholder).

[schauder]

@valkuc The reason to introduce the JSQLParser was that Spring Data JPA struggles with tons of problems resulting from failing to parse SQL properly.

That Hibernate supports stuff that looks almost like SQL, but isn't was surprise to everyone involved I guess.

[valkuc]

Looks looks like the problem is even more wider. Having JSQLParser on the classpath makes it not possible to use SpEL in native queries. For example:

@Query(value = "SELECT c.* FROM countries c WHERE c.name = :#{#example.name}", nativeQuery = true)
List<Country> findCountries(Country example);

will fail with Caused by: net.sf.jsqlparser.JSQLParserException: Encountered unexpected token: "{" "{".

I guess that if JSQLParser was introduced to validate query syntax, maybe it should be invoked as a last processor, just before query will be sent to DB? But... I'm not sure this is possible in scope of Spring Data JPA as this means that query should be processed after it passed to underlying persistence provider (Hibernate for example).

[DiegoKrupitza]

Looks looks like the problem is even more wider. Having JSQLParser on the classpath makes it not possible to use SpEL in native queries. For example:

@Query(value = "SELECT c.* FROM countries c WHERE c.name = :#{#example.name}", nativeQuery = true)
List<Country> findCountries(Country example);

will fail with Caused by: net.sf.jsqlparser.JSQLParserException: Encountered unexpected token: "{" "{".

I guess that if JSQLParser was introduced to validate query syntax, maybe it should be invoked as a last processor, just before query will be sent to DB? But... I'm not sure this is possible in scope of Spring Data JPA as this means that query should be processed after it passed to underlying persistence provider (Hibernate for example).

@valkuc
Ok so I tried to reproduce this error here DiegoKrupitza@98b53f9 but I works (See screenshots).

I hooked into the debugger to check whether it is really detected as a native query and saw that the query is rewritten before feeding it to the JSQLParserQueryEnhancer. Apparently JSQLParser accepts this placeholder, because it is in the format that is also used for prepared statements.

[valkuc]

@DiegoKrupitza Looks like my fault. I did not removed original query which uses {h-schema} placeholder.

After removing first query, second query successfully completed.

[DiegoKrupitza]

I created a proposal for this in #2639 . It is now up to the members to decide whether this should be considers or not.

[kender1314]

Keywords also have this problem, such as sql contains 'binary'

[DiegoKrupitza]

Keywords also have this problem, such as sql contains 'binary'

@kender1314 could you please provide an example query that has this problem?

[gregturn]

What about my idea over at #2639 (comment) in response to @DiegoKrupitza 's proposal.

[koszta5]

I can confirm that this bug is indeed present in spring-data-jpa 2.7.3
If you use #{#entityName} instead of actual table name in your nativeQuery you will encounter this problem

[DiegoKrupitza]

I can confirm that this bug is indeed present in spring-data-jpa 2.7.3 If you use #{#entityName} instead of actual table name in your nativeQuery you will encounter this problem

@koszta5 Could you provide a code snippet (method + all annotations + how you call it) and the stack trace you get, because I can not really reproduce the error.

[gregturn]

@DiegoKrupitza Sorry for pulling this change out. We appreciate your efforts. But further review has raised various questions, some of which are captured here.

For now, I've pulled these efforts aside to https://github.com/spring-projects/spring-data-jpa/tree/issue/gh-2564 so we can further review the scope of the problem (handling failing custom queries) and ensure the solution gives end-users a practical means to reach that solution.

In the mean time, the main branch has been reverted to its previous state.

[gregturn]

After further review, this {h-schema} is a feature we are not ready to support at this point in time.

If you are interested in something analogous, you can always implement it out-of-band to what Spring Data JPA offers by using currently supported bindings and variable names. You simply have to track the variable yourself and pass it along to the operations you have it implemented.