Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BadJpqlGrammarException on left join #3168

Closed
gpolet opened this issue Sep 20, 2023 · 7 comments
Closed

BadJpqlGrammarException on left join #3168

gpolet opened this issue Sep 20, 2023 · 7 comments
Assignees
Labels
in: query-parser Everything related to parsing JPQL or SQL status: duplicate A duplicate of another issue status: feedback-provided Feedback has been provided

Comments

@gpolet
Copy link

gpolet commented Sep 20, 2023

When executing the following query on a Repository, spring-data fails with the following error message:

org.springframework.data.jpa.repository.query.BadJpqlGrammarException: Line 1:46 no viable alternative at input 'Departmentd'

select e from Employee e left join Department d on e.id = d.id where d.name is null or d.name = 'foo'

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 20, 2023
@mp911de mp911de added type: bug A general bug in: query-parser Everything related to parsing JPQL or SQL and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 20, 2023
@gregturn gregturn added type: regression A regression from a previous release and removed type: bug A general bug labels Sep 20, 2023
@gregturn
Copy link
Contributor

Confirmed as failing on JPQL (works for HQL).

@gregturn
Copy link
Contributor

gregturn commented Sep 20, 2023

Okay, the problem appears to be that the query is illegal in pure JPQL.

select e
from Employee e
left join Department d
on e.id = d.id
where d.name is null or d.name = 'foo'

In the grammar rules for JPQL, the first part of the join_association_path_expression is supposed to be identification_variable .. Hence, the query SHOULD look more like:

select e
from Employee e
join e.Department d
on e.id = d.id
where d.name is null or d.name = 'foo'
Screenshot 2023-09-20 at 9 55 05 AM

Now these aren't my rules. They can be found in the grammar for JPQL at https://jakarta.ee/specifications/persistence/3.1/jakarta-persistence-spec-3.1.html#bnf ...

join_association_path_expression ::=
    join_collection_valued_path_expression |
    join_single_valued_path_expression |
    TREAT(join_collection_valued_path_expression AS subtype) |
    TREAT(join_single_valued_path_expression AS subtype)
join_association_path_expression ::=
    join_collection_valued_path_expression |
    join_single_valued_path_expression |
    TREAT(join_collection_valued_path_expression AS subtype) |
    TREAT(join_single_valued_path_expression AS subtype)
join_collection_valued_path_expression ::=
    identification_variable.{single_valued_embeddable_object_field.}* collection_valued_field
 ::=
    identification_variable.{single_valued_embeddable_object_field.}* collection_valued_field

You can see the join_collection_valued_path_expression requiring a dot-separated token. Now this query works in HQL with Hibernate just fine using our parser, as they are a superset of JPQL. So I'm assuming you're probably using EclipseLink. Has this query worked in the past for EQL?

We haven't built a counterpart for EQL yet because we A) haven't had the resources to invest in that yet and B) haven't had a lot of feedback from the community as needing that extension yet. I have done preliminary digging and not found much in documentation to guide me on what extensions to the grammar are even needed regarding EQL. (HQL has gobs of examples in their own documentation I turned into acceptance tests.)

@gregturn gregturn added status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged and removed type: regression A regression from a previous release labels Sep 20, 2023
@gpolet
Copy link
Author

gpolet commented Sep 21, 2023

@gregturn Thank you for this thorough investigation. I didn't think about looking at the JPA spec, but you're aboslutely right, it's invalid.
Indeed we're using EclipseLink and this kind of query used to be supported prior the 3.1.x releases.
Is there any way to work around this, like for example, disable the parsing of the query in some way?
Also, other than invoking every repository method, is there a way to trigger the parsing/validation of all the queries on all repositories?
Many thanks!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 21, 2023
@gregturn
Copy link
Contributor

Right now, using @Query forces the system to pick from one of the query parsers. If Hibernate is on the classpath, then it picks HQL. Otherwise, it falls back to JPQL. QueryUtils is only used for native queries.

An alternative solution is to leverage custom implementations (check out https://docs.spring.io/spring-data/jpa/docs/current/reference/html/#repositories.custom-implementations for more details on how to hook such a solution into your repository.)...or to rewrite your queries based upon JPQL standards. I don't know the scope of this task. If we're talking a single query failing because of this situation, then a rewrite may possibly fix it.

But if we're talking 100 queries, then that option becomes non-effective. It's also possible you are leaning on some of extension found in EQL (see https://eclipse.dev/eclipselink/documentation/3.0/jpa/extensions/jpql.htm) that will NOT be supported by JPQL. I'm more-so talking about custom functions and what not compared to a simple alias in this example.

In the meantime, I've opened #3170 to track implemented support for EQL's extension.

@gpolet
Copy link
Author

gpolet commented Sep 21, 2023

@gregturn Thanks again for raising that second ticket for EQL support (although I understand that without resourcse nor requests from the community, this doesn't get prioritized).
For the moment, this particular case seems to be limited to that single query which I manage to rewrite differently. What I'm more concerned about is to find out other issues in this under-tested legacy project for which I'd prefer to gain confidence that nothing else will break.
I wrote a relatively dummy test that invokes all methods annotated with @query which are not nativeQuery with some default values for primitives, String and collection of those; I hope this is sufficient to cover the parsing aspect of these queries.
For custom implementations, we have already performed quite a few extensions of the SimpleJpaRepository, in order to support updates and deletes based on Specification, facets, or other kind of aggregation queries.
Gratefully,
Guillaume

@gregturn
Copy link
Contributor

If it's any consolation, I already started implementing the EQL query parser. Managed to find one of their documents with a LOT of sample queries to craft acceptance tests, and your query up above already works with that parser (even though several other well-documented features don't, yet).

@gregturn
Copy link
Contributor

Since this is being tracked on #3170, I'm closing this ticket. Please following on the other one to track progress.

@gregturn gregturn closed this as not planned Won't fix, can't repro, duplicate, stale Sep 26, 2023
@gregturn gregturn added status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: query-parser Everything related to parsing JPQL or SQL status: duplicate A duplicate of another issue status: feedback-provided Feedback has been provided
Projects
None yet
Development

No branches or pull requests

4 participants