Various fixes for using "this" variable in JPQL (#2224)

* Fixes #2197, #2198, #2199 related to using "this" variable in JPQL When the "this" variable is used explicitly, it's virtual but needs to be skipped in expressions that expect a virtual variable. The problem was that expressions like 'this.field' were treated as if 'this' was a field on the implicit variable. * Adds a test for #2183 The issue is already fixed. This is only a test that verifies the fix.
eclipse-ee4j · Aug 5, 2024 · 0be635f · 0be635f
1 parent 5fe6040
commit 0be635f
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 33 deletions.
diff --git a/...ore/src/main/java/org/eclipse/persistence/internal/jpa/jpql/ExpressionBuilderVisitor.java b/...ore/src/main/java/org/eclipse/persistence/internal/jpa/jpql/ExpressionBuilderVisitor.java
@@ -1,6 +1,7 @@
 /*
  * Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2006, 2024 IBM Corporation. All rights reserved.
+ * Copyright (c) 2024 Contributors to the Eclipse Foundation. 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
@@ -26,6 +27,8 @@
 //       - Issue 317: Implement LOCAL DATE, LOCAL TIME and LOCAL DATETIME.
 //     06/02/2023: Radek Felcman
 //       - Issue 1885: Implement new JPQLGrammar for upcoming Jakarta Persistence 3.2
+//     07/24/2024: Ondro Mihalyi
+//       - Issues 2197, 2198, and 2199: JPQL query incorrectly parsed when "this" variable used explicitly in path expressions
 package org.eclipse.persistence.internal.jpa.jpql;
 
 import org.eclipse.persistence.descriptors.ClassDescriptor;
@@ -2471,7 +2474,7 @@ protected boolean resolveEnumConstant(AbstractPathExpression expression) {
 
         private void resolvePath(AbstractPathExpression expression) {
 
-            for (int index = expression.hasVirtualIdentificationVariable() ? 0 : 1, count = length; index < count; index++) {
+            for (int index = expression.hasImplicitIdentificationVariable() ? 0 : 1, count = length; index < count; index++) {
 
                 String path = expression.getPath(index);
                 DatabaseMapping mapping = descriptor.getObjectBuilder().getMappingForAttributeName(path);

diff --git a/...ersistence.core/src/main/java/org/eclipse/persistence/internal/jpa/jpql/HermesParser.java b/...ersistence.core/src/main/java/org/eclipse/persistence/internal/jpa/jpql/HermesParser.java
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2024 Contributors to the Eclipse Foundation. 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
@@ -16,6 +17,8 @@
 //       - Issue 1474: Update JPQL Grammar for Jakarta Persistence 2.2, 3.0 and 3.1
 //     06/02/2023: Radek Felcman
 //       - Issue 1885: Implement new JPQLGrammar for upcoming Jakarta Persistence 3.2
+//     07/24/2024: Ondro Mihalyi
+//       - Issues 2197, 2198, and 2199: JPQL query incorrectly parsed when "this" variable used explicitly in path expressions
 package org.eclipse.persistence.internal.jpa.jpql;
 
 import org.eclipse.persistence.Version;
@@ -162,6 +165,9 @@ private JPQLException buildException(JPQLQueryContext queryContext,
             sb.append(problem.getEndPosition());
             sb.append("] ");
             sb.append(message);
+            String rootExpressionText = problem.getExpression().getRoot().toActualText();
+            sb.append(" (" + rootExpressionText.substring(0, problem.getStartPosition())
+                    + " [ " + problem.getExpression().toActualText() + " ] ...");
         }
 
         String errorMessage = bundle.getString(messageKey);

diff --git a/.../eclipse/persistence/testing/tests/jpa/jpql/advanced/JUnitJPQLJakartaDataNoAliasTest.java b/.../eclipse/persistence/testing/tests/jpa/jpql/advanced/JUnitJPQLJakartaDataNoAliasTest.java
@@ -50,15 +50,17 @@
  * @see JUnitDomainObjectComparer
  */
 public class JUnitJPQLJakartaDataNoAliasTest extends JUnitTestCase {
+
     private static final String STRING_DATA = "A String";
-    private static final Room[] ROOMS = new Room[] {
-            null, // Skip array index 0
-            aRoom(1, 1, 1, 1),
-            aRoom(2, 1, 1, 1),
-            aRoom(3, 1, 1, 1),
-            aRoom(4, 1, 1, 1)
+    private static final String STRING_DATA_LIKE_EXPRESSION = "A%"; // should match STRING_DATA
+    private static final Room[] ROOMS = new Room[]{
+        null, // Skip array index 0
+        aRoom(1, 1, 1, 1),
+        aRoom(2, 1, 1, 1),
+        aRoom(3, 1, 1, 1),
+        aRoom(4, 1, 1, 1)
     };
-    private static final long ROOMS_COUNT = ROOMS.length -1; // we ignore the first one with index 0
+    private static final long ROOMS_COUNT = ROOMS.length - 1; // we ignore the first one with index 0
 
     private static int wrapperId;
 
@@ -102,8 +104,13 @@ public static Test suite() {
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testGeneratedSelect"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testUpdateQueryLengthInAssignmentAndExpression"));
         suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testSelectQueryLengthInAssignmentAndExpression"));
+        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("testThisVariableInPathExpressionDelete"));
+        suite.addTest(new JUnitJPQLJakartaDataNoAliasTest("testThisVariableInLikeExpressionDelete"));
         return suite;
     }
 
