#80 fix nested transaction behaviour keeping collections

Author: aburmeis

src/main/java/com/arangodb/springframework/transaction/ArangoTransactionManager.java

@@ -31,7 +31,7 @@ public ArangoTransactionManager(ArangoOperations operations, QueryTransactionBri
      * Creates a new transaction object. Any synchronized resource will be reused.
      */
     @Override
-    protected Object doGetTransaction() {
+    protected ArangoTransactionObject doGetTransaction() {
         DbName database = operations.getDatabaseName();
         if (logger.isDebugEnabled()) {
             logger.debug("Create new transaction for database " + database);
@@ -58,10 +58,13 @@ protected void doBegin(Object transaction, TransactionDefinition definition) thr
         }
         ArangoTransactionObject tx = (ArangoTransactionObject) transaction;
         tx.configure(definition);
-        DbName key = operations.getDatabaseName();
-        TransactionSynchronizationManager.unbindResourceIfPossible(key);
-        TransactionSynchronizationManager.bindResource(key, tx.getResource());
-        bridge.setCurrentTransaction(collections -> tx.getOrBegin(collections).getStreamTransactionId());
+        bridge.setCurrentTransaction(collections -> {
+            try {
+                return tx.getOrBegin(collections).getStreamTransactionId();
+            } catch (ArangoDBException error) {
+                throw new TransactionSystemException("Cannot begin transaction", error);
+            }
+        });
     }
 
     /**
@@ -118,8 +121,36 @@ protected void doSetRollbackOnly(DefaultTransactionStatus status) throws Transac
         tx.setRollbackOnly();
     }
 
+    @Override
+    protected DefaultTransactionStatus newTransactionStatus(TransactionDefinition definition, Object transaction, boolean newTransaction, boolean newSynchronization, boolean debug, Object suspendedResources) {
+        return super.newTransactionStatus(definition, transaction, newTransaction, newSynchronization, debug, suspendedResources);
+    }
+
+    /**
+     * Bind the resource for the first new transaction created.
+     */
+    @Override
+    protected void prepareSynchronization(DefaultTransactionStatus status, TransactionDefinition definition) {
+        super.prepareSynchronization(status, definition);
+        if (status.isNewTransaction()) {
+            ArangoTransactionResource resource = ((ArangoTransactionObject) status.getTransaction()).getResource();
+            resource.increaseReferences();
+            if (resource.isSingleReference()) {
+                TransactionSynchronizationManager.bindResource(operations.getDatabaseName(), resource);
+            }
+        }
+    }
+
+    /**
+     * Unbind the resource for the last transaction completed.
+     */
     @Override
     protected void doCleanupAfterCompletion(Object transaction) {
-        TransactionSynchronizationManager.unbindResourceIfPossible(operations.getDatabaseName());
+        ArangoTransactionResource resource = ((ArangoTransactionObject) transaction).getResource();
+        if (resource.isSingleReference()) {
+            TransactionSynchronizationManager.unbindResource(operations.getDatabaseName());
+        }
+        resource.decreasedReferences();
     }
+
 }

src/main/java/com/arangodb/springframework/transaction/ArangoTransactionObject.java

@@ -1,13 +1,13 @@
 package com.arangodb.springframework.transaction;
 
+import com.arangodb.ArangoDBException;
 import com.arangodb.ArangoDatabase;
 import com.arangodb.entity.StreamTransactionEntity;
 import com.arangodb.entity.StreamTransactionStatus;
 import com.arangodb.model.StreamTransactionOptions;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.lang.Nullable;
-import org.springframework.transaction.IllegalTransactionStateException;
 import org.springframework.transaction.TransactionDefinition;
 import org.springframework.transaction.interceptor.TransactionAttribute;
 import org.springframework.transaction.support.SmartTransactionObject;
@@ -53,7 +53,7 @@ void configure(TransactionDefinition definition) {
         }
     }
 
