Feature/s3 registry and caserecord - #31
Merged
Merged
Conversation
… write to an already existing key, s3-upload successful but db-save failed, two metadata lines point to the same s3-object and FileKeyConflictException
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a complete case file management system enabling secure file uploads and downloads for case records via S3 integration. It includes a new JPA entity, REST controller with CRUD operations, service layer with validation and error handling, custom exceptions, database migration, and comprehensive test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/main/resources/db/migration/V10__case_file.sql (1)
1-18: Add an index for the case-scoped lookup path.
findByCaseRecordId(...)will back the list-files endpoint, so this table will fall back to full scans as it grows unlesscase_record_idis indexed.📈 Suggested migration change
CREATE TABLE case_file ( id BIGINT GENERATED BY DEFAULT AS IDENTITY NOT NULL, case_record_id BIGINT NOT NULL, original_file_name VARCHAR(255) NOT NULL, s3_key VARCHAR(1024) NOT NULL, content_type VARCHAR(255) NOT NULL, size_in_bytes BIGINT NOT NULL, uploaded_at TIMESTAMP WITHOUT TIME ZONE NOT NULL, CONSTRAINT pk_case_file PRIMARY KEY (id), CONSTRAINT uk_case_file_s3_key UNIQUE (s3_key), CONSTRAINT fk_case_file_case_record FOREIGN KEY (case_record_id) REFERENCES case_record (id) ON DELETE CASCADE ); + +CREATE INDEX idx_case_file_case_record_id + ON case_file (case_record_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V10__case_file.sql` around lines 1 - 18, The migration creates table case_file but lacks an index on case_record_id, causing findByCaseRecordId(...) to trigger full table scans as the table grows; update the V10__case_file.sql migration to add an index on the case_file.case_record_id column (e.g., CREATE INDEX idx_case_file_case_record_id ON case_file(case_record_id)) so lookups by case_record_id are efficient and the list-files endpoint backed by findByCaseRecordId(...) will scale.src/main/java/backendlab/team4you/casefile/CaseFile.java (1)
84-85: Avoid exposingsetId(...)on a generated entity.This turns a persistence-generated identifier into mutable application state. If tests need to seed IDs, keep that concern out of the production entity API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/casefile/CaseFile.java` around lines 84 - 85, The public setter setId(long l) on the CaseFile entity exposes a persistence-generated identifier as mutable state; remove or restrict it so IDs cannot be changed by application code. Replace the public setId(long l) with either no setter at all or a non-public one (e.g., private or protected) inside the CaseFile class, keeping the id field managed exclusively by the persistence provider; if tests must seed IDs, do so outside the production entity API (test factories or reflection in test code) rather than reintroducing a public setter.src/test/java/backendlab/team4you/casefile/CaseFileControllerTest.java (1)
151-169: Add theapplication/octet-streamfallback case.The controller has a separate branch when
contentTypeis blank/null, but this suite only exercises the explicit MIME-type path. That fallback is easy to regress without a dedicated test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/casefile/CaseFileControllerTest.java` around lines 151 - 169, Add a new test in CaseFileControllerTest that mirrors downloadFile_shouldReturnBytesAndHeaders_whenFileExists but sets CaseFile.setContentType(null) (and/or empty string) and stubs caseFileService.getCaseFile(...) and caseFileService.downloadFile(...) the same way; then perform the same mockMvc GET and assert status is OK, Content-Disposition remains "attachment; filename=\"document.pdf\"" and the response content type equals "application/octet-stream" and body bytes match the stubbed stream to cover the fallback branch in the controller.src/main/java/backendlab/team4you/caserecord/CaseRecord.java (1)
180-182: Avoid exposing a public setter for a generated ID.
idis database-owned here. A publicsetId(...)makes it easy for production code to attach the wrong identity to an entity; tests should use a fixture/helper instead of widening the entity API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 180 - 182, The public setter setId(long l) on CaseRecord exposes a DB-generated identifier; remove or restrict it so callers cannot mutate IDs. Replace the public signature of setId with a non-public visibility (protected or package-private) or remove the method entirely and rely on the ORM/JPA provider and constructors/fixtures to set IDs; update any tests/fixtures to use a factory or reflection helper instead of calling CaseRecord.setId. Ensure the id field and any JPA annotations (on the id field) remain unchanged so the persistence provider can still set the value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileController.java`:
- Around line 42-60: The downloadFile method currently calls readAllBytes() and
closes the S3 InputStream, causing large objects to be loaded into heap; instead
return a streaming response (e.g., use InputStreamResource or
StreamingResponseBody) that wraps the InputStream from
caseFileService.downloadFile(caseRecordId, fileId) so data is streamed directly
to the client; do not use try-with-resources that closes the stream before the
response is written, set the Content-Type from caseFile.getContentType(), set
the Content-Disposition from caseFile.getOriginalFilename(), and if available
set Content-Length from caseFile.getSize() to allow proper client handling.
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 68-75: Replace the deferred save with an immediate flush so DB
constraint violations happen inside the try/catch: change the call to use
caseFileRepository.saveAndFlush(caseFile) instead of
caseFileRepository.save(caseFile) so unique constraint errors on s3_key are
raised before leaving the try block; keep the existing
cleanupUploadedObjectIfPossible(s3Key, exception) and uploadedToS3 logic so
orphaned S3 objects are removed when saveAndFlush throws.
- Around line 97-102: The deleteFile method deletes the S3 object before
removing the DB record which can leave an orphaned DB entry if the transaction
rolls back; change deleteFile (and usages of getCaseFile) to remove the CaseFile
via caseFileRepository.delete(caseFile) inside the transaction first, and then
perform s3Service.deleteFile(caseFile.getS3Key()) only after the transaction
successfully commits (e.g. schedule the S3 deletion with
TransactionSynchronizationManager.registerSynchronization or use an after-commit
event/@TransactionalEventListener) so S3 deletion runs post-commit and never
precedes a failed DB delete.
- Line 57: In CaseFileService (the method that calls
s3Service.uploadFileIfAbsent with file.getBytes()), add an explicit file size
validation before calling getBytes(): define a MAX_FILE_SIZE constant (or read
from configuration), check MultipartFile.getSize() and throw a specific
exception or return a validation error if the size exceeds the limit, and only
then call getBytes() (or better, stream the file via getInputStream()/transferTo
to avoid loading into heap). Update error handling/log message to indicate the
file was rejected due to size and reference the MAX_FILE_SIZE check in your
change.
In `@src/main/java/backendlab/team4you/exceptions/FileKeyConflictException.java`:
- Around line 5-9: The FileKeyConflictException currently embeds the raw storage
key in its exception message (constructors in FileKeyConflictException), which
leaks internal S3 key details to API clients; change both constructors to use a
generic message such as "A file with the same storage key already exists." (do
not append the key), and if you need to retain the key for internal logging or
debugging, store it in a private final field (e.g., private final String key)
with a package-private or protected accessor used only by server-side loggers,
ensuring the public message returned to clients never contains the raw key; keep
the existing Throwable cause handling unchanged.
In `@src/main/java/backendlab/team4you/exceptions/GlobalExceptionHandler.java`:
- Around line 30-50: Remove the duplicate REST-specific exception handlers from
GlobalExceptionHandler: delete the methods
handleCaseFileNotFound(CaseFileNotFoundException ex) and
handleInvalidFile(InvalidFileNameException ex) so those exceptions are handled
only by ApiExceptionHandler; leave other non-REST handlers intact and ensure no
references to ErrorResponseDto remain in GlobalExceptionHandler for these
exceptions.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFile.java`:
- Around line 84-85: The public setter setId(long l) on the CaseFile entity
exposes a persistence-generated identifier as mutable state; remove or restrict
it so IDs cannot be changed by application code. Replace the public setId(long
l) with either no setter at all or a non-public one (e.g., private or protected)
inside the CaseFile class, keeping the id field managed exclusively by the
persistence provider; if tests must seed IDs, do so outside the production
entity API (test factories or reflection in test code) rather than reintroducing
a public setter.
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 180-182: The public setter setId(long l) on CaseRecord exposes a
DB-generated identifier; remove or restrict it so callers cannot mutate IDs.
Replace the public signature of setId with a non-public visibility (protected or
package-private) or remove the method entirely and rely on the ORM/JPA provider
and constructors/fixtures to set IDs; update any tests/fixtures to use a factory
or reflection helper instead of calling CaseRecord.setId. Ensure the id field
and any JPA annotations (on the id field) remain unchanged so the persistence
provider can still set the value.
In `@src/main/resources/db/migration/V10__case_file.sql`:
- Around line 1-18: The migration creates table case_file but lacks an index on
case_record_id, causing findByCaseRecordId(...) to trigger full table scans as
the table grows; update the V10__case_file.sql migration to add an index on the
case_file.case_record_id column (e.g., CREATE INDEX idx_case_file_case_record_id
ON case_file(case_record_id)) so lookups by case_record_id are efficient and the
list-files endpoint backed by findByCaseRecordId(...) will scale.
In `@src/test/java/backendlab/team4you/casefile/CaseFileControllerTest.java`:
- Around line 151-169: Add a new test in CaseFileControllerTest that mirrors
downloadFile_shouldReturnBytesAndHeaders_whenFileExists but sets
CaseFile.setContentType(null) (and/or empty string) and stubs
caseFileService.getCaseFile(...) and caseFileService.downloadFile(...) the same
way; then perform the same mockMvc GET and assert status is OK,
Content-Disposition remains "attachment; filename=\"document.pdf\"" and the
response content type equals "application/octet-stream" and body bytes match the
stubbed stream to cover the fallback branch in the controller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9beea2f4-f2be-4a60-8d99-4328fec607b5
📒 Files selected for processing (17)
src/main/java/backendlab/team4you/casefile/CaseFile.javasrc/main/java/backendlab/team4you/casefile/CaseFileController.javasrc/main/java/backendlab/team4you/casefile/CaseFileRepository.javasrc/main/java/backendlab/team4you/casefile/CaseFileResponseDto.javasrc/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/main/java/backendlab/team4you/caserecord/CaseRecord.javasrc/main/java/backendlab/team4you/exceptions/ApiExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/CaseFileNotFoundException.javasrc/main/java/backendlab/team4you/exceptions/CaseRecordNotFoundException.javasrc/main/java/backendlab/team4you/exceptions/ErrorResponseDto.javasrc/main/java/backendlab/team4you/exceptions/FileKeyConflictException.javasrc/main/java/backendlab/team4you/exceptions/GlobalExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/InvalidFileNameException.javasrc/main/java/backendlab/team4you/s3/S3Service.javasrc/main/resources/db/migration/V10__case_file.sqlsrc/test/java/backendlab/team4you/casefile/CaseFileControllerTest.javasrc/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java
Comment on lines
+97
to
+102
| public void deleteFile(Long caseRecordId, Long fileId) { | ||
| CaseFile caseFile = getCaseFile(caseRecordId, fileId); | ||
|
|
||
| s3Service.deleteFile(caseFile.getS3Key()); | ||
| caseFileRepository.delete(caseFile); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current operation ordering in delete flow
rg -n --type=java -C6 'public void deleteFile\(|s3Service\.deleteFile\(|caseFileRepository\.delete\(|flush\('Repository: ithsjava25/project-backend-team4you
Length of output: 6398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full context of CaseFileService, especially transaction and exception handling
cat -n src/main/java/backendlab/team4you/casefile/CaseFileService.java | head -120Repository: ithsjava25/project-backend-team4you
Length of output: 5395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any try-catch or exception handling around deleteFile calls
rg -n --type=java 'try|catch|throws' src/main/java/backendlab/team4you/casefile/CaseFileService.javaRepository: ithsjava25/project-backend-team4you
Length of output: 322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for TransactionSynchronizationManager or afterCommit patterns in the codebase
rg -n --type=java 'afterCommit|TransactionSynchronization|registerSynchronization'Repository: ithsjava25/project-backend-team4you
Length of output: 61
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if flush() is used elsewhere in repositories or services
rg -n --type=java '\.flush\(\)' src/main/java/Repository: ithsjava25/project-backend-team4you
Length of output: 61
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the class-level annotations and imports for CaseFileService
head -30 src/main/java/backendlab/team4you/casefile/CaseFileService.java | cat -nRepository: ithsjava25/project-backend-team4you
Length of output: 1385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for global exception handlers or TransactionSynchronizationManager usage
rg -n --type=java '@ControllerAdvice|@ExceptionHandler|TransactionSynchronization' src/Repository: ithsjava25/project-backend-team4you
Length of output: 2222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check Spring configuration for transaction management
find src/main/resources -type f \( -name "*.properties" -o -name "*.yml" -o -name "*.yaml" \) 2>/dev/null | head -5 | xargs cat -n 2>/dev/nullRepository: ithsjava25/project-backend-team4you
Length of output: 721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any Event or Listener patterns that might be used
rg -n --type=java 'EventListener|ApplicationEvent|@Event' src/Repository: ithsjava25/project-backend-team4you
Length of output: 61
Reverse deletion order: delete from DB before S3 to prevent dangling references.
S3 is deleted before the DB transaction commits (line 100 before 101). If DB deletion fails or rolls back, the S3 object is already gone but the DB record persists. This creates an orphaned metadata entry pointing to a deleted blob.
Delete from the database first, then delete from S3 after commit succeeds:
Safer ordering (DB first, S3 after)
public void deleteFile(Long caseRecordId, Long fileId) {
CaseFile caseFile = getCaseFile(caseRecordId, fileId);
-
- s3Service.deleteFile(caseFile.getS3Key());
- caseFileRepository.delete(caseFile);
+ String s3Key = caseFile.getS3Key();
+ caseFileRepository.delete(caseFile);
+ try {
+ s3Service.deleteFile(s3Key);
+ } catch (Exception e) {
+ log.error("Failed to delete S3 object after DB deletion: {}", s3Key, e);
+ throw e;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java` around lines
97 - 102, The deleteFile method deletes the S3 object before removing the DB
record which can leave an orphaned DB entry if the transaction rolls back;
change deleteFile (and usages of getCaseFile) to remove the CaseFile via
caseFileRepository.delete(caseFile) inside the transaction first, and then
perform s3Service.deleteFile(caseFile.getS3Key()) only after the transaction
successfully commits (e.g. schedule the S3 deletion with
TransactionSynchronizationManager.registerSynchronization or use an after-commit
event/@TransactionalEventListener) so S3 deletion runs post-commit and never
precedes a failed DB delete.
Comment on lines
+30
to
+50
| @ExceptionHandler(CaseFileNotFoundException.class) | ||
| public ResponseEntity<ErrorResponseDto> handleCaseFileNotFound(CaseFileNotFoundException ex) { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND) | ||
| .body(new ErrorResponseDto( | ||
| HttpStatus.NOT_FOUND.value(), | ||
| "Not Found", | ||
| ex.getMessage(), | ||
| LocalDateTime.now() | ||
| )); | ||
| } | ||
|
|
||
| @ExceptionHandler(InvalidFileNameException.class) | ||
| public ResponseEntity<ErrorResponseDto> handleInvalidFile(InvalidFileNameException ex) { | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(new ErrorResponseDto( | ||
| HttpStatus.BAD_REQUEST.value(), | ||
| "Bad Request", | ||
| ex.getMessage(), | ||
| LocalDateTime.now() | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect each exception below to be mapped in a single advice class only.
rg -n -C2 '@ExceptionHandler\((InvalidFileNameException|CaseFileNotFoundException)\.class\)' src/main/javaRepository: ithsjava25/project-backend-team4you
Length of output: 2470
🏁 Script executed:
#!/bin/bash
# Check class annotations for both handlers
echo "=== ApiExceptionHandler ==="
head -20 src/main/java/backendlab/team4you/exceptions/ApiExceptionHandler.java
echo -e "\n=== GlobalExceptionHandler ==="
head -20 src/main/java/backendlab/team4you/exceptions/GlobalExceptionHandler.javaRepository: ithsjava25/project-backend-team4you
Length of output: 1505
Remove duplicate REST exception handlers from this class.
Both CaseFileNotFoundException and InvalidFileNameException are already mapped in ApiExceptionHandler (which is @RestControllerAdvice). Moving REST handlers (ResponseEntity<ErrorResponseDto>) into a general @ControllerAdvice creates unnecessary duplication and couples the REST error response format to web controller advice. These handlers should exist only in ApiExceptionHandler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/exceptions/GlobalExceptionHandler.java`
around lines 30 - 50, Remove the duplicate REST-specific exception handlers from
GlobalExceptionHandler: delete the methods
handleCaseFileNotFound(CaseFileNotFoundException ex) and
handleInvalidFile(InvalidFileNameException ex) so those exceptions are handled
only by ApiExceptionHandler; leave other non-REST handlers intact and ensure no
references to ErrorResponseDto remain in GlobalExceptionHandler for these
exceptions.
…d from save to saveAndFlush() to ensure constraint checks occur within the try/catch block.
…gling references after coderabbit feedback
…derabbit feedback
…t that "uploadFile should not access repositories or s3 when file is too large"
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/casefile/CaseFileService.java (1)
103-112:⚠️ Potential issue | 🔴 CriticalMove the S3 delete to an after-commit hook.
This is still running the external delete before the database transaction has committed.
caseFileRepository.delete(caseFile)only marks the entity for removal; if flush/commit fails after Line 109, the row survives while the blob is already gone.Safer pattern
+import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; ... String s3Key = caseFile.getS3Key(); caseFileRepository.delete(caseFile); - try { - s3Service.deleteFile(s3Key); - } catch (Exception e) { - log.error("Failed to delete S3 object after DB deletion: {}", s3Key, e); - throw e; - } + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + `@Override` + public void afterCommit() { + try { + s3Service.deleteFile(s3Key); + } catch (Exception e) { + log.error("Failed to delete S3 object after commit: {}", s3Key, e); + } + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java` around lines 103 - 112, The deleteFile method currently calls caseFileRepository.delete(caseFile) then deletes the S3 object immediately, risking S3 deletion before DB commit; change this to perform the DB delete within the transaction as-is but defer calling s3Service.deleteFile(s3Key) to an after-commit hook (e.g., TransactionSynchronizationManager.registerSynchronization with afterCommit or an `@TransactionalEventListener`(phase = AFTER_COMMIT) event) so the S3 delete runs only when the transaction successfully commits; reference the deleteFile method, the caseFileRepository.delete(caseFile) call, the s3Key variable and s3Service.deleteFile(...) for locating where to register the after-commit action and ensure any exceptions from the S3 delete are logged there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileController.java`:
- Around line 58-60: Replace the raw string concatenation used when building the
Content-Disposition header in CaseFileController (the method returning
ResponseEntity.ok() with .header(HttpHeaders.CONTENT_DISPOSITION, ...)) with
Spring's ContentDisposition builder to safely encode the filename: build a
ContentDisposition using
ContentDisposition.builder("attachment").filename(caseFile.getOriginalFilename(),
StandardCharsets.UTF_8).build() and use its toString() as the header value;
apply the same change to the analogous code in S3Controller (the header set
around line 41) so both controllers use ContentDisposition to avoid header
injection and encoding issues.
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 116-120: The normalizeContentType(String contentType) helper
currently only defaults blank values but doesn't validate client-controlled MIME
types; update normalizeContentType to validate the incoming contentType (e.g.,
by attempting to parse it with MediaType.parseMediaType or matching against a
safe whitelist/regex) and return "application/octet-stream" if
parsing/validation fails or if the type is not allowed; ensure this prevents
storing invalid values that would later cause MediaType.parseMediaType(...) in
CaseFileController to throw InvalidMediaTypeException.
In `@src/main/java/backendlab/team4you/exceptions/FileKeyConflictException.java`:
- Around line 8-9: The cause-taking constructor FileKeyConflictException(String
key, Throwable cause) currently uses a different message; update it so its
message matches the primary constructor by passing the same concatenated message
(e.g., "A file with the same name already exists: " + key) to super and still
forwarding the cause, ensuring both constructors produce identical user-facing
text.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 103-112: The deleteFile method currently calls
caseFileRepository.delete(caseFile) then deletes the S3 object immediately,
risking S3 deletion before DB commit; change this to perform the DB delete
within the transaction as-is but defer calling s3Service.deleteFile(s3Key) to an
after-commit hook (e.g.,
TransactionSynchronizationManager.registerSynchronization with afterCommit or an
`@TransactionalEventListener`(phase = AFTER_COMMIT) event) so the S3 delete runs
only when the transaction successfully commits; reference the deleteFile method,
the caseFileRepository.delete(caseFile) call, the s3Key variable and
s3Service.deleteFile(...) for locating where to register the after-commit action
and ensure any exceptions from the S3 delete are logged there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59d38d72-1df7-4b55-8600-1973d0df7a05
📒 Files selected for processing (6)
src/main/java/backendlab/team4you/casefile/CaseFileController.javasrc/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/main/java/backendlab/team4you/exceptions/FileKeyConflictException.javasrc/main/resources/application.propertiessrc/test/java/backendlab/team4you/casefile/CaseFileControllerTest.javasrc/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/application.properties
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/backendlab/team4you/casefile/CaseFileControllerTest.java
- src/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java
Comment on lines
+8
to
+9
| public FileKeyConflictException(String key, Throwable cause) { | ||
| super("A file with the same name already exists: ", cause); |
Contributor
There was a problem hiding this comment.
Keep the cause-based conflict message identical to the primary constructor.
S3Service.uploadFileIfAbsent(...) uses this overload on real S3 conflicts, so Line 9 currently returns a different user-facing message ("A file with the same name already exists: ") than Line 5.
Suggested cleanup
public class FileKeyConflictException extends RuntimeException {
+ private static final String MESSAGE = "A file with the same name already exists.";
+
public FileKeyConflictException(String key) {
- super("A file with the same name already exists.");
+ super(MESSAGE);
}
public FileKeyConflictException(String key, Throwable cause) {
- super("A file with the same name already exists: ", cause);
+ super(MESSAGE, cause);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/exceptions/FileKeyConflictException.java`
around lines 8 - 9, The cause-taking constructor FileKeyConflictException(String
key, Throwable cause) currently uses a different message; update it so its
message matches the primary constructor by passing the same concatenated message
(e.g., "A file with the same name already exists: " + key) to super and still
forwarding the cause, ensuring both constructors produce identical user-facing
text.
…java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… errors in CasefileController after coderabbit commit
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/casefile/CaseFileService.java (1)
104-116:⚠️ Potential issue | 🟠 MajorDB-then-S3 ordering still orphans the S3 object if the transaction commit fails.
Because
deleteFileis@Transactional,caseFileRepository.delete(caseFile)at line 109 only marks the entity for removal in the persistence context — the actual DB DELETE is flushed at commit, which happens after this method returns. The sequence is therefore:
- JPA delete queued (line 109)
- S3 object deleted (line 111)
- Method returns → transaction commits
If commit fails at step 3 (e.g., connection drop, optimistic lock, constraint, transient DB error), the S3 object is already gone but the DB row is rolled back → the same dangling-reference scenario flagged in the previous review. The rethrow on S3 failure (line 114) is correct and does cause rollback, but that path is not the concern — the concern is commit-time failure after S3 deletion has already happened.
The robust pattern is to perform the S3 deletion only after the transaction commits successfully, e.g. via
TransactionSynchronizationManager.registerSynchronization(...)with anafterCommithook, or a@TransactionalEventListener(phase = AFTER_COMMIT)handler. Consider also doing an explicitcaseFileRepository.flush()to surface DB errors before scheduling the S3 deletion.Sketch of post-commit deletion
`@Transactional` public void deleteFile(Long caseRecordId, Long fileId) { CaseFile caseFile = getCaseFile(caseRecordId, fileId); - - String s3Key = caseFile.getS3Key(); - caseFileRepository.delete(caseFile); - try { - s3Service.deleteFile(s3Key); - } catch (Exception e) { - log.error("Failed to delete S3 object after DB deletion: {}", s3Key, e); - throw e; - } + String s3Key = caseFile.getS3Key(); + caseFileRepository.delete(caseFile); + caseFileRepository.flush(); + TransactionSynchronizationManager.registerSynchronization( + new TransactionSynchronization() { + `@Override` + public void afterCommit() { + try { + s3Service.deleteFile(s3Key); + } catch (Exception e) { + log.error("Failed to delete S3 object after DB commit: {}", s3Key, e); + } + } + } + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java` around lines 104 - 116, The deleteFile method is currently removing the CaseFile via caseFileRepository.delete(...) then calling s3Service.deleteFile(...) inside the `@Transactional` method which can orphan S3 objects if the DB commit later fails; change the flow so S3 deletion runs only after a successful transaction commit — e.g. inside deleteFile (or a helper) call caseFileRepository.flush() to surface DB errors and then register a post-commit callback using TransactionSynchronizationManager.registerSynchronization(...) or publish a domain event handled by a `@TransactionalEventListener`(phase = AFTER_COMMIT) that invokes s3Service.deleteFile(s3Key); ensure you still remove the entity via caseFileRepository.delete(caseFile) and move the s3Service.deleteFile call into the after-commit hook (and keep logging/exception handling there).
🧹 Nitpick comments (2)
src/main/java/backendlab/team4you/casefile/CaseFileService.java (2)
122-126: Nit: indentation of line 123.
return MediaType.parseMediaType(contentType).toString();is indented with extra spaces relative to the surroundingtry { ... } catch { ... }block.try { - return MediaType.parseMediaType(contentType).toString(); + return MediaType.parseMediaType(contentType).toString(); } catch (InvalidMediaTypeException exception) { return MediaType.APPLICATION_OCTET_STREAM_VALUE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java` around lines 122 - 126, In CaseFileService adjust the indentation inside the try block so the line calling MediaType.parseMediaType(contentType).toString() aligns with the opening brace of the try (i.e., same indent level as the catch block), leaving the catch body unchanged; locate the try/catch that uses MediaType.parseMediaType, InvalidMediaTypeException and MediaType.APPLICATION_OCTET_STREAM_VALUE and remove the extra spaces before the return so formatting is consistent.
44-84: Consider streaming the upload instead offile.getBytes(), and rejecting empty files.Two small, non-blocking improvements for the upload path:
file.getBytes()at line 65 fully materializes the payload in heap. Bounded by 5MB today, butS3Service.uploadFileIfAbsentalready accepts bytes — if you intend to raise the limit later, switch tofile.getInputStream()withRequestBody.fromInputStream(stream, file.getSize())inS3Serviceto avoid the allocation.file.getSize() == 0currently passes validation and results in a zero-byte S3 object and DB row. Usually worth rejecting explicitly alongside the blank-filename check:if (file.getSize() > MAX_FILE_SIZE_BYTES) { throw new IllegalArgumentException("File exceeds maximum size"); } + if (file.isEmpty()) { + throw new IllegalArgumentException("File must not be empty"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java` around lines 44 - 84, In uploadFile ensure empty files are rejected and avoid materializing the whole payload: add an explicit check for file.getSize() == 0 alongside the blank-filename validation and throw an IllegalArgumentException (or InvalidFileNameException if you prefer) to prevent zero-byte S3 objects; then replace the call that uses file.getBytes() (s3Service.uploadFileIfAbsent(s3Key, file.getBytes(), contentType)) with a streaming approach—pass file.getInputStream() and file.getSize() to s3Service (update S3Service.uploadFileIfAbsent to accept an InputStream/length and internally use RequestBody.fromInputStream(stream, size) or equivalent) so uploadFile, s3Service.uploadFileIfAbsent, and cleanupUploadedObjectIfPossible continue to work without buffering the entire file in heap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 78-83: The current catch for RuntimeException | IOException
conflates all DataIntegrityViolationException causes and unconditionally deletes
the uploaded S3 object; update the handling in the catch (and the existing
DataIntegrityViolationException handling around cleanupUploadedObjectIfPossible
and the log message) to detect whether the root cause is a
ConstraintViolationException whose constraintName (or SQL state/message) equals
'uk_case_file_s3_key' before treating it as a duplicate-key error and deleting
s3Key; for all other DataIntegrityViolationException causes, log a generic
integrity violation message and avoid deleting the uploaded object. Use the
existing methods/variables (DataIntegrityViolationException,
cleanupUploadedObjectIfPossible, s3Key) and traverse exception.getCause() chain
to identify ConstraintViolationException and its constraint name.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 104-116: The deleteFile method is currently removing the CaseFile
via caseFileRepository.delete(...) then calling s3Service.deleteFile(...) inside
the `@Transactional` method which can orphan S3 objects if the DB commit later
fails; change the flow so S3 deletion runs only after a successful transaction
commit — e.g. inside deleteFile (or a helper) call caseFileRepository.flush() to
surface DB errors and then register a post-commit callback using
TransactionSynchronizationManager.registerSynchronization(...) or publish a
domain event handled by a `@TransactionalEventListener`(phase = AFTER_COMMIT) that
invokes s3Service.deleteFile(s3Key); ensure you still remove the entity via
caseFileRepository.delete(caseFile) and move the s3Service.deleteFile call into
the after-commit hook (and keep logging/exception handling there).
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 122-126: In CaseFileService adjust the indentation inside the try
block so the line calling MediaType.parseMediaType(contentType).toString()
aligns with the opening brace of the try (i.e., same indent level as the catch
block), leaving the catch body unchanged; locate the try/catch that uses
MediaType.parseMediaType, InvalidMediaTypeException and
MediaType.APPLICATION_OCTET_STREAM_VALUE and remove the extra spaces before the
return so formatting is consistent.
- Around line 44-84: In uploadFile ensure empty files are rejected and avoid
materializing the whole payload: add an explicit check for file.getSize() == 0
alongside the blank-filename validation and throw an IllegalArgumentException
(or InvalidFileNameException if you prefer) to prevent zero-byte S3 objects;
then replace the call that uses file.getBytes()
(s3Service.uploadFileIfAbsent(s3Key, file.getBytes(), contentType)) with a
streaming approach—pass file.getInputStream() and file.getSize() to s3Service
(update S3Service.uploadFileIfAbsent to accept an InputStream/length and
internally use RequestBody.fromInputStream(stream, size) or equivalent) so
uploadFile, s3Service.uploadFileIfAbsent, and cleanupUploadedObjectIfPossible
continue to work without buffering the entire file in heap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 005dab1f-e98a-4b86-a61b-f219b77ed832
📒 Files selected for processing (5)
src/main/java/backendlab/team4you/casefile/CaseFileController.javasrc/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/main/java/backendlab/team4you/exceptions/FileKeyConflictException.javasrc/main/resources/db/migration/V14__case_file.sqlsrc/test/java/backendlab/team4you/casefile/CaseFileControllerTest.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/backendlab/team4you/exceptions/FileKeyConflictException.java
- src/main/resources/db/migration/V14__case_file.sql
- src/main/java/backendlab/team4you/casefile/CaseFileController.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/backendlab/team4you/casefile/CaseFileControllerTest.java
Comment on lines
+78
to
+83
| } catch (RuntimeException | IOException exception) { | ||
| if (uploadedToS3) { | ||
| cleanupUploadedObjectIfPossible(s3Key, exception); | ||
| } | ||
| throw exception; | ||
| } |
Contributor
There was a problem hiding this comment.
DataIntegrityViolationException branch conflates unrelated constraint failures.
DataIntegrityViolationException is thrown for any JPA integrity violation — FK violations on case_record_id, NULL violations on original_file_name / content_type / size_in_bytes / uploaded_at, etc. — not just the uk_case_file_s3_key uniqueness constraint. The log message at line 156 will be misleading in those cases, and the cleanup logic unconditionally deletes the S3 object the caller just uploaded even when the root cause has nothing to do with a duplicate key.
Behavior is still safe (rethrow happens at line 82), but consider either narrowing the check (e.g., inspect the cause for a ConstraintViolationException carrying uk_case_file_s3_key) or adjusting the wording so it doesn't imply a key conflict:
- if (originalException instanceof DataIntegrityViolationException) {
- log.warn("Database integrity violation after S3 upload. Attempted cleanup for key={}", s3Key);
- }
+ if (originalException instanceof DataIntegrityViolationException) {
+ log.warn(
+ "Data integrity violation while persisting CaseFile (possible duplicate s3_key or invalid FK/NULL). Attempted cleanup for key={}",
+ s3Key
+ );
+ }Also applies to: 144-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java` around lines
78 - 83, The current catch for RuntimeException | IOException conflates all
DataIntegrityViolationException causes and unconditionally deletes the uploaded
S3 object; update the handling in the catch (and the existing
DataIntegrityViolationException handling around cleanupUploadedObjectIfPossible
and the log message) to detect whether the root cause is a
ConstraintViolationException whose constraintName (or SQL state/message) equals
'uk_case_file_s3_key' before treating it as a duplicate-key error and deleting
s3Key; for all other DataIntegrityViolationException causes, log a generic
integrity violation message and avoid deleting the uploaded object. Use the
existing methods/variables (DataIntegrityViolationException,
cleanupUploadedObjectIfPossible, s3Key) and traverse exception.getCause() chain
to identify ConstraintViolationException and its constraint name.
gvaguirres
approved these changes
Apr 17, 2026
This was referenced Apr 24, 2026
Closed
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Resolves #26
Summary by CodeRabbit
Release Notes
New Features