[MOSIP-27141] & [MOSIP-28564] Fixed the bulk upload issues #1263
Conversation
WalkthroughThe pull request adds delimiter validation to the bulk upload processing flow by introducing a new error code for invalid delimiters and implementing validation logic that checks file delimiters before batch processing. Additionally, transaction status update behavior is refined to prevent overwriting a "FAILED" status with "COMPLETED". Changes
Sequence DiagramsequenceDiagram
participant Client
participant BulkUploadService
participant TransactionDB
participant FileStream
Client->>BulkUploadService: importDataFromFile(file)
alt File validation
BulkUploadService->>BulkUploadService: Check file extension
BulkUploadService->>TransactionDB: Persist status=FAILED
BulkUploadService-->>Client: Throw error
end
alt Delimiter validation (NEW)
BulkUploadService->>FileStream: Read first line
BulkUploadService->>BulkUploadService: Verify delimiter matches config
alt Delimiter mismatch
BulkUploadService->>TransactionDB: Persist status=FAILED
BulkUploadService-->>Client: Throw INVALID_DELIMITER error
end
end
BulkUploadService->>BulkUploadService: Process batch/CSV entries
rect rgba(200, 220, 255, 0.3)
Note over BulkUploadService,TransactionDB: Updated status logic
BulkUploadService->>BulkUploadService: Check current status
alt Current status not FAILED
BulkUploadService->>TransactionDB: Set status=COMPLETED with UTC timestamp
else Current status already FAILED
BulkUploadService->>TransactionDB: Keep status=FAILED (no override)
end
end
BulkUploadService-->>Client: Return result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
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 |
...vice/src/main/java/io/mosip/admin/bulkdataupload/service/impl/BulkDataUploadServiceImpl.java
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
admin/admin-service/src/main/java/io/mosip/admin/bulkdataupload/service/impl/BulkDataUploadServiceImpl.java (1)
336-338: Good defensive logic to preserve FAILED status.The change correctly prevents overwriting a FAILED status with COMPLETED, addressing potential race conditions in the batch processing flow.
For slightly better readability, consider:
-if (bulkUploadTranscation.getStatusCode() == null || !bulkUploadTranscation.getStatusCode().equalsIgnoreCase("FAILED")) { - bulkUploadTranscation.setStatusCode("COMPLETED"); -} +String currentStatus = bulkUploadTranscation.getStatusCode(); +if (currentStatus == null || !currentStatus.equalsIgnoreCase("FAILED")) { + bulkUploadTranscation.setStatusCode("COMPLETED"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
admin/admin-service/src/main/java/io/mosip/admin/bulkdataupload/constant/BulkUploadErrorCode.java(1 hunks)admin/admin-service/src/main/java/io/mosip/admin/bulkdataupload/service/impl/BulkDataUploadServiceImpl.java(4 hunks)
🔇 Additional comments (4)
admin/admin-service/src/main/java/io/mosip/admin/bulkdataupload/constant/BulkUploadErrorCode.java (1)
20-21: LGTM! New error code added correctly.The new
INVALID_DELIMITERenum constant is properly added with a unique error code and clear message. The trailing comma on line 20 correctly allows for the new constant.admin/admin-service/src/main/java/io/mosip/admin/bulkdataupload/service/impl/BulkDataUploadServiceImpl.java (3)
19-21: LGTM! Import additions support the new validation logic.The added Jakarta persistence and I/O imports are necessary for the delimiter validation feature and proper dependency injection.
Also applies to: 63-63, 65-65
256-257: LGTM! Transaction status properly persisted before exceptions.These changes ensure that validation failures are recorded in the database before throwing exceptions, providing better traceability for bulk upload failures.
Also applies to: 263-264
271-272: Now I need to look at the actual code structure around line 289 to verify the execution flow:Based on my verification, the original review comment is based on a misunderstanding of Spring's MultipartFile behavior and Spring Batch's resource lifecycle.
The multiple getInputStream() calls are not a problem.
Spring's MultipartFile has a getInputStream method, but this one can be called multiple times. MultipartFileResource adapts MultipartFile to Resource, exposing the content as InputStream, and each call returns a fresh InputStream. The first call at line 271 is properly wrapped in try-with-resources and closes cleanly. When line 474 or 492 subsequently calls
file.getInputStream(), they get a new, independent InputStream.Spring Batch will automatically close the resources once the step is finished. The FlatFileItemReader and PoiItemReader are managed by Spring Batch's ItemStream lifecycle, which handles closing these resources appropriately. No resource leak occurs.
Likely an incorrect or invalid review comment.
| Boolean isCSV = file.getOriginalFilename().endsWith(".csv"); | ||
|
|
||
| try (BufferedReader reader = new BufferedReader(new InputStreamReader(file.getInputStream()))) { | ||
| String firstLine = reader.readLine(); | ||
| if (firstLine != null && !firstLine.contains(lineDelimiter)) { | ||
| bulkUploadTranscation.setStatusCode("FAILED"); | ||
| updateBulkUploadTransaction(bulkUploadTranscation); | ||
| throw new RequestException(BulkUploadErrorCode.INVALID_DELIMITER.getErrorCode(), | ||
| "Invalid delimiter used. Expected delimiter: " + lineDelimiter); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Delimiter validation incorrectly applied to Excel files.
The delimiter validation (lines 271-279) executes for ALL file types, including Excel (.xls, .xlsx), which don't use text delimiters. This contradicts the past review discussion where it was confirmed that delimiter checking is only for CSV files.
Excel files will either:
- Fail validation incorrectly (if the binary content doesn't contain the delimiter character)
- Pass validation incorrectly (if the binary content happens to contain the delimiter character)
Apply this diff to restrict validation to CSV files only:
Boolean isCSV = file.getOriginalFilename().endsWith(".csv");
-
-try (BufferedReader reader = new BufferedReader(new InputStreamReader(file.getInputStream()))) {
- String firstLine = reader.readLine();
- if (firstLine != null && !firstLine.contains(lineDelimiter)) {
- bulkUploadTranscation.setStatusCode("FAILED");
- updateBulkUploadTransaction(bulkUploadTranscation);
- throw new RequestException(BulkUploadErrorCode.INVALID_DELIMITER.getErrorCode(),
- "Invalid delimiter used. Expected delimiter: " + lineDelimiter);
- }
-}
+
+if (isCSV) {
+ try (BufferedReader reader = new BufferedReader(new InputStreamReader(file.getInputStream()))) {
+ String firstLine = reader.readLine();
+ if (firstLine != null && !firstLine.contains(lineDelimiter)) {
+ bulkUploadTranscation.setStatusCode("FAILED");
+ updateBulkUploadTransaction(bulkUploadTranscation);
+ throw new RequestException(BulkUploadErrorCode.INVALID_DELIMITER.getErrorCode(),
+ "Invalid delimiter used. Expected delimiter: " + lineDelimiter);
+ }
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Boolean isCSV = file.getOriginalFilename().endsWith(".csv"); | |
| try (BufferedReader reader = new BufferedReader(new InputStreamReader(file.getInputStream()))) { | |
| String firstLine = reader.readLine(); | |
| if (firstLine != null && !firstLine.contains(lineDelimiter)) { | |
| bulkUploadTranscation.setStatusCode("FAILED"); | |
| updateBulkUploadTransaction(bulkUploadTranscation); | |
| throw new RequestException(BulkUploadErrorCode.INVALID_DELIMITER.getErrorCode(), | |
| "Invalid delimiter used. Expected delimiter: " + lineDelimiter); | |
| } | |
| } | |
| Boolean isCSV = file.getOriginalFilename().endsWith(".csv"); | |
| if (isCSV) { | |
| try (BufferedReader reader = new BufferedReader(new InputStreamReader(file.getInputStream()))) { | |
| String firstLine = reader.readLine(); | |
| if (firstLine != null && !firstLine.contains(lineDelimiter)) { | |
| bulkUploadTranscation.setStatusCode("FAILED"); | |
| updateBulkUploadTransaction(bulkUploadTranscation); | |
| throw new RequestException(BulkUploadErrorCode.INVALID_DELIMITER.getErrorCode(), | |
| "Invalid delimiter used. Expected delimiter: " + lineDelimiter); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
admin/admin-service/src/main/java/io/mosip/admin/bulkdataupload/service/impl/BulkDataUploadServiceImpl.java
around lines 269 to 279, the delimiter validation is currently applied to all
uploaded files; restrict this validation to CSV files only by using the existing
isCSV boolean. Wrap the BufferedReader/firstLine delimiter check in an if
(isCSV) { ... } block (or guard the specific validation logic with if (isCSV))
so Excel files (.xls, .xlsx) skip this text-delimiter check; keep the rest of
the error handling (setting status to FAILED, updateBulkUploadTransaction and
throwing RequestException) unchanged when the validation runs for CSVs.
| throw new RequestException(BulkUploadErrorCode.INVALID_DELIMITER.getErrorCode(), | ||
| "Invalid delimiter used. Expected delimiter: " + lineDelimiter); |
There was a problem hiding this comment.
Minor: Error message deviates from enum message.
Unlike other error handling in this method (lines 258-259, 265-266), this code appends additional context to the enum's error message. While the extra information about the expected delimiter is helpful for debugging, it's inconsistent with the established pattern.
Consider either:
- Using only the enum message for consistency:
BulkUploadErrorCode.INVALID_DELIMITER.getErrorMessage() - Updating the enum to include a placeholder:
INVALID_DELIMITER("ADM-BLK-010","Invalid delimiter used. Expected: %s")
🤖 Prompt for AI Agents
In
admin/admin-service/src/main/java/io/mosip/admin/bulkdataupload/service/impl/BulkDataUploadServiceImpl.java
around lines 276 to 277, the thrown RequestException appends extra context to
the enum message causing inconsistency with other error handling; change it to
use the enum message directly or update the enum to support a formatted message.
Either replace the concatenated string with
BulkUploadErrorCode.INVALID_DELIMITER.getErrorMessage() to match existing
pattern, or modify the enum to add a placeholder (e.g., "Invalid delimiter used.
Expected: %s") and format that enum message with the expected delimiter before
throwing the RequestException.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes