#3665 Fix for delete() Skips Bean due to Hash Collision (#3669)

Change the underlying storage of deleting beans to be by type by id so:

  private Map<Class<?>, Set<Object>> deletingBeans;

Author: rbygrave

ebean-core/src/main/java/io/ebeaninternal/api/SpiTransaction.java

@@ -65,12 +65,12 @@ public interface SpiTransaction extends Transaction {
    * <p>
    * This is to handle bi-directional relationships where both sides Cascade.
    */
-  void registerDeleteBean(Integer hash);
+  void registerDeleteBean(Class<?> type, Object id);
 
   /**
    * Return true if this is a bean that has already been saved/deleted.
    */
-  boolean isRegisteredDeleteBean(Integer hash);
+  boolean isRegisteredDeleteBean(Class<?> type, Object id);
 
   /**
    * Unregister the persisted beans. Expected after persisting top level beans

ebean-core/src/main/java/io/ebeaninternal/api/SpiTransactionProxy.java

@@ -195,13 +195,13 @@ public void registerDeferred(PersistDeferredRelationship derived) {
   }
 
   @Override
-  public void registerDeleteBean(Integer hash) {
-    transaction.registerDeleteBean(hash);
+  public void registerDeleteBean(Class<?> type, Object id) {
+    transaction.registerDeleteBean(type, id);
   }
 
   @Override
-  public boolean isRegisteredDeleteBean(Integer hash) {
-    return transaction.isRegisteredDeleteBean(hash);
+  public boolean isRegisteredDeleteBean(Class<?> type, Object id) {
+    return transaction.isRegisteredDeleteBean(type, id);
   }
 
   @Override

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

@@ -54,10 +54,6 @@ public final class PersistRequestBean<T> extends PersistRequest implements BeanP
    * The unique id used for logging summary.
    */
   private Object idValue;
-  /**
-   * Hash value used to handle cascade delete both ways in a relationship.
-   */
-  private Integer beanHash;
   private boolean statelessUpdate;
   private boolean notifyCache;
   /**
@@ -285,7 +281,9 @@ private void onUpdateGeneratedProperties() {
   private void onFailedUpdateUndoGeneratedProperties() {
     for (BeanProperty prop : beanDescriptor.propertiesGenUpdate()) {
       Object oldVal = intercept.origValue(prop.propertyIndex());
-      prop.setValue(entityBean, oldVal);
+      if (oldVal != null) {
+        prop.setValue(entityBean, oldVal);
+      }
     }
   }
 
@@ -553,34 +551,17 @@ public void unRegisterBean() {
     }
   }
 
-  /**
-   * The hash used to register the bean with the transaction.
-   * <p>
-   * Takes into account the class type and id value.
-   */
-  private Integer beanHash() {
-    if (beanHash == null) {
-      Object id = beanDescriptor.getId(entityBean);
-      int hc = 92821 * bean.getClass().getName().hashCode();
-      if (id != null) {
-        hc += id.hashCode();
-      }
-      beanHash = hc;
-    }
-    return beanHash;
-  }
-
   public void registerDeleteBean() {
-    Integer hash = beanHash();
-    transaction.registerDeleteBean(hash);
+    final Object id = beanDescriptor.id(entityBean);
+    transaction.registerDeleteBean(beanDescriptor.type(), id);
   }
 
   public boolean isRegisteredForDeleteBean() {
     if (transaction == null) {
       return false;
     } else {
-      Integer hash = beanHash();
-      return transaction.isRegisteredDeleteBean(hash);
+      final Object id = beanDescriptor.id(entityBean);
+      return transaction.isRegisteredDeleteBean(beanDescriptor.type(), id);
     }
   }
 
@@ -809,7 +790,9 @@ public void setBoundId(Object idValue) {
   public void checkRowCount(int rowCount) {
     if (rowCount != 1 && rowCount != Statement.SUCCESS_NO_INFO) {
       if (ConcurrencyMode.VERSION == concurrencyMode) {
-        onFailedUpdateUndoGeneratedProperties();
+        if (type == Type.UPDATE) {
+          onFailedUpdateUndoGeneratedProperties();
+        }
         throw new OptimisticLockException("Data has changed. updated row count " + rowCount, null, bean);
       } else if (rowCount == 0 && type == Type.UPDATE) {
         throw new EntityNotFoundException("No rows updated");

ebean-core/src/main/java/io/ebeaninternal/server/transaction/ImplicitReadOnlyTransaction.java

@@ -200,15 +200,15 @@ public void registerDeferred(PersistDeferredRelationship derived) {
   }
 
   @Override
-  public void registerDeleteBean(Integer persistingBean) {
+  public void registerDeleteBean(Class<?> type, Object id) {
     throw new IllegalStateException(notExpectedMessage);
   }
 
   /**
    * Return true if this is a bean that has already been saved/deleted.
    */
   @Override
-  public boolean isRegisteredDeleteBean(Integer persistingBean) {
+  public boolean isRegisteredDeleteBean(Class<?> type, Object id) {
     return false;
   }
 

ebean-core/src/main/java/io/ebeaninternal/server/transaction/JdbcTransaction.java

@@ -68,7 +68,7 @@ class JdbcTransaction implements SpiTransaction, TxnProfileEventCodes {
   private int depth;
   private boolean autoCommit;
   private IdentityHashMap<Object, Object> persistingBeans;
-  private HashSet<Integer> deletingBeansHash;
+  private Map<Class<?>, Set<Object>> deletingBeans;
   private HashMap<String, String> m2mIntersectionSave;
   private Map<String, Object> userObjects;
   private List<TransactionCallback> callbackList;
@@ -328,40 +328,28 @@ public final void registerDeferred(PersistDeferredRelationship derived) {
     deferredList.add(derived);
   }
 
-  /**
-   * Add a bean to the registed list.
-   * <p>
-   * This is to handle bi-directional relationships where both sides Cascade.
-   * </p>
-   */
   @Override
-  public final void registerDeleteBean(Integer persistingBean) {
-    if (deletingBeansHash == null) {
-      deletingBeansHash = new HashSet<>();
-    }
-    deletingBeansHash.add(persistingBean);
+  public final void registerDeleteBean(Class<?> type, Object id) {
+    deleteBeanIds(type).add(id);
   }
 
-  /**
-   * Return true if this is a bean that has already been saved/deleted.
-   */
   @Override
-  public final boolean isRegisteredDeleteBean(Integer persistingBean) {
-    return deletingBeansHash != null && deletingBeansHash.contains(persistingBean);
+  public final boolean isRegisteredDeleteBean(Class<?> type, Object id) {
+    return deleteBeanIds(type).contains(id);
+  }
+
+  private Set<Object> deleteBeanIds(Class<?> type) {
+    if (deletingBeans == null) {
+      deletingBeans = new HashMap<>();
+    }
+    return deletingBeans.computeIfAbsent(type, k -> new HashSet<>());
   }
 
-  /**
-   * Unregister the persisted beans (when persisting at the top level).
-   */
   @Override
   public final void unregisterBeans() {
     persistingBeans.clear();
   }
 
-  /**
-   * Return true if this is a bean that has already been saved. This will
-   * register the bean if it is not already.
-   */
   @Override
   public final boolean isRegisteredBean(Object bean) {
     if (persistingBeans == null) {
@@ -370,10 +358,6 @@ public final boolean isRegisteredBean(Object bean) {
     return (persistingBeans.put(bean, PLACEHOLDER) != null);
   }
 
-  /**
-   * Return true if the m2m intersection save is allowed from a given bean direction.
-   * This is to stop m2m intersection management via both directions of a m2m.
-   */
   @Override
   public final boolean isSaveAssocManyIntersection(String intersectionTable, String beanName) {
     if (m2mIntersectionSave == null) {

ebean-core/src/main/java/io/ebeaninternal/server/transaction/NoTransaction.java

@@ -154,12 +154,12 @@ public void registerDeferred(PersistDeferredRelationship derived) {
   }
 
   @Override
-  public void registerDeleteBean(Integer hash) {
+  public void registerDeleteBean(Class<?> type, Object id) {
 
   }
 
   @Override
-  public boolean isRegisteredDeleteBean(Integer hash) {
+  public boolean isRegisteredDeleteBean(Class<?> type, Object id) {
     return false;
   }
 

ebean-test/src/test/java/org/tests/iud/TestCarWheelIud.java

@@ -56,7 +56,6 @@ public void test() {
     Car car2 = DB.find(Car.class, car.getId());
 
     DB.delete(car2);
-
   }
 
   @Test