-    ArangoTransactionResource getOrBegin(Collection<String> collections) {
+    ArangoTransactionResource getOrBegin(Collection<String> collections) throws ArangoDBException {
         addCollections(collections);
         if (resource.getStreamTransactionId() != null) {
             return getResource();
@@ -70,13 +70,13 @@ ArangoTransactionResource getOrBegin(Collection<String> collections) {
         return getResource();
     }
 
-    void commit() {
+    void commit() throws ArangoDBException {
         if (isStatus(StreamTransactionStatus.running)) {
             database.commitStreamTransaction(resource.getStreamTransactionId());
         }
     }
 
-    void rollback() {
+    void rollback() throws ArangoDBException {
         if (isStatus(StreamTransactionStatus.running)) {
             database.abortStreamTransaction(resource.getStreamTransactionId());
         }
@@ -104,15 +104,16 @@ public String toString() {
 
     private void addCollections(Collection<String> collections) {
         if (resource.getStreamTransactionId() != null) {
-            if (!resource.getCollectionNames().containsAll(collections)) {
+            if (!resource.getCollectionNames().containsAll(collections) && logger.isDebugEnabled()) {
                 Set<String> additional = new HashSet<>(collections);
                 additional.removeAll(resource.getCollectionNames());
-                throw new IllegalTransactionStateException("Stream transaction already started on collections " + resource.getCollectionNames() + ", no additional collections allowed: " + additional);
+                logger.debug("Stream transaction already started on collections " + resource.getCollectionNames() + ", assuming additional collections are read only: " + additional);
             }
+        } else {
+            Set<String> all = new HashSet<>(resource.getCollectionNames());
+            all.addAll(collections);
+            resource.setCollectionNames(all);
         }
-        HashSet<String> all = new HashSet<>(resource.getCollectionNames());
-        all.addAll(collections);
-        resource.setCollectionNames(all);
     }
 
     private boolean isStatus(StreamTransactionStatus status) {

src/main/java/com/arangodb/springframework/transaction/ArangoTransactionResource.java

@@ -18,6 +18,7 @@ class ArangoTransactionResource {
     private Set<String> collectionNames;
 
     private boolean rollbackOnly;
+    int references = 0;
 
     ArangoTransactionResource(@Nullable String streamTransactionId, Set<String> collectionNames, boolean rollbackOnly) {
         this.streamTransactionId = streamTransactionId;
@@ -48,4 +49,16 @@ boolean isRollbackOnly() {
     void setRollbackOnly(boolean rollbackOnly) {
         this.rollbackOnly = rollbackOnly;
     }
+
+    void increaseReferences() {
+        ++references;
+    }
+
+    boolean isSingleReference() {
+        return references <= 1;
+    }
+
+    void decreasedReferences() {
+        --references;
+    }
 }

src/test/java/com/arangodb/springframework/transaction/ArangoTransactionManagerTest.java

@@ -32,7 +32,9 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.hasItems;
+import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.when;
@@ -95,7 +97,7 @@ public void getTransactionReturnsNewTransactionWithoutStreamTransaction() {
     public void nestedGetTransactionReturnsNewTransactionWithFormerCollections() {
         DefaultTransactionAttribute first = new DefaultTransactionAttribute();
         first.setLabels(Collections.singleton("foo"));
-        underTest.getTransaction(first);
+        TransactionStatus outer = underTest.getTransaction(first);
         DefaultTransactionAttribute second = new DefaultTransactionAttribute();
         second.setLabels(Collections.singleton("bar"));
         TransactionStatus inner = underTest.getTransaction(second);
@@ -141,13 +143,19 @@ public void nestedGetTransactionReturnsExistingTransactionWithFormerCollections(
 
         DefaultTransactionAttribute second = new DefaultTransactionAttribute();
         second.setLabels(Collections.singleton("bar"));
-        TransactionStatus inner = underTest.getTransaction(second);
-        assertThat(inner.isNewTransaction(), is(false));
-        ArangoTransactionObject transactionObject = getTransactionObject(inner);
-        assertThat(transactionObject.getResource().getStreamTransactionId(), is("123"));
-        underTest.commit(inner);
+        TransactionStatus inner1 = underTest.getTransaction(second);
+        assertThat(inner1.isNewTransaction(), is(false));
+        ArangoTransactionObject tx1 = getTransactionObject(inner1);
+        assertThat(tx1.getResource().getStreamTransactionId(), is("123"));
+        underTest.commit(inner1);
+        TransactionStatus inner2 = underTest.getTransaction(second);
+        assertThat(inner2.isNewTransaction(), is(false));
+        ArangoTransactionObject tx2 = getTransactionObject(inner1);
+        assertThat(tx2.getResource().getStreamTransactionId(), is("123"));
+        underTest.commit(inner2);
         underTest.commit(outer);
         verify(database).commitStreamTransaction("123");
+        assertThat(TransactionSynchronizationManager.getResource(DATABASE_NAME), nullValue());
     }
 
     @Test
@@ -163,14 +171,16 @@ public void getTransactionWithMultipleBridgeCallsWorksForKnownCollections() {
         assertThat(collections.getWrite(), hasItems("baz", "foo"));
     }
 
-    @Test(expected = IllegalTransactionStateException.class)
-    public void getTransactionWithMultipleBridgeCallsFailsForAdditionalCollection() {
+    @Test
+    public void getTransactionWithMultipleBridgeCallsIgnoresAdditionalCollections() {
         DefaultTransactionAttribute definition = new DefaultTransactionAttribute();
-        definition.setLabels(Collections.singleton("baz"));
+        definition.setLabels(Collections.singleton("bar"));
         definition.setTimeout(20);
-        underTest.getTransaction(definition);
+        TransactionStatus state = underTest.getTransaction(definition);
         beginTransaction("123", "foo");
-        beginPassed.getValue().apply(Collections.singletonList("bar"));
+        beginPassed.getValue().apply(Collections.singletonList("baz"));
+        assertThat(getTransactionObject(state).getResource().getCollectionNames(), hasItems("foo", "bar"));
+        assertThat(getTransactionObject(state).getResource().getCollectionNames(), not(hasItem("baz")));
     }
 
     @Test(expected = InvalidIsolationLevelException.class)