JPQL this alias implicit generation bugfix and some JPQL tests refactor

JPQL SELECT not allowing entity attribute without optional entity identification variable eclipse-ee4j#2182 - bug fix (main + tests) NullPointerException when JPQL UPDATE assignment operation omits optional identification variable eclipse-ee4j#2184 - bug fix (main + tests) Unable to omit optional entity identification variable from arguments to built-in aggregate functions eclipse-ee4j#2192 - bug fix (main + tests) Arithmetic expressions in SELECT clause broken when entity identifier variable omitted eclipse-ee4j#2247 - bug fix (main + tests) Signed-off-by: Radek Felcman <[email protected]>
rfelcman · Sep 11, 2024 · 8847d06 · 8847d06
1 parent 2933409
commit 8847d06
Show file tree

Hide file tree

Showing 20 changed files with 326 additions and 94 deletions.
diff --git a/.../eclipse/persistence/testing/tests/jpa/jpql/advanced/JUnitJPQLJakartaDataNoAliasTest.java b/.../eclipse/persistence/testing/tests/jpa/jpql/advanced/JUnitJPQLJakartaDataNoAliasTest.java
@@ -107,11 +107,15 @@ public static Test suite() {
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testGeneratedSelect"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testUpdateQueryLengthInAssignmentAndExpression"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testSelectQueryLengthInAssignmentAndExpression"));
+        suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testSelectQueryImplicitThisVariableInPath"));
+        suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testSelectQueryImplicitThisVariableInAggregateFunctionPath"));
+        suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testSelectQueryImplicitThisVariableInArithmeticExpression"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testUpdateImplicitVariableInArithmeticExpression"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testDeleteQueryLengthInExpressionOnLeft"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testDeleteQueryLengthInExpressionOnRight"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("tesUpdateQueryWithThisVariable"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testThisVariableInPathExpressionUpdate"));
+        suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testThisVariableInPathAndIdFunctionExpressionUpdate"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testThisVariableInPathExpressionDelete"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testThisVariableInLikeExpressionDelete"));
         return suite;
@@ -331,6 +335,38 @@ public void testSelectQueryLengthInAssignmentAndExpression() {
         assertTrue("Number of rooms with ID = 1", roomsWithIdOne.size() == 1);
     }
 
+
+    // Covers https://github.com/eclipse-ee4j/eclipselink/issues/2182
+    public void testSelectQueryImplicitThisVariableInPath() {
+        resetRooms();
+        List<Integer> roomsLengthWithIdOne = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                "SELECT length FROM Room WHERE length IS NOT NULL AND id = :idParam", Integer.class)
+                .setParameter("idParam", ROOMS[1].getId())
+                .getResultList());
+        assertTrue("Number of rooms with ID = 1", roomsLengthWithIdOne.size() == 1);
+        assertTrue("Room ID = " + ROOMS[1].getId() + " has length of ", roomsLengthWithIdOne.get(0) == ROOMS[1].getLength());
+    }
+
+
+    // Covers https://github.com/eclipse-ee4j/eclipselink/issues/2192
+    public void testSelectQueryImplicitThisVariableInAggregateFunctionPath() {
+        resetRooms();
+        Integer roomsMaxWidth = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                        "SELECT MAX(width) FROM Room", Integer.class)
+                .getSingleResult());
+        assertTrue("Some Room with max width exist ", roomsMaxWidth != null);
+    }
+
+    // Covers https://github.com/eclipse-ee4j/eclipselink/issues/2247
+    public void testSelectQueryImplicitThisVariableInArithmeticExpression() {
+        resetRooms();
+        int roomCapacity = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                        "SELECT length * width * height  FROM Room WHERE id = :idParam", Integer.class)
+                .setParameter("idParam", ROOMS[1].getId())
+                .getSingleResult());
+        assertEquals(ROOMS[1].getLength() * ROOMS[1].getWidth() * ROOMS[1].getHeight(), roomCapacity);
+    }
+
     public void testUpdateImplicitVariableInArithmeticExpression() {
         resetRooms();
         int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
@@ -384,6 +420,18 @@ public void testThisVariableInPathExpressionUpdate() {
         assertTrue("Length of room with ID = 1 is " + length, length == 10);
     }
 
+    // Covers https://github.com/eclipse-ee4j/eclipselink/issues/2184
+    public void testThisVariableInPathAndIdFunctionExpressionUpdate() {
+        resetRooms();
+        int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                "UPDATE Room SET length = 10 WHERE ID(this) = :idParam")
+                .setParameter("idParam", 1)
+                .executeUpdate());
+        assertTrue("Number of rooms with ID = 1 modified is " + numberOfChanges, numberOfChanges == 1);
+        int length = findRoomById(1).getLength();
+        assertTrue("Length of room with ID = 1 is " + length, length == 10);
+    }
+
     // Covers https://github.com/eclipse-ee4j/eclipselink/issues/2198
     public void testThisVariableInPathExpressionDelete() {
         resetRooms();

diff --git a/...ersistence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/AbstractValidator.java b/...ersistence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/AbstractValidator.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006, 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -36,6 +36,7 @@
 import org.eclipse.persistence.jpa.jpql.parser.FromClause;
 import org.eclipse.persistence.jpa.jpql.parser.GroupByClause;
 import org.eclipse.persistence.jpa.jpql.parser.HavingClause;
+import org.eclipse.persistence.jpa.jpql.parser.IdentificationVariable;
 import org.eclipse.persistence.jpa.jpql.parser.JPQLGrammar;
 import org.eclipse.persistence.jpa.jpql.parser.JPQLQueryBNF;
 import org.eclipse.persistence.jpa.jpql.parser.NullExpression;
@@ -45,6 +46,7 @@
 import org.eclipse.persistence.jpa.jpql.parser.SimpleFromClause;
 import org.eclipse.persistence.jpa.jpql.parser.SimpleSelectClause;
 import org.eclipse.persistence.jpa.jpql.parser.SimpleSelectStatement;
+import org.eclipse.persistence.jpa.jpql.parser.StateFieldPathExpression;
 import org.eclipse.persistence.jpa.jpql.parser.SubExpression;
 import org.eclipse.persistence.jpa.jpql.parser.UnionClause;
 import org.eclipse.persistence.jpa.jpql.parser.UnknownExpression;
@@ -967,6 +969,19 @@ public void visit(NullExpression expression) {
             valid = true;
         }
 
+        @Override
+        public void visit(StateFieldPathExpression expression) {
+            JPQLQueryBNF originQueryBNF = queryBNF;
+            if (Expression.THIS.equalsIgnoreCase(expression.toString()) &&
+                    expression.getParentExpression().isGenerateImplicitThisAlias() &&
+                    expression.getIdentificationVariable() != null &&
+                    ((IdentificationVariable)(expression.getIdentificationVariable())).isVirtual()) {
+                queryBNF = expression.getQueryBNF();
+            }
+            visit((Expression) expression);
+            queryBNF = originQueryBNF;
+        }
+
         @Override
         public void visit(SubExpression expression) {
             if (expression.hasExpression()) {

diff --git a/...ce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractExpression.java b/...ce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractExpression.java
@@ -525,7 +525,7 @@ public final JPQLExpression getRoot() {
      * @return  Parent expression
      */
     public final ParentExpression getParentExpression() {
-        if (!(this.isSubExpression()) && (this.isParentExpression())) {
+        if (parent == null || (!(this.isSubExpression()) && (this.isParentExpression()))) {
             return (ParentExpression)this;
         } else {
             return parent.getParentExpression();
@@ -602,6 +602,12 @@ protected boolean isNull() {
         return false;
     }
 
+
+    /**
+     * Flag if expression is a parent/root in the tree.
+     *
+     * @return {@code boolean} {@code true} - yes it's parent/root , {@code false} - if not
+     */
     public boolean isParentExpression() {
         return false;
     }

diff --git a/...ce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractFromClause.java b/...ce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractFromClause.java
@@ -344,7 +344,7 @@ else if (hierarchicalQueryClause == null) {
             identificationVariableDeclaration.hasRangeVariableDeclaration() && identificationVariableDeclaration.getRangeVariableDeclaration() instanceof RangeVariableDeclaration rangeVariableDeclaration &&
             rangeVariableDeclaration.hasIdentificationVariable() && rangeVariableDeclaration.getIdentificationVariable() instanceof IdentificationVariable identificationVariable &&
             Expression.THIS.equals(identificationVariable.getText())) {
-            this.getParentExpression().setGenerateThisPrefix(true);
+            this.getParentExpression().setGenerateImplicitThisAlias(true);
         }
     }
 

diff --git a/.../main/java/org/eclipse/persistence/jpa/jpql/parser/FullyQualifyPathExpressionVisitor.java b/.../main/java/org/eclipse/persistence/jpa/jpql/parser/FullyQualifyPathExpressionVisitor.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006, 2022 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0 which is available at
@@ -17,8 +17,6 @@
 //        - 527415: Fix code when locale is tr, az or lt
 package org.eclipse.persistence.jpa.jpql.parser;
 
-import java.util.Locale;
-
 /**
  * This visitor makes sure that all path expressions are fully qualified with a "virtual"
  * identification variable if the range variable declaration does not define one. This only applies
@@ -56,8 +54,8 @@ private GeneralIdentificationVariableVisitor generalIdentificationVariableVisito
 
     @Override
     public void visit(AbstractSchemaName expression) {
-        // The "virtual" variable name will be the entity name
-        variableName = expression.toActualText().toLowerCase(Locale.ROOT);
+        // The "virtual" variable name will be "this" entity name
+        variableName = Expression.THIS;
     }
 
     @Override
@@ -68,7 +66,7 @@ public void visit(CollectionMemberDeclaration expression) {
     @Override
     public void visit(CollectionValuedPathExpression expression) {
         visitAbstractPathExpression(expression);
-        variableName = expression.toActualText().toLowerCase(Locale.ROOT);
+        variableName = Expression.THIS;
     }
 
     @Override

diff --git a/...pa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/IdentificationVariable.java b/...pa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/IdentificationVariable.java
@@ -95,25 +95,7 @@ public final class IdentificationVariable extends AbstractExpression {
      */
     public IdentificationVariable(AbstractExpression parent, String identificationVariable) {
         super(parent, identificationVariable);
-        //In subqueries "this" generation is not allowed. There are expected qualified IdentificationVariable from query string
-        if (!Expression.THIS.equalsIgnoreCase(identificationVariable) && getParentExpression().isGenerateThisPrefix() && !isInsideSubquery()) {
-            this.setVirtualIdentificationVariable(Expression.THIS);
-        }
-    }
-
-    private boolean isInsideSubquery() {
-        boolean result = isSubquery(this);
-        return result;
-    }
-
-    private boolean isSubquery(Expression expression) {
-        if (expression == null) {
-            return false;
-        } else if (expression instanceof AbstractSelectStatement && expression.getParent() != null && expression.getParent() instanceof SubExpression) {
-            return true;
-        } else {
-            return isSubquery(expression.getParent());
-        }
+        setThisVirtualIdentificationVariable(true);
     }
 
     /**
@@ -133,7 +115,7 @@ public IdentificationVariable(AbstractExpression parent, String identificationVa
     public void accept(ExpressionVisitor visitor) {
         //In "this" (Jakarta Data) generation mode pass for a validation stateFieldPathExpression
         //generated by this.setVirtualIdentificationVariable(Expression.THIS); in constructor above
-        if (getParentExpression().isGenerateThisPrefix() && isVirtual() && stateFieldPathExpression != null) {
+        if (getParentExpression().isGenerateImplicitThisAlias() && isVirtual() && stateFieldPathExpression != null) {
             visitor.visit(getStateFieldPathExpression());
         } else {
             visitor.visit(this);
@@ -199,6 +181,17 @@ protected void parse(WordParser wordParser, boolean tolerant) {
         wordParser.moveForward(getText());
     }
 
+    public void setThisVirtualIdentificationVariable(boolean addToList) {
+        //In subqueries "this" generation is not allowed. There are expected qualified IdentificationVariable from query string
+        if (!Expression.THIS.equalsIgnoreCase(getText()) && getParentExpression().isGenerateImplicitThisAlias() && !isInsideSubquery()) {
+            this.setVirtualIdentificationVariable(Expression.THIS);
+        } else {
+            if (getParentExpression().getIdentificationVariablesWithoutAlias() != null && addToList) {
+                getParentExpression().getIdentificationVariablesWithoutAlias().add(this);
+            }
+        }
+    }
+
     /**
      * Sets a virtual identification variable because the abstract schema name was parsed without
      * one. This is valid in an <b>UPDATE</b> and <b>DELETE</b> queries. This internally transforms
@@ -226,4 +219,19 @@ public String toParsedText() {
     protected void toParsedText(StringBuilder writer, boolean actual) {
         writer.append(getText());
     }
+
+    private boolean isInsideSubquery() {
+        boolean result = isSubquery(this);
+        return result;
+    }
+
+    private boolean isSubquery(Expression expression) {
+        if (expression == null) {
+            return false;
+        } else if (expression instanceof AbstractSelectStatement && expression.getParent() != null && expression.getParent() instanceof SubExpression) {
+            return true;
+        } else {
+            return isSubquery(expression.getParent());
+        }
+    }
 }
diff --git a/...stence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/JPQLExpression.java b/...stence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/JPQLExpression.java
@@ -16,8 +16,13 @@
 //
 package org.eclipse.persistence.jpa.jpql.parser;
 
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
+import java.util.Locale;
+
 import org.eclipse.persistence.jpa.jpql.ExpressionTools;
 import org.eclipse.persistence.jpa.jpql.JPAVersion;
 import org.eclipse.persistence.jpa.jpql.WordParser;
@@ -89,7 +94,15 @@ public final class JPQLExpression extends AbstractExpression implements ParentEx
      */
     private boolean jakartaData = false;
 
-    private boolean generateThisPrefix = false;
+    /**
+     * Flag if missing alias {@code this} is automatically generated.
+     */
+    private boolean generateImplicitThisAlias = false;
+
+    /**
+     * List of possible {@code IdentificationVariable} where {@code this} should be generated.
+     */
+    private List<IdentificationVariable> identificationVariablesWithoutAlias = new ArrayList<>();
 
     /**
      * Creates a new <code>JPQLExpression</code>, which is the root of the JPQL parsed tree.
@@ -162,6 +175,12 @@ public JPQLExpression(CharSequence jpqlFragment,
             jpqlFragment = preParse(jpqlFragment);
         }
         parse(new WordParser(jpqlFragment), tolerant);
+        if (generateImplicitThisAlias && getIdentificationVariablesWithoutAlias() != null) {
+            for (IdentificationVariable identificationVariable : getIdentificationVariablesWithoutAlias()) {
+                identificationVariable.setThisVirtualIdentificationVariable(false);
+            }
+            identificationVariablesWithoutAlias = new ArrayList<>();
+        }
     }
 
     /**
@@ -257,13 +276,13 @@ public JPQLQueryBNF getQueryBNF() {
     }
 
     @Override
-    public boolean isGenerateThisPrefix() {
-        return generateThisPrefix;
+    public boolean isGenerateImplicitThisAlias() {
+        return generateImplicitThisAlias;
     }
 
     @Override
-    public void setGenerateThisPrefix(boolean generateThisPrefix) {
-        this.generateThisPrefix = generateThisPrefix;
+    public void setGenerateImplicitThisAlias(boolean generateImplicitThisAlias) {
+        this.generateImplicitThisAlias = generateImplicitThisAlias;
     }
 
     @Override
@@ -301,6 +320,11 @@ public Expression getUnknownEndingStatement() {
         return unknownEndingStatement;
     }
 
+    @Override
+    public List<IdentificationVariable> getIdentificationVariablesWithoutAlias() {
+        return identificationVariablesWithoutAlias;
+    }
+
     /**
      * Determines whether a query was parsed. The query may be incomplete but it started with one of
      * the three clauses (<b>SELECT</b>, <b>DELETE FROM</b>, or <b>UPDATE</b>).

diff --git a/...stence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/JPQLGrammar3_2.java b/...stence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/JPQLGrammar3_2.java
@@ -143,6 +143,8 @@ protected void initializeBNFs() {
         // ID function
         addChildBNF(SelectExpressionBNF.ID,              IdExpressionBNF.ID);
         addChildBNF(ComparisonExpressionBNF.ID,          IdExpressionBNF.ID);
+        addChildBNF(IdExpressionBNF.ID, GeneralIdentificationVariableBNF.ID);
+        addChildBNF(IdExpressionBNF.ID, SingleValuedObjectPathExpressionBNF.ID);
 
         // VERSION function
         addChildBNF(SelectExpressionBNF.ID,              VersionExpressionBNF.ID);

diff --git a/...ence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/ParentExpression.java b/...ence.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/ParentExpression.java
@@ -14,14 +14,20 @@
 //     21/07/2024: Ondro Mihalyi - implicit variable in sub-expressions
 package org.eclipse.persistence.jpa.jpql.parser;
 
+import java.util.List;
+
 public interface ParentExpression extends Expression {
 
     /**
-     * Whether should automatically add missing "this" prefix into where field variables if it doesn't exist.
+     * Whether Hermes parser should automatically add missing "this" prefix into where field variables if it doesn't exist.
+     * @return {@code boolean} {@code true} - if this aliases should be generated , {@code false} - if not
      */
-    boolean isGenerateThisPrefix();
+    boolean isGenerateImplicitThisAlias();
 
-    void setGenerateThisPrefix(boolean generateThisPrefix);
+    void setGenerateImplicitThisAlias(boolean generateImplicitThisAlias);
 
-    boolean isParentExpression();
+    /**
+     * Get list of {@code IdentificationVariable} where this alias should be added.
+     */
+    List<IdentificationVariable> getIdentificationVariablesWithoutAlias();
 }