Backport to branch(3) : Fix NPE that occurs with DELETE w/o clustering key (#2982)

feeblefakie · komamitsu · web-flow · commit 8ee38801e004 · 2025-08-27T18:41:40.000+09:00
Co-authored-by: Mitsunori Komatsu &lt;komamitsu@gmail.com&gt;
diff --git a/core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitMutationComposer.java b/core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitMutationComposer.java
@@ -9,6 +9,7 @@
 import com.scalar.db.api.ConditionBuilder;
 import com.scalar.db.api.Consistency;
 import com.scalar.db.api.Delete;
+import com.scalar.db.api.DeleteBuilder;
 import com.scalar.db.api.Mutation;
 import com.scalar.db.api.Operation;
 import com.scalar.db.api.Put;
@@ -114,17 +115,20 @@ private Put composePut(Operation base, @Nullable TransactionResult result)
 
   private Delete composeDelete(Operation base, @Nullable TransactionResult result)
       throws ExecutionException {
-    return Delete.newBuilder()
-        .namespace(base.forNamespace().get())
-        .table(base.forTable().get())
-        .partitionKey(getPartitionKey(base, result))
-        .clusteringKey(getClusteringKey(base, result).orElse(null))
-        .consistency(Consistency.LINEARIZABLE)
-        .condition(
-            ConditionBuilder.deleteIf(ConditionBuilder.column(ID).isEqualToText(id))
-                .and(ConditionBuilder.column(STATE).isEqualToInt(TransactionState.DELETED.get()))
-                .build())
-        .build();
+    DeleteBuilder.Buildable deleteBuilder =
+        Delete.newBuilder()
+            .namespace(base.forNamespace().get())
+            .table(base.forTable().get())
+            .partitionKey(getPartitionKey(base, result))
+            .consistency(Consistency.LINEARIZABLE)
+            .condition(
+                ConditionBuilder.deleteIf(ConditionBuilder.column(ID).isEqualToText(id))
+                    .and(
+                        ConditionBuilder.column(STATE).isEqualToInt(TransactionState.DELETED.get()))
+                    .build());
+    getClusteringKey(base, result).ifPresent(deleteBuilder::clusteringKey);
+
+    return deleteBuilder.build();
   }
 
   private Key getPartitionKey(Operation base, @Nullable TransactionResult result)
diff --git a/core/src/main/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposer.java b/core/src/main/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposer.java
@@ -8,8 +8,10 @@
 import com.scalar.db.api.ConditionBuilder;
 import com.scalar.db.api.Consistency;
 import com.scalar.db.api.Delete;
+import com.scalar.db.api.DeleteBuilder;
 import com.scalar.db.api.DistributedStorage;
 import com.scalar.db.api.Get;
+import com.scalar.db.api.GetBuilder;
 import com.scalar.db.api.Mutation;
 import com.scalar.db.api.Operation;
 import com.scalar.db.api.Put;
@@ -121,17 +123,19 @@ private Delete composeDelete(Operation base, TransactionResult result) throws Ex
     Key partitionKey = ScalarDbUtils.getPartitionKey(result, tableMetadata);
     Optional<Key> clusteringKey = ScalarDbUtils.getClusteringKey(result, tableMetadata);
 
-    return Delete.newBuilder()
-        .namespace(base.forNamespace().get())
-        .table(base.forTable().get())
-        .partitionKey(partitionKey)
-        .clusteringKey(clusteringKey.orElse(null))
-        .condition(
-            ConditionBuilder.deleteIf(ConditionBuilder.column(ID).isEqualToText(id))
-                .and(ConditionBuilder.column(STATE).isEqualToInt(result.getState().get()))
-                .build())
-        .consistency(Consistency.LINEARIZABLE)
-        .build();
+    DeleteBuilder.Buildable deleteBuilder =
+        Delete.newBuilder()
+            .namespace(base.forNamespace().get())
+            .table(base.forTable().get())
+            .partitionKey(partitionKey)
+            .condition(
+                ConditionBuilder.deleteIf(ConditionBuilder.column(ID).isEqualToText(id))
+                    .and(ConditionBuilder.column(STATE).isEqualToInt(result.getState().get()))
+                    .build())
+            .consistency(Consistency.LINEARIZABLE);
+    clusteringKey.ifPresent(deleteBuilder::clusteringKey);
+
+    return deleteBuilder.build();
   }
 
   private Optional<TransactionResult> getLatestResult(
@@ -157,15 +161,16 @@ private Optional<TransactionResult> getLatestResult(
       }
     }
 
