From 8d5a6133648e5529b844fa1dde0fa542fe4ce88a Mon Sep 17 00:00:00 2001 From: Patrick Schmitt Date: Thu, 5 Oct 2023 09:47:18 +0200 Subject: [PATCH] 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 --- ...ryKeyToNewObjectsDuplicateObjectsTest.java | 55 +++++++++++++++++++ .../tests/unitofwork/UnitOfWorkTestSuite.java | 5 +- .../internal/sessions/UnitOfWorkImpl.java | 14 ++--- .../advanced/EntityManagerJUnitTestSuite.java | 38 +++++++++++++ 4 files changed, 104 insertions(+), 8 deletions(-) create mode 100644 foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/unitofwork/UOWPrimaryKeyToNewObjectsDuplicateObjectsTest.java diff --git a/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/unitofwork/UOWPrimaryKeyToNewObjectsDuplicateObjectsTest.java b/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/unitofwork/UOWPrimaryKeyToNewObjectsDuplicateObjectsTest.java new file mode 100644 index 00000000000..38283aeb665 --- /dev/null +++ b/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()); + } +} \ No newline at end of file diff --git a/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/unitofwork/UnitOfWorkTestSuite.java b/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/unitofwork/UnitOfWorkTestSuite.java index afecac7ab59..61098365895 100644 --- a/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/unitofwork/UnitOfWorkTestSuite.java +++ b/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()); diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java index 088b9c5be1b..2c84a161995 100644 --- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java +++ b/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> primaryKeyToNewObjects; + protected Map 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> getPrimaryKeyToNewObjects() { + public Map 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> pkToNewObjects = getPrimaryKeyToNewObjects(); + Map pkToNewObjects = getPrimaryKeyToNewObjects(); if (pkToNewObjects.containsKey(primaryKey)) { - List newObjects = pkToNewObjects.get(primaryKey); + IdentityHashSet newObjects = pkToNewObjects.get(primaryKey); newObjects.remove(object); if (newObjects.isEmpty()) { pkToNewObjects.remove(primaryKey); diff --git a/jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/advanced/EntityManagerJUnitTestSuite.java b/jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/advanced/EntityManagerJUnitTestSuite.java index 7cdcc9f5280..d176e1501f6 100644 --- a/jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/advanced/EntityManagerJUnitTestSuite.java +++ b/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;