Backport to branch(3) : Fix to put state even in read-only transactions (#187)

Co-authored-by: Jun Nemoto <[email protected]>

Author: feeblefakie

ledger/build.gradle

@@ -54,6 +54,7 @@ dependencies {
     implementation group: 'com.github.everit-org.json-schema', name: 'org.everit.json.schema', version: '1.14.4'
 
     testImplementation group: 'com.scalar-labs', name: 'scalardb-schema-loader', version: "${scalarDbVersion}"
+    testImplementation group: 'org.junit.jupiter', name: 'junit-jupiter-params', version: "${junitVersion}"
 
     // for Error Prone
     errorprone "com.google.errorprone:error_prone_core:${errorproneVersion}"

ledger/src/main/java/com/scalar/dl/ledger/database/scalardb/ScalarTamperEvidentAssetLedger.java

@@ -133,17 +133,17 @@ public List<AssetProof> commit() {
       if (snapshot.hasWriteSet()) {
         puts = assetComposer.compose(snapshot, request);
 
-        if (config.isTxStateManagementEnabled()) {
-          stateManager.putCommit(transaction, transaction.getId());
-        }
-
         transaction.put(puts);
 
         if (!config.isDirectAssetAccessEnabled()) {
           metadata.put(snapshot.getWriteSet().values());
         }
       }
 
+      if (config.isTxStateManagementEnabled()) {
+        stateManager.putCommit(transaction, transaction.getId());
+      }
+
       transaction.commit();
     } catch (CrudConflictException e) {
       throw new ConflictException(

ledger/src/test/java/com/scalar/dl/ledger/database/scalardb/ScalarTamperEvidentAssetLedgerTest.java

@@ -51,6 +51,8 @@
 import java.util.Optional;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
@@ -466,6 +468,7 @@ public void commit_EmptySnapshotGiven_ShouldDoNothing()
           CrudException {
     // Arrange
     when(config.isDirectAssetAccessEnabled()).thenReturn(false);
+    when(config.isTxStateManagementEnabled()).thenReturn(false);
 
     // Act
     ledger.commit();
@@ -482,6 +485,7 @@ public void commit_EmptySnapshotGivenAndDirectAssetAccessEnabled_ShouldDoNothing
           CrudException {
     // Arrange
     when(config.isDirectAssetAccessEnabled()).thenReturn(true);
+    when(config.isTxStateManagementEnabled()).thenReturn(false);
 
     // Act
     ledger.commit();
@@ -741,12 +745,14 @@ public void commit_CommitExceptionThrownInCommit_ShouldThrowDatabaseException()
     assertThat(proofs).containsOnly(proof);
   }
 
-  @Test
-  public void commit_TxStateManagementEnabled_ShouldPutWithStateManager()
+  @ParameterizedTest
+  @ValueSource(booleans = {true, false})
+  public void commit_WriteTransactionGiven_ShouldPutWithStateManagerAccordingToConfig(
+      boolean txStateManagementEnabled)
       throws CrudException, CommitException,
           com.scalar.db.exception.transaction.UnknownTransactionStatusException {
     // Arrange
-    when(config.isTxStateManagementEnabled()).thenReturn(true);
+    when(config.isTxStateManagementEnabled()).thenReturn(txStateManagementEnabled);
     snapshot.put(ANY_ID, asset);
     snapshot.put(ANY_ID, ANY_DATA);
     doNothing().when(transaction).put(any(List.class));
@@ -758,32 +764,40 @@ public void commit_TxStateManagementEnabled_ShouldPutWithStateManager()
     ledger.commit();
 
     // Assert
-    verify(stateManager).putCommit(transaction, ANY_NONCE);
+    if (txStateManagementEnabled) {
+      verify(stateManager).putCommit(transaction, ANY_NONCE);
+    } else {
+      verify(stateManager, never()).putCommit(any(DistributedTransaction.class), anyString());
+    }
     verify(transaction).put(any(List.class));
     verify(transaction).put(any(Put.class));
     verify(transaction).commit();
   }
 
-  @Test
-  public void commit_TxStateManagementDisabled_ShouldNotPutWithStateManager()
+  @ParameterizedTest
+  @ValueSource(booleans = {true, false})
+  public void commit_ReadOnlyTransactionGiven_ShouldPutWithStateManagerAccordingToConfig(
+      boolean txStateManagementEnabled)
       throws CrudException, CommitException,
           com.scalar.db.exception.transaction.UnknownTransactionStatusException {
     // Arrange
-    when(config.isTxStateManagementEnabled()).thenReturn(false);
-    snapshot.put(ANY_ID, asset);
-    snapshot.put(ANY_ID, ANY_DATA);
-    doNothing().when(transaction).put(any(List.class));
-    doNothing().when(transaction).put(any(Put.class));
+    when(config.isTxStateManagementEnabled()).thenReturn(txStateManagementEnabled);
+    when(config.isDirectAssetAccessEnabled()).thenReturn(false);
     when(transaction.getId()).thenReturn(ANY_NONCE);
     doNothing().when(stateManager).putCommit(any(DistributedTransaction.class), anyString());
+    snapshot.put(ANY_ID, asset);
 
     // Act
     ledger.commit();
 
     // Assert
-    verify(stateManager, never()).putCommit(transaction, ANY_NONCE);
-    verify(transaction).put(any(List.class));
-    verify(transaction).put(any(Put.class));
+    if (txStateManagementEnabled) {
+      verify(stateManager).putCommit(transaction, ANY_NONCE);
+    } else {
+      verify(stateManager, never()).putCommit(any(DistributedTransaction.class), anyString());
+    }
+    verify(transaction, never()).put(any(List.class));
+    verify(transaction, never()).put(any(Put.class));
     verify(transaction).commit();
   }