[CUS-9787] clear cell value from csv file. #286
Conversation
WalkthroughThis PR updates project and dependency versions, introduces four new CSV WebAction classes for cell manipulation (read, write, delete operations) with local file and HTTP(S) URL support, and enhances input validation in an existing CSV handler class. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/WriteCsvFileandStorePath.java (1)
169-191: Add cleanup for temporary files downloaded from URLs.Similar to other files in this PR, temporary files created from URLs via
convertToFileare not cleaned up. This can lead to disk space exhaustion over time.Consider adding
tempFile.deleteOnExit()after creating the temp file on line 187, or implement explicit cleanup logic.Also note that this
convertToFilemethod differs slightly fromurlToCSVFileConverterused in other files. Consider consolidating these into a single shared utility method as mentioned in comments on other files.
♻️ Duplicate comments (3)
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/DeleteRowContentFromCsv.java (1)
74-74: Consolidate duplicated URL-to-file conversion logic.This file contains the same issues as
ReadCellValueFromCsv.java:
- The
urlToCSVFileConvertermethod is duplicated- Hard-coded
"csvFileName"parameter- Temporary files are not cleaned up
Refer to the comments on
ReadCellValueFromCsv.javafor detailed recommendations. Consider extracting this to a shared utility class.Also applies to: 127-149
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/WriteCellValueInCsv.java (2)
77-77: Consolidate duplicated code and fix temp file cleanup.This file has the same issues identified in
ReadCellValueFromCsv.javaandDeleteRowContentFromCsv.java:
urlToCSVFileConvertermethod duplication- Hard-coded
"csvFileName"parameter- Missing temporary file cleanup
Please refer to the detailed comments on
ReadCellValueFromCsv.javafor recommended solutions.Also applies to: 130-152
100-107: Use atomic file replacement pattern.Similar to
DeleteRowContentFromCsv.java, writing directly to the CSV file could cause data corruption if concurrent modifications occur or if the process crashes mid-write.Consider adopting the pattern used in
WriteCsvFileandStorePathandDeleteRowContentFromCsvAndStorePath, which use a temporary file and atomic copy operations.
🧹 Nitpick comments (4)
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/ReadCellValueFromCsv.java (3)
82-82: Replace hard-coded filename with a meaningful value.The parameter
"csvFileName"is a hard-coded placeholder. Consider deriving a meaningful name from the URL or using a descriptive prefix.Apply this diff:
- File tempFile = urlToCSVFileConverter("csvFileName", filePathString); + File tempFile = urlToCSVFileConverter("csv_download", filePathString);
146-151: Use appropriate log level for error conditions.Lines 146 and 151 use
logger.infofor error conditions. Uselogger.errororlogger.warnfor exceptions and failures.Apply this diff:
} catch (Exception e) { - logger.info("Error while accessing: " + url); + logger.error("Error while accessing: " + url, e); throw new RuntimeException("Unable to access or validate the CSV file. Please check the inputs.", e); }
96-96: Remove redundant bounds check.The condition
rowIndex < rows.size()is sufficient. The checkrowIndex >= 0is redundant since line 66 already validatestargetRow >= 1, ensuringrowIndex = targetRow - 1 >= 0.Apply this diff:
- if (rowIndex < rows.size()) { + if (rowIndex < rows.size()) {Actually, the current code already has the correct logic. Consider this a note that
rowIndex >= 0is always true and could be removed from similar checks in other files for clarity.csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/WriteCsvFileandStorePath.java (1)
125-125: Address the inline comment about logger usage.The comment suggests using
logger.errorfor exceptions, but the code still useslogger.warn. Either update the code to match the comment or remove the comment iflogger.warnis the intended level.Apply this diff:
- logger.warn("Error processing CSV file: " + e); // Use logger.error for exceptions + logger.error("Error processing CSV file: " + e);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
csv_file_update_from_upload_section/pom.xml(2 hunks)csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/DeleteRowContentFromCsv.java(1 hunks)csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/DeleteRowContentFromCsvAndStorePath.java(1 hunks)csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/ReadCellValueFromCsv.java(1 hunks)csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/WriteCellValueInCsv.java(1 hunks)csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/WriteCsvFileandStorePath.java(7 hunks)
🔇 Additional comments (4)
csv_file_update_from_upload_section/pom.xml (1)
9-9: LGTM!The version bump to 1.0.5 and the commons-lang3 upgrade to 3.17.0 align with the new CSV manipulation features being added.
Also applies to: 78-78
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/WriteCsvFileandStorePath.java (1)
57-76: LGTM!The addition of robust input validation for row and column numbers is well-implemented. The validation ensures numeric inputs and enforces 1-based indexing constraints with clear error messages.
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/DeleteRowContentFromCsvAndStorePath.java (2)
80-158: LGTM - Good use of atomic file replacement pattern!This implementation correctly uses a temporary file and
Files.copywithREPLACE_EXISTINGto atomically replace the original file. This approach prevents data corruption from concurrent modifications or crashes mid-write, which is superior to the direct file write approach used inDeleteRowContentFromCsvandWriteCellValueInCsv.
153-153: Add missing space in log message.The log message is missing a space after the colon.
Apply this diff:
- logger.info("Successfully copied content to original file: " + csvFile.getAbsolutePath()); + logger.info("Successfully copied content to original file: " + csvFile.getAbsolutePath());Wait, checking the code again... the message already has the space. This is correct. Disregard this comment.
| try (CSVWriter writer = new CSVWriter(new FileWriter(csvFilePath))) { | ||
| writer.writeAll(rows); | ||
| } catch (IOException e) { | ||
| logger.warn("Error writing to CSV file: " + ExceptionUtils.getStackTrace(e)); | ||
| setErrorMessage("Error writing to CSV file: " + e.getMessage()); | ||
| result = com.testsigma.sdk.Result.FAILED; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Potential data loss if concurrent modifications occur.
When processing a URL-based CSV, the downloaded temp file is modified and written back to the same temp file. For local file paths, writing back to the original file could cause data corruption if multiple actions run concurrently or if the process crashes mid-write.
Consider using the same pattern as DeleteRowContentFromCsvAndStorePath and WriteCsvFileandStorePath, which create a separate temporary file for the modified content and then copy it back atomically using Files.copy with StandardCopyOption.REPLACE_EXISTING.
🤖 Prompt for AI Agents
In
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/DeleteRowContentFromCsv.java
around lines 97 to 104, the current code writes modified CSV rows directly back
to the same file which risks data loss/corruption on concurrent runs or crashes;
change the logic to write to a new temporary file (use Files.createTempFile or
the same temp pattern used in DeleteRowContentFromCsvAndStorePath and
WriteCsvFileandStorePath), close the writer, then atomically replace the
original target file by copying the temp file to the original path with
Files.copy(tempPath, originalPath, StandardCopyOption.REPLACE_EXISTING); ensure
proper try-with-resources, exception handling, and cleanup of the temp file in
finally so partial writes do not corrupt the original.
| private File convertToFile(String pathOrUrl) throws IOException { | ||
| if (pathOrUrl.startsWith("https://") || pathOrUrl.startsWith("http://")) { | ||
| // Extract the original file name from the URL | ||
| String originalFileName = FilenameUtils.getName(new URL(pathOrUrl).getPath()); | ||
| // Generate a unique file name by appending a timestamp | ||
| String uniqueFileName = "temp_" + System.currentTimeMillis() + "_" + originalFileName; | ||
| logger.info("Given is a URL... Original file name: " + originalFileName + ", Unique file name: " | ||
| + uniqueFileName); | ||
|
|
||
| // Create the full path for the temporary file | ||
| String filePath = String.format("%s%s%s", FileUtils.getTempDirectoryPath(), File.separator, uniqueFileName); | ||
| File tempFile = new File(filePath); | ||
|
|
||
| // Download the file from the URL to the temporary location | ||
| FileUtils.copyURLToFile(new URL(pathOrUrl), tempFile, 10000, 10000); | ||
| logger.info("Temp file created for URL file: " + uniqueFileName + " at path " + filePath); | ||
|
|
||
| return tempFile; | ||
| } else { | ||
| logger.info("Given is a local file path..."); | ||
| return new File(pathOrUrl); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consolidate convertToFile with similar methods and add temp file cleanup.
The convertToFile method is duplicated in WriteCsvFileandStorePath. Additionally, this method is functionally similar to urlToCSVFileConverter used in other CSV action classes, though with slightly different implementation details.
- Extract this method to a shared utility class to eliminate duplication
- Add
tempFile.deleteOnExit()after line 187 to clean up downloaded files - Consider adding connection/read timeouts to
FileUtils.copyURLToFile(note: this file already has 10-second timeouts on line 190, which is good)
🤖 Prompt for AI Agents
In
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/DeleteRowContentFromCsvAndStorePath.java
around lines 176 to 198, the convertToFile method is duplicated elsewhere and
lacks automatic cleanup of downloaded temp files; extract this logic into a
shared utility class (e.g., CsvFileUtils or FileDownloadUtils) and replace
duplicate implementations in WriteCsvFileandStorePath and other CSV action
classes with calls to the new utility, add tempFile.deleteOnExit() immediately
after creating the tempFile (after line 187) so downloaded files are scheduled
for removal, and keep the existing FileUtils.copyURLToFile call with the 10s
connect/read timeouts (or make them configurable in the utility) to avoid
blocking.
| public File urlToCSVFileConverter(String fileName, String url) { | ||
| try { | ||
| logger.info("Given is URL... File name: " + fileName); | ||
| URL urlObject = new URL(url); | ||
|
|
||
| // Extract file extension if present | ||
| String baseName = fileName; | ||
| String extension = ".csv"; // default csv format | ||
| int lastDotIndex = fileName.lastIndexOf('.'); | ||
| if (lastDotIndex > 0) { | ||
| baseName = fileName.substring(0, lastDotIndex); | ||
| extension = fileName.substring(lastDotIndex); | ||
| } | ||
|
|
||
| File tempFile = File.createTempFile(baseName, extension); | ||
| FileUtils.copyURLToFile(urlObject, tempFile); | ||
| logger.info("CSV file created: " + tempFile.getAbsolutePath()); | ||
| return tempFile; | ||
| } catch (Exception e) { | ||
| logger.info("Error while accessing: " + url); | ||
| throw new RuntimeException("Unable to access or validate the CSV file. Please check the inputs.", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated helper method to a shared utility class.
The urlToCSVFileConverter method is duplicated across ReadCellValueFromCsv, DeleteRowContentFromCsv, and WriteCellValueInCsv. Additionally, WriteCsvFileandStorePath and DeleteRowContentFromCsvAndStorePath have a similar convertToFile method with slightly different implementation.
Consider creating a shared utility class (e.g., CsvFileHelper) to consolidate these URL-to-file conversion methods and ensure consistent behavior across all CSV actions.
🤖 Prompt for AI Agents
In
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/ReadCellValueFromCsv.java
around lines 132 to 154, the urlToCSVFileConverter implementation is duplicated
across multiple classes; extract this logic into a new shared utility class
(e.g., com.testsigma.addons.web.util.CsvFileHelper) with a single public static
method (e.g., urlToCSVFileConverter(String fileName, String url)) that contains
the file-extension extraction, temp file creation, FileUtils.copyURLToFile call,
consistent logging, and exception wrapping; replace the duplicated methods in
ReadCellValueFromCsv, DeleteRowContentFromCsv, WriteCellValueInCsv,
WriteCsvFileandStorePath, and DeleteRowContentFromCsvAndStorePath with calls to
CsvFileHelper.urlToCSVFileConverter, keep identical behavior (default .csv
extension, same log messages, and same RuntimeException wrapping) and add a
small unit test for the helper to validate extension handling and URL copy.
Add cleanup for temporary files created from URLs.
Temporary files downloaded from URLs are not cleaned up, which can lead to disk space exhaustion over time.
Consider one of these approaches:
- Call
tempFile.deleteOnExit()after creating the temp file - Use try-with-resources with a custom AutoCloseable wrapper
- Explicitly delete the file in a finally block after CSV processing completes
Example:
File tempFile = File.createTempFile(baseName, extension);
+tempFile.deleteOnExit();
FileUtils.copyURLToFile(urlObject, tempFile);🤖 Prompt for AI Agents
In
csv_file_update_from_upload_section/src/main/java/com/testsigma/addons/web/ReadCellValueFromCsv.java
around lines 132 to 154, the method creates temporary files from URLs but never
schedules or performs cleanup; to fix this, after creating the tempFile call
tempFile.deleteOnExit() to ensure JVM cleans it up as a minimal change, or
alternatively (preferred for long-running processes) change the API so the
caller is responsible for deletion (document it) or provide a try-with-resources
style wrapper/AutoCloseable that deletes the file in close() so the file is
explicitly deleted after CSV processing completes.
please review this addon and publish as PUBLIC
Addon name : csv_file_update_from_upload_section
Addon accont: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira: https://testsigma.atlassian.net/browse/CUS-9787
fix
Added 4 New Nlps, 2 Nlp's to clear cell value at given index, 1 Nlp to write contents in a cell, 1 Nlp to read the contents
Updated existing class to use 1-based indexing.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.