From d549568b3e74660ed2df9ba95ded95f2d2a0b712 Mon Sep 17 00:00:00 2001 From: thongdk8 Date: Tue, 3 Jun 2025 16:13:31 +0900 Subject: [PATCH 1/4] Add validation for import/export interger params to make sure they are positive --- .../com/scalar/db/common/error/CoreError.java | 8 ++ .../cli/command/dataexport/ExportCommand.java | 5 + .../cli/command/dataimport/ImportCommand.java | 12 +++ .../cli/util/CommandLineInputUtils.java | 14 +++ .../command/dataexport/ExportCommandTest.java | 2 + .../command/dataimport/ImportCommandTest.java | 4 + .../cli/util/CommandLineInputUtilsTest.java | 94 +++++++++++++++++++ 7 files changed, 139 insertions(+) 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 acaa4566dd..197f8c3dec 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 @@ -911,6 +911,14 @@ public enum CoreError implements ScalarDbError { Category.USER_ERROR, "0203", "Delimiter must not be null", "", ""), DATA_LOADER_CONFIG_FILE_PATH_BLANK( Category.USER_ERROR, "0204", "Config file path must not be blank", "", ""), + DATA_LOADER_INVALID_DATA_CHUNK_SIZE( + Category.USER_ERROR, "0206", "Data chunk size must be greater than 0", "", ""), + DATA_LOADER_INVALID_TRANSACTION_SIZE( + Category.USER_ERROR, "0207", "Transaction size must be greater than 0", "", ""), + DATA_LOADER_INVALID_MAX_THREADS( + Category.USER_ERROR, "0208", "Number of max threads must be greater than 0", "", ""), + DATA_LOADER_INVALID_DATA_CHUNK_QUEUE_SIZE( + Category.USER_ERROR, "0209", "Data chunk queue size must be greater than 0", "", ""), // // Errors for the concurrency error category diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java index fdedbeef2c..664bb079e8 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java @@ -1,5 +1,6 @@ package com.scalar.db.dataloader.cli.command.dataexport; +import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.validatePositiveValue; import static java.nio.file.StandardOpenOption.APPEND; import static java.nio.file.StandardOpenOption.CREATE; @@ -56,6 +57,10 @@ public Integer call() throws Exception { try { validateOutputDirectory(); FileUtils.validateFilePath(scalarDbPropertiesFilePath); + validatePositiveValue( + spec.commandLine(), dataChunkSize, CoreError.DATA_LOADER_INVALID_DATA_CHUNK_SIZE); + validatePositiveValue( + spec.commandLine(), maxThreads, CoreError.DATA_LOADER_INVALID_MAX_THREADS); StorageFactory storageFactory = StorageFactory.create(scalarDbPropertiesFilePath); TableMetadataService metaDataService = diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommand.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommand.java index 604adc0586..a505a42ade 100755 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommand.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommand.java @@ -1,5 +1,7 @@ package com.scalar.db.dataloader.cli.command.dataimport; +import static com.scalar.db.dataloader.cli.util.CommandLineInputUtils.validatePositiveValue; + import com.fasterxml.jackson.databind.ObjectMapper; import com.scalar.db.api.DistributedStorageAdmin; import com.scalar.db.api.TableMetadata; @@ -52,6 +54,16 @@ public class ImportCommand extends ImportCommandOptions implements Callable importCommand.call()); } diff --git a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java index 7faebef61a..49e735b522 100644 --- a/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java +++ b/data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtilsTest.java @@ -1,13 +1,16 @@ package com.scalar.db.dataloader.cli.util; import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; import com.scalar.db.common.error.CoreError; import java.util.Map; import org.junit.jupiter.api.Test; +import picocli.CommandLine; class CommandLineInputUtilsTest { @@ -97,4 +100,95 @@ void splitByDelimiter_nullDelimiter_shouldThrowException() { .getMessage() .contains(CoreError.DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL.buildMessage())); } + + @Test + public void validatePositiveValue_positiveValue_shouldNotThrowException() { + // Arrange + CommandLine commandLine = mock(CommandLine.class); + int positiveValue = 5; + + // Act & Assert - No exception should be thrown + assertDoesNotThrow( + () -> + CommandLineInputUtils.validatePositiveValue( + commandLine, positiveValue, CoreError.DATA_LOADER_INVALID_DATA_CHUNK_SIZE)); + } + + @Test + public void validatePositiveValue_one_shouldNotThrowException() { + // Arrange + CommandLine commandLine = mock(CommandLine.class); + int minimumPositiveValue = 1; + + // Act & Assert - No exception should be thrown + assertDoesNotThrow( + () -> + CommandLineInputUtils.validatePositiveValue( + commandLine, minimumPositiveValue, CoreError.DATA_LOADER_INVALID_DATA_CHUNK_SIZE)); + } + + @Test + public void validatePositiveValue_zero_shouldThrowException() { + // Arrange + CommandLine commandLine = mock(CommandLine.class); + int zeroValue = 0; + CoreError error = CoreError.DATA_LOADER_INVALID_DATA_CHUNK_SIZE; + + // Act & Assert + CommandLine.ParameterException exception = + assertThrows( + CommandLine.ParameterException.class, + () -> CommandLineInputUtils.validatePositiveValue(commandLine, zeroValue, error)); + + // Verify the exception message contains the error message + assertTrue(exception.getMessage().contains(error.buildMessage())); + } + + @Test + public void validatePositiveValue_negativeValue_shouldThrowException() { + // Arrange + CommandLine commandLine = mock(CommandLine.class); + int negativeValue = -5; + CoreError error = CoreError.DATA_LOADER_INVALID_TRANSACTION_SIZE; + + // Act & Assert + CommandLine.ParameterException exception = + assertThrows( + CommandLine.ParameterException.class, + () -> CommandLineInputUtils.validatePositiveValue(commandLine, negativeValue, error)); + + // Verify the exception message contains the error message + assertTrue(exception.getMessage().contains(error.buildMessage())); + } + + @Test + public void validatePositiveValue_differentErrorTypes_shouldUseCorrectErrorMessage() { + // Arrange + CommandLine commandLine = mock(CommandLine.class); + int negativeValue = -1; + + // Act & Assert for DATA_LOADER_INVALID_MAX_THREADS + CommandLine.ParameterException exception1 = + assertThrows( + CommandLine.ParameterException.class, + () -> + CommandLineInputUtils.validatePositiveValue( + commandLine, negativeValue, CoreError.DATA_LOADER_INVALID_MAX_THREADS)); + assertTrue( + exception1.getMessage().contains(CoreError.DATA_LOADER_INVALID_MAX_THREADS.buildMessage())); + + // Act & Assert for DATA_LOADER_INVALID_DATA_CHUNK_QUEUE_SIZE + CommandLine.ParameterException exception2 = + assertThrows( + CommandLine.ParameterException.class, + () -> + CommandLineInputUtils.validatePositiveValue( + commandLine, + negativeValue, + CoreError.DATA_LOADER_INVALID_DATA_CHUNK_QUEUE_SIZE)); + assertTrue( + exception2 + .getMessage() + .contains(CoreError.DATA_LOADER_INVALID_DATA_CHUNK_QUEUE_SIZE.buildMessage())); + } } From 01800c06b9193729904cb2d4c3086222d07a5736 Mon Sep 17 00:00:00 2001 From: thongdk8 Date: Tue, 3 Jun 2025 18:57:30 +0900 Subject: [PATCH 2/4] Correct the error code id --- .../main/java/com/scalar/db/common/error/CoreError.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 197f8c3dec..8cce90e7f0 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 @@ -912,13 +912,13 @@ public enum CoreError implements ScalarDbError { DATA_LOADER_CONFIG_FILE_PATH_BLANK( Category.USER_ERROR, "0204", "Config file path must not be blank", "", ""), DATA_LOADER_INVALID_DATA_CHUNK_SIZE( - Category.USER_ERROR, "0206", "Data chunk size must be greater than 0", "", ""), + Category.USER_ERROR, "0205", "Data chunk size must be greater than 0", "", ""), DATA_LOADER_INVALID_TRANSACTION_SIZE( - Category.USER_ERROR, "0207", "Transaction size must be greater than 0", "", ""), + Category.USER_ERROR, "0206", "Transaction size must be greater than 0", "", ""), DATA_LOADER_INVALID_MAX_THREADS( - Category.USER_ERROR, "0208", "Number of max threads must be greater than 0", "", ""), + Category.USER_ERROR, "0207", "Number of max threads must be greater than 0", "", ""), DATA_LOADER_INVALID_DATA_CHUNK_QUEUE_SIZE( - Category.USER_ERROR, "0209", "Data chunk queue size must be greater than 0", "", ""), + Category.USER_ERROR, "0208", "Data chunk queue size must be greater than 0", "", ""), // // Errors for the concurrency error category From 2209ff1829b079eceb6d6a11892d8c789b1ecd60 Mon Sep 17 00:00:00 2001 From: thongdk8 Date: Wed, 4 Jun 2025 15:34:12 +0900 Subject: [PATCH 3/4] resolve conflict --- core/src/main/java/com/scalar/db/common/error/CoreError.java | 2 ++ 1 file changed, 2 insertions(+) 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 5e38b9cc17..b6e450107e 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 @@ -1202,6 +1202,8 @@ public enum CoreError implements ScalarDbError { Category.INTERNAL_ERROR, "0052", "Failed to read JSON file. Details: %s.", "", ""), DATA_LOADER_JSONLINES_FILE_READ_FAILED( Category.INTERNAL_ERROR, "0053", "Failed to read JSON Lines file. Details: %s.", "", ""), + JDBC_TRANSACTION_GETTING_SCANNER_FAILED( + Category.INTERNAL_ERROR, "0054", "Getting the scanner failed. Details: %s", "", ""), // // Errors for the unknown transaction status error category From 064b8c6e472242b863d4a10d67f8d4b21f70e545 Mon Sep 17 00:00:00 2001 From: thongdk8 Date: Thu, 5 Jun 2025 13:09:10 +0900 Subject: [PATCH 4/4] update java doc --- .../scalar/db/dataloader/cli/util/CommandLineInputUtils.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java index 64ec3ec577..e3c617d509 100644 --- a/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java +++ b/data-loader/cli/src/main/java/com/scalar/db/dataloader/cli/util/CommandLineInputUtils.java @@ -53,7 +53,8 @@ public static String[] splitByDelimiter(String value, String delimiter, int limi * {@link CommandLine.ParameterException} with the specified error message. * * @param commandLine the {@link CommandLine} instance used to provide context for the exception - * @param value the integer value to validate @ + * @param value the integer value to validate + * @param error the error that is thrown when the value is invalid */ public static void validatePositiveValue(CommandLine commandLine, int value, CoreError error) { if (value < 1) {