@@ -121,17 +128,7 @@ public void testSetup() {
         new DataTypesTableCreator().replaceTables(session);
         new AdvancedTableCreator().replaceTables(session);
         clearCache();
-        EntityManager em = createEntityManager();
-        WrapperTypes wt;
-
-        beginTransaction(em);
-        wt = new WrapperTypes(BigDecimal.ZERO, BigInteger.ZERO, Boolean.FALSE,
-                Byte.valueOf("0"), 'A', Short.valueOf("0"),
-                0, 0L, 0.0f, 0.0, STRING_DATA);
-        em.persist(wt);
-        wrapperId = wt.getId();
-        commitTransaction(em);
-        closeEntityManager(em);
+        resetWrapperTypes();
     }
 
     public void testNoAlias() {
@@ -173,15 +170,15 @@ public void testNoAliasCASTCOUNT() {
     }
 
     public void testCorrectAliases() {
-          EntityManager em = createEntityManager();
+        EntityManager em = createEntityManager();
 
-          WrapperTypes wrapperTypes = (WrapperTypes) em.createQuery("SELECT this FROM WrapperTypes this").getResultList().get(0);
-          clearCache();
-          ReadObjectQuery tlQuery = new ReadObjectQuery(WrapperTypes.class);
-          tlQuery.setSelectionCriteria(tlQuery.getExpressionBuilder().get("id").equal(wrapperId));
+        WrapperTypes wrapperTypes = (WrapperTypes) em.createQuery("SELECT this FROM WrapperTypes this").getResultList().get(0);
+        clearCache();
+        ReadObjectQuery tlQuery = new ReadObjectQuery(WrapperTypes.class);
+        tlQuery.setSelectionCriteria(tlQuery.getExpressionBuilder().get("id").equal(wrapperId));
 
-          WrapperTypes tlWrapperTypes = (WrapperTypes) getPersistenceUnitServerSession().executeQuery(tlQuery);
-          Assert.assertTrue("CorrectAliases Test Failed", comparer.compareObjects(wrapperTypes, tlWrapperTypes));
+        WrapperTypes tlWrapperTypes = (WrapperTypes) getPersistenceUnitServerSession().executeQuery(tlQuery);
+        Assert.assertTrue("CorrectAliases Test Failed", comparer.compareObjects(wrapperTypes, tlWrapperTypes));
     }
 
     public void testNoAliasWhere() {
@@ -281,11 +278,24 @@ public void testUpdateQueryLengthInAssignmentAndExpression() {
     }
 
     public void testSelectQueryLengthInAssignmentAndExpression() {
-        List<Room> roomsWithIdOne  = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+        resetRooms();
+        List<Room> roomsWithIdOne = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
                 "SELECT this FROM Room WHERE id + length = length + 1", Room.class).getResultList());
         assertTrue("Number of rooms with ID = 1", roomsWithIdOne.size() == 1);
     }
 
