Add util classes for data loader CLI #2616
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 378effb.
6 tasks
This was referenced Apr 30, 2025
There was a problem hiding this comment.
Pull Request Overview
A PR to add file and directory utility classes for the data loader CLI along with comprehensive tests and relevant error messages.
- Added FileUtils and DirectoryUtils for file path and directory validations.
- Updated related tests and command usage to reflect the new validation methods.
- Introduced new error codes and messages in CoreError for improved error handling.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| FileUtilsTest.java | Added tests for file path validation using FileUtils. |
| DirectoryUtilsTest.java | Updated tests to use the new validateOrCreateTargetDirectory method. |
| InvalidFilePathException.java | Introduced a custom exception for invalid file paths. |
| FileUtils.java | Implemented a file path validation utility. |
| DirectoryUtils.java | Implemented enhanced directory validation/creation and added working directory validation. |
| ExportCommand.java | Updated directory validation method calls to use the new utility method. |
| CoreError.java | Added multiple new error messages for the data loader CLI. |
Comments suppressed due to low confidence (2)
data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/util/FileUtilsTest.java:10
- [nitpick] Since this is a constant, consider renaming 'currentPath' to all uppercase (e.g., CURRENT_PATH) to follow Java naming conventions.
private static final String currentPath = Paths.get("").toAbsolutePath().toString();
core/src/main/java/com/scalar/db/common/error/CoreError.java:869
- There is a spacing issue in the error message ('and'--table' should likely be 'and '--table'') to improve readability.
DATA_LOADER_IMPORT_TARGET_MISSING( Category.USER_ERROR, "0189", "Missing option: either '--namespace' and'--table' options must be specified.", "", ""),
komamitsu
reviewed
May 12, 2025
| if (!pathToCheck.isAbsolute()) { | ||
| // If the path is not absolute, it's either a file name or a relative path | ||
| Path currentDirectory = Paths.get("").toAbsolutePath(); | ||
| Path fileInCurrentDirectory = currentDirectory.resolve(pathToCheck); |
There was a problem hiding this comment.
It's not sufficient to use only pathToCheck without currentDirectory?
There was a problem hiding this comment.
Yes, only need to check pathToCheck exists. The specific condition is not required. I had added this in an early phase where I tried testing with a file name passed as an argument and that scenario is no longer required.
I have removed this part.
Thank you.
brfrn169
reviewed
May 12, 2025
| */ | ||
| public static void validateFilePath(String filePath) throws InvalidFilePathException { | ||
| if (StringUtils.isBlank(filePath)) { | ||
| throw new IllegalArgumentException("File path must not be blank."); |
There was a problem hiding this comment.
Please move this message to CoreError.
| Path fileInCurrentDirectory = currentDirectory.resolve(pathToCheck); | ||
|
|
||
| if (!fileInCurrentDirectory.toFile().exists()) { | ||
| throw new InvalidFilePathException("File not found: " + pathToCheck); |
|
|
||
| // It's an absolute path | ||
| if (!pathToCheck.toFile().exists()) { | ||
| throw new InvalidFilePathException("File not found: " + pathToCheck); |
|
@komamitsu san, @brfrn169 san, I have made changes based on the feedback. |
This was referenced May 14, 2025
feeblefakie
pushed a commit
that referenced
this pull request
May 14, 2025
Co-authored-by: Peckstadt Yves <[email protected]> Co-authored-by: Toshihiro Suzuki <[email protected]>
inv-jishnu
added a commit
that referenced
this pull request
May 14, 2025
Co-authored-by: Peckstadt Yves <[email protected]> Co-authored-by: Toshihiro Suzuki <[email protected]>
inv-jishnu
added a commit
that referenced
this pull request
May 14, 2025
Co-authored-by: Peckstadt Yves <[email protected]> Co-authored-by: Toshihiro Suzuki <[email protected]>
inv-jishnu
added a commit
that referenced
this pull request
May 14, 2025
Co-authored-by: Peckstadt Yves <[email protected]> Co-authored-by: Toshihiro Suzuki <[email protected]>
inv-jishnu
added a commit
that referenced
this pull request
May 14, 2025
Co-authored-by: Peckstadt Yves <[email protected]> Co-authored-by: Toshihiro Suzuki <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adding file and directory util class for data loader CLI for validation.
Related issues and/or PRs
Please review this PR once the below PR is merged and the changes from master branch is updated in this branch
Changes made
Add a util classs to validate files, file path, log directory, write access to directory etc
Checklist
Additional notes (optional)
This is the first PR for data loader CLI
Release notes
NA