Fix for JPQL queries where identifier clashes with function #2218

OndroMih · 2024-07-23T22:09:07Z

Provides test cases and fix for #2187

Adds some more test cases and fixes for implicit variable this in JPQL queries.

…as a function E.g., if Box entity has field "length", "delete from Box where length = 1" fails This commit fixes it. To do: - fix copyright headers

Tests with entityManager for SELECT, UPDATE, DELETE with implicit variable

* fix tests in AbstractGrammarValidatorTest for invalid numeric literals. Simplified my previous changes. Recompute word type after reverting functions without arguments. This avoids a literal would be parsed as a number * fix test advanced.JUnitJPQLModifyTest.updateUnqualifiedAttributeInWhere. An implicit variable was incorrectly added in a nested statement, in which an explicit variable was defined

OndroMih · 2024-07-23T22:11:53Z

...stence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/AbstractGrammarValidator.java

-     * @return <code>true</code> if the given sequence of characters is a valid numeric literal;
-     * <code>false</code> otherwise
-     */
-    protected boolean isNumericLiteral(String text) {


I didn't delete this method, just moved it to the NumericLiteral class and renamed to hasValidValue, so that it can be used also in the parser, not only in the validator.

So you did an API change in a public API. Is it required to fix the bug? If so, re-do it in a backward compatible way, with appropriate deprecation step eventually, please.

I found a different way to fix the bug, so I reverted removing this method.

OndroMih · 2024-07-23T22:33:59Z

....eclipse.persistence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/WordParser.java

@@ -256,6 +257,7 @@ public int length() {
     */
    public void moveBackward(CharSequence word) {
        cursor -= word.length();
+        wordEndPosition();


Needed to reset the word parser

OndroMih · 2024-07-23T22:41:43Z

...tence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractExpression.java

+     *
+     * @return  Parent expression
+     */
+    public final ParentExpression getParentExpression() {


For a query "SELECT e Employee e WHERE (SELECT COUNT(m) FROM managedEmployees m) > 0)", it will return root expression for subexpressions related to the a variable, and will return the nested SELECT COUNT(m) expression for subexpressions related to the m variable

lukasj

In general, the code change/fix looks (mostly) good, but given the bug is in the JPQL parser related tests belong to the jpa.test.jpql test app where a test can just reuse existing Room/SimpleRoom entity from the advanced model available there instead of changing jpa.test.persistence32 app dedicated to 3.2 related features

lukasj · 2024-07-30T10:02:06Z

...stence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/AbstractGrammarValidator.java

-     * @return <code>true</code> if the given sequence of characters is a valid numeric literal;
-     * <code>false</code> otherwise
-     */
-    protected boolean isNumericLiteral(String text) {


So you did an API change in a public API. Is it required to fix the bug? If so, re-do it in a backward compatible way, with appropriate deprecation step eventually, please.

...rsistence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/NumericLiteral.java

...nce.jpa.jpql/src/test/java/org/eclipse/persistence/jpa/tests/jpql/parser/JPQLParserTest.java

lukasj · 2024-07-30T11:20:15Z

note that JUnitJPQLJakartaDataNoAliasTest may be a good place for more complex tests for queries with no alias

lukasj · 2024-07-30T11:38:55Z

...tence32/src/test/java/org/eclipse/persistence/testing/tests/jpa/persistence32/QueryTest.java

@@ -11,10 +12,15 @@
 */
 package org.eclipse.persistence.testing.tests.jpa.persistence32;

+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;


There is no single use of hamcrest in the repo and the project itself is happy without it, so do not add it, please.

Removed Hamcrest assertions. The hamcrest dependency is transitively brought to the project by junit. Maybe it would be good to exclude hamcrest from the project. I wouldn't use it if I saw it's excluded.

inability to avoid hamcrest dependency is known junit 3/4 issue - see ie junit-team/junit4#1145 and/or junit-team/junit4#1741 if you're interested in details

This is no longer true with latest versions of JUnit 4. The Hamcrest dependency of JUnit can be excluded, see: OmniFish-EE@1e6a11c. Should I raise a PR to do it?

This is no longer true with latest versions of JUnit 4. The Hamcrest dependency of JUnit can be excluded, see: OmniFish-EE@1e6a11c. Should I raise a PR to do it?

I did it in #2226

This is no longer true with latest versions of JUnit 4. The Hamcrest dependency of JUnit can be excluded, see: OmniFish-EE@1e6a11c. Should I raise a PR to do it?

It looks not so easy in EclipseLink project. There are test failures in #2226
mvn clean verify -pl :org.eclipse.persistence.jpa.jpql throws exception in org.eclipse.persistence.jpa.tests.jpql.AllHermesTests . I think it's releated with @RunWith, org.junit.runner.RunWith.

https://github.com/eclipse-ee4j/eclipselink/actions/runs/10266202341/job/28404036811

It looks like hamcrest is used somewhere. Maybe by JUnit itself. Here seems to be the same problem: #261. At that time, it looks like it was necessary to add hamcrest on the classpath in Ant configuration. But the problem was the same - something needed Hamcrest.

Yes, JUnit 4 uses hamcrest in several places: https://github.com/search?q=repo%3Ajunit-team%2Fjunit4+hamcrest+language%3AJava&type=code&l=Java. So it's not possible to completely remove it.

A solution could be to make hamcrest a runtime dependency. JUnit would work but it couldn't be used in compiled code.

Moved the check for invalid expression to general places in expression and factory.

This method is no longer needed in NumericLiteral after fixes in the previous commit.

Uses existing Room entity with a field named "length" Note: the test "JUnitJPQLJakartaDataNoAliasTest.tesUpdateQueryWithThisVariable" is still failing

OndroMih · 2024-08-01T15:49:40Z

Thank you for your review, @lukasj . I made the changes you requested, please review again.

I moved the tests into JUnitJPQLJakartaDataNoAliasTest and used the Room entity as you suggested.

lukasj · 2024-08-02T08:58:27Z

thanks!

See eclipse-ee4j#2218 (comment)

…clipse-ee4j#2218)" This reverts commit 5fe6040.

lukasj · 2024-08-14T12:29:19Z

this caused regressions reported in #2234 and #2235 therefore I had to revert this in #2238

…clipse-ee4j#2218)" This reverts commit 5fe6040.

OndroMih added 6 commits July 24, 2024 00:06

(eclipse-ee4j#2217) Fix when an unnamed entity has a field with name …

e7f1fba

…as a function E.g., if Box entity has field "length", "delete from Box where length = 1" fails This commit fixes it. To do: - fix copyright headers

(eclipse-ee4j#2217) Test for length field in an expression

5d1620a

(eclipse-ee4j#2217) Support implicit variable this in UPDATE

e07df3a

(eclipse-ee4j#2217) Support implicit variable this in DELETE. Add tests

a0c5fa1

Tests with entityManager for SELECT, UPDATE, DELETE with implicit variable

(eclipse-ee4j#2217) Fix copyright headers

a09bfc9

OndroMih commented Jul 23, 2024

View reviewed changes

OndroMih commented Jul 24, 2024

View reviewed changes

OndroMih mentioned this pull request Jul 24, 2024

Various fixes related to using the "this" variable explicitly OmniFish-EE/eclipselink#1

Closed

lukasj requested changes Jul 30, 2024

View reviewed changes

lukasj reviewed Jul 30, 2024

View reviewed changes

...nce.jpa.jpql/src/test/java/org/eclipse/persistence/jpa/tests/jpql/parser/JPQLParserTest.java Outdated Show resolved Hide resolved

lukasj reviewed Jul 30, 2024

View reviewed changes

OndroMih added 5 commits July 31, 2024 02:30

(eclipse-ee4j#2217) Removed usage of Hamcrest

6221df4

(eclipse-ee4j#2217) Apply fix for all functions, not only length

4b7b8f9

Moved the check for invalid expression to general places in expression and factory.

(eclipse-ee4j#2217) Revert moving isNumericLiteral method

87fad3c

This method is no longer needed in NumericLiteral after fixes in the previous commit.

(eclipse-ee4j#2217) Moved tests to jpa.test.jpql

5a0a76b

Uses existing Room entity with a field named "length" Note: the test "JUnitJPQLJakartaDataNoAliasTest.tesUpdateQueryWithThisVariable" is still failing

(eclipse-ee4j#2217) Removed failing test currently out of scope

ec20853

lukasj approved these changes Aug 2, 2024

View reviewed changes

lukasj merged commit 5fe6040 into eclipse-ee4j:master Aug 2, 2024
6 checks passed

This was referenced Aug 3, 2024

Assignment to computed value misinterpreted in JDQL UPDATE when entity identification variable is ommitted #2183

Closed

Fix for Jakarta data issue #2183 #2222

Open

OndroMih added a commit to OmniFish-EE/eclipselink that referenced this pull request Aug 3, 2024

Exclude Hamcrest

1e6a11c

See eclipse-ee4j#2218 (comment)

lukasj added a commit to lukasj/eclipselink that referenced this pull request Aug 14, 2024

Revert "Fix for JPQL queries where identifier clashes with function (e…

6acfbd8

…clipse-ee4j#2218)" This reverts commit 5fe6040.

lukasj added a commit to lukasj/eclipselink that referenced this pull request Aug 15, 2024

Revert "Fix for JPQL queries where identifier clashes with function (e…

f1da811

…clipse-ee4j#2218)" This reverts commit 5fe6040.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for JPQL queries where identifier clashes with function #2218

Fix for JPQL queries where identifier clashes with function #2218

OndroMih commented Jul 23, 2024 •

edited

Loading

OndroMih Jul 23, 2024

lukasj Jul 30, 2024

OndroMih Aug 1, 2024

OndroMih Jul 23, 2024

OndroMih Jul 23, 2024 •

edited

Loading

lukasj left a comment •

edited

Loading

lukasj Jul 30, 2024

lukasj commented Jul 30, 2024

lukasj Jul 30, 2024

OndroMih Aug 1, 2024

lukasj Aug 2, 2024

OndroMih Aug 3, 2024

rfelcman Aug 6, 2024

rfelcman Aug 6, 2024

rfelcman Aug 6, 2024

OndroMih Aug 6, 2024

OndroMih Aug 6, 2024

OndroMih Aug 6, 2024

OndroMih commented Aug 1, 2024 •

edited

Loading

lukasj commented Aug 2, 2024

lukasj commented Aug 14, 2024

Fix for JPQL queries where identifier clashes with function #2218

Fix for JPQL queries where identifier clashes with function #2218

Conversation

OndroMih commented Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndroMih Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

lukasj left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasj commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndroMih commented Aug 1, 2024 • edited Loading

lukasj commented Aug 2, 2024

lukasj commented Aug 14, 2024

OndroMih commented Jul 23, 2024 •

edited

Loading

OndroMih Jul 23, 2024 •

edited

Loading

lukasj left a comment •

edited

Loading

OndroMih commented Aug 1, 2024 •

edited

Loading