+    public void testUpdateImplicitVariableInArithmeticExpression() {
+        resetRooms();
+        int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                "UPDATE Room SET width = width * :widthMultiplicator WHERE id = :id")
+                .setParameter("widthMultiplicator", 5)
+                .setParameter("id", 1)
+                .executeUpdate());
+        assertTrue("Number of rooms with ID = 1 updated", numberOfChanges == 1);
+        int roomWidth = findRoomById(1).getWidth();
+        assertTrue("Room ID = 1 has width of ", roomWidth == 5);
+    }
+
     public void testDeleteQueryLengthInExpressionOnLeft() {
         resetRooms();
         assertTrue("Number of remaining rooms", getAllRooms().count() == ROOMS_COUNT);
@@ -304,11 +314,63 @@ public void testDeleteQueryLengthInExpressionOnRight() {
         assertTrue("Number of remaining rooms", getAllRooms().count() == ROOMS_COUNT - 1);
     }
 
+    public void tesUpdateQueryWithThisVariable() {
+        resetRooms();
+        long numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                "UPDATE Room SET length = this.length + 1").executeUpdate());
+        assertTrue("All rooms should be updated", numberOfChanges == ROOMS_COUNT);
+
+        long numberOfRoomsWithLengthChanged = getAllRooms()
+                .filter(room -> room.getLength() == 2)
+                .count();
+        assertTrue("All rooms should have increased length", numberOfRoomsWithLengthChanged == ROOMS_COUNT);
+    }
+
+
+    // Covers https://github.com/eclipse-ee4j/eclipselink/issues/2197
+    public void testThisVariableInPathExpressionUpdate() {
+        resetRooms();
+        int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                "UPDATE Room SET this.length = 10 WHERE this.id = 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();
+        int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                "DELETE FROM Room WHERE this.length < 10").executeUpdate());
+        assertTrue("Number of rooms deleted is " + numberOfChanges, numberOfChanges == ROOMS_COUNT);
+        long numberOfRemainingRooms = getAllRooms().count();
+        assertTrue("Number of remaining rooms is " + numberOfRemainingRooms, numberOfRemainingRooms == 0);
+    }
+
+    // Covers https://github.com/eclipse-ee4j/eclipselink/issues/2199
+    public void testThisVariableInLikeExpressionDelete() {
+        try {
+            int numberOfChanges = getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                    "DELETE FROM WrapperTypes WHERE this.stringData LIKE '" + STRING_DATA_LIKE_EXPRESSION + "'").executeUpdate());
+            assertTrue("Number of wrapper types deleted", numberOfChanges == 1);
+                long remainingTypes = getAllWrapperTypes().count();
+            assertTrue("Number of remaining wrapper types is " + remainingTypes, remainingTypes == 0);
+        } finally {
+            resetWrapperTypes();
+        }
+    }
+
     private Stream<Room> getAllRooms() {
         return getEntityManagerFactory().callInTransaction(em -> em.createQuery(
                 "SELECT r FROM Room r", Room.class).getResultStream());
     }
 
+    private Room findRoomById(int i) {
+        return getEntityManagerFactory().callInTransaction(em -> {
+            return em.find(Room.class, 1);
+        });
+    }
+
     private static Room aRoom(int id, int width, int length, int height) {
         Room room = new Room();
         room.setId(id);
@@ -326,4 +388,21 @@ private void resetRooms() {
             }
         });
     }
