From 703d430a22cb877523389c2f7bc9ceaa55601715 Mon Sep 17 00:00:00 2001 From: Toshihiro Date: Tue, 5 Nov 2024 04:42:46 +0000 Subject: [PATCH 1/2] Empty commit [skip ci] From 9233087b28ac5c297234b3f9c8ab8a414e15eb7b Mon Sep 17 00:00:00 2001 From: Toshihiro Suzuki Date: Tue, 5 Nov 2024 13:42:24 +0900 Subject: [PATCH 2/2] Add validation for primary key columns in Cosmos DB adapter (#2292) --- .../com/scalar/db/common/error/CoreError.java | 7 + .../storage/cosmos/ConcatenationVisitor.java | 4 +- .../cosmos/CosmosOperationChecker.java | 93 +++++ .../CoordinatorGroupCommitter.java | 2 +- .../cosmos/CosmosOperationCheckerTest.java | 329 +++++++++++++++++- .../scalar/db/storage/cosmos/CosmosTest.java | 106 ++++++ .../CoordinatorGroupCommitterTest.java | 18 +- 7 files changed, 532 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/com/scalar/db/common/error/CoreError.java b/core/src/main/java/com/scalar/db/common/error/CoreError.java index c06d060d8..b6c64d752 100644 --- a/core/src/main/java/com/scalar/db/common/error/CoreError.java +++ b/core/src/main/java/com/scalar/db/common/error/CoreError.java @@ -647,6 +647,13 @@ public enum CoreError implements ScalarDbError { + "If you want to modify a condition, please use clearConditions() to remove all existing conditions first", "", ""), + COSMOS_PRIMARY_KEY_CONTAINS_ILLEGAL_CHARACTER( + Category.USER_ERROR, + "0145", + "The value of the column %s in the primary key contains an illegal character. " + + "Primary-key columns must not contain any of the following characters in Cosmos DB: ':', '/', '\\', '#', '?'. Value: %s", + "", + ""), // // Errors for the concurrency error category diff --git a/core/src/main/java/com/scalar/db/storage/cosmos/ConcatenationVisitor.java b/core/src/main/java/com/scalar/db/storage/cosmos/ConcatenationVisitor.java index d0b569ae5..0a55c8756 100644 --- a/core/src/main/java/com/scalar/db/storage/cosmos/ConcatenationVisitor.java +++ b/core/src/main/java/com/scalar/db/storage/cosmos/ConcatenationVisitor.java @@ -14,7 +14,8 @@ import javax.annotation.concurrent.NotThreadSafe; /** - * A visitor class to make a concatenated key string for the partition key + * A visitor class to make a concatenated key string for the partition key. This uses a colon as a + * key separator, so the text column value should not contain colons. * * @author Yuji Ito */ @@ -27,7 +28,6 @@ public ConcatenationVisitor() { } public String build() { - // TODO What if the string or blob value includes `:`? return String.join(":", values); } diff --git a/core/src/main/java/com/scalar/db/storage/cosmos/CosmosOperationChecker.java b/core/src/main/java/com/scalar/db/storage/cosmos/CosmosOperationChecker.java index d8f866947..9c81442d4 100644 --- a/core/src/main/java/com/scalar/db/storage/cosmos/CosmosOperationChecker.java +++ b/core/src/main/java/com/scalar/db/storage/cosmos/CosmosOperationChecker.java @@ -2,25 +2,105 @@ import com.scalar.db.api.ConditionalExpression; import com.scalar.db.api.Delete; +import com.scalar.db.api.Get; import com.scalar.db.api.Mutation; +import com.scalar.db.api.Operation; import com.scalar.db.api.Put; +import com.scalar.db.api.Scan; import com.scalar.db.api.TableMetadata; import com.scalar.db.common.TableMetadataManager; import com.scalar.db.common.checker.OperationChecker; import com.scalar.db.common.error.CoreError; import com.scalar.db.config.DatabaseConfig; import com.scalar.db.exception.storage.ExecutionException; +import com.scalar.db.io.BigIntColumn; +import com.scalar.db.io.BlobColumn; +import com.scalar.db.io.BooleanColumn; +import com.scalar.db.io.ColumnVisitor; import com.scalar.db.io.DataType; +import com.scalar.db.io.DoubleColumn; +import com.scalar.db.io.FloatColumn; +import com.scalar.db.io.IntColumn; +import com.scalar.db.io.TextColumn; public class CosmosOperationChecker extends OperationChecker { + + private static final char[] ILLEGAL_CHARACTERS_IN_PRIMARY_KEY = { + // Colons are not allowed in primary-key columns due to the `ConcatenationVisitor` limitation. + ':', + + // The following characters are not allowed in primary-key columns because they are restricted + // and cannot be used in the `Id` property of a Cosmos DB document. For more information, see: + // https://learn.microsoft.com/en-us/dotnet/api/microsoft.azure.cosmos.databaseproperties.id?view=azure-dotnet#remarks + '/', + '\\', + '#', + '?' + }; + + private static final ColumnVisitor PRIMARY_KEY_COLUMN_CHECKER = + new ColumnVisitor() { + @Override + public void visit(BooleanColumn column) {} + + @Override + public void visit(IntColumn column) {} + + @Override + public void visit(BigIntColumn column) {} + + @Override + public void visit(FloatColumn column) {} + + @Override + public void visit(DoubleColumn column) {} + + @Override + public void visit(TextColumn column) { + String value = column.getTextValue(); + assert value != null; + + for (char illegalCharacter : ILLEGAL_CHARACTERS_IN_PRIMARY_KEY) { + if (value.indexOf(illegalCharacter) != -1) { + throw new IllegalArgumentException( + CoreError.COSMOS_PRIMARY_KEY_CONTAINS_ILLEGAL_CHARACTER.buildMessage( + column.getName(), value)); + } + } + } + + @Override + public void visit(BlobColumn column) {} + }; + public CosmosOperationChecker( DatabaseConfig databaseConfig, TableMetadataManager metadataManager) { super(databaseConfig, metadataManager); } + @Override + public void check(Get get) throws ExecutionException { + super.check(get); + checkPrimaryKey(get); + } + + @Override + public void check(Scan scan) throws ExecutionException { + super.check(scan); + checkPrimaryKey(scan); + scan.getStartClusteringKey() + .ifPresent( + c -> c.getColumns().forEach(column -> column.accept(PRIMARY_KEY_COLUMN_CHECKER))); + scan.getEndClusteringKey() + .ifPresent( + c -> c.getColumns().forEach(column -> column.accept(PRIMARY_KEY_COLUMN_CHECKER))); + } + @Override public void check(Put put) throws ExecutionException { super.check(put); + checkPrimaryKey(put); + TableMetadata metadata = getTableMetadata(put); checkCondition(put, metadata); } @@ -28,10 +108,23 @@ public void check(Put put) throws ExecutionException { @Override public void check(Delete delete) throws ExecutionException { super.check(delete); + checkPrimaryKey(delete); + TableMetadata metadata = getTableMetadata(delete); checkCondition(delete, metadata); } + private void checkPrimaryKey(Operation operation) { + operation + .getPartitionKey() + .getColumns() + .forEach(column -> column.accept(PRIMARY_KEY_COLUMN_CHECKER)); + operation + .getClusteringKey() + .ifPresent( + c -> c.getColumns().forEach(column -> column.accept(PRIMARY_KEY_COLUMN_CHECKER))); + } + private void checkCondition(Mutation mutation, TableMetadata metadata) { if (!mutation.getCondition().isPresent()) { return; diff --git a/core/src/main/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitter.java b/core/src/main/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitter.java index 0871ff3da..d414aa381 100644 --- a/core/src/main/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitter.java +++ b/core/src/main/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitter.java @@ -38,7 +38,7 @@ public static boolean isEnabled(ConsensusCommitConfig config) { public static class CoordinatorGroupCommitKeyManipulator implements KeyManipulator { private static final int PRIMARY_KEY_SIZE = 24; - private static final char DELIMITER = ':'; + private static final char DELIMITER = '$'; private static final int MAX_FULL_KEY_SIZE = 64; private static final int MAX_CHILD_KEY_SIZE = MAX_FULL_KEY_SIZE - PRIMARY_KEY_SIZE - 1 /* delimiter */; diff --git a/core/src/test/java/com/scalar/db/storage/cosmos/CosmosOperationCheckerTest.java b/core/src/test/java/com/scalar/db/storage/cosmos/CosmosOperationCheckerTest.java index 31c67ef57..d6f111d4d 100644 --- a/core/src/test/java/com/scalar/db/storage/cosmos/CosmosOperationCheckerTest.java +++ b/core/src/test/java/com/scalar/db/storage/cosmos/CosmosOperationCheckerTest.java @@ -13,11 +13,14 @@ import static org.mockito.MockitoAnnotations.openMocks; import com.scalar.db.api.Delete; +import com.scalar.db.api.Get; import com.scalar.db.api.MutationCondition; import com.scalar.db.api.Put; +import com.scalar.db.api.Scan; import com.scalar.db.api.TableMetadata; import com.scalar.db.common.TableMetadataManager; import com.scalar.db.config.DatabaseConfig; +import com.scalar.db.exception.storage.ExecutionException; import com.scalar.db.io.DataType; import com.scalar.db.io.Key; import java.nio.charset.StandardCharsets; @@ -34,6 +37,26 @@ public class CosmosOperationCheckerTest { private static final String CKEY1 = "c1"; private static final String COL1 = "v1"; private static final String COL2 = "v2"; + + private static final TableMetadata TABLE_METADATA1 = + TableMetadata.newBuilder() + .addColumn(PKEY1, DataType.INT) + .addColumn(CKEY1, DataType.INT) + .addColumn(COL1, DataType.INT) + .addColumn(COL2, DataType.BLOB) + .addPartitionKey(PKEY1) + .addClusteringKey(CKEY1) + .addSecondaryIndex(COL1) + .build(); + + private static final TableMetadata TABLE_METADATA2 = + TableMetadata.newBuilder() + .addColumn(PKEY1, DataType.TEXT) + .addColumn(CKEY1, DataType.TEXT) + .addPartitionKey(PKEY1) + .addClusteringKey(CKEY1) + .build(); + @Mock private DatabaseConfig databaseConfig; @Mock private TableMetadataManager metadataManager; private CosmosOperationChecker operationChecker; @@ -41,23 +64,14 @@ public class CosmosOperationCheckerTest { @BeforeEach public void setUp() throws Exception { openMocks(this).close(); - TableMetadata tableMetadata = - TableMetadata.newBuilder() - .addColumn(PKEY1, DataType.INT) - .addColumn(CKEY1, DataType.INT) - .addColumn(COL1, DataType.INT) - .addColumn(COL2, DataType.BLOB) - .addPartitionKey(PKEY1) - .addClusteringKey(CKEY1) - .addSecondaryIndex(COL1) - .build(); - when(metadataManager.getTableMetadata(any())).thenReturn(tableMetadata); + operationChecker = new CosmosOperationChecker(databaseConfig, metadataManager); } @Test - public void check_ForPutWithCondition_ShouldBehaveProperly() { + public void check_ForPutWithCondition_ShouldBehaveProperly() throws ExecutionException { // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA1); // Act Assert assertThatCode(() -> operationChecker.check(buildPutWithCondition(putIfExists()))) @@ -128,8 +142,9 @@ public void check_ForPutWithCondition_ShouldBehaveProperly() { } @Test - public void check_ForDeleteWithCondition_ShouldBehaveProperly() { + public void check_ForDeleteWithCondition_ShouldBehaveProperly() throws ExecutionException { // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA1); // Act Assert assertThatCode(() -> operationChecker.check(buildDeleteWithCondition(deleteIfExists()))) @@ -199,8 +214,11 @@ public void check_ForDeleteWithCondition_ShouldBehaveProperly() { } @Test - public void check_ForMutationsWithPutWithCondition_ShouldBehaveProperly() { + public void check_ForMutationsWithPutWithCondition_ShouldBehaveProperly() + throws ExecutionException { // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA1); + Put put = Put.newBuilder() .namespace(NAMESPACE_NAME) @@ -298,8 +316,11 @@ public void check_ForMutationsWithPutWithCondition_ShouldBehaveProperly() { } @Test - public void check_ForMutationsWithDeleteWithCondition_ShouldBehaveProperly() { + public void check_ForMutationsWithDeleteWithCondition_ShouldBehaveProperly() + throws ExecutionException { // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA1); + Delete delete = Delete.newBuilder() .namespace(NAMESPACE_NAME) @@ -397,6 +418,284 @@ public void check_ForMutationsWithDeleteWithCondition_ShouldBehaveProperly() { .isInstanceOf(IllegalArgumentException.class); } + @Test + public void + check_GetGiven_WhenIllegalCharacterInPrimaryKeyColumn_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA2); + + Get get1 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Get get2 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a:b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Get get3 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "a:b")) + .build(); + Get get4 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a/b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Get get5 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "a/b")) + .build(); + Get get6 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a\\b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Get get7 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "a\\b")) + .build(); + Get get8 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a#b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Get get9 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "a#b")) + .build(); + Get get10 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a?b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Get get11 = + Get.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "a?b")) + .build(); + + // Act Assert + assertThatCode(() -> operationChecker.check(get1)).doesNotThrowAnyException(); + assertThatThrownBy(() -> operationChecker.check(get2)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get3)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get4)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get5)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get6)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get7)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get8)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get9)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get10)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(get11)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + check_ScanGiven_WhenIllegalCharacterInPrimaryKeyColumn_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA2); + + Scan scan1 = + Scan.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .start(Key.ofText(CKEY1, "ab")) + .end(Key.ofText(CKEY1, "ab")) + .build(); + Scan scan2 = + Scan.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a:b")) + .start(Key.ofText(CKEY1, "ab")) + .end(Key.ofText(CKEY1, "ab")) + .build(); + Scan scan3 = + Scan.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .start(Key.ofText(CKEY1, "a:b")) + .end(Key.ofText(CKEY1, "ab")) + .build(); + Scan scan4 = + Scan.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .start(Key.ofText(CKEY1, "ab")) + .end(Key.ofText(CKEY1, "a:b")) + .build(); + + // Act Assert + assertThatCode(() -> operationChecker.check(scan1)).doesNotThrowAnyException(); + assertThatThrownBy(() -> operationChecker.check(scan2)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(scan3)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(scan4)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + check_PutGiven_WhenIllegalCharacterInPrimaryKeyColumn_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA2); + + Put put1 = + Put.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Put put2 = + Put.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a:b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Put put3 = + Put.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "a:b")) + .build(); + + // Act Assert + assertThatCode(() -> operationChecker.check(put1)).doesNotThrowAnyException(); + assertThatThrownBy(() -> operationChecker.check(put2)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(put3)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + check_DeleteGiven_WhenIllegalCharacterInPrimaryKeyColumn_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA2); + + Delete delete1 = + Delete.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Delete delete2 = + Delete.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a:b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Delete delete3 = + Delete.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "a:b")) + .build(); + + // Act Assert + assertThatCode(() -> operationChecker.check(delete1)).doesNotThrowAnyException(); + assertThatThrownBy(() -> operationChecker.check(delete2)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(delete3)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + check_MutationsGiven_WhenIllegalCharacterInPrimaryKeyColumn_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + when(metadataManager.getTableMetadata(any())).thenReturn(TABLE_METADATA2); + + Put put1 = + Put.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Put put2 = + Put.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a:b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Delete delete1 = + Delete.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "ab")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + Delete delete2 = + Delete.newBuilder() + .namespace(NAMESPACE_NAME) + .table(TABLE_NAME) + .partitionKey(Key.ofText(PKEY1, "a:b")) + .clusteringKey(Key.ofText(CKEY1, "ab")) + .build(); + + // Act Assert + assertThatCode(() -> operationChecker.check(Arrays.asList(put1, delete1))) + .doesNotThrowAnyException(); + assertThatThrownBy(() -> operationChecker.check(Arrays.asList(put2, delete1))) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> operationChecker.check(Arrays.asList(put1, delete2))) + .isInstanceOf(IllegalArgumentException.class); + } + private Put buildPutWithCondition(MutationCondition condition) { return Put.newBuilder() .namespace(NAMESPACE_NAME) diff --git a/core/src/test/java/com/scalar/db/storage/cosmos/CosmosTest.java b/core/src/test/java/com/scalar/db/storage/cosmos/CosmosTest.java index 55482672d..498534497 100644 --- a/core/src/test/java/com/scalar/db/storage/cosmos/CosmosTest.java +++ b/core/src/test/java/com/scalar/db/storage/cosmos/CosmosTest.java @@ -1,7 +1,9 @@ package com.scalar.db.storage.cosmos; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -9,7 +11,9 @@ import com.azure.cosmos.CosmosClient; import com.scalar.db.api.ConditionBuilder; import com.scalar.db.api.ConditionalExpression; +import com.scalar.db.api.Delete; import com.scalar.db.api.Get; +import com.scalar.db.api.Put; import com.scalar.db.api.Result; import com.scalar.db.api.Scan; import com.scalar.db.api.Scanner; @@ -18,6 +22,7 @@ import com.scalar.db.config.DatabaseConfig; import com.scalar.db.exception.storage.ExecutionException; import com.scalar.db.io.Key; +import java.util.Arrays; import java.util.Optional; import java.util.Properties; import org.junit.jupiter.api.BeforeEach; @@ -216,4 +221,105 @@ public void scan_WithConjunctionAndProjections_ShouldHandledWithExtendedProjecti Scan actualScan = captor.getValue(); assertThat(actualScan.getProjections()).containsExactlyInAnyOrder("col1", "col2"); } + + @Test + public void + get_IllegalArgumentExceptionThrownByOperationChecker_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + Get get = Get.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + doThrow(IllegalArgumentException.class).when(operationChecker).check(get); + + // Act Assert + assertThatThrownBy(() -> cosmos.get(get)).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + scan_IllegalArgumentExceptionThrownByOperationChecker_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + Scan scan = Scan.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + doThrow(IllegalArgumentException.class).when(operationChecker).check(scan); + + // Act Assert + assertThatThrownBy(() -> cosmos.scan(scan)).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + put_IllegalArgumentExceptionThrownByOperationChecker_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + Put put = Put.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + doThrow(IllegalArgumentException.class).when(operationChecker).check(put); + + // Act Assert + assertThatThrownBy(() -> cosmos.put(put)).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + put_MultiplePutsGiven_IllegalArgumentExceptionThrownByOperationChecker_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + Put put1 = Put.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + Put put2 = Put.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + + doThrow(IllegalArgumentException.class).when(operationChecker).check(Arrays.asList(put1, put2)); + + // Act Assert + assertThatThrownBy(() -> cosmos.put(Arrays.asList(put1, put2))) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + delete_IllegalArgumentExceptionThrownByOperationChecker_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + Delete delete = + Delete.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + doThrow(IllegalArgumentException.class).when(operationChecker).check(delete); + + // Act Assert + assertThatThrownBy(() -> cosmos.delete(delete)).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + delete_MultipleDeletesGiven_IllegalArgumentExceptionThrownByOperationChecker_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + Delete delete1 = + Delete.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + Delete delete2 = + Delete.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + + doThrow(IllegalArgumentException.class) + .when(operationChecker) + .check(Arrays.asList(delete1, delete2)); + + // Act Assert + assertThatThrownBy(() -> cosmos.delete(Arrays.asList(delete1, delete2))) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void + mutate_IllegalArgumentExceptionThrownByOperationChecker_ShouldThrowIllegalArgumentException() + throws ExecutionException { + // Arrange + Put put = Put.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + Delete delete = + Delete.newBuilder().namespace("ns").table("tbl").partitionKey(partitionKey).build(); + + doThrow(IllegalArgumentException.class) + .when(operationChecker) + .check(Arrays.asList(put, delete)); + + // Act Assert + assertThatThrownBy(() -> cosmos.mutate(Arrays.asList(put, delete))) + .isInstanceOf(IllegalArgumentException.class); + } } diff --git a/core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitterTest.java b/core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitterTest.java index 14318c1c4..40d2491c5 100644 --- a/core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitterTest.java +++ b/core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorGroupCommitterTest.java @@ -270,12 +270,12 @@ void isFullKey_GivenValidFullKey_ShouldReturnTrue() { // Act // Assert - assertThat(keyManipulator.isFullKey("012345678901234567890123:" + childTxId)).isTrue(); - assertThat(keyManipulator.isFullKey("abcdefghijklmnopqrstuvwx:" + childTxId)).isTrue(); - assertThat(keyManipulator.isFullKey("cdefghijklmnopqrstuvwxyz:" + childTxId)).isTrue(); - assertThat(keyManipulator.isFullKey("ABCDEFGHIJKLMNOPQRSTUVWX:" + childTxId)).isTrue(); - assertThat(keyManipulator.isFullKey("CDEFGHIJKLMNOPQRSTUVWXYZ:" + childTxId)).isTrue(); - assertThat(keyManipulator.isFullKey("0123456789abcdefghijWXYZ:" + childTxId)).isTrue(); + assertThat(keyManipulator.isFullKey("012345678901234567890123$" + childTxId)).isTrue(); + assertThat(keyManipulator.isFullKey("abcdefghijklmnopqrstuvwx$" + childTxId)).isTrue(); + assertThat(keyManipulator.isFullKey("cdefghijklmnopqrstuvwxyz$" + childTxId)).isTrue(); + assertThat(keyManipulator.isFullKey("ABCDEFGHIJKLMNOPQRSTUVWX$" + childTxId)).isTrue(); + assertThat(keyManipulator.isFullKey("CDEFGHIJKLMNOPQRSTUVWXYZ$" + childTxId)).isTrue(); + assertThat(keyManipulator.isFullKey("0123456789abcdefghijWXYZ$" + childTxId)).isTrue(); } @Test @@ -285,8 +285,8 @@ void isFullKey_GivenInvalidFullKey_ShouldReturnFalse() { // Act // Assert - assertThat(keyManipulator.isFullKey("01234567890123456789012:" + childTxId)).isFalse(); - assertThat(keyManipulator.isFullKey("0123456789012345678901234:" + childTxId)).isFalse(); + assertThat(keyManipulator.isFullKey("01234567890123456789012$" + childTxId)).isFalse(); + assertThat(keyManipulator.isFullKey("0123456789012345678901234$" + childTxId)).isFalse(); assertThat(keyManipulator.isFullKey("012345678901234567890123" + childTxId)).isFalse(); assertThat(keyManipulator.isFullKey("0123456789012345678901234" + childTxId)).isFalse(); } @@ -331,7 +331,7 @@ void fullKey_GivenArbitraryParentKeyAndChildKey_ShouldReturnProperValue() { // Act // Assert - assertThat(keyManipulator.fullKey(parentKey, childKey)).isEqualTo(parentKey + ":" + childKey); + assertThat(keyManipulator.fullKey(parentKey, childKey)).isEqualTo(parentKey + "$" + childKey); } } }