From abbcd35ee648833473cc3d58d8252ecab5b2cea0 Mon Sep 17 00:00:00 2001
From: Rob Bygrave <robin.bygrave@gmail.com>
Date: Tue, 27 Mar 2018 09:14:45 +1300
Subject: [PATCH] #1353 - Use of ebeanServer.commitTransaction() in nested
 transaction doesn't match transction.commit() and closes parent scope

---
 src/main/java/io/ebean/EbeanServer.java       | 52 ++++++++--------
 .../server/core/DefaultServer.java            |  9 ++-
 .../DefaultTransactionScopeManager.java       | 15 -----
 .../transaction/TransactionScopeManager.java  | 16 -----
 .../transaction/TestNestedSubTransaction.java | 62 +++++++++++++++++++
 5 files changed, 94 insertions(+), 60 deletions(-)
 create mode 100644 src/test/java/org/tests/transaction/TestNestedSubTransaction.java

diff --git a/src/main/java/io/ebean/EbeanServer.java b/src/main/java/io/ebean/EbeanServer.java
index 6fbc54b24..1b9157c1f 100644
--- a/src/main/java/io/ebean/EbeanServer.java
+++ b/src/main/java/io/ebean/EbeanServer.java
@@ -521,30 +521,44 @@ public interface EbeanServer {
    * etc.
    * </p>
    * <p>
+   * <h3>Using try with resources</h3>
    * <pre>{@code
    *
    *    // start a transaction (stored in a ThreadLocal)
-   *    ebeanServer.beginTransaction();
+   *
+   *    try (Transaction txn = ebeanServer.beginTransaction()) {
+   *
+   * 	    Order order = ebeanServer.find(Order.class,10);
+   * 	    ...
+   * 	    ebeanServer.save(order);
+   *
+   * 	    txn.commit();
+   *    }
+   *
+   * }</pre>
+   * <p>
+   * <h3>Using try finally block</h3>
+   * <pre>{@code
+   *
+   *    // start a transaction (stored in a ThreadLocal)
+   *    Transaction txn = ebeanServer.beginTransaction();
    *    try {
    * 	    Order order = ebeanServer.find(Order.class,10);
    *
    * 	    ebeanServer.save(order);
    *
-   * 	    ebeanServer.commitTransaction();
+   * 	    txn.commit();
    *
    *    } finally {
-   * 	    // rollback if we didn't commit
-   * 	    // i.e. an exception occurred before commitTransaction().
-   * 	    ebeanServer.endTransaction();
+   * 	    txn.end();
    *    }
    *
    * }</pre>
    * <p>
-   * <h3>Transaction options:</h3>
+   * <h3>Transaction options</h3>
    * <pre>{@code
    *
-   *     Transaction txn = ebeanServer.beginTransaction();
-   *     try {
+   *     try (Transaction txn = ebeanServer.beginTransaction()) {
    *       // explicitly turn on/off JDBC batch use
    *       txn.setBatchMode(true);
    *       txn.setBatchSize(50);
@@ -565,10 +579,6 @@ public interface EbeanServer {
    *       ...
    *
    *       txn.commit();
-   *
-   *    } finally {
-   *       // rollback if necessary
-   *       txn.end();
    *    }
    *
    * }</pre>
@@ -598,19 +608,16 @@ public interface EbeanServer {
    * <pre>{@code
    * // Start a new transaction. If there is a current transaction
    * // suspend it until this transaction ends
-   * Transaction txn = server.beginTransaction(TxScope.requiresNew());
-   * try {
+   * try (Transaction txn = server.beginTransaction(TxScope.requiresNew())) {
    *
    *   ...
    *
    *   // commit the transaction
    *   txn.commit();
    *
-   * } finally {
-   *   // end this transaction which:
-   *   //  A) will rollback transaction if it has not been committed already
+   *   // At end this transaction will:
+   *   //  A) will rollback transaction if it has not been committed
    *   //  B) will restore a previously suspended transaction
-   *   txn.end();
    * }
    *
    * }</pre>
@@ -619,20 +626,13 @@ public interface EbeanServer {
    * <pre>{@code
    *
    * // start a new transaction if there is not a current transaction
-   * Transaction txn = server.beginTransaction(TxScope.required());
-   * try {
+   * try (Transaction txn = server.beginTransaction(TxScope.required())) {
    *
    *   ...
    *
    *   // commit the transaction if it was created or
    *   // do nothing if there was already a current transaction
    *   txn.commit();
-   *
-   * } finally {
-   *   // end this transaction which will rollback the transaction
-   *   // if it was created for this try finally scope and has not
-   *   // already been committed
-   *   txn.end();
    * }
    *
    * }</pre>
diff --git a/src/main/java/io/ebeaninternal/server/core/DefaultServer.java b/src/main/java/io/ebeaninternal/server/core/DefaultServer.java
index b4827d155..c131f6722 100644
--- a/src/main/java/io/ebeaninternal/server/core/DefaultServer.java
+++ b/src/main/java/io/ebeaninternal/server/core/DefaultServer.java
@@ -802,7 +802,7 @@ public final class DefaultServer implements SpiServer, SpiEbeanServer {
    */
   @Override
   public void commitTransaction() {
-    transactionManager.scope().commit();
+    currentTransaction().commit();
   }
 
   /**
@@ -810,7 +810,7 @@ public final class DefaultServer implements SpiServer, SpiEbeanServer {
    */
   @Override
   public void rollbackTransaction() {
-    transactionManager.scope().rollback();
+    currentTransaction().rollback();
   }
 
   /**
@@ -843,7 +843,10 @@ public final class DefaultServer implements SpiServer, SpiEbeanServer {
    */
   @Override
   public void endTransaction() {
-    transactionManager.scope().end();
+    Transaction transaction = currentTransaction();
+    if (transaction != null) {
+      transaction.end();
+    }
   }
 
   /**
diff --git a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java
index 0f376b3cb..bf949c231 100644
--- a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java
+++ b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java
@@ -17,16 +17,6 @@ public class DefaultTransactionScopeManager extends TransactionScopeManager {
     // do nothing
   }
 
-  @Override
-  public void commit() {
-    DefaultTransactionThreadLocal.commit(serverName);
-  }
-
-  @Override
-  public void end() {
-    DefaultTransactionThreadLocal.end(serverName);
-  }
-
   @Override
   public SpiTransaction getInScope() {
     return DefaultTransactionThreadLocal.get(serverName);
@@ -47,11 +37,6 @@ public class DefaultTransactionScopeManager extends TransactionScopeManager {
     DefaultTransactionThreadLocal.replace(serverName, trans);
   }
 
-  @Override
-  public void rollback() {
-    DefaultTransactionThreadLocal.rollback(serverName);
-  }
-
   @Override
   public void set(SpiTransaction trans) {
     DefaultTransactionThreadLocal.set(serverName, trans);
diff --git a/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java b/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java
index 541b07b1d..61f2b8050 100644
--- a/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java
+++ b/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java
@@ -34,22 +34,6 @@ public abstract class TransactionScopeManager implements SpiTransactionScopeMana
    */
   public abstract void set(SpiTransaction trans);
 
-  /**
-   * Commit the current transaction.
-   */
-  public abstract void commit();
-
-  /**
-   * Rollback the current transaction.
-   */
-  public abstract void rollback();
-
-
-  /**
-   * Rollback if required.
-   */
-  public abstract void end();
-
   /**
    * Replace the current transaction with this one.
    * <p>
diff --git a/src/test/java/org/tests/transaction/TestNestedSubTransaction.java b/src/test/java/org/tests/transaction/TestNestedSubTransaction.java
new file mode 100644
index 000000000..f7e9e62e2
--- /dev/null
+++ b/src/test/java/org/tests/transaction/TestNestedSubTransaction.java
@@ -0,0 +1,62 @@
+package org.tests.transaction;
+
+import io.ebean.BaseTestCase;
+import io.ebean.Ebean;
+import io.ebean.EbeanServer;
+import io.ebean.PersistenceContextScope;
+import io.ebean.Transaction;
+import org.junit.Test;
+import org.tests.model.basic.EBasic;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertNotNull;
+
+public class TestNestedSubTransaction extends BaseTestCase {
+
+  @Test
+  public void ebeanServer_commitTransaction_expect_sameAsTransactionCommit() {
+
+    EbeanServer server = server();
+
+    EBasic bean = new EBasic("x1");
+
+    try (Transaction txn0 = server.beginTransaction()) {
+      txn0.setRollbackOnly();
+
+      server.save(bean);
+
+      try (Transaction txn1 = server.beginTransaction()) {
+        bean.setName("x2");
+        server.save(bean);
+        //txn1.commit();
+        server.commitTransaction();
+      }
+
+      EBasic fresh = server.find(EBasic.class, bean.getId());
+      assertNotNull(fresh); // FAILS
+      assertThat(fresh.getName()).isEqualTo("x2");
+
+      try (Transaction txn2 = server.beginTransaction()) {
+        bean.setName("barney");
+        Ebean.save(bean);
+        //txn2.commit();
+        server.commitTransaction();
+      }
+
+      fresh = server.find(EBasic.class)
+        .setId(bean.getId())
+        .setPersistenceContextScope(PersistenceContextScope.QUERY)
+        .findOne();
+
+      assertNotNull(fresh);
+      assertThat(fresh.getName()).isEqualTo("barney");
+
+    } finally {
+      cleanup(bean);
+    }
+  }
+
+  private void cleanup(EBasic bean) {
+    Ebean.delete(EBasic.class, bean.getId());
+  }
+}