-    Get get =
+    GetBuilder.BuildableGetWithPartitionKey getBuilder =
         Get.newBuilder()
             .namespace(operation.forNamespace().get())
             .table(operation.forTable().get())
             .partitionKey(partitionKey)
-            .clusteringKey(clusteringKey)
-            .consistency(Consistency.LINEARIZABLE)
-            .build();
+            .consistency(Consistency.LINEARIZABLE);
+    if (clusteringKey != null) {
+      getBuilder.clusteringKey(clusteringKey);
+    }
 
-    return storage.get(get).map(TransactionResult::new);
+    return storage.get(getBuilder.build()).map(TransactionResult::new);
   }
 }
diff --git a/core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitMutationComposerTest.java b/core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitMutationComposerTest.java
@@ -118,6 +118,25 @@ private Delete prepareDelete() {
         .build();
   }
 
+  private Put preparePutWithoutCk() {
+    Key partitionKey = Key.ofText(ANY_NAME_1, ANY_TEXT_1);
+    return Put.newBuilder()
+        .namespace(ANY_NAMESPACE_NAME)
+        .table(ANY_TABLE_NAME)
+        .partitionKey(partitionKey)
+        .intValue(ANY_NAME_3, ANY_INT_1)
+        .build();
+  }
+
+  private Delete prepareDeleteWithoutCk() {
+    Key partitionKey = Key.ofText(ANY_NAME_1, ANY_TEXT_1);
+    return Delete.newBuilder()
+        .namespace(ANY_NAMESPACE_NAME)
+        .table(ANY_TABLE_NAME)
+        .partitionKey(partitionKey)
+        .build();
+  }
+
   private Get prepareGet() {
     Key partitionKey = Key.ofText(ANY_NAME_1, ANY_TEXT_1);
     Key clusteringKey = Key.ofText(ANY_NAME_2, ANY_TEXT_2);
@@ -220,6 +239,52 @@ public void add_PutAndNullResultGiven_ShouldComposePutWithPutIfCondition()
     assertThat(actual).isEqualTo(expected);
   }
 
+  @Test
+  public void add_PutWithoutCkAndPreparedResultGiven_ShouldComposePutWithPutIfCondition()
+      throws ExecutionException {
+    // Arrange
+    Put put = preparePutWithoutCk();
+    TransactionResult result = prepareResult(TransactionState.PREPARED);
+
+    // Act
+    composer.add(put, result);
+
+    // Assert
+    Put actual = (Put) composer.get().get(0);
+    Put expected =
+        Put.newBuilder()
+            .namespace(put.forNamespace().get())
+            .table(put.forTable().get())
+            .partitionKey(put.getPartitionKey())
+            .consistency(Consistency.LINEARIZABLE)
+            .condition(
+                ConditionBuilder.putIf(ConditionBuilder.column(ID).isEqualToText(ANY_ID))
+                    .and(
+                        ConditionBuilder.column(STATE)
+                            .isEqualToInt(TransactionState.PREPARED.get()))
+                    .build())
+            .bigIntValue(COMMITTED_AT, ANY_TIME_2)
+            .intValue(STATE, TransactionState.COMMITTED.get())
+            .textValue(BEFORE_ID, null)
+            .intValue(BEFORE_STATE, null)
+            .intValue(BEFORE_VERSION, null)
+            .bigIntValue(BEFORE_PREPARED_AT, null)
+            .bigIntValue(BEFORE_COMMITTED_AT, null)
+            .intValue(BEFORE_PREFIX + ANY_NAME_3, null)
+            .booleanValue(BEFORE_PREFIX + ANY_NAME_4, null)
+            .bigIntValue(BEFORE_PREFIX + ANY_NAME_5, null)
+            .floatValue(BEFORE_PREFIX + ANY_NAME_6, null)
+            .doubleValue(BEFORE_PREFIX + ANY_NAME_7, null)
+            .textValue(BEFORE_PREFIX + ANY_NAME_8, null)
+            .blobValue(BEFORE_PREFIX + ANY_NAME_9, (byte[]) null)
+            .dateValue(BEFORE_PREFIX + ANY_NAME_10, null)
+            .timeValue(BEFORE_PREFIX + ANY_NAME_11, null)
+            .timestampValue(BEFORE_PREFIX + ANY_NAME_12, null)
+            .timestampTZValue(BEFORE_PREFIX + ANY_NAME_13, null)
+            .build();
+    assertThat(actual).isEqualTo(expected);
+  }
+
   @Test
   public void add_DeleteAndDeletedResultGiven_ShouldComposeDeleteWithDeleteIfCondition()
       throws ExecutionException {
@@ -267,6 +332,30 @@ public void add_DeleteAndNullResultGiven_ShouldComposeDeleteWithDeleteIfConditio
     assertThat(actual).isEqualTo(delete);
   }
 
