Skip to content

Backport to branch(3) : Add validation for import/export integer params to make sure they are positive #2745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions core/src/main/java/com/scalar/db/common/error/CoreError.java
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,14 @@ public enum CoreError implements ScalarDbError {
"Some scanners were not closed. All scanners must be closed before preparing the transaction.",
"",
""),
DATA_LOADER_INVALID_DATA_CHUNK_SIZE(
Category.USER_ERROR, "0207", "Data chunk size must be greater than 0", "", ""),
DATA_LOADER_INVALID_TRANSACTION_SIZE(
Category.USER_ERROR, "0208", "Transaction size must be greater than 0", "", ""),
DATA_LOADER_INVALID_MAX_THREADS(
Category.USER_ERROR, "0209", "Number of max threads must be greater than 0", "", ""),
DATA_LOADER_INVALID_DATA_CHUNK_QUEUE_SIZE(
Category.USER_ERROR, "0210", "Data chunk queue size must be greater than 0", "", ""),

//
// Errors for the concurrency error category
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -52,6 +54,16 @@ public class ImportCommand extends ImportCommandOptions implements Callable<Inte
public Integer call() throws Exception {
validateImportTarget(controlFilePath, namespace, tableName);
validateLogDirectory(logDirectory);
validatePositiveValue(
spec.commandLine(), dataChunkSize, CoreError.DATA_LOADER_INVALID_DATA_CHUNK_SIZE);
validatePositiveValue(
spec.commandLine(), transactionSize, CoreError.DATA_LOADER_INVALID_TRANSACTION_SIZE);
validatePositiveValue(
spec.commandLine(), maxThreads, CoreError.DATA_LOADER_INVALID_MAX_THREADS);
validatePositiveValue(
spec.commandLine(),
dataChunkQueueSize,
CoreError.DATA_LOADER_INVALID_DATA_CHUNK_QUEUE_SIZE);
ControlFile controlFile = parseControlFileFromPath(controlFilePath).orElse(null);
ImportOptions importOptions = createImportOptions(controlFile);
ImportLoggerConfig config =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.Map;
import java.util.Objects;
import org.apache.commons.lang3.StringUtils;
import picocli.CommandLine;

public class CommandLineInputUtils {

Expand Down Expand Up @@ -46,4 +47,18 @@ public static String[] splitByDelimiter(String value, String delimiter, int limi
delimiter, CoreError.DATA_LOADER_SPLIT_INPUT_DELIMITER_NULL.buildMessage());
return value.split(delimiter, limit);
}

/**
* Validates that a given integer value is positive. If the value is less than 1, it throws a
* {@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 error the error that is thrown when the value is invalid
*/
public static void validatePositiveValue(CommandLine commandLine, int value, CoreError error) {
if (value < 1) {
throw new CommandLine.ParameterException(commandLine, error.buildMessage());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ void call_withBlankScalarDBConfigurationFile_shouldThrowException() {
ExportCommand exportCommand = new ExportCommand();
exportCommand.configFilePath = "";
exportCommand.dataChunkSize = 100;
exportCommand.maxThreads = 4;
exportCommand.namespace = "scalar";
exportCommand.table = "asset";
exportCommand.outputDirectory = "";
Expand All @@ -46,6 +47,7 @@ void call_withInvalidScalarDBConfigurationFile_shouldReturnOne() throws Exceptio
ExportCommand exportCommand = new ExportCommand();
exportCommand.configFilePath = "scalardb.properties";
exportCommand.dataChunkSize = 100;
exportCommand.maxThreads = 4;
exportCommand.namespace = "scalar";
exportCommand.table = "asset";
exportCommand.outputDirectory = "";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ void call_WithoutValidConfigFile_ShouldThrowException() throws Exception {
importCommand.sourceFileFormat = FileFormat.JSON;
importCommand.sourceFilePath = importFile.toString();
importCommand.importMode = ImportMode.UPSERT;
importCommand.dataChunkSize = 100;
importCommand.transactionSize = 10;
importCommand.maxThreads = 4;
importCommand.dataChunkQueueSize = 64;
assertThrows(IllegalArgumentException.class, () -> importCommand.call());
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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 {

Expand Down Expand Up @@ -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()));
}
}