#467 - Delete bean with unloaded version property throws OptimisticLockException - API Change, return boolean

src/main/java/com/avaje/ebean/Ebean.java

@@ -697,12 +697,20 @@ public final class Ebean {
   /**
    * Delete the bean.
    * <p>
+   * This will return true if the bean was deleted successfully or JDBC batch is being used.
+   * </p>
+   * <p>
    * If there is no current transaction one will be created and committed for
    * you automatically.
    * </p>
+   * <p>
+   * If the Bean does not have a version property (or loaded version property) and
+   * the bean does not exist then this returns false indicating that nothing was
+   * deleted. Note that, if JDBC batch mode is used then this always returns true.
+   * </p>
    */
-  public static void delete(Object bean) throws OptimisticLockException {
-    serverMgr.getDefaultServer().delete(bean);
+  public static boolean delete(Object bean) throws OptimisticLockException {
+    return serverMgr.getDefaultServer().delete(bean);
   }
 
   /**

src/main/java/com/avaje/ebean/EbeanServer.java

@@ -1246,16 +1246,32 @@ public interface EbeanServer {
   /**
    * Delete the bean.
    * <p>
+   * This will return true if the bean was deleted successfully or JDBC batch is being used.
+   * </p>
+   * <p>
    * If there is no current transaction one will be created and committed for
    * you automatically.
    * </p>
+   * <p>
+   * If the Bean does not have a version property (or loaded version property) and
+   * the bean does not exist then this returns false indicating that nothing was
+   * deleted. Note that, if JDBC batch mode is used then this always returns true.
+   * </p>
    */
-  void delete(Object bean) throws OptimisticLockException;
+  boolean delete(Object bean) throws OptimisticLockException;
 
   /**
    * Delete the bean with an explicit transaction.
+   * <p>
+   * This will return true if the bean was deleted successfully or JDBC batch is being used.
+   * </p>
+   * <p>
+   * If the Bean does not have a version property (or loaded version property) and
+   * the bean does not exist then this returns false indicating that nothing was
+   * deleted. However, if JDBC batch mode is used then this always returns true.
+   * </p>
    */
-  void delete(Object bean, Transaction transaction) throws OptimisticLockException;
+  boolean delete(Object bean, Transaction transaction) throws OptimisticLockException;
 
   /**
    * Delete the bean given its type and id.

src/main/java/com/avaje/ebean/Model.java

@@ -251,12 +251,24 @@ public abstract class Model {
   }
 
   /**
-   * Delete this entity.
+   * Delete this bean.
+   * <p>
+   * This will return true if the bean was deleted successfully or JDBC batch is being used.
+   * </p>
+   * <p>
+   * If there is no current transaction one will be created and committed for
+   * you automatically.
+   * </p>
+   * <p>
+   * If the Bean does not have a version property (or loaded version property) and
+   * the bean does not exist then this returns false indicating that nothing was
+   * deleted. Note that, if JDBC batch mode is used then this always returns true.
+   * </p>
    *
    * @see EbeanServer#delete(Object)
    */
-  public void delete() {
-    db().delete(this);
+  public boolean delete() {
+    return db().delete(this);
   }
 
   /**
@@ -276,8 +288,8 @@ public abstract class Model {
   /**
    * Perform a delete using this entity against the specified server.
    */
-  public void delete(String server) {
-    db(server).delete(this);
+  public boolean delete(String server) {
+    return db(server).delete(this);
   }
 
   /**

src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java

@@ -1867,16 +1867,16 @@ public final class DefaultServer implements SpiServer, SpiEbeanServer {
   /**
    * Delete the bean.
    */
-  public void delete(Object bean) {
-    delete(bean, null);
+  public boolean delete(Object bean) {
+    return delete(bean, null);
   }
 
   /**
    * Delete the bean with the explicit transaction.
    */
-  public void delete(Object bean, Transaction t) {
+  public boolean delete(Object bean, Transaction t) {
     
-    persister.delete(checkEntityBean(bean), t);
+    return persister.delete(checkEntityBean(bean), t);
   }
 
   /**

src/main/java/com/avaje/ebeaninternal/server/core/PersistRequestBean.java

@@ -480,8 +480,7 @@ public final class PersistRequestBean<T> extends PersistRequest implements BeanP
         return -1;
 
       case DELETE:
-        persistExecute.executeDeleteBean(this);
-        return -1;
+        return persistExecute.executeDeleteBean(this);
 
       default:
         throw new RuntimeException("Invalid type " + type);

src/main/java/com/avaje/ebeaninternal/server/core/Persister.java

@@ -66,7 +66,7 @@ public interface Persister {
   /**
    * Delete the bean.
    */
-  void delete(EntityBean entityBean, Transaction t);
+  boolean delete(EntityBean entityBean, Transaction t);
 
   /**
    * Delete multiple beans given a collection of Id values.

src/main/java/com/avaje/ebeaninternal/server/persist/BeanPersister.java

@@ -22,6 +22,6 @@ public interface BeanPersister {
   /**
    * execute the delete bean request.
    */
-  void delete(PersistRequestBean<?> request) throws PersistenceException;
+  int delete(PersistRequestBean<?> request) throws PersistenceException;
 
 }

src/main/java/com/avaje/ebeaninternal/server/persist/DefaultPersistExecute.java

@@ -73,15 +73,17 @@ public final class DefaultPersistExecute implements PersistExecute {
   /**
    * execute the bean delete request.
    */
-  public <T> void executeDeleteBean(PersistRequestBean<T> request) {
+  public <T> int executeDeleteBean(PersistRequestBean<T> request) {
 
     BeanManager<T> mgr = request.getBeanManager();
     BeanPersister persister = mgr.getBeanPersister();
 
     BeanPersistController controller = request.getBeanController();
     if (controller == null || controller.preDelete(request)) {
-      persister.delete(request);
+      return persister.delete(request);
     }
+    // delete handled by the BeanController so return 0
+    return 0;
   }
 
   /**

src/main/java/com/avaje/ebeaninternal/server/persist/DefaultPersister.java

@@ -498,24 +498,25 @@ public final class DefaultPersister implements Persister {
 
   /**
    * Delete the bean with the explicit transaction.
+   * Return false if the delete is executed without OCC and 0 rows were deleted.
    */
-  public void delete(EntityBean bean, Transaction t) {
+  public boolean delete(EntityBean bean, Transaction t) {
 
     PersistRequestBean<EntityBean> request = createRequest(bean, t, Type.DELETE);
-    deleteRequest(request);
+    boolean deleted = deleteRequest(request);
 
     if (request.isDraftable()) {
       // we have just deleting a draft bean so now we need to delete the
       // associated 'live' bean. This is effectively an 'automatic publish'.
-      try {
-        deleteRequest(createRequest(request.createReference(), t, Type.DELETE, true));
-      } catch (OptimisticLockException e) {
-        SUM.debug("Ignore OptimisticLockException - did not delete live row as draft not published for bean:{} id:{}", request.getFullName(), request.getBeanId());
-      }
+      deleteRequest(createRequest(request.createReference(), t, Type.DELETE, true));
     }
+    return deleted;
   }
 
-  private void deleteRequest(PersistRequestBean<?> req) {
+  /**
+   * Execute the delete request returning true if a delete occurred.
+   */
+  private boolean deleteRequest(PersistRequestBean<?> req) {
 
     if (req.isRegisteredForDeleteBean()) {
       // skip deleting bean. Used where cascade is on
@@ -523,15 +524,17 @@ public final class DefaultPersister implements Persister {
       if (logger.isDebugEnabled()) {
         logger.debug("skipping delete on alreadyRegistered " + req.getBean());
       }
-      return;
+      return false;
     }
 
     try {
       req.initTransIfRequiredWithBatchCascade();
-      delete(req);
+      boolean deleted = delete(req);
       req.commitTransIfRequired();
       req.flushBatchOnCascade();
 
+      return deleted;
+
     } catch (RuntimeException ex) {
       req.rollbackTransIfRequired();
       throw ex;
@@ -710,7 +713,7 @@ public final class DefaultPersister implements Persister {
    * Note that preDelete fires before the deletion of children.
    * </p>
    */
-  private void delete(PersistRequestBean<?> request) {
+  private boolean delete(PersistRequestBean<?> request) {
 
     DeleteUnloadedForeignKeys unloadedForeignKeys = null;
 
@@ -729,7 +732,7 @@ public final class DefaultPersister implements Persister {
       }
     }
 
-    request.executeOrQueue();
+    int count = request.executeOrQueue();
 
     if (request.isPersistCascade()) {
       deleteAssocOne(request);
@@ -739,6 +742,8 @@ public final class DefaultPersister implements Persister {
       }
     }
 
+    // return true if using JDBC batch (as we can't tell until the batch is flushed)
+    return count != 0;
   }
 
   /**

src/main/java/com/avaje/ebeaninternal/server/persist/PersistExecute.java

@@ -33,7 +33,7 @@ public interface PersistExecute {
   /**
    * Execute a Bean (or MapBean) delete.
    */
-  <T> void executeDeleteBean(PersistRequestBean<T> request);
+  <T> int executeDeleteBean(PersistRequestBean<T> request);
 
   /**
    * Execute a Update.

src/main/java/com/avaje/ebeaninternal/server/persist/dml/DeleteHandler.java

@@ -46,9 +46,10 @@ public class DeleteHandler extends DmlHandler {
 	 * Execute the delete non-batch.
 	 */
   @Override
-	public void execute() throws SQLException, OptimisticLockException {
+	public int execute() throws SQLException, OptimisticLockException {
 		int rowCount = dataBind.executeUpdate();
 		checkRowCount(rowCount);
+    return rowCount;
 	}
 
   @Override

src/main/java/com/avaje/ebeaninternal/server/persist/dml/DmlBeanPersister.java

@@ -35,10 +35,10 @@ public final class DmlBeanPersister implements BeanPersister {
 	/**
 	 * execute the bean delete request.
 	 */
-	public void delete(PersistRequestBean<?> request) {
+	public int delete(PersistRequestBean<?> request) {
 
 		DeleteHandler delete = new DeleteHandler(request, deleteMeta);
-		execute(request, delete);
+		return execute(request, delete);
 	}
 
 	/**
@@ -62,15 +62,17 @@ public final class DmlBeanPersister implements BeanPersister {
 	/**
 	 * execute request taking batching into account.
 	 */
-	private void execute(PersistRequestBean<?> request, PersistHandler handler) {
+	private int execute(PersistRequestBean<?> request, PersistHandler handler) {
 
     boolean batched = request.isBatched();
 		try {
 			handler.bind();
 			if (batched) {
 				handler.addBatch();
+        return -1;
+
 			} else {
-				handler.execute();
+				return handler.execute();
 			}
 
 		} catch (SQLException e) {

src/main/java/com/avaje/ebeaninternal/server/persist/dml/DmlHandler.java

@@ -75,7 +75,7 @@ public abstract class DmlHandler implements PersistHandler, BindableRequest {
    * Execute now for non-batch execution.
    */
   @Override
-  public abstract void execute() throws SQLException;
+  public abstract int execute() throws SQLException;
 
   /**
    * Check the rowCount.

src/main/java/com/avaje/ebeaninternal/server/persist/dml/InsertHandler.java

@@ -125,8 +125,8 @@ public class InsertHandler extends DmlHandler {
    * getGeneratedKeys if required.
    */
   @Override
-  public void execute() throws SQLException, OptimisticLockException {
-    int rc = dataBind.executeUpdate();
+  public int execute() throws SQLException, OptimisticLockException {
+    int rowCount = dataBind.executeUpdate();
     if (useGeneratedKeys) {
       // get the auto-increment value back and set into the bean
       getGeneratedKeys();
@@ -136,8 +136,9 @@ public class InsertHandler extends DmlHandler {
       fetchGeneratedKeyUsingSelect();
     }
 
-    checkRowCount(rc);
+    checkRowCount(rowCount);
     executeDerivedRelationships();
+    return rowCount;
   }
 
   protected void executeDerivedRelationships() {
@@ -210,16 +211,14 @@ public class InsertHandler extends DmlHandler {
           rset.close();
         }
       } catch (SQLException ex) {
-        String msg = "Error closing rset for fetchGeneratedKeyUsingSelect?";
-        logger.warn(msg, ex);
+        logger.warn("Error closing ResultSet for fetchGeneratedKeyUsingSelect?", ex);
       }
       try {
         if (stmt != null) {
           stmt.close();
         }
       } catch (SQLException ex) {
-        String msg = "Error closing stmt for fetchGeneratedKeyUsingSelect?";
-        logger.warn(msg, ex);
+        logger.warn("Error closing Statement for fetchGeneratedKeyUsingSelect?", ex);
       }
     }
   }

src/main/java/com/avaje/ebeaninternal/server/persist/dml/PersistHandler.java

@@ -25,7 +25,7 @@ public interface PersistHandler {
   /**
    * Execute now for non-batch execution.
    */
-  void execute() throws SQLException;
+  int execute() throws SQLException;
 
   /**
    * Close resources including underlying preparedStatement.

src/main/java/com/avaje/ebeaninternal/server/persist/dml/UpdateHandler.java

@@ -67,11 +67,13 @@ public class UpdateHandler extends DmlHandler {
    * Execute the update in non-batch.
    */
   @Override
-  public void execute() throws SQLException, OptimisticLockException {
+  public int execute() throws SQLException, OptimisticLockException {
     if (!emptySetClause) {
       int rowCount = dataBind.executeUpdate();
       checkRowCount(rowCount);
+      return rowCount;
     }
+    return 0;
   }
 
   @Override

src/test/java/com/avaje/ebeaninternal/api/TDSpiEbeanServer.java

@@ -548,8 +548,8 @@ public class TDSpiEbeanServer implements SpiEbeanServer {
   }
 
   @Override
-  public void delete(Object bean) throws OptimisticLockException {
-
+  public boolean delete(Object bean) throws OptimisticLockException {
+    return false;
   }
 
   @Override
@@ -658,8 +658,8 @@ public class TDSpiEbeanServer implements SpiEbeanServer {
   }
 
   @Override
-  public void delete(Object bean, Transaction t) throws OptimisticLockException {
-
+  public boolean delete(Object bean, Transaction t) throws OptimisticLockException {
+    return false;
   }
 
   @Override

src/test/java/com/avaje/tests/delete/TestDeleteWithoutOptimisticLocking.java

@@ -2,19 +2,69 @@ package com.avaje.tests.delete;
 
 import com.avaje.ebean.BaseTestCase;
 import com.avaje.ebean.Ebean;
+import com.avaje.ebean.EbeanServer;
+import com.avaje.ebean.Transaction;
+import com.avaje.ebean.config.PersistBatch;
 import com.avaje.tests.model.basic.Contact;
+import com.avaje.tests.model.basic.EBasicVer;
+import com.avaje.tests.model.converstation.Group;
 import org.junit.Test;
 
+import static org.assertj.core.api.StrictAssertions.assertThat;
+
 public class TestDeleteWithoutOptimisticLocking extends BaseTestCase {
 
   @Test
-  public void test() {
+  public void testSimpleBeanDelete_missingBean_returnsFalse() {
 
     // delete by by without version loaded ... should not throw OptimisticLockException
     Contact ref = Ebean.getReference(Contact.class, 999999);
-    Ebean.delete(ref);
+    assertThat(Ebean.delete(ref)).isFalse();
 
-    Ebean.delete(Contact.class, 999999);
+    assertThat(Ebean.delete(Contact.class, 999999)).isEqualTo(0);
+
+    // same as above but using Model.delete()
+    Group modelRef = Ebean.getReference(Group.class, 999999);
+    assertThat(modelRef.delete()).isFalse();
+  }
+
+  @Test
+  public void testSimpleBeanDelete_existingBean_returnsTrue() {
+
+    EBasicVer basic = new EBasicVer();
+    basic.setName("DelTest");
+    Ebean.save(basic);
+
+    EBasicVer basicRef = Ebean.getReference(EBasicVer.class, basic.getId());
+    assertThat(Ebean.delete(basicRef)).isTrue();
+  }
+
+
+  @Test
+  public void testSimpleBeanDelete_existingBeanWithJdbcBatch_returnsTrue() {
+
+    EBasicVer basic = new EBasicVer();
+    basic.setName("DelTestBatch");
+    Ebean.save(basic);
+
+    EbeanServer server = Ebean.getDefaultServer();
+    Transaction transaction = server.beginTransaction();
+    try {
+      transaction.setBatch(PersistBatch.ALL);
+
+      // returns true even though the delete has not occurred yet
+      assertThat(server.delete(Ebean.getReference(EBasicVer.class, basic.getId()), transaction)).isTrue();
+
+      // returns true even though the bean does not exist
+      assertThat(server.delete(Ebean.getReference(EBasicVer.class, 999999), transaction)).isTrue();
+
+      transaction.commit();
+
+      assertThat(Ebean.find(EBasicVer.class, basic.getId())).isNull();
+
+    } finally {
+      transaction.end();
+    }
   }
 
 }