#1833 - When cascade persisting twice to ElementCollection ... we get do extra delete and inserts

src/main/java/io/ebeaninternal/api/SpiTransaction.java

@@ -75,9 +75,10 @@ public interface SpiTransaction extends Transaction {
   boolean isRegisteredDeleteBean(Integer hash);
 
   /**
-   * Unregister the persisted bean.
+   * Unregister the persisted beans. Expected after persisting top level beans
+   * and hence including all beans that were cascade persisted.
    */
-  void unregisterBean(Object bean);
+  void unregisterBeans();
 
   /**
    * Return true if this is a bean that has already been persisted in the

src/main/java/io/ebeaninternal/api/SpiTransactionProxy.java

@@ -189,8 +189,8 @@ public abstract class SpiTransactionProxy implements SpiTransaction {
   }
 
   @Override
-  public void unregisterBean(Object bean) {
-    transaction.unregisterBean(bean);
+  public void unregisterBeans() {
+    transaction.unregisterBeans();
   }
 
   @Override

src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java

@@ -83,6 +83,8 @@ public final class PersistRequestBean<T> extends PersistRequest implements BeanP
 
   private int flags;
 
+  private boolean saveRecurse;
+
   private DocStoreMode docStoreMode;
 
   private final ConcurrencyMode concurrencyMode;
@@ -529,8 +531,6 @@ public final class PersistRequestBean<T> extends PersistRequest implements BeanP
   public void addToQueue(DocStoreUpdates docStoreUpdates) {
     switch (type) {
       case INSERT:
-        docStoreUpdates.queueIndex(beanDescriptor.getDocStoreQueueId(), idValue);
-        break;
       case UPDATE:
       case DELETE_SOFT:
         docStoreUpdates.queueIndex(beanDescriptor.getDocStoreQueueId(), idValue);
@@ -585,7 +585,10 @@ public final class PersistRequestBean<T> extends PersistRequest implements BeanP
   }
 
   public void unRegisterBean() {
-    transaction.unregisterBean(bean);
+    if (!saveRecurse) {
+      // only clear all persisted beans when persisting at the top level
+      transaction.unregisterBeans();
+    }
   }
 
   /**
@@ -1464,4 +1467,11 @@ public final class PersistRequestBean<T> extends PersistRequest implements BeanP
   public boolean isForcedUpdate() {
     return Flags.isUpdateForce(flags);
   }
+
+  /**
+   * Set when this request is from cascading persist.
+   */
+  public void setSaveRecurse() {
+    saveRecurse = true;
+  }
 }

src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java

