[master] detach() fixes (#2037)

There are two fixes for EntityManager.detach(): - Lazy collections are fetched on detach - StackOverflowError because of cyclic DETACH Signed-off-by: Radek Felcman <radek.felcman@oracle.com>
eclipse-ee4j · Jan 22, 2024 · 4027605 · 4027605
1 parent fc392cb
commit 4027605
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 11 deletions.
diff --git a/...e.core/src/main/java/org/eclipse/persistence/internal/descriptors/DescriptorIterator.java b/...e.core/src/main/java/org/eclipse/persistence/internal/descriptors/DescriptorIterator.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 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
@@ -28,6 +28,7 @@
 import org.eclipse.persistence.queries.FetchGroup;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.IdentityHashMap;
 import java.util.List;
@@ -53,6 +54,7 @@ public abstract class DescriptorIterator {
     public static final int CascadePrivateParts = 2;
     public static final int CascadeAllParts = 3;
     protected Map visitedObjects;
+    protected Map<Integer, DatabaseMapping> visitedMappings;
     protected Stack visitedStack;
     protected AbstractSession session;
     protected DatabaseMapping currentMapping;
@@ -92,6 +94,7 @@ public abstract class DescriptorIterator {
     protected DescriptorIterator() {
         // 2612538 - the default size of Map (32) is appropriate
         this.visitedObjects = new IdentityHashMap();
+        this.visitedMappings = new HashMap<>();
         this.visitedStack = new Stack();
         this.cascadeDepth = CascadeAllParts;
         this.shouldIterateOverIndirectionObjects = true;// process the objects contained by ValueHolders...
@@ -158,6 +161,10 @@ public Map getVisitedObjects() {
         return visitedObjects;
     }
 
+    public Map<Integer, DatabaseMapping> getVisitedMappings() {
+        return visitedMappings;
+    }
+
     /**
      * Return the last object visited.
      */
@@ -271,8 +278,15 @@ public void iterateIndirectContainerForMapping(IndirectContainer container, Data
             internalIterateIndirectContainer(container);
         }
 
-        if (shouldIterateOverUninstantiatedIndirectionObjects() || (shouldIterateOverIndirectionObjects() && container.isInstantiated()) || isForDetach()) {
+        //Extended condition to allow to detach entities in the lazy loaded collections but without instantiation
+        //and basic check to avoid StackOverflowError in cyclic detach
+        if (shouldIterateOverUninstantiatedIndirectionObjects() ||
+            (shouldIterateOverIndirectionObjects() && container.isInstantiated()) ||
+            (isForDetach() && !getVisitedMappings().containsKey(mapping.hashCode() + container.hashCode()) && getVisitedMappings().size() < 100)) {
             // force instantiation only if specified
+            if (isForDetach()) {
+                getVisitedMappings().put(mapping.hashCode() + container.hashCode(), mapping);
+            }
             mapping.iterateOnRealAttributeValue(this, container);
         } else if (shouldIterateOverIndirectionObjects()) {
             // PERF: Allow the indirect container to iterate any cached elements.
@@ -441,7 +455,9 @@ protected void internalIterateReferenceObjects(Object sourceObject) {
             this.currentItem = currentItemOriginal;
         } else {
             for (DatabaseMapping mapping : mappings) {
-                mapping.iterate(this);
+                if (this.cascadeCondition.shouldCascade(mapping)) {
+                    mapping.iterate(this);
+                }
             }
         }
     }
@@ -691,8 +707,12 @@ public class CascadeCondition{
         public CascadeCondition() {
         }
 
+        public boolean shouldCascade(DatabaseMapping mapping){
+            return shouldCascadeAllParts() || (shouldCascadePrivateParts() && mapping.isPrivateOwned());
+        }
+
         public boolean shouldNotCascade(DatabaseMapping mapping){
-            return !(shouldCascadeAllParts() || (shouldCascadePrivateParts() && mapping.isPrivateOwned()));
+            return !shouldCascade(mapping);
         }
     }
 

diff --git a/...sistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java b/...sistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 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
@@ -5684,9 +5684,14 @@ public void iterate(Object object) {
         iterator.setForDetach(forDetach);
         if (forDetach){
             CascadeCondition detached = iterator.new CascadeCondition(){
+                @Override
+                public boolean shouldCascade(DatabaseMapping mapping){
+                    return mapping.isForeignReferenceMapping() && ((ForeignReferenceMapping)mapping).isCascadeDetach();
+                }
+
                 @Override
                 public boolean shouldNotCascade(DatabaseMapping mapping){
-                    return ! (mapping.isForeignReferenceMapping() && ((ForeignReferenceMapping)mapping).isCascadeDetach());
+                    return !shouldCascade(mapping);
                 }
             };
             iterator.setCascadeCondition(detached);

diff --git a/....advanced/src/main/java/org/eclipse/persistence/testing/models/jpa/advanced/Material.java b/....advanced/src/main/java/org/eclipse/persistence/testing/models/jpa/advanced/Material.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2022 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 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
@@ -18,6 +18,7 @@
 
 import jakarta.persistence.CascadeType;
 import jakarta.persistence.Entity;
+import jakarta.persistence.FetchType;
 import jakarta.persistence.GeneratedValue;
 import jakarta.persistence.Id;
 import jakarta.persistence.OneToMany;
@@ -42,9 +43,12 @@ public class Material {
     @OneToOne
     private MaterialHist lastHist;
 
-    @OneToMany(mappedBy = "material", cascade = { CascadeType.PERSIST, CascadeType.DETACH })
+    @OneToMany(mappedBy = "material", cascade = { CascadeType.PERSIST, CascadeType.DETACH }, fetch = FetchType.LAZY)
     private List<PlanArbeitsgang> restproduktionsweg = new ArrayList<>();
 
+    @OneToMany(mappedBy = "material", cascade = { CascadeType.PERSIST }, fetch = FetchType.LAZY)
+    private List<PlanArbeitsgang> restproduktionswegNoCascadeDetach = new ArrayList<>();
+
     private String ident;
 
     private String wert;
@@ -97,6 +101,14 @@ public void setRestproduktionsweg(List<PlanArbeitsgang> restproduktionsweg) {
         this.restproduktionsweg = restproduktionsweg;
     }
 
+    public List<PlanArbeitsgang> getRestproduktionswegNoCascadeDetach() {
+        return restproduktionswegNoCascadeDetach;
+    }
+
+    public void setRestproduktionswegNoCascadeDetach(List<PlanArbeitsgang> restproduktionswegDetach) {
+        this.restproduktionswegNoCascadeDetach = restproduktionswegDetach;
+    }
+
     @Override
     public String toString() {
         return String.format("%s [id=%s, version=%s, ident=%s]", getClass().getSimpleName(), id, version, ident);

diff --git a/.../java/org/eclipse/persistence/testing/tests/jpa/advanced/EntityManagerJUnitTestSuite.java b/.../java/org/eclipse/persistence/testing/tests/jpa/advanced/EntityManagerJUnitTestSuite.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2023 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024 Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 1998, 2023 IBM Corporation. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
@@ -517,6 +517,7 @@ public static Test suiteSpring() {
         tests.add("testInheritanceFetchJoinSecondCall");
 
         tests.add("testDetachChildObjects");
+        tests.add("testDetachLazyLoadedCollection");
         tests.add("testAutoCloseable");
 
 
@@ -13109,7 +13110,7 @@ public void testServerDetectionLogging(){
     public void testDetachChildObjects() {
     	EntityManager em = createEntityManager();
     	beginTransaction(em);
-		InitTestDetachChildObjects(em);
+		InitTestDetachChildObjects1(em);
 		UnitOfWorkImpl uow = (UnitOfWorkImpl) em.unwrap(JpaEntityManager.class).getActiveSession();
 
 		Material mat = em.find(Material.class, 1L);
@@ -13129,10 +13130,37 @@ public void testDetachChildObjects() {
 		for (PlanArbeitsgangHist pag : pagList) {
 			assertTrue(em.contains(pag));
 		}
+        assertTrue(em.contains(matEreignis1.getChangedObject()));
+        assertFalse(em.contains(matEreignis1));
+        assertFalse(em.contains(matEreignis1.getBeforeChange()));
+        assertFalse(em.contains(matEreignis1.getAfterChange()));
+        for (PlanArbeitsgangHist planArbeitsgangHist: matEreignis1.getBeforeChange().getRestproduktionsweg()) {
+            assertFalse(em.contains(planArbeitsgangHist));
+        }
+        for (PlanArbeitsgangHist planArbeitsgangHist: matEreignis1.getAfterChange().getRestproduktionsweg()) {
+            assertFalse(em.contains(planArbeitsgangHist));
+        }
+
 		rollbackTransaction(em);
 		closeEntityManager(em);
     }
 
+    public void testDetachLazyLoadedCollection() {
+        EntityManager em = createEntityManager();
+        beginTransaction(em);
+        InitTestDetachChildObjects2(em);
+        Material mat = em.find(Material.class, 2L);
+        IndirectList<PlanArbeitsgang> restproduktionsweg = (IndirectList<PlanArbeitsgang>)mat.getRestproduktionsweg();
+        IndirectList<PlanArbeitsgang> restproduktionswegNoCascadeDetach = (IndirectList<PlanArbeitsgang>)mat.getRestproduktionswegNoCascadeDetach();
+        assertFalse(restproduktionsweg.isInstantiated());
+        assertFalse(restproduktionswegNoCascadeDetach.isInstantiated());
+        em.detach(mat);
+        assertTrue(restproduktionsweg.isInstantiated());
+        assertFalse(restproduktionswegNoCascadeDetach.isInstantiated());
+        rollbackTransaction(em);
+        closeEntityManager(em);
+    }
+
     public void testAutoCloseable() {
         EntityManagerFactory emfOuter = null;
         EntityManager emOuter = null;
@@ -13150,7 +13178,7 @@ public void testAutoCloseable() {
         }
     }
 
-    private void InitTestDetachChildObjects(EntityManager em) {
+    private void InitTestDetachChildObjects1(EntityManager em) {
         // test data - manual creation
         try {
             em.createNativeQuery("INSERT INTO MATERIAL (ID, VERSION, IDENT, WERT) VALUES (1, 1, 'MAT', 'WERT2')").executeUpdate();
@@ -13170,6 +13198,18 @@ private void InitTestDetachChildObjects(EntityManager em) {
             fail("Error creating test data: " + e.getMessage());
         } 
     }
+
+    private void InitTestDetachChildObjects2(EntityManager em) {
+        // test data - manual creation
+        try {
+            em.createNativeQuery("INSERT INTO MATERIAL (ID, VERSION, IDENT, WERT) VALUES (2, 2, 'MAT', 'WERT22')").executeUpdate();
+            em.createNativeQuery("INSERT INTO PLANARBEITSGANG (ID, VERSION, NAME, MATERIAL_ID) VALUES (11, 11, 'PAG1', 2)").executeUpdate();
+            em.createNativeQuery("INSERT INTO PLANARBEITSGANG (ID, VERSION, NAME, MATERIAL_ID) VALUES (22, 11, 'PAG2', 2)").executeUpdate();
+        } catch (Exception e) {
+            fail("Error creating test data: " + e.getMessage());
+        }
+    }
+
     private MaterialHist makeHistCopy(Material mat) {
 		MaterialHist histCopy = new MaterialHist();
 		mat.setLastHist(histCopy);