Use EclipseLink JPQLParser to parse JPQL queries

[DiegoKrupitza]

I have seen that there are few issues open, that are related to JPQL queries not parsed correctly.

• StringQuery incorrect alias definition for complex native queries [DATAJPA-1613] #1919
• count query fails to be auto generated when using @Query annotation with fetch join #2348
• Count query removes distinct phrase if from part of JPA query contains newline character  #2341
• StringQuery incorrect alias definition for hql have join query [DATAJPA-1781] #2074
• Sorting by function result not possible if function contains parameter and Sort.by is used [DATAJPA-1750] #2045
• CountQuery fails to be auto generated when not using explicit alias [DATAJPA-1737] #2032

Since EclipseLink is already present in the project it does not bring in a new dependency.
JPQLParser is located in org.eclipse.persistence.internal.jpa.parsing.jpql.

Why use JQPLParser over the current approach? The current approach uses REGEX and String formatting magic that can be quite hard. A REGEX expression is quite limited and has less power than a full parser that utilizes the power of context free grammars. Using only REGEX is not a good idea since JPQL is not a regular language. Therefore JPQL cannot be represented only using REGEX. (see more here and here why REGEX is not powerful enough).

[schauder]

EclipseLink is only an optional dependency and we certainly don't want force Hibernate users to also depend on EclipsesLink.

But I guess somewhere in the Hibernate code is a JPQL parser as well.
Maybe using the one that is present on the classpath is a valid approach?

[DiegoKrupitza]

EclipseLink is only an optional dependency

Yeah is just realised that... IntelliJ tricked me here 😂

But I guess somewhere in the Hibernate code is a JPQL parser as well

I just took a closer look at Hibernate-Core 5.6 and as far as I can see there is "only" a HQL parser, BUT here it is stated that A JPQL query is always a valid HQL query, the reverse is not true however.. So as far as I can see it should be possible to parse JPQL with the HQL parsers inside Hibernate (located ´org.hibernate.hql.*´) but I can not manage to find more detailed documentation for that parser and how to use it.

Maybe using the one that is present on the classpath is a valid approach?

I think this is a valid approach. We could expand QueryEnhancer with a EclipseLinkQueryEnhancer and a HibernateQueryEnhancer and choose the right one based on the provided query.

@schauder

[DiegoKrupitza]

Apparently there was an experimental version located https://github.com/hibernate/hibernate-hql-parser. It seems this work is no longer experimental since it will be part of ORM 6.0.

I could not find the exact commit that verifies this but I assume since it is a official hibernate repo that this is true.

[gregturn]

How does this stack of feature tie into using the newly added JSqlParser?

[DiegoKrupitza]

How does this stack of feature tie into using the newly added JSqlParser?

@gregturn the newly added JSqlParser is only used for native SQL queries. For JPQL queries we are using DefaultQueryEnhancer which forwards it to QueryUtils where a lot of regex magic is happening.
This feature would add another QueryEnhancer for JPQL queries, that either uses EclipeLink or Hibernate (if present in classpath). If both are not present, DefaultQueryEnhancer can be used (which in my opinion should be last resort due to the explanation in the issue description above).

While working on JSQLParser I realised that using a proper parser only benefits the project in the long run since you do not have to tinker with complex regex (more reasons see in issue description). This is why I started to investigate if there are good alternatives, so we can get rid of the regex in QueryUtils.

[gregturn]

@DiegoKrupitza In light of your latest comments, then this makes sense, if you wish to proceed with working up a modification.

[gregturn]

@DiegoKrupitza Considering EclipseLink is an optional dependency, we could alter QueryEnhancerFactory as follows:

/**
 * Creates a new {@link QueryEnhancer} for the given {@link DeclaredQuery}.
 *
 * @param query must not be {@literal null}.
 * @return an implementation of {@link QueryEnhancer} that suits the query the most
 */
public static QueryEnhancer forQuery(DeclaredQuery query) {

	if (qualifiesForJSqlParserUsage(query)) {
		return new JSqlParserQueryEnhancer(query);
	} else if (/* EclipseLink is on the classpath*/) {
		return new EclipseLinkJPQLParserQueryEnhancer(query);
	} else {
		return new DefaultQueryEnhancer(query);
	}
}

This would let us outsource to EclipseLink if it were on the classpath. And eventually, when Hibernate gets their own parser on the classpath (Hibernate 6?), we can enable that as well. We can do this bit by bit.

[DiegoKrupitza]

@DiegoKrupitza Considering EclipseLink is an optional dependency, we could alter QueryEnhancerFactory as follows:

/**
 * Creates a new {@link QueryEnhancer} for the given {@link DeclaredQuery}.
 *
 * @param query must not be {@literal null}.
 * @return an implementation of {@link QueryEnhancer} that suits the query the most
 */
public static QueryEnhancer forQuery(DeclaredQuery query) {

	if (qualifiesForJSqlParserUsage(query)) {
		return new JSqlParserQueryEnhancer(query);
	} else if (/* EclipseLink is on the classpath*/) {
		return new EclipseLinkJPQLParserQueryEnhancer(query);
	} else {
		return new DefaultQueryEnhancer(query);
	}
}

This would let us outsource to EclipseLink if it were on the classpath. And eventually, when Hibernate gets their own parser on the classpath (Hibernate 6?), we can enable that as well. We can do this bit by bit.

Yes this was what I was thinking about! I could not verify (as stated above) if the experimental parser of hibernate made it into Hibernate 6, but a big by bit integration would make the most sense to me. First start with EclipseLink and as soon as hibernate supports this also include hibernates support.

If both Eclipse and Hibernate are on the classpath (is that even possible or makes sense to do it?) it would be great to configure which should dominate in this case. I was thinking about the situation where some undesired behaviour in Eclipselink/Hibernate (maybe a bug?) is happening and therefore maybe switch to the other implementation (only for the parser).

@gregturn

[DiegoKrupitza]

If I find time I could start working on this but I can't promise since uni workload is sometimes unpredictable.

[DiegoKrupitza]

I looked closer at this issue and saw that an EclipseLink implementation of QueryEnhancer currently is not a wise decision.

There are two "problems":

1. As written here and here in the JavaDoc, the API will change and is currently not in a final state.

Provisional API: This interface is part of an interim API that is still under development and expected to change significantly before reaching stability. It is available at this early stage to solicit feedback from pioneering adopters on the understanding that any code that uses this API will almost certainly be broken (repeatedly) as the API evolves.

2. I got stuck in modifying queries after they are parsed (see my last commit). E.g. for applySorting(...) and createCountQueryFor(...) I need to modify the parsed query and so far I could not find a way to do so. As far as I can see the API does not provide such functionality.

I suggest to revisit this issues later as soon as the API is stable and in the meantime focus on leveraging the Hibernates 6 parser (after spring data jpa upgrades to hibernate 6 #2423 ).

[gregturn]

Superceded by #2814.

[gregturn]

With #2814 merged to main, I'm closing this ticket.