@@ -473,6 +473,7 @@ public final class DefaultPersister implements Persister {
   }
 
   private void saveRecurse(PersistRequestBean<?> request) {
+    request.setSaveRecurse();
     if (request.isReference()) {
       // its a reference...
       if (request.isPersistCascade()) {

src/main/java/io/ebeaninternal/server/transaction/ImplicitReadOnlyTransaction.java

@@ -201,7 +201,7 @@ class ImplicitReadOnlyTransaction implements SpiTransaction, TxnProfileEventCode
   }
 
   @Override
-  public void unregisterBean(Object bean) {
+  public void unregisterBeans() {
     throw new IllegalStateException(notExpectedMessage);
   }
 
@@ -280,7 +280,6 @@ class ImplicitReadOnlyTransaction implements SpiTransaction, TxnProfileEventCode
 
   @Override
   public void setBatchMode(boolean batchMode) {
-
   }
 
   @Override

src/main/java/io/ebeaninternal/server/transaction/JdbcTransaction.java

@@ -446,11 +446,11 @@ class JdbcTransaction implements SpiTransaction, TxnProfileEventCodes {
   }
 
   /**
-   * Unregister the persisted bean.
+   * Unregister the persisted beans (when persisting at the top level).
    */
   @Override
-  public void unregisterBean(Object bean) {
-    persistingBeans.remove(bean);
+  public void unregisterBeans() {
+    persistingBeans.clear();
   }
 
   /**

src/main/java/io/ebeaninternal/server/transaction/NoTransaction.java

@@ -129,8 +129,7 @@ class NoTransaction implements SpiTransaction {
   }
 
   @Override
-  public void unregisterBean(Object bean) {
-
+  public void unregisterBeans() {
   }
 
   @Override
@@ -160,7 +159,6 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void register(TransactionCallback callback) {
-
   }
 
   @Override
@@ -170,12 +168,10 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setReadOnly(boolean readOnly) {
-
   }
 
   @Override
   public void setRollbackOnly() {
-
   }
 
   @Override
@@ -186,27 +182,22 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setDocStoreMode(DocStoreMode mode) {
-
   }
 
   @Override
   public void setDocStoreBatchSize(int batchSize) {
-
   }
 
   @Override
   public void setPersistCascade(boolean persistCascade) {
-
   }
 
   @Override
   public void setUpdateAllLoadedProperties(boolean updateAllLoadedProperties) {
-
   }
 
   @Override
   public void setSkipCache(boolean skipCache) {
-
   }
 
   @Override
@@ -216,7 +207,6 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setBatchMode(boolean useBatch) {
-
   }
 
   @Override
@@ -235,7 +225,6 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setBatchSize(int batchSize) {
-
   }
 
   @Override
@@ -245,17 +234,14 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setBatchGetGeneratedKeys(boolean getGeneratedKeys) {
-
   }
 
   @Override
   public void setBatchFlushOnMixed(boolean batchFlushOnMixed) {
-
   }
 
   @Override
   public void setBatchFlushOnQuery(boolean batchFlushOnQuery) {
-
   }
 
   @Override
@@ -265,12 +251,10 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void flush() throws PersistenceException {
-
   }
 
   @Override
   public void flushBatch() throws PersistenceException {
-
   }
 
   @Override
@@ -280,12 +264,10 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void addModification(String tableName, boolean inserts, boolean updates, boolean deletes) {
-
   }
 
   @Override
   public void putUserObject(String name, Object value) {
-
   }
 
   @Override
@@ -300,7 +282,6 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void depth(int diff) {
-
   }
 
   @Override
@@ -335,7 +316,6 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setBatchControl(BatchControl control) {
-
   }
 
   @Override
@@ -345,7 +325,6 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setPersistenceContext(PersistenceContext context) {
-
   }
 
   @Override
@@ -365,12 +344,10 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void flushBatchOnCascade() {
-
   }
 
   @Override
   public void flushBatchOnRollback() {
-
   }
 
   @Override
@@ -380,27 +357,22 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void markNotQueryOnly() {
-
   }
 
   @Override
   public void checkBatchEscalationOnCollection() {
-
   }
 
   @Override
   public void flushBatchOnCollection() {
-
   }
 
   @Override
   public void addBeanChange(BeanChange beanChange) {
-
   }
 
   @Override
   public void sendChangeLog(ChangeSet changeSet) {
-
   }
 
   @Override
@@ -410,7 +382,6 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setTenantId(Object tenantId) {
-
   }
 
   @Override
@@ -425,12 +396,10 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void profileEvent(SpiProfileTransactionEvent event) {
-
   }
 
   @Override
   public void setProfileStream(ProfileStream profileStream) {
-
   }
 
   @Override
@@ -440,7 +409,6 @@ class NoTransaction implements SpiTransaction {
 
   @Override
   public void setProfileLocation(ProfileLocation profileLocation) {
-
   }
 
   @Override

src/test/java/org/tests/model/elementcollection/EcTop.java

@@ -0,0 +1,71 @@
+package org.tests.model.elementcollection;
+
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.ManyToMany;
+import javax.persistence.ManyToOne;
+import javax.persistence.Version;
+import java.util.List;
+
+import static javax.persistence.CascadeType.ALL;
+
+@Entity
+public class EcTop {
+
+  @Id
+  private long id;
+
+  @Version
+  private long version;
+
+  private final String name;
+
+  @ManyToOne(cascade = ALL)
+  private EcsPerson person;
+
+  /**
+   * Not normally expect cascade on ManyToMany but here for this test.
+   */
+  @ManyToMany(cascade = ALL)
+  private List<EcsPerson> people;
+
+  public EcTop(String name) {
+    this.name = name;
+  }
+
+  public long getId() {
+    return id;
+  }
+
+  public void setId(long id) {
+    this.id = id;
+  }
+
+  public long getVersion() {
+    return version;
+  }
+
+  public void setVersion(long version) {
+    this.version = version;
+  }
+
+  public String getName() {
+    return name;
+  }
+
+  public EcsPerson getPerson() {
+    return person;
+  }
+
+  public void setPerson(EcsPerson person) {
+    this.person = person;
+  }
+
+  public List<EcsPerson> getPeople() {
+    return people;
+  }
+
+  public void setPeople(List<EcsPerson> people) {
+    this.people = people;
+  }
+}

src/test/java/org/tests/model/elementcollection/TestElementCollectionCascadeMultiple.java

@@ -0,0 +1,51 @@
+package org.tests.model.elementcollection;
+
+import io.ebean.BaseTestCase;
+import io.ebean.DB;
+import io.ebean.annotation.Transactional;
+import org.ebeantest.LoggedSqlCollector;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class TestElementCollectionCascadeMultiple extends BaseTestCase {
+
+  @Test
+  public void test() {
+
+    EcTop top = new EcTop("top0");
+
+    EcsPerson person = new EcsPerson("Ethan027");
+    person.getPhoneNumbers().add("027 1234");
+    person.getPhoneNumbers().add("027 4321");
+
+    top.setPerson(person);
+    top.getPeople().add(person);
+
+    LoggedSqlCollector.start();
+
+    save(top);
+
+    final List<String> sql = LoggedSqlCollector.stop();
+
+    assertThat(sql.get(0)).contains("insert into ecs_person");
+    assertThat(sql.get(1)).contains("-- bind");
+    assertThat(sql.get(2)).contains("insert into ecs_person_phone");
+    assertThat(sql.get(3)).contains("-- bind");
+    assertThat(sql.get(4)).contains("-- bind");
+    assertThat(sql.get(5)).contains("insert into ec_top");
+    assertThat(sql.get(6)).contains("-- bind");
+    assertThat(sql.get(7)).contains("insert into ec_top_ecs_person");
+    assertThat(sql.get(8)).contains("-- bind");
+
+    assertThat(sql).hasSize(9);
+  }
+
+  @Transactional(batchSize = 20)
+  public void save(EcTop top) {
+    DB.save(top);
+  }
+
+}

src/test/resources/logback-test.xml

@@ -79,8 +79,8 @@
   <logger name="io.ebean.docker" level="TRACE"/>
 <!--  <logger name="io.ebean.DDL" level="DEBUG"/>-->
 
-<!--  <logger name="io.ebean.SQL" level="TRACE"/> -->
-<!--  <logger name="io.ebean.TXN" level="TRACE"/> -->
+  <logger name="io.ebean.SQL" level="TRACE"/>
+  <logger name="io.ebean.TXN" level="TRACE"/>
 <!--  <logger name="io.ebean.SUM" level="TRACE"/> -->
 
 <!--  <logger name="io.ebean.cache" level="TRACE"/> -->