From 9569b1b0d5ad30c4824bbbf5e21c9a1737111f94 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Tue, 8 Nov 2016 09:56:51 +1300 Subject: [PATCH 1/4] Added test for exception safety when using batchOnCascade --- .../TestBatchOnCascadeExceptionHandling.java | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 src/test/java/com/avaje/tests/batchinsert/TestBatchOnCascadeExceptionHandling.java diff --git a/src/test/java/com/avaje/tests/batchinsert/TestBatchOnCascadeExceptionHandling.java b/src/test/java/com/avaje/tests/batchinsert/TestBatchOnCascadeExceptionHandling.java new file mode 100644 index 000000000..8a011199d --- /dev/null +++ b/src/test/java/com/avaje/tests/batchinsert/TestBatchOnCascadeExceptionHandling.java @@ -0,0 +1,105 @@ +package com.avaje.tests.batchinsert; + +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.ebeaninternal.api.SpiTransaction; +import com.avaje.ebeaninternal.server.persist.BatchControl; +import com.avaje.tests.model.basic.EBasicWithUniqueCon; +import com.avaje.tests.model.basic.EOptOneB; +import com.avaje.tests.model.basic.EOptOneC; +import javax.persistence.PersistenceException; +import java.sql.SQLException; +import java.sql.Savepoint; +import org.assertj.core.api.Assertions; +import org.junit.Test; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.StrictAssertions.assertThat; + +public class TestBatchOnCascadeExceptionHandling extends BaseTestCase { + + @Test + public void testBatchScenarioWithSavepoint() throws SQLException { + EbeanServer server = Ebean.getDefaultServer(); + Transaction txn = server.beginTransaction(); + try { + server.save(createEntityWithName("before-savepoint")); + txn.flushBatch(); + Savepoint sp = txn.getConnection().setSavepoint(); + try { + server.save(createEntityWithName("confict")); + server.save(createEntityWithName("confict")); // unique key violation + } catch (PersistenceException e) { + txn.getConnection().rollback(sp); + server.save(createEntityWithName("after-savepoint")); + } + + txn.commit(); + } finally { + txn.end(); + } + + assertThat(server.find(EBasicWithUniqueCon.class).where().eq("name", "before-savepoint").findList()).isNotEmpty(); + assertThat(server.find(EBasicWithUniqueCon.class).where().eq("name", "after-savepoint").findList()).isNotEmpty(); + assertThat(server.find(EBasicWithUniqueCon.class).where().eq("name", "confict").findList()).isEmpty(); + } + + @Test + public void testBatchedInsertFailure() { + final EbeanServer server = Ebean.getDefaultServer(); + server.save(createEntityWithName("foo")); + testBatchOnCascadeIsExceptionSafe(server, () -> { + server.save(createEntityWithName("foo")); // duplicate name on insert + }); + } + + @Test + public void testBatchedUpdateFailure() { + final EbeanServer server = Ebean.getDefaultServer(); + server.save(createEntityWithName("bla")); + final EBasicWithUniqueCon bar = createEntityWithName("bar"); + server.save(bar); + testBatchOnCascadeIsExceptionSafe(server, () -> { + bar.setName("bla"); + server.save(bar); // duplicate name on update + }); + } + + @Test + public void testBatchedDeleteFailure() { + final EbeanServer server = Ebean.getDefaultServer(); + final EOptOneC c = new EOptOneC(); + server.save(c); + final EOptOneB b = new EOptOneB(); + b.setC(c); + server.save(b); + testBatchOnCascadeIsExceptionSafe(server, () -> { + server.delete(c); // foreign key violation + }); + } + + protected void testBatchOnCascadeIsExceptionSafe(EbeanServer server, Runnable failingOperation) { + Transaction txn = server.beginTransaction(); + assertThat(txn.getBatch()).isSameAs(PersistBatch.NONE); + assertThat(txn.getBatchOnCascade()).isSameAs(PersistBatch.ALL); + + try { + failingOperation.run(); + Assertions.fail("PersistenceException expected"); + } catch (PersistenceException e) { + assertThat(txn.getBatch()).as("batch mode").isSameAs(PersistBatch.NONE); // should not have changed + BatchControl bc = ((SpiTransaction) txn).getBatchControl(); + assertThat(bc == null || bc.isEmpty()).as("batch emtpy").isTrue(); + } finally { + txn.end(); + } + } + + protected EBasicWithUniqueCon createEntityWithName(String name) { + EBasicWithUniqueCon it = new EBasicWithUniqueCon(); + it.setName(name); + return it; + } +} From 8661e1fd3f17ed46aeceac80923b969e55bf630d Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Wed, 9 Nov 2016 10:41:51 +1300 Subject: [PATCH 2/4] Reset batch on rollback --- .../ebeaninternal/api/ScopedTransaction.java | 8 +++-- .../ebeaninternal/api/SpiTransaction.java | 6 ++-- .../server/core/PersistRequestBean.java | 16 +++++++++- .../server/persist/BatchControl.java | 18 +++++++---- .../server/persist/BatchedPstmtHolder.java | 12 ++++--- .../server/transaction/JdbcTransaction.java | 31 ++++++++++++++----- 6 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/avaje/ebeaninternal/api/ScopedTransaction.java b/src/main/java/com/avaje/ebeaninternal/api/ScopedTransaction.java index 735675c58..0b437b36d 100644 --- a/src/main/java/com/avaje/ebeaninternal/api/ScopedTransaction.java +++ b/src/main/java/com/avaje/ebeaninternal/api/ScopedTransaction.java @@ -6,11 +6,10 @@ import com.avaje.ebean.bean.PersistenceContext; import com.avaje.ebean.config.PersistBatch; import com.avaje.ebean.event.changelog.BeanChange; import com.avaje.ebean.event.changelog.ChangeSet; +import com.avaje.ebeaninternal.server.core.PersistDeferredRelationship; import com.avaje.ebeaninternal.server.core.PersistRequest; import com.avaje.ebeaninternal.server.core.PersistRequestBean; -import com.avaje.ebeaninternal.server.core.PersistDeferredRelationship; import com.avaje.ebeaninternal.server.persist.BatchControl; - import javax.persistence.PersistenceException; import javax.persistence.RollbackException; import java.io.IOException; @@ -369,6 +368,11 @@ public class ScopedTransaction implements SpiTransaction { transaction.flushBatchOnCascade(); } + @Override + public void flushBatchOnRollback() { + transaction.flushBatchOnRollback(); + } + @Override public void markNotQueryOnly() { transaction.markNotQueryOnly(); diff --git a/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java b/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java index 1a22bfb66..27f130e1d 100644 --- a/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java +++ b/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java @@ -5,11 +5,10 @@ import com.avaje.ebean.annotation.DocStoreMode; import com.avaje.ebean.bean.PersistenceContext; import com.avaje.ebean.event.changelog.BeanChange; import com.avaje.ebean.event.changelog.ChangeSet; +import com.avaje.ebeaninternal.server.core.PersistDeferredRelationship; import com.avaje.ebeaninternal.server.core.PersistRequest; import com.avaje.ebeaninternal.server.core.PersistRequestBean; -import com.avaje.ebeaninternal.server.core.PersistDeferredRelationship; import com.avaje.ebeaninternal.server.persist.BatchControl; - import java.sql.Connection; /** @@ -117,6 +116,7 @@ public interface SpiTransaction extends Transaction { * Returning 0 implies to use the system wide default batch size. *

*/ + @Override int getBatchSize(); /** @@ -235,6 +235,8 @@ public interface SpiTransaction extends Transaction { */ void flushBatchOnCascade(); + void flushBatchOnRollback(); + /** * Mark the transaction explicitly as not being query only. */ diff --git a/src/main/java/com/avaje/ebeaninternal/server/core/PersistRequestBean.java b/src/main/java/com/avaje/ebeaninternal/server/core/PersistRequestBean.java index 0ba67bfeb..71f29d6d1 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/core/PersistRequestBean.java +++ b/src/main/java/com/avaje/ebeaninternal/server/core/PersistRequestBean.java @@ -25,7 +25,6 @@ import com.avaje.ebeaninternal.server.transaction.BeanPersistIdMap; import com.avaje.ebeanservice.docstore.api.DocStoreUpdate; import com.avaje.ebeanservice.docstore.api.DocStoreUpdateContext; import com.avaje.ebeanservice.docstore.api.DocStoreUpdates; - import javax.persistence.OptimisticLockException; import javax.persistence.PersistenceException; import java.io.IOException; @@ -233,6 +232,15 @@ public final class PersistRequestBean extends PersistRequest implements BeanP } } + @Override + public void rollbackTransIfRequired() { + if (batchOnCascadeSet) { + transaction.flushBatchOnRollback(); + batchOnCascadeSet = false; + } + super.rollbackTransIfRequired(); + } + /** * Return true is this request was added to the JDBC batch. */ @@ -366,6 +374,7 @@ public final class PersistRequestBean extends PersistRequest implements BeanP /** * Process the persist request updating the document store. */ + @Override public void docStoreUpdate(DocStoreUpdateContext txn) throws IOException { switch (type) { @@ -387,6 +396,7 @@ public final class PersistRequestBean extends PersistRequest implements BeanP /** * Add this event to the queue entries in IndexUpdates. */ + @Override public void addToQueue(DocStoreUpdates docStoreUpdates) { switch (type) { case INSERT: @@ -550,6 +560,7 @@ public final class PersistRequestBean extends PersistRequest implements BeanP /** * Return the bean associated with this request. */ + @Override public T getBean() { return bean; } @@ -700,6 +711,7 @@ public final class PersistRequestBean extends PersistRequest implements BeanP /** * Set the generated key back to the bean. Only used for inserts with getGeneratedKeys. */ + @Override public void setGeneratedKey(Object idValue) { if (idValue != null) { // remember it for logging summary @@ -718,6 +730,7 @@ public final class PersistRequestBean extends PersistRequest implements BeanP /** * Check for optimistic concurrency exception. */ + @Override public final void checkRowCount(int rowCount) { if (ConcurrencyMode.VERSION == concurrencyMode && rowCount != 1) { String m = Message.msg("persist.conc2", "" + rowCount); @@ -761,6 +774,7 @@ public final class PersistRequestBean extends PersistRequest implements BeanP /** * Post processing. */ + @Override public void postExecute() { changeLog(); diff --git a/src/main/java/com/avaje/ebeaninternal/server/persist/BatchControl.java b/src/main/java/com/avaje/ebeaninternal/server/persist/BatchControl.java index 07927bfef..a6834b0a6 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/persist/BatchControl.java +++ b/src/main/java/com/avaje/ebeaninternal/server/persist/BatchControl.java @@ -1,15 +1,13 @@ package com.avaje.ebeaninternal.server.persist; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.HashMap; - -import javax.persistence.PersistenceException; - import com.avaje.ebeaninternal.api.SpiTransaction; import com.avaje.ebeaninternal.server.core.PersistRequest; import com.avaje.ebeaninternal.server.core.PersistRequestBean; import com.avaje.ebeaninternal.server.deploy.BeanDescriptor; +import javax.persistence.PersistenceException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; /** * Controls the batch ordering of persist requests. @@ -226,6 +224,14 @@ public final class BatchControl { flush(true); } + /** + * Clears the batch, discarding all batched statements. + */ + public void clear() { + pstmtHolder.clear(); + beanHoldMap.clear(); + } + /** * execute all the requests currently queued or batched. */ diff --git a/src/main/java/com/avaje/ebeaninternal/server/persist/BatchedPstmtHolder.java b/src/main/java/com/avaje/ebeaninternal/server/persist/BatchedPstmtHolder.java index c726b7a8f..249910ddf 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/persist/BatchedPstmtHolder.java +++ b/src/main/java/com/avaje/ebeaninternal/server/persist/BatchedPstmtHolder.java @@ -1,11 +1,9 @@ package com.avaje.ebeaninternal.server.persist; +import javax.persistence.PersistenceException; import java.sql.PreparedStatement; import java.sql.SQLException; import java.util.LinkedHashMap; - -import javax.persistence.PersistenceException; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -119,8 +117,7 @@ public class BatchedPstmtHolder { } // clear the batch cache - stmtMap.clear(); - maxSize = 0; + clear(); if (firstError != null) { String msg = "Error when batch flush on sql: " + errorSql; @@ -128,6 +125,11 @@ public class BatchedPstmtHolder { } } + public void clear() { + stmtMap.clear(); + maxSize = 0; + } + /** * Return the size of the biggest batched statement. *

diff --git a/src/main/java/com/avaje/ebeaninternal/server/transaction/JdbcTransaction.java b/src/main/java/com/avaje/ebeaninternal/server/transaction/JdbcTransaction.java index 759fccc9b..256cbf68b 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/transaction/JdbcTransaction.java +++ b/src/main/java/com/avaje/ebeaninternal/server/transaction/JdbcTransaction.java @@ -10,14 +10,11 @@ import com.avaje.ebean.event.changelog.BeanChange; import com.avaje.ebean.event.changelog.ChangeSet; import com.avaje.ebeaninternal.api.SpiTransaction; import com.avaje.ebeaninternal.api.TransactionEvent; +import com.avaje.ebeaninternal.server.core.PersistDeferredRelationship; import com.avaje.ebeaninternal.server.core.PersistRequest; import com.avaje.ebeaninternal.server.core.PersistRequestBean; -import com.avaje.ebeaninternal.server.core.PersistDeferredRelationship; import com.avaje.ebeaninternal.server.lib.util.Str; import com.avaje.ebeaninternal.server.persist.BatchControl; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import javax.persistence.PersistenceException; import javax.persistence.RollbackException; import java.io.IOException; @@ -29,6 +26,8 @@ import java.util.HashSet; import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * JDBC Connection based transaction. @@ -235,6 +234,7 @@ public class JdbcTransaction implements SpiTransaction { return logPrefix; } + @Override public String toString() { return logPrefix; } @@ -324,6 +324,7 @@ public class JdbcTransaction implements SpiTransaction { this.docStoreBatchSize = docStoreBatchSize; } + @Override public DocStoreMode getDocStoreMode() { return docStoreMode; } @@ -400,7 +401,7 @@ public class JdbcTransaction implements SpiTransaction { @Override public boolean isSaveAssocManyIntersection(String intersectionTable, String beanName) { if (m2mIntersectionSave == null) { - // first attempt so yes allow this m2m intersection direction + // first attempt so yes allow this m2m intersection direction m2mIntersectionSave = new HashMap<>(); m2mIntersectionSave.put(intersectionTable, beanName); return true; @@ -412,8 +413,8 @@ public class JdbcTransaction implements SpiTransaction { return true; } - // only allow if save coming from the same bean type - // to stop saves coming from both directions of m2m + // only allow if save coming from the same bean type + // to stop saves coming from both directions of m2m return existingBean.equals(beanName); } @@ -480,6 +481,7 @@ public class JdbcTransaction implements SpiTransaction { this.updateAllLoadedProperties = updateAllLoadedProperties; } + @Override public Boolean isUpdateAllLoadedProperties() { return updateAllLoadedProperties; } @@ -599,6 +601,7 @@ public class JdbcTransaction implements SpiTransaction { } } + @Override public void checkBatchEscalationOnCollection() { if (batchMode == PersistBatch.NONE && batchOnCascadeMode != PersistBatch.NONE) { batchMode = batchOnCascadeMode; @@ -606,6 +609,7 @@ public class JdbcTransaction implements SpiTransaction { } } + @Override public void flushBatchOnCollection() { if (batchOnCascadeSet) { if (batchControl != null) { @@ -634,6 +638,19 @@ public class JdbcTransaction implements SpiTransaction { batchMode = oldBatchMode; } + @Override + public void flushBatchOnRollback() { + if (batchControl != null) { + if (logger.isTraceEnabled()) { + logger.trace("... flushBatchOnRollback"); + } + batchControl.clear(); + } + // restore the previous batch mode + batchMode = oldBatchMode; + } + + @Override public boolean checkBatchEscalationOnCascade(PersistRequestBean request) { if (isBatch(batchMode, request.getType())) { From 5c142a61f6913a9807c252b0be450e885f947e4e Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Fri, 11 Nov 2016 17:11:15 +1300 Subject: [PATCH 3/4] No effective change - add javadoc --- src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java b/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java index 27f130e1d..5224acf57 100644 --- a/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java +++ b/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java @@ -235,6 +235,9 @@ public interface SpiTransaction extends Transaction { */ void flushBatchOnCascade(); + /** + * If batch was on then effectively clear the batch such that we can handle exceptions and continue. + */ void flushBatchOnRollback(); /** From 2953418683ae049b8ee909756eb0daab5292f2ce Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Fri, 11 Nov 2016 17:14:12 +1300 Subject: [PATCH 4/4] No effective change - add .editorconfig --- .editorconfig | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .editorconfig diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 000000000..837d975fa --- /dev/null +++ b/.editorconfig @@ -0,0 +1,12 @@ +# editorconfig.org + +root = true + +[*] +charset = utf-8 +end_of_line = lf +indent_size = 2 +indent_style = space +insert_final_newline = true +trim_trailing_whitespace = true +spaces_around_operators = true