+  @Test
+  public void add_DeleteWithoutCkAndDeletedResultGiven_ShouldComposeDeleteWithDeleteIfCondition()
+      throws ExecutionException {
+    // Arrange
+    Delete delete = prepareDeleteWithoutCk();
+    TransactionResult result = prepareResult(TransactionState.DELETED);
+
+    // Act
+    composer.add(delete, result);
+
+    // Assert
+    Delete actual = (Delete) composer.get().get(0);
+    delete =
+        Delete.newBuilder(delete)
+            .consistency(Consistency.LINEARIZABLE)
+            .condition(
+                ConditionBuilder.deleteIf(ConditionBuilder.column(ID).isEqualToText(ANY_ID))
+                    .and(
+                        ConditionBuilder.column(STATE).isEqualToInt(TransactionState.DELETED.get()))
+                    .build())
+            .build();
+    assertThat(actual).isEqualTo(delete);
+  }
+
   @Test
   public void add_SelectionAndPreparedResultGiven_ShouldComposePutForRollforward()
       throws ExecutionException {
diff --git a/core/src/test/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposerTest.java b/core/src/test/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposerTest.java
@@ -188,6 +188,25 @@ private Put preparePut() {
         .build();
   }
 
+  private Put preparePutWithoutCk() {
+    return Put.newBuilder()
+        .namespace(ANY_NAMESPACE_NAME)
+        .table(ANY_TABLE_NAME)
+        .partitionKey(Key.ofText(ANY_NAME_1, ANY_TEXT_1))
+        .intValue(ANY_NAME_3, ANY_INT_3)
+        .booleanValue(ANY_NAME_4, false)
+        .bigIntValue(ANY_NAME_5, ANY_BIGINT_3)
+        .floatValue(ANY_NAME_6, ANY_FLOAT_3)
+        .doubleValue(ANY_NAME_7, ANY_DOUBLE_3)
+        .textValue(ANY_NAME_8, ANY_TEXT_4)
+        .blobValue(ANY_NAME_9, ANY_BLOB_3)
+        .dateValue(ANY_NAME_10, ANY_DATE_3)
+        .timeValue(ANY_NAME_11, ANY_TIME_3)
+        .timestampValue(ANY_NAME_12, ANY_TIMESTAMP_3)
+        .timestampTZValue(ANY_NAME_13, ANY_TIMESTAMPTZ_3)
+        .build();
+  }
+
   private TransactionResult prepareResult(TransactionState state) {
     ImmutableMap<String, Column<?>> columns =
         ImmutableMap.<String, Column<?>>builder()
@@ -548,6 +567,22 @@ public void add_PutAndNullResultGivenAndOldResultGivenFromStorage_ShouldDoNothin
     verify(storage).get(any(Get.class));
   }
 
+  @Test
+  public void add_PutWithoutCkAndNullResultGivenAndOldResultGivenFromStorage_ShouldDoNothing()
+      throws ExecutionException {
+    // Arrange
+    TransactionResult result = prepareInitialResult(ANY_ID_1, TransactionState.PREPARED);
+    when(storage.get(any(Get.class))).thenReturn(Optional.of(result));
+    Put put = preparePutWithoutCk();
+
+    // Act
+    composer.add(put, null);
+
+    // Assert
+    assertThat(composer.get().size()).isEqualTo(0);
+    verify(storage).get(any(Get.class));
+  }
+
   @Test
   public void add_PutAndNullResultGivenAndEmptyResultGivenFromStorage_ShouldDoNothing()
       throws ExecutionException {
