Feature#8 s3 storage#50
Merged
Merged
Conversation
- Add spring-cloud-aws-starter-s3 dependency - Configure MinIO in docker-compose.yml - Add S3 and multipart configurations in application.properties - Create DocumentEntity, Repository, DTO, and Mapper - Implement DocumentService for S3 operations - Create DocumentUiController for upload/download/delete - Update Case detail view to support document management - Add role helper methods to Actor
- Add DocumentServiceTest with mocked S3Template - Add DocumentIntegrationTest for UI controller and service interaction - Update test application.properties with S3 mock configuration
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds document management: MinIO and Spring Cloud AWS S3 support, JPA DocumentEntity + repository, DocumentService with S3-backed upload/list/download/delete and authorization, Document UI/controller/templates, DTOs/mappers, multipart limits, tests, and related config/dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client/Actor
participant Controller as DocumentUiController
participant Service as DocumentService
participant CaseRepo as CaseRepository
participant DocRepo as DocumentRepository
participant S3 as S3Template
participant DB as Database
User->>Controller: POST /ui/cases/{caseId}/documents/upload (file)
Controller->>Service: uploadDocument(actor, caseId, file)
Service->>CaseRepo: findById(caseId)
CaseRepo->>DB: Query Case
DB-->>CaseRepo: CaseEntity
CaseRepo-->>Service: CaseEntity
Note over Service: validate file & authorize (manager or owner/handler)
Service->>S3: upload(bucket, s3Key, inputStream, metadata)
S3-->>Service: Success
Service->>DocRepo: save(DocumentEntity)
DocRepo->>DB: Insert Document
DB-->>DocRepo: Persisted
DocRepo-->>Service: DocumentEntity
Service-->>Controller: DocumentDTO
Controller-->>User: Redirect to /ui/cases/{caseId}
sequenceDiagram
participant User as Client/Actor
participant Controller as DocumentUiController
participant Service as DocumentService
participant DocRepo as DocumentRepository
participant S3 as S3Template
participant TX as Transaction
User->>Controller: POST /ui/cases/{caseId}/documents/{docId}/delete
Controller->>Service: deleteDocument(actor, documentId)
Service->>DocRepo: findById(documentId)
DocRepo-->>Service: DocumentEntity
Note over Service: authorize & capture S3 key
Service->>DocRepo: delete(entity) within TX
DocRepo-->>TX: Deleted
TX-->>Service: afterCommit()
Service->>S3: deleteObject(bucket, s3Key)
S3-->>Service: Success (or logged failure)
Service-->>Controller: Success
Controller-->>User: Redirect to case
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
src/main/java/org/example/projektarendehantering/presentation/dto/CaseDTO.java (1)
51-52: Keepdocumentsnull-safe in the setter.Assigning
nullhere can propagate null checks into callers/templates. Consider enforcing a non-null list invariant.Proposed tweak
public void setDocuments(List<DocumentDTO> documents) { this.documents = documents; } +public void setDocuments(List<DocumentDTO> documents) { + this.documents = (documents != null) ? documents : new ArrayList<>(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/dto/CaseDTO.java` around lines 51 - 52, The setter setDocuments currently allows assigning null which forces callers to guard against null; update setDocuments to enforce a non-null list invariant by replacing any null input with an empty list (e.g., Collections.emptyList() or new ArrayList<>()) and/or defensively copying the provided list so the documents field is never null; reference setDocuments and getDocuments (and the documents field) when making the change.src/main/java/org/example/projektarendehantering/presentation/web/DocumentUiController.java (2)
38-39: ThecaseIdpath variable is not validated against the document's actual case.In both
downloadDocumentanddeleteDocument, thecaseIdfrom the URL path is not used or validated. This means a user could construct a URL with anycaseIdin the path as long as they have access to thedocumentId. While this may not be a security issue (authorization is checked against the document's actual case), it could lead to confusion or allow URL tampering. Consider validating that the document belongs to the specified case.🔧 Suggested validation
`@GetMapping`("/{documentId}/download") public ResponseEntity<Resource> downloadDocument(`@PathVariable` UUID caseId, `@PathVariable` UUID documentId) throws IOException { Actor actor = securityActorAdapter.currentUser(); DocumentEntity entity = documentService.getEntity(documentId); // Validate document belongs to the specified case if (!entity.getCaseEntity().getId().equals(caseId)) { throw new BadRequestException("Document does not belong to the specified case"); } S3Resource s3Resource = documentService.downloadDocument(actor, documentId); // ... rest of method }Also applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/DocumentUiController.java` around lines 38 - 39, Validate that the path caseId matches the document's actual case in both downloadDocument and deleteDocument: inside each method call documentService.getEntity(documentId) to obtain the DocumentEntity, compare entity.getCaseEntity().getId() to the `@PathVariable` caseId, and if they differ throw a BadRequestException (or similar) before proceeding with securityActorAdapter.currentUser(), documentService.downloadDocument(...) or documentService.deleteDocument(...); this ensures URL caseId cannot be tampered with and maps to the document's real case.
31-36: Consider handlingIOExceptionmore gracefully.The
uploadDocumentmethod declaresthrows IOException, which will result in a 500 error if the S3 upload fails. Consider catching and translating this to a more user-friendly error response, especially for a UI controller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/DocumentUiController.java` around lines 31 - 36, The uploadDocument method in DocumentUiController currently declares throws IOException which will bubble up as a 500; catch IOException around the call to documentService.uploadDocument(actor, caseId, file) and translate it into a user-friendly response: log the exception, add an error message (e.g. via RedirectAttributes or Model) and return a redirect to the case view or an error page instead of throwing; keep securityActorAdapter.currentUser() and documentService.uploadDocument(...) as-is and only wrap the service call in the try/catch to handle upload failures gracefully.src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java (1)
115-129: Consider adding additional delete authorization tests.The
deleteDocument_shouldAllowOwnertest covers the owner scenario, but tests for manager authorization and unauthorized access denial for delete operations are missing. This would provide parity with the upload tests.📝 Additional test suggestions
`@Test` void deleteDocument_shouldAllowManager() { UUID docId = UUID.randomUUID(); DocumentEntity docEntity = new DocumentEntity(); docEntity.setId(docId); docEntity.setCaseEntity(caseEntity); docEntity.setS3Key("some-key"); when(documentRepository.findById(docId)).thenReturn(Optional.of(docEntity)); documentService.deleteDocument(managerActor, docId); verify(s3Template).deleteObject(eq("test-bucket"), eq("some-key")); verify(documentRepository).delete(docEntity); } `@Test` void deleteDocument_shouldDenyUnauthorized() { Actor unauthorizedActor = new Actor(UUID.randomUUID(), Role.DOCTOR, "Unauthorized", "unauthorized_user"); UUID docId = UUID.randomUUID(); DocumentEntity docEntity = new DocumentEntity(); docEntity.setId(docId); docEntity.setCaseEntity(caseEntity); when(documentRepository.findById(docId)).thenReturn(Optional.of(docEntity)); assertThatThrownBy(() -> documentService.deleteDocument(unauthorizedActor, docId)) .isInstanceOf(NotAuthorizedException.class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java` around lines 115 - 129, Add two more unit tests in DocumentServiceTest mirroring deleteDocument_shouldAllowOwner: one (deleteDocument_shouldAllowManager) that calls documentService.deleteDocument(managerActor, docId) with a mocked documentRepository returning the DocumentEntity and then verifies s3Template.deleteObject(...) and documentRepository.delete(...) were invoked; and one (deleteDocument_shouldDenyUnauthorized) that constructs an unauthorized Actor, stubs documentRepository.findById(docId) to return the entity, calls documentService.deleteDocument(unauthorizedActor, docId) and asserts that a NotAuthorizedException is thrown. Ensure you reuse the same setup (docId, DocumentEntity, caseEntity) and the same verification of s3Template and documentRepository behavior as in deleteDocument_shouldAllowOwner.src/test/java/org/example/projektarendehantering/DocumentIntegrationTest.java (1)
96-106: Clarify the unauthorized test scenario.The test uses
@WithMockUser(username = "other")but no employee with that username is created in setup. This tests the scenario whereSecurityActorAdaptercan't find the employee (likely throwingNotAuthorizedException), rather than testing when a valid user lacks authorization for the specific case.To test actual case-level authorization denial, consider creating a second employee (DOCTOR) who exists but doesn't own the case:
📝 Suggested test setup for clearer authorization testing
`@BeforeEach` void setUp() { mockMvc = webAppContextSetup(context) .apply(springSecurity()) .build(); // Setup manager employee for SecurityActorAdapter EmployeeEntity manager = new EmployeeEntity(); manager.setId(UUID.nameUUIDFromBytes("manager".getBytes(java.nio.charset.StandardCharsets.UTF_8))); manager.setRole(Role.MANAGER); manager.setGithubUsername("manager"); manager.setDisplayName("Manager"); employeeRepository.save(manager); + // Setup unauthorized doctor employee + EmployeeEntity doctor = new EmployeeEntity(); + doctor.setId(UUID.nameUUIDFromBytes("other".getBytes(java.nio.charset.StandardCharsets.UTF_8))); + doctor.setRole(Role.DOCTOR); + doctor.setGithubUsername("other"); + doctor.setDisplayName("Other Doctor"); + employeeRepository.save(doctor); CaseEntity caseEntity = new CaseEntity(); caseEntity.setTitle("Test Case"); caseEntity.setOwnerId(manager.getId()); CaseEntity saved = caseRepository.save(caseEntity); caseId = saved.getId(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/DocumentIntegrationTest.java` around lines 96 - 106, The test uploadDocument_shouldDenyUnauthorized currently uses `@WithMockUser`("other") but never creates an Employee with username "other", so it exercises missing-employee behavior in SecurityActorAdapter instead of case-level denial; update the test setup to create a second Employee (e.g., username "other", role DOCTOR) distinct from the case owner and persist it before calling mockMvc.perform in uploadDocument_shouldDenyUnauthorized so that SecurityActorAdapter finds the user but the authorization check on the case denies access (still asserting NotAuthorizedException); reference the test method uploadDocument_shouldDenyUnauthorized, SecurityActorAdapter, and NotAuthorizedException when adding the new employee creation in the test setup.docker-compose.yml (2)
19-30: Consider adding a healthcheck for MinIO.The PostgreSQL service has a healthcheck configured, but the MinIO service does not. If the application depends on MinIO being ready at startup, this could cause race conditions or startup failures.
🔧 Proposed healthcheck configuration
minio: image: minio/minio container_name: projekt-arendehantering-minio environment: MINIO_ROOT_USER: minioadmin MINIO_ROOT_PASSWORD: minioadmin ports: - "9000:9000" - "9001:9001" command: server /data --console-address ":9001" volumes: - minio_data:/data + healthcheck: + test: ["CMD", "mc", "ready", "local"] + interval: 5s + timeout: 5s + retries: 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 19 - 30, Add a Docker healthcheck to the minio service so Compose waits for MinIO readiness; update the minio service (identified by service name "minio" and container_name "projekt-arendehantering-minio", using image "minio/minio" and command "server /data --console-address \":9001\"") to include a healthcheck that probes the MinIO readiness endpoint (e.g., a curl --fail against http://localhost:9000/minio/health/ready) with sensible options (interval, timeout, retries, start_period) so the service is marked healthy only after MinIO is ready.
20-20: Pin the MinIO image version for reproducibility.The unpinned
minio/minioimage defaults tolatest, which can cause unexpected behavior when the image is updated. Pinning to a specific version ensures consistent deployments. Note that the postgres service (line 3) is already pinned to a specific version—MinIO should follow the same pattern.🔧 Example with version pinning
- image: minio/minio + image: minio/minio:RELEASE.2025-09-07T16-13-09Z🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 20, Replace the unpinned MinIO image tag "image: minio/minio" with a specific, immutable version (e.g., "minio/minio:<version>" or a RELEASE tag) to ensure reproducible deployments; update the docker-compose.yml entry for the MinIO service to mirror how the postgres service is pinned so future updates won't change the runtime image unexpectedly.src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (1)
49-50: Refactor duplicated authorization checks into one helperThe same predicate is repeated in four places. Centralizing it reduces drift and future authorization bugs.
Refactor sketch
+ private void assertCaseAccess(Actor actor, CaseEntity caseEntity, String action) { + if (!actor.isManager() + && !actor.userId().equals(caseEntity.getOwnerId()) + && !actor.userId().equals(caseEntity.getHandlerId())) { + throw new NotAuthorizedException("Not authorized to " + action + " for this case"); + } + }Then call
assertCaseAccess(...)from upload/list/download/delete.Also applies to: 79-80, 95-96, 110-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 49 - 50, Extract the repeated authorization predicate into a private helper method in DocumentService, e.g. assertCaseAccess(Actor actor, CaseEntity caseEntity), that checks if actor.isManager() || actor.userId().equals(caseEntity.getOwnerId()) || actor.userId().equals(caseEntity.getHandlerId()) and throws NotAuthorizedException("Not authorized to upload documents to this case") when false; then replace the duplicated inline checks in the upload/list/download/delete methods (the blocks currently using actor.isManager() && actor.userId().equals(caseEntity.getOwnerId()) && actor.userId().equals(caseEntity.getHandlerId()) and throwing NotAuthorizedException) with a single call to assertCaseAccess(...) so all four call sites use the centralized check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 115-119: Dependency version mismatch: the pom declares
io.awspring.cloud:spring-cloud-aws-starter-s3 at 3.3.0 which is incompatible
with Spring Boot 4.0.4. Update the <groupId>io.awspring.cloud</groupId>
<artifactId>spring-cloud-aws-starter-s3</artifactId> <version> to a 4.x.x
release that is compatible with Spring Boot 4.0.4 (e.g., 4.0.x or the latest
4.x), verify against the Spring Cloud AWS compatibility matrix, then rebuild
(mvn) to ensure no dependency conflicts; if you manage versions via a BOM,
update the BOM entry instead of the direct version.
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 44-67: The uploadDocument method in DocumentService must validate
MultipartFile metadata before persisting: check file.getOriginalFilename() and
file.getContentType() (and optionally file.getSize()) for null/blank values and
throw a BadRequestException with a clear message if invalid; update the logic
around creating DocumentEntity (setting fileName, contentType, fileSize) to only
proceed after these guards so DB non-null constraints won't cause a 500, and
ensure the thrown exception type matches existing API error handling.
- Around line 55-71: The S3 upload in DocumentService currently happens before
persisting DocumentEntity, risking orphaned S3 objects if
documentRepository.save(...) fails; fix by wrapping the repository save and
mapping steps in a try/catch so that if any exception occurs during save or
mapping you call s3Template.delete(bucket, s3Key) to remove the uploaded object,
log the failure, and then rethrow (or wrap) the original exception as an
AppException; update the method that contains this logic in DocumentService (the
block creating DocumentEntity, calling documentRepository.save, and returning
documentMapper.toDTO) to perform this compensating cleanup on persistence
failure.
- Around line 102-122: The current deleteDocument flow calls
s3Template.deleteObject(...) before documentRepository.delete(...), risking DB
rows pointing to missing S3 objects if the DB commit fails; change the flow so
the DB deletion is performed inside the transaction and the S3 deletion runs
only after successful commit: call documentRepository.delete(entity) (and flush
if needed) inside deleteDocument, then register a post-commit callback (using
Spring's TransactionSynchronization/TransactionSynchronizationManager or
TransactionSynchronizationAdapter) to invoke s3Template.deleteObject(bucket,
entity.getS3Key()) in afterCommit; this ensures the metadata is removed
atomically from the DB and S3 cleanup happens only when the transaction
successfully commits.
In
`@src/main/java/org/example/projektarendehantering/presentation/web/DocumentUiController.java`:
- Around line 38-49: The Content-Disposition header in downloadDocument uses
entity.getFileName() directly, risking header injection; sanitize and encode the
filename before inserting it into the header in downloadDocument (and any other
places using DocumentEntity.getFileName()). Specifically, strip or reject CR/LF
characters, escape or remove double quotes, and set both a safe quoted filename
and an RFC 5987 encoded filename* (e.g., use URLEncoder with UTF-8 for the
filename* value) when building the header instead of inserting
entity.getFileName() raw.
In `@src/main/resources/application.properties`:
- Around line 20-23: Currently S3 credentials are hardcoded in
application.properties under keys spring.cloud.aws.credentials.access-key and
spring.cloud.aws.credentials.secret-key; replace those literal secrets with
environment/secret placeholders (e.g. use ${ENV_VAR_NAME:default} style) so
credentials are supplied from env vars or a secret store at runtime and only
sensible defaults are present for local dev, and remove any literal "minioadmin"
values; ensure the keys to update are spring.cloud.aws.credentials.access-key,
spring.cloud.aws.credentials.secret-key and optionally
spring.cloud.aws.s3.endpoint if you want a local default, and document the
required env var names in README or deployment config.
- Line 17: The runtime config currently sets
logging.level.org.springframework.security=DEBUG which may expose sensitive auth
details; change this property to a non-verbose level (e.g., INFO or WARN) in
application.properties and move any DEBUG-level Spring Security logging into a
dev/test-specific profile (e.g., application-dev.properties or a Spring profile
override) so DEBUG is not enabled in production environments.
In `@src/main/resources/templates/cases/detail.html`:
- Line 59: The template currently uses integer division in the expression
"${doc.fileSize / 1024} + ' KB'" which truncates small sizes to "0 KB"; change
the expression in the cases/detail.html span to perform floating-point division
and format decimals (e.g. divide by 1024.0 and use Thymeleaf number formatting
like `#numbers.formatDecimal`(..., 1 or 2)) so file sizes display with one or two
decimal places and then append "KB"; update the span using the same identifier
expression to ensure UX shows "0.1 KB" etc. instead of "0 KB".
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 19-30: Add a Docker healthcheck to the minio service so Compose
waits for MinIO readiness; update the minio service (identified by service name
"minio" and container_name "projekt-arendehantering-minio", using image
"minio/minio" and command "server /data --console-address \":9001\"") to include
a healthcheck that probes the MinIO readiness endpoint (e.g., a curl --fail
against http://localhost:9000/minio/health/ready) with sensible options
(interval, timeout, retries, start_period) so the service is marked healthy only
after MinIO is ready.
- Line 20: Replace the unpinned MinIO image tag "image: minio/minio" with a
specific, immutable version (e.g., "minio/minio:<version>" or a RELEASE tag) to
ensure reproducible deployments; update the docker-compose.yml entry for the
MinIO service to mirror how the postgres service is pinned so future updates
won't change the runtime image unexpectedly.
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 49-50: Extract the repeated authorization predicate into a private
helper method in DocumentService, e.g. assertCaseAccess(Actor actor, CaseEntity
caseEntity), that checks if actor.isManager() ||
actor.userId().equals(caseEntity.getOwnerId()) ||
actor.userId().equals(caseEntity.getHandlerId()) and throws
NotAuthorizedException("Not authorized to upload documents to this case") when
false; then replace the duplicated inline checks in the
upload/list/download/delete methods (the blocks currently using
actor.isManager() && actor.userId().equals(caseEntity.getOwnerId()) &&
actor.userId().equals(caseEntity.getHandlerId()) and throwing
NotAuthorizedException) with a single call to assertCaseAccess(...) so all four
call sites use the centralized check.
In
`@src/main/java/org/example/projektarendehantering/presentation/dto/CaseDTO.java`:
- Around line 51-52: The setter setDocuments currently allows assigning null
which forces callers to guard against null; update setDocuments to enforce a
non-null list invariant by replacing any null input with an empty list (e.g.,
Collections.emptyList() or new ArrayList<>()) and/or defensively copying the
provided list so the documents field is never null; reference setDocuments and
getDocuments (and the documents field) when making the change.
In
`@src/main/java/org/example/projektarendehantering/presentation/web/DocumentUiController.java`:
- Around line 38-39: Validate that the path caseId matches the document's actual
case in both downloadDocument and deleteDocument: inside each method call
documentService.getEntity(documentId) to obtain the DocumentEntity, compare
entity.getCaseEntity().getId() to the `@PathVariable` caseId, and if they differ
throw a BadRequestException (or similar) before proceeding with
securityActorAdapter.currentUser(), documentService.downloadDocument(...) or
documentService.deleteDocument(...); this ensures URL caseId cannot be tampered
with and maps to the document's real case.
- Around line 31-36: The uploadDocument method in DocumentUiController currently
declares throws IOException which will bubble up as a 500; catch IOException
around the call to documentService.uploadDocument(actor, caseId, file) and
translate it into a user-friendly response: log the exception, add an error
message (e.g. via RedirectAttributes or Model) and return a redirect to the case
view or an error page instead of throwing; keep
securityActorAdapter.currentUser() and documentService.uploadDocument(...) as-is
and only wrap the service call in the try/catch to handle upload failures
gracefully.
In
`@src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java`:
- Around line 115-129: Add two more unit tests in DocumentServiceTest mirroring
deleteDocument_shouldAllowOwner: one (deleteDocument_shouldAllowManager) that
calls documentService.deleteDocument(managerActor, docId) with a mocked
documentRepository returning the DocumentEntity and then verifies
s3Template.deleteObject(...) and documentRepository.delete(...) were invoked;
and one (deleteDocument_shouldDenyUnauthorized) that constructs an unauthorized
Actor, stubs documentRepository.findById(docId) to return the entity, calls
documentService.deleteDocument(unauthorizedActor, docId) and asserts that a
NotAuthorizedException is thrown. Ensure you reuse the same setup (docId,
DocumentEntity, caseEntity) and the same verification of s3Template and
documentRepository behavior as in deleteDocument_shouldAllowOwner.
In
`@src/test/java/org/example/projektarendehantering/DocumentIntegrationTest.java`:
- Around line 96-106: The test uploadDocument_shouldDenyUnauthorized currently
uses `@WithMockUser`("other") but never creates an Employee with username "other",
so it exercises missing-employee behavior in SecurityActorAdapter instead of
case-level denial; update the test setup to create a second Employee (e.g.,
username "other", role DOCTOR) distinct from the case owner and persist it
before calling mockMvc.perform in uploadDocument_shouldDenyUnauthorized so that
SecurityActorAdapter finds the user but the authorization check on the case
denies access (still asserting NotAuthorizedException); reference the test
method uploadDocument_shouldDenyUnauthorized, SecurityActorAdapter, and
NotAuthorizedException when adding the new employee creation in the test setup.
🪄 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: 78746f23-58b4-4618-a3bf-f5dc79eeee13
📒 Files selected for processing (17)
docker-compose.ymlpom.xmlsrc/main/java/org/example/projektarendehantering/application/service/CaseMapper.javasrc/main/java/org/example/projektarendehantering/application/service/DocumentMapper.javasrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/common/Actor.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/DocumentEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/DocumentRepository.javasrc/main/java/org/example/projektarendehantering/presentation/dto/CaseDTO.javasrc/main/java/org/example/projektarendehantering/presentation/dto/DocumentDTO.javasrc/main/java/org/example/projektarendehantering/presentation/web/DocumentUiController.javasrc/main/resources/application.propertiessrc/main/resources/templates/cases/detail.htmlsrc/test/java/org/example/projektarendehantering/DocumentIntegrationTest.javasrc/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.javasrc/test/resources/application.properties
- Exclude Spring Cloud AWS autoconfigurations in ProjektArendehanteringApplication - Manually configure S3Client, S3Presigner, and S3Template in S3Config - Update DocumentService to use ObjectMetadata for uploads - Fix DocumentServiceTest and DocumentIntegrationTest to match new signatures and mock requirements
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (3)
45-69:⚠️ Potential issue | 🟠 MajorValidate multipart metadata before using it in S3 key/entity fields.
Line 54, Line 58, Line 66, and Line 68 rely on
MultipartFilemetadata without null/blank guards. Invalid input can surface as server errors instead of a clear client-side validation failure.Suggested fix
`@Transactional` public DocumentDTO uploadDocument(Actor actor, UUID caseId, MultipartFile file) throws IOException { + if (file == null || file.isEmpty()) { + throw new BadRequestException("File is required"); + } + String originalFilename = file.getOriginalFilename(); + if (originalFilename == null || originalFilename.isBlank()) { + throw new BadRequestException("File name is required"); + } + String contentType = file.getContentType(); + if (contentType == null || contentType.isBlank()) { + throw new BadRequestException("Content type is required"); + } + CaseEntity caseEntity = caseRepository.findById(caseId) .orElseThrow(() -> new BadRequestException("Case not found")); @@ - String s3Key = UUID.randomUUID().toString() + "-" + file.getOriginalFilename(); + String s3Key = UUID.randomUUID().toString() + "-" + originalFilename; @@ ObjectMetadata metadata = ObjectMetadata.builder() - .contentType(file.getContentType()) + .contentType(contentType) .build(); @@ - entity.setFileName(file.getOriginalFilename()); + entity.setFileName(originalFilename); @@ - entity.setContentType(file.getContentType()); + entity.setContentType(contentType);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 45 - 69, In uploadDocument, validate MultipartFile metadata before using it: check file != null, non-blank file.getOriginalFilename(), non-null file.getContentType(), and file.getSize() > 0 and throw a BadRequestException for invalid input; only then build s3Key (use a sanitized/or fallback filename when combining UUID with file.getOriginalFilename()), create ObjectMetadata with the validated contentType, call s3Template.upload with file.getInputStream(), and set entity fields via entity.setFileName(...), entity.setS3Key(...), entity.setContentType(...), and entity.setFileSize(...) using the validated values to avoid NPEs or empty values being persisted.
56-75:⚠️ Potential issue | 🔴 CriticalPrevent orphaned S3 objects when DB persistence fails after upload.
Line 60 writes externally before Line 74 persists metadata. If save/mapping fails, uploaded objects remain orphaned.
Suggested fix
try { ObjectMetadata metadata = ObjectMetadata.builder() .contentType(file.getContentType()) .build(); s3Template.upload(bucket, s3Key, file.getInputStream(), metadata); } catch (Exception e) { throw new AppException("S3_UPLOAD_FAILED", "Failed to upload file to S3: " + e.getMessage()); } DocumentEntity entity = new DocumentEntity(); @@ -DocumentEntity saved = documentRepository.save(entity); -return documentMapper.toDTO(saved); +try { + DocumentEntity saved = documentRepository.save(entity); + return documentMapper.toDTO(saved); +} catch (RuntimeException ex) { + try { + s3Template.deleteObject(bucket, s3Key); + } catch (Exception cleanupEx) { + // add logging/alerting here + } + throw ex; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 56 - 75, The upload flow in DocumentService currently writes to S3 via s3Template.upload before persisting metadata with documentRepository.save/documentMapper.toDTO, risking orphaned S3 objects if the DB save fails; modify the method to catch persistence/mapping exceptions around documentRepository.save and documentMapper.toDTO and, in that failure path, call s3Template.delete(bucket, s3Key) (or equivalent) to remove the uploaded object, then rethrow the original exception as an AppException; ensure any deletion errors are handled/logged but do not suppress the original persistence error so callers see the real failure.
118-127:⚠️ Potential issue | 🔴 CriticalAvoid S3-first delete to prevent DB rows pointing to missing objects.
Line 119 deletes from S3 before Line 126 deletes DB metadata. If DB delete/commit fails, the system can keep a row that references a non-existent object.
Use DB deletion as the transactional source of truth, and run S3 deletion only after successful commit (or via outbox/retry worker).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 118 - 127, The current delete flow in DocumentService deletes the S3 object via s3Template.deleteObject(bucket, entity.getS3Key()) before removing the DB row (documentRepository.delete(entity)), which risks DB rows referencing missing S3 objects if the DB delete/commit fails; change the flow so the database delete is the source of truth: perform documentRepository.delete(entity) inside the transaction and defer S3 deletion until after successful commit (use a transaction synchronization/afterCommit hook or publish an outbox/event for a retryable worker to call s3Template.deleteObject(bucket, entity.getS3Key())); ensure any thrown S3 errors are handled/retried by the async/outbox worker rather than rolling back the DB deletion.
🤖 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/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 49-52: The document service is using a broader authorization check
(actor.isManager() || actor.userId().equals(caseEntity.getOwnerId()) ||
actor.userId().equals(caseEntity.getHandlerId())) that is inconsistent with the
role-aware policy in CaseService (doctor-owner, nurse-handler, or manager);
extract a shared role-aware helper (e.g.,
CaseAuthorizationHelper.isAuthorizedToModifyCase(actor, caseEntity) or
CaseAuthorizationService.authorizeDocumentOperation(actor, caseEntity)) that
encapsulates the exact logic used in CaseService and replace the inline checks
in DocumentService (the blocks using actor.isManager(), actor.userId(),
caseEntity.getOwnerId(), caseEntity.getHandlerId() around the upload/delete
methods) with calls to that helper so all services use the same role-specific
policy.
---
Duplicate comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 45-69: In uploadDocument, validate MultipartFile metadata before
using it: check file != null, non-blank file.getOriginalFilename(), non-null
file.getContentType(), and file.getSize() > 0 and throw a BadRequestException
for invalid input; only then build s3Key (use a sanitized/or fallback filename
when combining UUID with file.getOriginalFilename()), create ObjectMetadata with
the validated contentType, call s3Template.upload with file.getInputStream(),
and set entity fields via entity.setFileName(...), entity.setS3Key(...),
entity.setContentType(...), and entity.setFileSize(...) using the validated
values to avoid NPEs or empty values being persisted.
- Around line 56-75: The upload flow in DocumentService currently writes to S3
via s3Template.upload before persisting metadata with
documentRepository.save/documentMapper.toDTO, risking orphaned S3 objects if the
DB save fails; modify the method to catch persistence/mapping exceptions around
documentRepository.save and documentMapper.toDTO and, in that failure path, call
s3Template.delete(bucket, s3Key) (or equivalent) to remove the uploaded object,
then rethrow the original exception as an AppException; ensure any deletion
errors are handled/logged but do not suppress the original persistence error so
callers see the real failure.
- Around line 118-127: The current delete flow in DocumentService deletes the S3
object via s3Template.deleteObject(bucket, entity.getS3Key()) before removing
the DB row (documentRepository.delete(entity)), which risks DB rows referencing
missing S3 objects if the DB delete/commit fails; change the flow so the
database delete is the source of truth: perform
documentRepository.delete(entity) inside the transaction and defer S3 deletion
until after successful commit (use a transaction synchronization/afterCommit
hook or publish an outbox/event for a retryable worker to call
s3Template.deleteObject(bucket, entity.getS3Key())); ensure any thrown S3 errors
are handled/retried by the async/outbox worker rather than rolling back the DB
deletion.
🪄 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: 24ba2162-9ea5-4a24-b2eb-a446007ada2f
📒 Files selected for processing (5)
src/main/java/org/example/projektarendehantering/ProjektArendehanteringApplication.javasrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/infrastructure/config/S3Config.javasrc/test/java/org/example/projektarendehantering/DocumentIntegrationTest.javasrc/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java
…ibility - Use Spring Cloud AWS BOM 4.0.0 - Revert manual S3Config workaround - Remove autoconfiguration exclusions in ProjektArendehanteringApplication - Update tests to verify S3 upload with ObjectMetadata
…-coded environment variables to .env and .env.*, updated gitignore
… safer variant, also included some light lombok logging
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (1)
66-67:⚠️ Potential issue | 🟠 MajorAuthorization is still broader than the case policy and can drift from
CaseService.Line 66/109/125/140 uses manager-or-owner-or-handler regardless of actor role, while
CaseService.requireCanReadis role-aware (doctor-owner, nurse-handler, manager). This can over-authorize and create inconsistent behavior across services.Also applies to: 109-110, 125-126, 140-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 66 - 67, The authorization check in DocumentService (the manager-or-owner-or-handler conditional used around upload/delete points) is broader than the role-aware policy in CaseService.requireCanRead and risks drifting; replace the inline checks by delegating to the CaseService role-aware method (e.g., call CaseService.requireCanRead(caseEntity, actor) or, if semantics differ, add a CaseService.requireCanUpload/requireCanModify that enforces doctor-owner, nurse-handler, manager rules) and use that single authoritative method in DocumentService to validate access before proceeding.
🧹 Nitpick comments (1)
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (1)
77-79: Preserve the root cause when wrapping S3 upload errors.Line 78 drops the original exception, which makes diagnostics harder.
AppExceptionalready supports a cause.💡 Proposed fix
- } catch (Exception e) { - throw new AppException("S3_UPLOAD_FAILED", "Failed to upload file to S3"); + } catch (Exception e) { + throw new AppException("S3_UPLOAD_FAILED", "Failed to upload file to S3", e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 77 - 79, The catch block in DocumentService that currently does "throw new AppException(\"S3_UPLOAD_FAILED\", \"Failed to upload file to S3\");" loses the original exception; change it to pass the caught exception as the cause (e.g., throw new AppException("S3_UPLOAD_FAILED", "Failed to upload file to S3", e)) so the root cause is preserved for diagnostics.
🤖 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/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 153-154: The call to
TransactionSynchronizationManager.registerSynchronization in deleteDocument can
throw IllegalStateException when no transaction synchronization is active (e.g.,
tests using `@InjectMocks`); wrap the registerSynchronization(...) call with a
guard using TransactionSynchronizationManager.isSynchronizationActive(), and if
that returns false run the synchronization cleanup logic immediately (or call
the same cleanup method/path directly) so non-transactional contexts have a safe
fallback instead of registering a TransactionSynchronization; ensure you
reference the existing TransactionSynchronization instance/class and the
deleteDocument method when applying the change.
---
Duplicate comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 66-67: The authorization check in DocumentService (the
manager-or-owner-or-handler conditional used around upload/delete points) is
broader than the role-aware policy in CaseService.requireCanRead and risks
drifting; replace the inline checks by delegating to the CaseService role-aware
method (e.g., call CaseService.requireCanRead(caseEntity, actor) or, if
semantics differ, add a CaseService.requireCanUpload/requireCanModify that
enforces doctor-owner, nurse-handler, manager rules) and use that single
authoritative method in DocumentService to validate access before proceeding.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 77-79: The catch block in DocumentService that currently does
"throw new AppException(\"S3_UPLOAD_FAILED\", \"Failed to upload file to S3\");"
loses the original exception; change it to pass the caught exception as the
cause (e.g., throw new AppException("S3_UPLOAD_FAILED", "Failed to upload file
to S3", e)) so the root cause is preserved for diagnostics.
🪄 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: 2b53a54c-66e5-481b-bb2d-0b9e8408ce26
📒 Files selected for processing (4)
.gitignorepom.xmlsrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/resources/application.properties
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- pom.xml
- src/main/resources/application.properties
…it tests - Check TransactionSynchronizationManager.isSynchronizationActive() before registering synchronization - Execute S3 delete immediately if no transaction is active (unit test support) - All tests passing with Spring Cloud AWS 4.0.0
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (2)
73-76: Use the validatedcontentTypevariable instead of callingfile.getContentType()again.Line 74 calls
file.getContentType()directly, but this value was already validated and stored in thecontentTypevariable at line 57. For consistency and to ensure the validated value is used throughout:♻️ Proposed fix
try { ObjectMetadata metadata = ObjectMetadata.builder() - .contentType(file.getContentType()) + .contentType(contentType) .build(); s3Template.upload(bucket, s3Key, file.getInputStream(), metadata);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 73 - 76, Replace the direct call to file.getContentType() with the previously validated contentType variable when building ObjectMetadata and uploading to S3: update the ObjectMetadata.builder().contentType(...) and any subsequent use in the upload call within DocumentService (method handling the file upload, around s3Key and metadata creation) to use contentType so the validated MIME type is consistently used.
49-49:throws IOExceptionis never actually thrown.The
file.getInputStream()call at line 76 is inside a try-catch that catchesException, which includesIOException. Any IOException will be wrapped and rethrown asAppException. The declaredthrows IOExceptionis misleading to callers.♻️ Proposed fix
`@Transactional` - public DocumentDTO uploadDocument(Actor actor, UUID caseId, MultipartFile file) throws IOException { + public DocumentDTO uploadDocument(Actor actor, UUID caseId, MultipartFile file) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` at line 49, The method signature for DocumentService.uploadDocument erroneously declares "throws IOException" even though any IOExceptions from file.getInputStream() are caught and wrapped in AppException; remove the "throws IOException" from the uploadDocument method signature and ensure callers rely on the method's runtime AppException handling (no additional checked exception propagation). Update the method declaration (DocumentService.uploadDocument) accordingly and run/adjust callers if any assume a checked IOException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 73-76: Replace the direct call to file.getContentType() with the
previously validated contentType variable when building ObjectMetadata and
uploading to S3: update the ObjectMetadata.builder().contentType(...) and any
subsequent use in the upload call within DocumentService (method handling the
file upload, around s3Key and metadata creation) to use contentType so the
validated MIME type is consistently used.
- Line 49: The method signature for DocumentService.uploadDocument erroneously
declares "throws IOException" even though any IOExceptions from
file.getInputStream() are caught and wrapped in AppException; remove the "throws
IOException" from the uploadDocument method signature and ensure callers rely on
the method's runtime AppException handling (no additional checked exception
propagation). Update the method declaration (DocumentService.uploadDocument)
accordingly and run/adjust callers if any assume a checked IOException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e08f869-d28b-4169-87b4-464d783564ec
📒 Files selected for processing (1)
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java
- Extract validateAccess helper in DocumentService - Ensure roles (Manager, Doctor-Owner, Nurse-Handler) are checked consistently with CaseService - Update all document operations (upload, list, download, delete) to use validateAccess
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 160-162: getEntity currently returns a DocumentEntity from
documentRepository.findById(...) without enforcing case-level access controls;
update DocumentService.getEntity(UUID documentId) to fetch the DocumentEntity,
call the service's validateAccess(...) (using the document's caseId or
equivalent) before returning, and throw the appropriate authorization exception
if validation fails (or make getEntity private and expose an access-checked
public method); reference DocumentService.getEntity, DocumentEntity,
documentRepository.findById, and validateAccess when applying the change.
- Around line 69-98: When uploading to S3 in DocumentService you currently only
delete the object if documentRepository.save(...) throws immediately, leaving
orphans if the DB transaction later rolls back; register a transaction
synchronization right after s3Template.upload(...) that on rollback (or
non-committed completion) will delete the uploaded object (using
s3Template.deleteObject(bucket, s3Key)), and ensure the synchronization is no-op
on afterCommit so normal success keeps the object; use Spring's
TransactionSynchronizationManager.registerSynchronization or
TransactionSynchronizationAdapter within the same method surrounding the call to
s3Template.upload(...) and documentRepository.save(...) to guarantee cleanup on
commit-time rollback.
🪄 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: 4ef34b96-3835-4edf-954b-bc185a21be52
📒 Files selected for processing (1)
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java
…r injection - Use URLEncoder.encode with UTF-8 for filename - Replace + with %20 for RFC 5987 compatibility - Use filename*=UTF-8'' format in header
- Add Actor parameter to getEntity in DocumentService - Call validateAccess inside getEntity - Update DocumentUiController to pass Actor to getEntity
- Register TransactionSynchronization to delete S3 object if transaction is not committed - Keep immediate cleanup for non-transactional contexts (unit tests) - Ensure consistent cleanup regardless of when the rollback occurs
OskarLundqvist33
approved these changes
Apr 13, 2026
OskarLundqvist33
left a comment
OskarLundqvist33
left a comment
Collaborator
There was a problem hiding this comment.
It looks really good. EdgeCase handling is done well and most edge cases is caught and handled nicely. It implements S3 Storage correctly and seemingly works well.
Only minor thing that might be missing would be a limit to file size/uploads to prevent malicious uploads, could be that I just didn't find it, but if it's not there it could be worth implementing, maybe as a separate PR?
mattknatt
approved these changes
Apr 13, 2026
mattknatt
left a comment
mattknatt
left a comment
Collaborator
There was a problem hiding this comment.
Very nice implementation. One minor thing to add could be a bucket configurator that automatically creates a documents bucket when the app is launched. But not necessary, just a nice-to-have feature.
Great job (Y)
This was referenced Apr 21, 2026
Closed
Merged
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
3 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.
closes issue #8
Summary by CodeRabbit
New Features
Configuration
Tests
Chores