Issue 1950: Duplicate objects in UnitOfWorkImpl.primaryKeyToNewObjects (

#1956)

- use IdentityHashSet instead of ArrayList for primaryKeyToNewObjects to prevent duplicates in the future
- added unit tests

Signed-off-by: Patrick Schmitt <Patrick.Schmitt@cpb-software.com>

foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/unitofwork/UOWPrimaryKeyToNewObjectsDuplicateObjectsTest.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2023 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
+ * http://www.eclipse.org/legal/epl-2.0,
+ * or the Eclipse Distribution License v. 1.0 which is available at
+ * http://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause
+ */
+package org.eclipse.persistence.testing.tests.unitofwork;
+
+import org.eclipse.persistence.internal.sessions.UnitOfWorkImpl;
+import org.eclipse.persistence.sessions.Session;
+import org.eclipse.persistence.testing.framework.TestCase;
+import org.eclipse.persistence.testing.models.employee.domain.Employee;
+
+import java.util.IdentityHashMap;
+
+/**
+ * This test is in response to issue 1950: Duplicate objects in UnitOfWorkImpl.primaryKeyToNewObjects
+ *
+ * When a new object is registered for persist by calling registerNewObjectForPersist
+ * the object is potentially added twice to primaryKeyToNewObjects. Once during assignSequenceNumber and
+ * then in registerNewObjectClone. This test verifies that the object is contained only once in primaryKeyToNewObjects.
+ */
+public class UOWPrimaryKeyToNewObjectsDuplicateObjectsTest extends TestCase {
+    public UOWPrimaryKeyToNewObjectsDuplicateObjectsTest() {
+        setDescription("This test verifies that no duplicates exist in primaryKeyToNewObjects after registering a new object");
+    }
+
+    @Override
+    public void reset() {
+        getAbstractSession().rollbackTransaction();
+        getSession().getIdentityMapAccessor().initializeAllIdentityMaps();
+    }
+
+    @Override
+    public void setup() {
+        getAbstractSession().beginTransaction();
+    }
+
+    @Override
+    public void test() {
+        Session session = getSession();
+        UnitOfWorkImpl uow = (UnitOfWorkImpl) session.acquireUnitOfWork();
+        Employee emp = new Employee();
+        emp.setFirstName("UOWPrimaryKeyToNewObjectsDuplicateObjectsTest");
+        uow.registerNewObjectForPersist(emp, new IdentityHashMap<>());
+        // there should be only one object in primaryKeyToNewObjects
+        assertEquals("Unexpected amount of entities in primaryKeyToNewObjects", 1,
+                uow.getPrimaryKeyToNewObjects().get(emp.getId()).size());
+    }
+}

foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/unitofwork/UnitOfWorkTestSuite.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2018 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2023 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
@@ -69,6 +69,9 @@ public void addTests() {
 
         addTest(new UnregisterUnitOfWorkTest());
 
+        // Issue 1950 - Duplicate objects in UnitOfWorkImpl.primaryKeyToNewObjects
+        addTest(new UOWPrimaryKeyToNewObjectsDuplicateObjectsTest());
+
         // EL Bug 252047 - Mutable attributes are not cloned when isMutable is enabled on a Direct Mapping
         addTest(new CloneAttributeIfMutableTest());
 

foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java

@@ -158,7 +158,7 @@ public class UnitOfWorkImpl extends AbstractSession implements org.eclipse.persi
     /**
      * Map of primary key to list of new objects. Used to speedup in-memory querying.
      */
-    protected Map<Object, List<Object>>        primaryKeyToNewObjects;
+    protected Map<Object, IdentityHashSet>        primaryKeyToNewObjects;
     /**
      * Stores a map from the clone to the original merged object, as a different instance is used as the original for merges.
      */
@@ -546,7 +546,7 @@ public Object assignSequenceNumber(Object object, ClassDescriptor descriptor) th
             try {
                 value = builder.assignSequenceNumber(object, this);
                 getPrimaryKeyToNewObjects().putIfAbsent(value,
-                    new ArrayList<>());
+                    new IdentityHashSet());
                 getPrimaryKeyToNewObjects().get(value).add(object);
             } catch (RuntimeException exception) {
                 handleException(exception);
@@ -2405,7 +2405,7 @@ public Map getNewObjectsCloneToOriginal() {
      * The primaryKeyToNewObjects stores a list of objects for every primary key.
      * It is used to speed up in-memory-querying.
      */
-    public Map<Object, List<Object>> getPrimaryKeyToNewObjects() {
+    public Map<Object, IdentityHashSet> getPrimaryKeyToNewObjects() {
       if (primaryKeyToNewObjects == null) {
         primaryKeyToNewObjects = new HashMap<>();
       }
@@ -2562,7 +2562,7 @@ public Object getObjectFromNewObjects(Class theClass, Object selectionKey) {
         boolean readSubclassesOrNoInheritance = (!descriptor.hasInheritance() || descriptor.getInheritancePolicy().shouldReadSubclasses());
 
         ObjectBuilder objectBuilder = descriptor.getObjectBuilder();
-        for (Iterator newObjectsEnum = getPrimaryKeyToNewObjects().getOrDefault(selectionKey, new ArrayList<>(0)).iterator();
+        for (Iterator newObjectsEnum = getPrimaryKeyToNewObjects().getOrDefault(selectionKey, new IdentityHashSet(0)).iterator();
             newObjectsEnum.hasNext(); ) {
            Object object = newObjectsEnum.next();
            // bug 327900
@@ -5055,7 +5055,7 @@ protected void addNewObjectToPrimaryKeyToNewObjects(Object newObject,
         Object pk = objectBuilder.extractPrimaryKeyFromObject(newObject, this,
             true);
         if (pk != null) {
-          getPrimaryKeyToNewObjects().putIfAbsent(pk, new ArrayList<>());
+          getPrimaryKeyToNewObjects().putIfAbsent(pk, new IdentityHashSet());
           getPrimaryKeyToNewObjects().get(pk).add(newObject);
         }
       }
@@ -5078,9 +5078,9 @@ protected void removeObjectFromPrimaryKeyToNewObjects(Object object) {
        */
       protected void removeObjectFromPrimaryKeyToNewObjects(Object object,
           Object primaryKey) {
-        Map<Object, List<Object>> pkToNewObjects = getPrimaryKeyToNewObjects();
+        Map<Object, IdentityHashSet> pkToNewObjects = getPrimaryKeyToNewObjects();
         if (pkToNewObjects.containsKey(primaryKey)) {
-          List<Object> newObjects = pkToNewObjects.get(primaryKey);
+          IdentityHashSet newObjects = pkToNewObjects.get(primaryKey);
           newObjects.remove(object);
           if (newObjects.isEmpty()) {
             pkToNewObjects.remove(primaryKey);

jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/advanced/EntityManagerJUnitTestSuite.java

@@ -336,6 +336,7 @@ public static Test suiteSpring() {
         tests.add("testBeginTransactionClose");
         tests.add("testClose");
         tests.add("testPersistOnNonEntity");
+        tests.add("testPersistRemoveFind");
         tests.add("testWRITELock");
         tests.add("testOPTIMISTIC_FORCE_INCREMENTLock");
         tests.add("testReadOnlyTransactionalData");
@@ -5004,6 +5005,43 @@ public void testPersistOnNonEntity()
         Assert.assertTrue(testPass);
     }
 
+    /**
+     * Test for issue 1950 Duplicate objects in UnitOfWorkImpl.primaryKeyToNewObjects
+     *
+     * Objects may potentially be added to UnitOfWorkImpl.primaryKeyToNewObjects both during
+     * the call to UnitOfWorkImpl#assignSequenceNumber and then again in UnitOfWorkImpl#registerNewObjectClone.
+     * This can cause the EntityManager to return already removed entities, as only one of the saved
+     * references is removed.
+     */
+    public void testPersistRemoveFind()
+    {
+        EntityManager em = createEntityManager();
+        Employee employee = new Employee();
+        employee.setFirstName("Employee");
+        employee.setLastName("1");
+        Employee employee2 = new Employee();
+        employee2.setFirstName("Employee");
+        employee2.setLastName("2");
+        beginTransaction(em);
+        try {
+            em.persist(employee);
+            /* In order to hit the problematic code, we have to make sure there are still objects in the cache after
+             * we remove the first one, because otherwise the call to UnitOfWorkImpl#getObjectFromNewObjects
+             * will be skipped. Therefore, we register another employee object, which we won't remove. */
+            em.persist(employee2);
+            Employee clone = em.find(Employee.class, employee.getId());
+            // remove employee 1
+            em.remove(clone);
+            // a find call should not return employee 1, since we removed it earlier
+            clone = em.find(Employee.class, employee.getId());
+            assertNull("Removed employee was returned by em.find!", clone);
+        } catch (Exception e) {
+            fail("Unexpected exception thrown during test: " + e);
+        } finally {
+            rollbackTransaction(em);
+        }
+    }
+
     //detach(nonentity) throws illegalArgumentException
     public void testDetachNonEntity() {
         boolean testPass = false;