Backport to branch(3) : Disable implicit pre-read by default for Put …

…operations (#1339)

Co-authored-by: Toshihiro Suzuki <[email protected]>

core/src/main/java/com/scalar/db/api/OperationBuilder.java

@@ -108,7 +108,7 @@ interface Projection<T> {
 
   interface ClearProjections<T> {
     /**
-     * Clear the list of projections
+     * Clears the list of projections.
      *
      * @return the operation builder
      */
@@ -127,7 +127,7 @@ interface Condition<T> {
 
   interface ClearCondition<T> {
     /**
-     * Remove the condition
+     * Removes the condition.
      *
      * @return the operation builder
      */
@@ -266,14 +266,14 @@ interface Values<T> {
 
   interface ClearValues<T> {
     /**
-     * Clear the list of values
+     * Clears the list of values.
      *
      * @return the operation builder
      */
     T clearValues();
 
     /**
-     * Clear the value for the given column
+     * Clears the value for the given column.
      *
      * @param columnName a column name
      * @return the operation builder
@@ -283,12 +283,19 @@ interface ClearValues<T> {
 
   interface ImplicitPreReadEnabled<T> {
     /**
-     * Disable implicit pre-read for this put operation.
+     * Disables implicit pre-read for this put operation.
      *
      * @return the operation builder
      */
     T disableImplicitPreRead();
 
+    /**
+     * Enables implicit pre-read for this put operation.
+     *
+     * @return the operation builder
+     */
+    T enableImplicitPreRead();
+
     /**
      * Sets whether implicit pre-read is enabled or not for this put operation.
      *
@@ -338,7 +345,7 @@ interface Ordering<T> {
 
   interface ClearOrderings<T> {
     /**
-     * Clear the list of orderings
+     * Clears the list of orderings.
      *
      * @return the scan operation builder
      */
@@ -387,14 +394,14 @@ default T end(Key clusteringKey) {
 
   interface ClearBoundaries<T> {
     /**
-     * Remove the scan starting boundary
+     * Removes the scan starting boundary.
      *
      * @return the scan operation builder
      */
     T clearStart();
 
     /**
-     * Remove the scan ending boundary
+     * Removes the scan ending boundary.
      *
      * @return the scan operation builder
      */
@@ -403,7 +410,7 @@ interface ClearBoundaries<T> {
 
   interface All<T> {
     /**
-     * Specify the Scan operation will retrieve all the entries of the database
+     * Specifies the Scan operation will retrieve all the entries of the database.
      *
      * @return the scan operation builder
      */
@@ -504,7 +511,7 @@ interface Or<T> {
 
   interface ClearConditions<T> {
     /**
-     * Clear all conditions
+     * Clears all conditions.
      *
      * @return the scan operation builder
      */

core/src/main/java/com/scalar/db/api/Put.java

@@ -39,7 +39,7 @@ public class Put extends Mutation {
 
   private final Map<String, Column<?>> columns;
 
-  private boolean implicitPreReadEnabled = true;
+  private boolean implicitPreReadEnabled;
 
   /**
    * Constructs a {@code Put} with the specified partition {@link Key}.

core/src/main/java/com/scalar/db/api/PutBuilder.java

@@ -83,7 +83,7 @@ public static class Buildable extends OperationBuilder.Buildable<Put>
     @Nullable Key clusteringKey;
     @Nullable com.scalar.db.api.Consistency consistency;
     @Nullable MutationCondition condition;
-    boolean implicitPreReadEnabled = true;
+    boolean implicitPreReadEnabled;
 
     private Buildable(@Nullable String namespace, String table, Key partitionKey) {
       super(namespace, table, partitionKey);
@@ -208,6 +208,12 @@ public Buildable disableImplicitPreRead() {
       return this;
     }
 
+    @Override
+    public Buildable enableImplicitPreRead() {
+      implicitPreReadEnabled = true;
+      return this;
+    }
+
     @Override
     public Buildable implicitPreReadEnabled(boolean implicitPreReadEnabled) {
       this.implicitPreReadEnabled = implicitPreReadEnabled;
@@ -410,13 +416,19 @@ public BuildableFromExisting clearNamespace() {
     }
 
     @Override
-    public Buildable disableImplicitPreRead() {
+    public BuildableFromExisting disableImplicitPreRead() {
       super.disableImplicitPreRead();
       return this;
     }
 
     @Override
-    public Buildable implicitPreReadEnabled(boolean implicitPreReadEnabled) {
+    public BuildableFromExisting enableImplicitPreRead() {
+      super.enableImplicitPreRead();
+      return this;
+    }
+
+    @Override
+    public BuildableFromExisting implicitPreReadEnabled(boolean implicitPreReadEnabled) {
       super.implicitPreReadEnabled(implicitPreReadEnabled);
       return this;
     }

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java

@@ -171,15 +171,20 @@ private List<Result> createScanResults(Scan scan, List<String> projections, List
   }
 
   public void put(Put put) throws CrudException {
-    if (put.getCondition().isPresent() && !put.isImplicitPreReadEnabled()) {
+    Snapshot.Key key = new Snapshot.Key(put);
+
+    if (put.getCondition().isPresent()
+        && (!put.isImplicitPreReadEnabled() && !snapshot.containsKeyInReadSet(key))) {
       throw new IllegalArgumentException(
-          "Put cannot have a condition when implicit pre-read is disabled: " + put);
+          "Put cannot have a condition when the target record is unread and implicit pre-read is disabled."
+              + " Please read the target record beforehand or enable implicit pre-read: "
+              + put);
     }
 
-    Snapshot.Key key = new Snapshot.Key(put);
-
     if (put.getCondition().isPresent()) {
-      readUnread(key, createGet(key));
+      if (put.isImplicitPreReadEnabled()) {
+        readUnread(key, createGet(key));
+      }
       mutationConditionsValidator.checkIfConditionIsSatisfied(
           put, snapshot.getFromReadSet(key).orElse(null));
     }

core/src/test/java/com/scalar/db/api/PutBuilderTest.java

@@ -186,7 +186,7 @@ public void build_FromExistingWithoutChange_ShouldCopy() {
             .withIntValue("int2", Integer.valueOf(Integer.MAX_VALUE))
             .withTextValue("text", "a_value")
             .withCondition(condition1)
-            .setImplicitPreReadEnabled(false);
+            .setImplicitPreReadEnabled(true);
 
     // Act
     Put newPut = Put.newBuilder(existingPut).build();
@@ -243,7 +243,7 @@ public void build_FromExistingAndUpdateAllParameters_ShouldBuildPutWithUpdatedPa
             .textValue("text", "another_value")
             .value(TextColumn.of("text2", "foo"))
             .condition(condition2)
-            .implicitPreReadEnabled(false)
+            .enableImplicitPreRead()
             .build();
 
     // Assert
@@ -268,7 +268,7 @@ public void build_FromExistingAndUpdateAllParameters_ShouldBuildPutWithUpdatedPa
                 .withTextValue("text", "another_value")
                 .withTextValue("text2", "foo")
                 .withCondition(condition2)
-                .setImplicitPreReadEnabled(false));
+                .setImplicitPreReadEnabled(true));
   }
 
   @Test

core/src/test/java/com/scalar/db/transaction/consensuscommit/CrudHandlerTest.java

@@ -503,15 +503,17 @@ public void put_PutWithoutConditionGiven_ShouldCallAppropriateMethods() throws C
   }
 
   @Test
-  public void put_PutWithConditionGiven_WithResultInReadSet_ShouldCallAppropriateMethods()
-      throws CrudException {
+  public void
+      put_PutWithConditionAndImplicitPreReadEnabledGiven_WithResultInReadSet_ShouldCallAppropriateMethods()
+          throws CrudException {
     // Arrange
     Put put =
         Put.newBuilder()
             .namespace("ns")
             .table("tbl")
             .partitionKey(Key.ofText("c1", "foo"))
             .condition(ConditionBuilder.putIfExists())
+            .enableImplicitPreRead()
             .build();
     Snapshot.Key key = new Snapshot.Key(put);
     when(snapshot.containsKeyInReadSet(any())).thenReturn(true);
@@ -539,19 +541,23 @@ public void put_PutWithConditionGiven_WithResultInReadSet_ShouldCallAppropriateM
   }
 
   @Test
-  public void put_PutWithConditionGiven_WithoutResultInReadSet_ShouldCallAppropriateMethods()
-      throws CrudException {
+  public void
+      put_PutWithConditionAndImplicitPreReadEnabledGiven_WithoutResultInReadSet_ShouldCallAppropriateMethods()
+          throws CrudException {
     // Arrange
     Put put =
         Put.newBuilder()
             .namespace("ns")
             .table("tbl")
             .partitionKey(Key.ofText("c1", "foo"))
             .condition(ConditionBuilder.putIfExists())
+            .enableImplicitPreRead()
             .build();
     Snapshot.Key key = new Snapshot.Key(put);
     when(snapshot.containsKeyInReadSet(any())).thenReturn(false);
-    when(snapshot.getFromReadSet(any())).thenReturn(Optional.empty());
+    TransactionResult result = mock(TransactionResult.class);
+    when(result.isCommitted()).thenReturn(true);
+    when(snapshot.getFromReadSet(any())).thenReturn(Optional.of(result));
 
     Get getForKey =
         Get.newBuilder()
@@ -569,13 +575,50 @@ public void put_PutWithConditionGiven_WithoutResultInReadSet_ShouldCallAppropria
     // Assert
     verify(spied).readUnread(key, getForKey);
     verify(snapshot).getFromReadSet(key);
-    verify(mutationConditionsValidator).checkIfConditionIsSatisfied(put, null);
+    verify(mutationConditionsValidator).checkIfConditionIsSatisfied(put, result);
+    verify(snapshot).put(key, put);
+  }
+
+  @Test
+  public void
+      put_PutWithConditionAndImplicitPreReadDisabledGiven_WithResultInReadSet_ShouldCallAppropriateMethods()
+          throws CrudException {
+    // Arrange
+    Put put =
+        Put.newBuilder()
+            .namespace("ns")
+            .table("tbl")
+            .partitionKey(Key.ofText("c1", "foo"))
+            .condition(ConditionBuilder.putIfExists())
+            .build();
+    Snapshot.Key key = new Snapshot.Key(put);
+    when(snapshot.containsKeyInReadSet(any())).thenReturn(true);
+    TransactionResult result = mock(TransactionResult.class);
+    when(result.isCommitted()).thenReturn(true);
+    when(snapshot.getFromReadSet(any())).thenReturn(Optional.of(result));
+
+    Get getForKey =
+        Get.newBuilder()
+            .namespace(key.getNamespace())
+            .table(key.getTable())
+            .partitionKey(key.getPartitionKey())
+            .build();
+
+    CrudHandler spied = spy(handler);
+
+    // Act
+    spied.put(put);
+
+    // Assert
+    verify(spied, never()).readUnread(key, getForKey);
+    verify(snapshot).getFromReadSet(key);
+    verify(mutationConditionsValidator).checkIfConditionIsSatisfied(put, result);
     verify(snapshot).put(key, put);
   }
 
   @Test
   public void
-      put_PutWithImplicitPreReadDisabledAndConditionGiven_ShouldThrowIllegalArgumentException() {
+      put_PutWithConditionAndImplicitPreReadDisabledGiven_WithoutResultInReadSet_ShouldThrowIllegalArgumentException() {
     // Arrange
     Put put =
         Put.newBuilder()

core/src/test/java/com/scalar/db/util/ProtoUtilsTest.java

@@ -911,7 +911,7 @@ public void toMutation_ProtoPutGiven_ShouldConvertProperly() {
                             .build())
                     .build())
             .setConsistency(com.scalar.db.rpc.Consistency.CONSISTENCY_EVENTUAL)
-            .setImplicitPreReadEnabled(false)
+            .setImplicitPreReadEnabled(true)
             .setNamespace("ns")
             .setTable("tbl")
             .build();
@@ -945,7 +945,7 @@ public void toMutation_ProtoPutGiven_ShouldConvertProperly() {
                         .and(ConditionBuilder.column("col7").isNotNullBlob())
                         .build())
                 .consistency(Consistency.EVENTUAL)
-                .disableImplicitPreRead()
+                .enableImplicitPreRead()
                 .build());
   }
 
@@ -1016,7 +1016,7 @@ public void toMutation_PutWithNullValuesGiven_ShouldConvertProperly() {
                         .setType(com.scalar.db.rpc.MutateCondition.Type.PUT_IF_NOT_EXISTS))
                 .setNamespace("ns")
                 .setTable("tbl")
-                .setImplicitPreReadEnabled(true)
+                .setImplicitPreReadEnabled(false)
                 .build());
   }
 
@@ -1057,7 +1057,7 @@ public void toMutation_ProtoPutWithNullValuesGiven_ShouldConvertProperly() {
                 MutateCondition.newBuilder()
                     .setType(com.scalar.db.rpc.MutateCondition.Type.PUT_IF_NOT_EXISTS))
             .setConsistency(com.scalar.db.rpc.Consistency.CONSISTENCY_EVENTUAL)
-            .setImplicitPreReadEnabled(true)
+            .setImplicitPreReadEnabled(false)
             .setNamespace("ns")
             .setTable("tbl")
             .build();
@@ -1082,7 +1082,7 @@ public void toMutation_ProtoPutWithNullValuesGiven_ShouldConvertProperly() {
                 .blobValue("col7", (byte[]) null)
                 .condition(ConditionBuilder.putIfNotExists())
                 .consistency(Consistency.EVENTUAL)
-                .implicitPreReadEnabled(true)
+                .disableImplicitPreRead()
                 .build());
   }
 
@@ -1317,7 +1317,7 @@ public void toMutation_ProtoPutWithNullValuesFromOldClientGiven_ShouldConvertPro
                         .and(ConditionBuilder.column("col5").isLessThanOrEqualToDouble(4.56))
                         .build())
                 .consistency(Consistency.EVENTUAL)
-                .implicitPreReadEnabled(true)
+                .enableImplicitPreRead()
                 .build());
   }