+
+    private Stream<WrapperTypes> getAllWrapperTypes() {
+        return getEntityManagerFactory().callInTransaction(em -> em.createQuery(
+                "SELECT wt FROM WrapperTypes wt", WrapperTypes.class).getResultStream());
+    }
+
+    private void resetWrapperTypes() {
+        getEntityManagerFactory().runInTransaction(em -> {
+            em.createQuery("DELETE FROM WrapperTypes").executeUpdate();
+            WrapperTypes wt = new WrapperTypes(BigDecimal.ZERO, BigInteger.ZERO, Boolean.FALSE,
+                    Byte.valueOf("0"), 'A', Short.valueOf("0"),
+                    0, 0L, 0.0f, 0.0, STRING_DATA);
+            em.persist(wt);
+            wrapperId = wt.getId();
+        });
+    }
+
 }
diff --git a/...ce32/src/test/java/org/eclipse/persistence/testing/tests/jpa/persistence32/QueryTest.java b/...ce32/src/test/java/org/eclipse/persistence/testing/tests/jpa/persistence32/QueryTest.java
@@ -108,4 +108,4 @@ public void testGetSingleResultOrNullWithMultipleResults() {
                         "SELECT p FROM Pokemon p ", Pokemon.class).getSingleResultOrNull()));
     }
 
-}
+    }
diff --git a/...ce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/AbstractSemanticValidator.java b/...ce.jpa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/AbstractSemanticValidator.java
@@ -1,7 +1,8 @@
 /*
  * Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2021, 2024 IBM Corporation. All rights reserved.
- *
+ * Copyright (c) 2024 Contributors to the Eclipse Foundation. 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
  * http://www.eclipse.org/legal/epl-2.0,
@@ -944,7 +945,7 @@ else if (collectionTypeOnly && !helper.isCollectionMapping(mapping) ||
 
     /**
      * Validates the given {@link Expression} and makes sure it's a valid collection value path expression.
-     * 
+     *
      * join_collection_valued_path_expression::=
      *     identification_variable.{single_valued_embeddable_object_field.}*collection_valued_field
      * join_single_valued_path_expression::=
@@ -992,8 +993,8 @@ protected boolean validateJoinCollectionValuedPathExpression(Expression expressi
 
                 valid = false;
             }
-            else if (!helper.isCollectionMapping(mapping) && 
-                    !helper.isRelationshipMapping(mapping) && 
+            else if (!helper.isCollectionMapping(mapping) &&
+                    !helper.isRelationshipMapping(mapping) &&
                     !helper.isEmbeddableMapping(mapping)) {
 
                 int startPosition = position(expression);
@@ -2636,7 +2637,7 @@ protected boolean validateUpdateItem(UpdateItem expression) {
                 if (managedType != null) {
 
                     // Continue to traverse the path expression
-                    for (int index = pathExpression.hasVirtualIdentificationVariable() ? 0 : 1, count = pathExpression.pathSize(); index < count; index++) {
+                    for (int index = pathExpression.hasImplicitIdentificationVariable() ? 0 : 1, count = pathExpression.pathSize(); index < count; index++) {
 
                         // Retrieve the mapping
                         String path = pathExpression.getPath(index);

diff --git a/...pa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractPathExpression.java b/...pa.jpql/src/main/java/org/eclipse/persistence/jpa/jpql/parser/AbstractPathExpression.java
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2006, 2024 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2024 Contributors to the Eclipse Foundation. 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
@@ -281,6 +282,16 @@ public final boolean hasVirtualIdentificationVariable() {
         return identificationVariable.isVirtual();
     }
 
+    /**
+     * Determines whether the path's identification variable is virtual and not used in the query with the {@code this} keyword.
+     *
+     * @return <code>true</code> if this identification variable was virtually created and is not explicitly used in this path expression; <code>false</code> otherwise (is not virtual or is virtual and referenced with the {@code this} keyword)
+     */
+    public final boolean hasImplicitIdentificationVariable() {
+        checkPaths();
+        return identificationVariable.isVirtual() && !paths.get(0).equals(Expression.THIS);
+    }
+
     @Override
     protected final void parse(WordParser wordParser, boolean tolerant) {
         wordParser.moveForward(getText());
-Original file line number
+Diff line change
@@ Expand Up @@
                             "SELECT p FROM Pokemon p ", Pokemon.class).getSingleResultOrNull()));
         }
-    }
+        }