From 1631cdde647e9a3321eebb18a911e7955f7bd727 Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Wed, 2 Jul 2025 11:33:22 +0200 Subject: [PATCH 1/3] Undo generated properties on general persistenceExceptions With #2372 we introduced a rollback mechanism when an optimisticLock happend, so that you can correct the issue and retry. This PR also handles the situation for other persistence exceptions like DuplicateKey --- .../server/core/PersistRequestBean.java | 2 +- .../server/persist/BatchPostExecute.java | 7 +++ .../server/persist/BatchedPstmt.java | 7 ++- .../server/persist/dml/DmlBeanPersister.java | 1 + .../tests/insert/TestInsertDuplicateKey.java | 59 +++++++++++++++++++ 5 files changed, 74 insertions(+), 2 deletions(-) diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java b/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java index 93358d0c6d..8df5eba7dc 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java @@ -282,7 +282,7 @@ private void onUpdateGeneratedProperties() { } } - private void onFailedUpdateUndoGeneratedProperties() { + public void onFailedUpdateUndoGeneratedProperties() { for (BeanProperty prop : beanDescriptor.propertiesGenUpdate()) { Object oldVal = intercept.origValue(prop.propertyIndex()); prop.setValue(entityBean, oldVal); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchPostExecute.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchPostExecute.java index c17be92d15..d649fcd600 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchPostExecute.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchPostExecute.java @@ -45,4 +45,11 @@ public interface BatchPostExecute { * Add timing metrics for batch persist. */ void addTimingBatch(long startNanos, int batch); + + /** + * Undos generated properties. + */ + default void onFailedUpdateUndoGeneratedProperties() { + + } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmt.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmt.java index e39f79629d..55245fdb8c 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmt.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmt.java @@ -155,7 +155,12 @@ private void postExecute() { private void executeAndCheckRowCounts() throws SQLException { try { - results = pstmt.executeBatch(); + try { + results = pstmt.executeBatch(); + } catch (SQLException ex) { + list.forEach(BatchPostExecute::onFailedUpdateUndoGeneratedProperties); + throw ex; + } if (transaction.isLogSql()) { transaction.logSql(" -- executeBatch() size:{0} sql:{1}", results.length, sql); } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlBeanPersister.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlBeanPersister.java index 4a4abe17d5..e3a7345f39 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlBeanPersister.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlBeanPersister.java @@ -74,6 +74,7 @@ private int execute(PersistRequestBean request, PersistHandler handler) { if (request.transaction().isLogSummary()) { request.transaction().logSummary(msg); } + request.onFailedUpdateUndoGeneratedProperties(); throw dbPlatform.translate(msg, e); } finally { if (!batched) { diff --git a/ebean-test/src/test/java/org/tests/insert/TestInsertDuplicateKey.java b/ebean-test/src/test/java/org/tests/insert/TestInsertDuplicateKey.java index b894d4de6a..aa00a0dcf4 100644 --- a/ebean-test/src/test/java/org/tests/insert/TestInsertDuplicateKey.java +++ b/ebean-test/src/test/java/org/tests/insert/TestInsertDuplicateKey.java @@ -2,6 +2,7 @@ import io.ebean.DB; import io.ebean.DuplicateKeyException; +import io.ebean.Transaction; import io.ebean.annotation.Transactional; import io.ebean.xtest.BaseTestCase; import org.junit.jupiter.api.BeforeEach; @@ -13,6 +14,7 @@ import java.util.List; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; public class TestInsertDuplicateKey extends BaseTestCase { @@ -100,4 +102,61 @@ private void insertTheBatch_duplicateKey_catchAndContinue() { doc0.setBody("insertTheBatch_duplicateKey_catchAndContinue-1"); doc0.save(); } + + + @Test + public void insert_duplicateKey_retry() { + Document doc1 = new Document(); + doc1.setTitle("KeyABC"); + doc1.setBody("one"); + doc1.save(); + + Document doc2 = new Document(); + doc2.setTitle("KeyABC"); + doc2.setBody("clashes with doc1"); + Long version = doc2.getVersion(); + assertThrows(DuplicateKeyException.class, doc2::save); + assertEquals(version, doc2.getVersion()); + + doc2.setTitle("KeyABCD"); + + doc2.save(); + + doc1.setTitle("KeyABCD"); + assertThrows(DuplicateKeyException.class, doc1::save); + doc1.setTitle("KeyABCDE"); + doc1.save(); + } + + @Test + public void insert_duplicateKey_retryWithBatch() { + Document doc1 = new Document(); + doc1.setTitle("KeyABC"); + doc1.setBody("one"); + doc1.save(); + + Document doc2 = new Document(); + doc2.setTitle("KeyABC"); + doc2.setBody("clashes with doc1"); + Long version = doc2.getVersion(); + try (Transaction tx = DB.beginTransaction()) { + tx.setBatchMode(true); + doc2.save(); + assertThrows(DuplicateKeyException.class, tx::commit); + } + assertEquals(version, doc2.getVersion()); + + doc2.setTitle("KeyABCD"); + + doc2.save(); + + doc1.setTitle("KeyABCD"); + assertThrows(DuplicateKeyException.class, doc1::save); + doc1.setTitle("KeyABCDE"); + try (Transaction tx = DB.beginTransaction()) { + tx.setBatchMode(true); + doc1.save(); + tx.commit(); + } + } } From b383606912bf3ed51a2a6a09bf203d7a72efcf3c Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Wed, 2 Jul 2025 11:43:01 +0200 Subject: [PATCH 2/3] fix test --- .../tests/insert/TestInsertDuplicateKey.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/ebean-test/src/test/java/org/tests/insert/TestInsertDuplicateKey.java b/ebean-test/src/test/java/org/tests/insert/TestInsertDuplicateKey.java index aa00a0dcf4..f6b63fc703 100644 --- a/ebean-test/src/test/java/org/tests/insert/TestInsertDuplicateKey.java +++ b/ebean-test/src/test/java/org/tests/insert/TestInsertDuplicateKey.java @@ -107,36 +107,36 @@ private void insertTheBatch_duplicateKey_catchAndContinue() { @Test public void insert_duplicateKey_retry() { Document doc1 = new Document(); - doc1.setTitle("KeyABC"); + doc1.setTitle("Key1ABC"); doc1.setBody("one"); doc1.save(); Document doc2 = new Document(); - doc2.setTitle("KeyABC"); + doc2.setTitle("Key1ABC"); doc2.setBody("clashes with doc1"); Long version = doc2.getVersion(); assertThrows(DuplicateKeyException.class, doc2::save); assertEquals(version, doc2.getVersion()); - doc2.setTitle("KeyABCD"); + doc2.setTitle("Key1ABCD"); doc2.save(); - doc1.setTitle("KeyABCD"); + doc1.setTitle("Key1ABCD"); assertThrows(DuplicateKeyException.class, doc1::save); - doc1.setTitle("KeyABCDE"); + doc1.setTitle("Key1ABCDE"); doc1.save(); } @Test public void insert_duplicateKey_retryWithBatch() { Document doc1 = new Document(); - doc1.setTitle("KeyABC"); + doc1.setTitle("Key2ABC"); doc1.setBody("one"); doc1.save(); Document doc2 = new Document(); - doc2.setTitle("KeyABC"); + doc2.setTitle("Key2ABC"); doc2.setBody("clashes with doc1"); Long version = doc2.getVersion(); try (Transaction tx = DB.beginTransaction()) { @@ -146,13 +146,13 @@ public void insert_duplicateKey_retryWithBatch() { } assertEquals(version, doc2.getVersion()); - doc2.setTitle("KeyABCD"); + doc2.setTitle("Key2ABCD"); doc2.save(); - doc1.setTitle("KeyABCD"); + doc1.setTitle("Key2ABCD"); assertThrows(DuplicateKeyException.class, doc1::save); - doc1.setTitle("KeyABCDE"); + doc1.setTitle("Key2ABCDE"); try (Transaction tx = DB.beginTransaction()) { tx.setBatchMode(true); doc1.save(); From 53eeb15d797012fe6d0ad813841144017123c817 Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Wed, 2 Jul 2025 15:44:25 +0200 Subject: [PATCH 3/3] Handle batch situations and deletes We must handle batch updates differently. Here we have to roll back all statements in a batch. Also we must not roll back generated properties, if we did not modify them --- .../server/core/PersistRequestBean.java | 29 +++++++++---- .../server/persist/BatchPostExecute.java | 4 +- .../server/persist/BatchedPstmt.java | 15 +++---- .../server/persist/BatchedPstmtHolder.java | 1 + .../server/persist/dml/DmlBeanPersister.java | 1 - .../server/persist/dml/DmlHandler.java | 5 ++- .../org/tests/cascade/TestDeleteRestrict.java | 41 +++++++++++++++++++ 7 files changed, 77 insertions(+), 19 deletions(-) create mode 100644 ebean-test/src/test/java/org/tests/cascade/TestDeleteRestrict.java diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java b/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java index 8df5eba7dc..7716f61653 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/core/PersistRequestBean.java @@ -126,6 +126,7 @@ public final class PersistRequestBean extends PersistRequest implements BeanP */ private List saveMany; private InsertOptions insertOptions; + private BeanProperty[] dirtyGenerated; public PersistRequestBean(SpiEbeanServer server, T bean, Object parentBean, BeanManager mgr, SpiTransaction t, PersistExecute persistExecute, PersistRequest.Type type, int flags) { @@ -262,6 +263,7 @@ private void initGeneratedProperties() { } private void onUpdateGeneratedProperties() { + dirtyGenerated = beanDescriptor.propertiesGenUpdate(); for (BeanProperty prop : beanDescriptor.propertiesGenUpdate()) { GeneratedProperty generatedProperty = prop.generatedProperty(); if (prop.isVersion()) { @@ -282,20 +284,32 @@ private void onUpdateGeneratedProperties() { } } - public void onFailedUpdateUndoGeneratedProperties() { - for (BeanProperty prop : beanDescriptor.propertiesGenUpdate()) { - Object oldVal = intercept.origValue(prop.propertyIndex()); - prop.setValue(entityBean, oldVal); - } - } - private void onInsertGeneratedProperties() { + dirtyGenerated = beanDescriptor.propertiesGenInsert(); for (BeanProperty prop : beanDescriptor.propertiesGenInsert()) { Object value = prop.generatedProperty().getInsertValue(prop, entityBean, now()); prop.setValueChanged(entityBean, value); } } + /** + * Undos the update of generated properties. + */ + @Override + public void undo() { + if (dirtyGenerated != null) { + // Do an undo once, and undo only modified properties. + for (BeanProperty prop : dirtyGenerated) { + if (!prop.isVersion() || isLoadedProperty(prop)) { + Object oldVal = intercept.origValue(prop.propertyIndex()); + prop.setValue(entityBean, oldVal); + } + } + dirtyGenerated = null; + } + } + + /** * If using batch on cascade flush if required. */ @@ -809,7 +823,6 @@ public void setBoundId(Object idValue) { public void checkRowCount(int rowCount) { if (rowCount != 1 && rowCount != Statement.SUCCESS_NO_INFO) { if (ConcurrencyMode.VERSION == concurrencyMode) { - 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"); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchPostExecute.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchPostExecute.java index d649fcd600..b7827cb7da 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchPostExecute.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchPostExecute.java @@ -47,9 +47,9 @@ public interface BatchPostExecute { void addTimingBatch(long startNanos, int batch); /** - * Undos generated properties. + * Tries to undo the request. */ - default void onFailedUpdateUndoGeneratedProperties() { + default void undo() { } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmt.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmt.java index 55245fdb8c..7263653542 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmt.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmt.java @@ -117,7 +117,7 @@ public void executeBatch(boolean getGeneratedKeys) throws SQLException { } postExecute(); addTimingMetrics(); - list.clear(); + list.clear(); // CHECKME: This list may cause problems when undo is done on multiple batches. transaction.profileEvent(this); } @@ -155,12 +155,7 @@ private void postExecute() { private void executeAndCheckRowCounts() throws SQLException { try { - try { - results = pstmt.executeBatch(); - } catch (SQLException ex) { - list.forEach(BatchPostExecute::onFailedUpdateUndoGeneratedProperties); - throw ex; - } + results = pstmt.executeBatch(); if (transaction.isLogSql()) { transaction.logSql(" -- executeBatch() size:{0} sql:{1}", results.length, sql); } @@ -176,6 +171,11 @@ private void executeAndCheckRowCounts() throws SQLException { } } + public void undo() { + list.forEach(BatchPostExecute::undo); + } + + private void getGeneratedKeys() throws SQLException { if (DB2_HACK.getGeneratedKeys(pstmt, list)) { return; @@ -220,4 +220,5 @@ private void closeInputStreams() { inputStreams = null; } } + } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmtHolder.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmtHolder.java index 46b6673f75..5acb21d51b 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmtHolder.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchedPstmtHolder.java @@ -119,6 +119,7 @@ public void flush(boolean getGeneratedKeys, boolean reset) throws BatchedSqlExce loadBack(copyMap); } } catch (BatchedSqlException e) { + copy.forEach(BatchedPstmt::undo); closeStatements(copy); throw e; } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlBeanPersister.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlBeanPersister.java index e3a7345f39..4a4abe17d5 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlBeanPersister.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlBeanPersister.java @@ -74,7 +74,6 @@ private int execute(PersistRequestBean request, PersistHandler handler) { if (request.transaction().isLogSummary()) { request.transaction().logSummary(msg); } - request.onFailedUpdateUndoGeneratedProperties(); throw dbPlatform.translate(msg, e); } finally { if (!batched) { diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlHandler.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlHandler.java index f7f2f669fd..e1142aef81 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlHandler.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/persist/dml/DmlHandler.java @@ -8,8 +8,8 @@ import io.ebeaninternal.server.persist.BatchedPstmtHolder; import io.ebeaninternal.server.persist.dmlbind.BindableRequest; import io.ebeaninternal.server.bind.DataBind; - import jakarta.persistence.OptimisticLockException; + import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.SQLException; @@ -86,6 +86,9 @@ public final int executeNoBatch() throws SQLException { final long startNanos = System.nanoTime(); try { return execute(); + } catch (Throwable t) { + persistRequest.undo(); + throw t; } finally { persistRequest.addTimingNoBatch(startNanos); } diff --git a/ebean-test/src/test/java/org/tests/cascade/TestDeleteRestrict.java b/ebean-test/src/test/java/org/tests/cascade/TestDeleteRestrict.java new file mode 100644 index 0000000000..379432218c --- /dev/null +++ b/ebean-test/src/test/java/org/tests/cascade/TestDeleteRestrict.java @@ -0,0 +1,41 @@ +package org.tests.cascade; + +import io.ebean.DataIntegrityException; +import io.ebean.xtest.BaseTestCase; +import org.junit.jupiter.api.Test; +import org.tests.model.basic.Customer; +import org.tests.model.basic.Order; +import org.tests.model.basic.ResetBasicData; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * @author Roland Praml, Foconis Analytics GmbH + */ +public class TestDeleteRestrict extends BaseTestCase { + + @Test + void test() { + ResetBasicData.reset(); + + Customer customer = new Customer(); + customer.setName("Roland"); + server().save(customer); + + Order order = new Order(); + order.setCustomer(customer); + server().save(order); + + assertThat(customer.getVersion()).isEqualTo(1L); + assertThatThrownBy(() -> server().delete(customer)).isInstanceOf(DataIntegrityException.class); + assertThat(customer.getVersion()).isEqualTo(1L); + + customer.setName("Roland-inactive"); + server().save(customer); + + // cleanup + server().delete(order); + server().delete(customer); + } +}