Files+storage - #8
Merged
Merged
Conversation
…e upload/download
|
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 43 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 (5)
📝 WalkthroughWalkthroughThis pull request adds MinIO/S3-backed file attachment functionality to the application. It includes configuration classes for S3 properties and MinIO client setup, a transactional attachment upload service with error compensation, a MinIO storage service for object operations, attachment download capability, and REST API endpoints for upload, download, and listing attachments by ticket. Documentation and configuration properties for local development are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AttachmentController
participant AttachmentService
participant TicketRepository
participant MinioStorageService
participant MinIO
participant AttachmentRepository
participant AuditService
Client->>AttachmentController: POST /upload (ticketId, file)
AttachmentController->>AttachmentController: Validate file
AttachmentController->>AttachmentService: uploadToTicket(ticketId, file)
AttachmentService->>TicketRepository: findById(ticketId)
alt Ticket not found
TicketRepository-->>AttachmentService: Empty
AttachmentService-->>Client: 404 NOT_FOUND
else Ticket exists
TicketRepository-->>AttachmentService: Ticket
AttachmentService->>MinioStorageService: upload(file)
MinioStorageService->>MinIO: PutObjectArgs
MinIO-->>MinioStorageService: objectKey
MinioStorageService-->>AttachmentService: objectKey
AttachmentService->>AttachmentRepository: save(Attachment)
AttachmentRepository-->>AttachmentService: Attachment (persisted)
AttachmentService->>AuditService: log(ATTACHMENT_ADDED, ...)
AuditService-->>AttachmentService: void
alt Success
AttachmentService-->>AttachmentController: Attachment
AttachmentController-->>Client: 200 OK (AttachmentViewDTO)
else Exception during save/audit
AttachmentService->>MinioStorageService: delete(objectKey)
MinioStorageService->>MinIO: RemoveObjectArgs
MinIO-->>MinioStorageService: void
MinioStorageService-->>AttachmentService: void
AttachmentService-->>Client: 5xx Error
end
end
sequenceDiagram
participant Client
participant AttachmentDownloadController
participant AttachmentRepository
participant MinioStorageService
participant MinIO
Client->>AttachmentDownloadController: GET /api/files/{id}/download
AttachmentDownloadController->>AttachmentRepository: findById(id)
alt Attachment not found
AttachmentRepository-->>AttachmentDownloadController: Empty
AttachmentDownloadController-->>Client: 404 NOT_FOUND
else Attachment exists
AttachmentRepository-->>AttachmentDownloadController: Attachment
AttachmentDownloadController->>MinioStorageService: download(s3Key)
MinioStorageService->>MinIO: GetObjectArgs
MinIO-->>MinioStorageService: GetObjectResponse
MinioStorageService-->>AttachmentDownloadController: GetObjectResponse
AttachmentDownloadController->>AttachmentDownloadController: Build Content-Disposition header
AttachmentDownloadController->>AttachmentDownloadController: Extract Content-Length
AttachmentDownloadController->>Client: 200 OK + InputStreamResource
Client->>MinIO: Stream file content
MinIO-->>Client: File bytes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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: 13
🧹 Nitpick comments (4)
src/main/java/org/example/alfs/service/AuditService.java (1)
20-28: Consider capturing actor identity in audit writes.
AuditLogsupportsuser, but this API cannot set it. Adding an overload that accepts the acting user (when available) would improve audit traceability without breaking anonymous paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/service/AuditService.java` around lines 20 - 28, The current AuditService.log method creates an AuditLog but never sets the acting user; add an overloaded method public void log(AuditAction action, String fieldName, String oldValue, String newValue, Ticket ticket, User actor) that mirrors the existing AuditService.log implementation but calls log.setUser(actor) before auditLogRepository.save(log), and keep the existing log(...) signature to preserve anonymous usage; update any callers that can provide a User to call the new overload.src/main/java/org/example/alfs/controllers/FileController.java (2)
50-51: Import@GetMappinginstead of using fully qualified name.Other annotations like
@PostMappingare imported. Use the same pattern for consistency.Proposed fix
Add to imports:
import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.RequestMapping;Then:
- `@org.springframework.web.bind.annotation.GetMapping` + `@GetMapping` public ResponseEntity<?> listByTicket(`@RequestParam`(name = "ticketId", required = true) Long ticketId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/FileController.java` around lines 50 - 51, The `@GetMapping` annotation is used via its fully-qualified name in the FileController.listByTicket method; change it to the imported form for consistency with other mappings by adding an import for org.springframework.web.bind.annotation.GetMapping and replacing the fully-qualified annotation on the listByTicket method with `@GetMapping` (leave the method signature and RequestParam as-is).
39-45: Duplicate DTO mapping logic — extract to a helper method.The
Attachment→AttachmentViewDTOmapping is repeated verbatim. Extract to a private method or add a static factory/constructor on the DTO.Proposed refactor
+ private AttachmentViewDTO toDto(Attachment att) { + return new AttachmentViewDTO( + att.getId(), + att.getTicket() != null ? att.getTicket().getId() : null, + att.getFileName(), + att.getS3Key(), + att.getUploadedAt() + ); + } + `@PostMapping`("/upload") public ResponseEntity<?> upload(...) throws Exception { ... Attachment att = attachmentService.uploadToTicket(ticketId, file); - AttachmentViewDTO dto = new AttachmentViewDTO( - att.getId(), - att.getTicket() != null ? att.getTicket().getId() : null, - att.getFileName(), - att.getS3Key(), - att.getUploadedAt() - ); + AttachmentViewDTO dto = toDto(att); return ResponseEntity.ok(dto); } ... - var dtoList = attachments.stream().map(att -> new AttachmentViewDTO( - att.getId(), - att.getTicket() != null ? att.getTicket().getId() : null, - att.getFileName(), - att.getS3Key(), - att.getUploadedAt() - )).toList(); + var dtoList = attachments.stream().map(this::toDto).toList();Also applies to: 60-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/FileController.java` around lines 39 - 45, The Attachment→AttachmentViewDTO construction is duplicated in FileController; extract the mapping into a single reusable method or DTO factory to avoid repetition. Add either a private method in FileController (e.g., private AttachmentViewDTO mapAttachmentToDTO(Attachment att)) that returns new AttachmentViewDTO(att.getId(), att.getTicket() != null ? att.getTicket().getId() : null, att.getFileName(), att.getS3Key(), att.getUploadedAt()), or add a static factory on AttachmentViewDTO (e.g., AttachmentViewDTO.fromAttachment(Attachment att)) that encapsulates the same logic, and replace both verbatim constructions (the one at the shown diff and the one at lines 60-66) with a call to that new method/factory.src/main/java/org/example/alfs/service/storage/MinioStorageService.java (1)
57-59: Filename sanitization is minimal — consider expanding it.The current implementation only replaces
\and/. Depending on downstream usage (e.g., Content-Disposition headers), other characters like null bytes, quotes, or control characters could cause issues. This is acceptable for now since the objectKey is UUID-prefixed, but worth noting for future hardening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/service/storage/MinioStorageService.java` around lines 57 - 59, The sanitize method in MinioStorageService only replaces backslashes and slashes, leaving other problematic characters (e.g., null bytes, quotes, control chars, and path traversal sequences) that can cause issues downstream; update sanitize(String name) to normalize and strip or replace a broader set of unsafe characters — e.g., remove control characters (chars <= 0x1F and 0x7F), strip NULs, replace quotes and whitespace like CR/LF, and collapse sequences like "../" — while preserving the existing UUID-prefixing logic for objectKey generation so behavior remains stable; ensure the method is referenced where object keys are composed so all callers use the hardened sanitizer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 68-70: The README notes that /api/files upload and download
endpoints are unauthenticated; before release, add authentication and
authorization checks to those handlers (the /api/files upload and download route
handlers) so only authenticated users can access them and only authorized users
can upload/download files tied to a Ticket they own or are permitted to access;
update the upload handler to validate user identity (e.g., via existing auth
middleware/session/token check), enforce ownership/permission tied to the
provided ticketId, and reject unauthenticated/unauthorized requests, and do the
same for the download handler while logging authorization failures.
In `@src/main/java/org/example/alfs/config/S3Properties.java`:
- Around line 5-12: Add fail-fast validation to the S3Properties configuration
by annotating the class S3Properties with `@Validated` and adding Jakarta Bean
Validation constraints (`@NotBlank`) to required fields: endpoint, accessKey,
secretKey, bucket, and region; keep the existing `@ConfigurationProperties`(prefix
= "storage.s3") and ensure the boolean secure field remains optional. This will
cause Spring Boot to validate these properties on startup and fail fast if any
of the annotated fields are missing or blank.
In `@src/main/java/org/example/alfs/config/StorageConfig.java`:
- Around line 13-17: In StorageConfig.minioClient(S3Properties props) add
.region(props.getRegion()) to the MinioClient.builder() so the configured region
from S3Properties is respected; also remove any attempt to call a non-existent
.secure() on the builder and instead ensure TLS is handled by either including
the scheme in props.getEndpoint() (https://...) or by building the client with
the endpoint(host, port, secure) overload if you parse
host/port/props.getSecure() from S3Properties—update minioClient to use one of
these approaches and document that props.getSecure() is applied via endpoint
formation, not a .secure() builder method.
In `@src/main/java/org/example/alfs/controllers/FileController.java`:
- Around line 51-57: In FileController.listByTicket the null check for the
`@RequestParam` Long ticketId is unreachable because `@RequestParam`(required =
true) causes Spring to reject missing or non-numeric params before the method is
invoked; either remove the redundant "ticketId == null" branch and only validate
"ticketId <= 0" in listByTicket, or if you intended the parameter to be optional
change the annotation to `@RequestParam`(required = false) and keep null-handling
logic — update the method signature/annotation and the conditional accordingly
in FileController.listByTicket.
In `@src/main/java/org/example/alfs/controllers/FileDownloadController.java`:
- Around line 33-40: The current FileDownloadController uses
storageService.download(att.getS3Key()) and then
GetObjectResponse.readAllBytes(), which loads the entire file into memory;
replace that pattern by streaming the response instead: obtain the
InputStream/ReadableByteChannel from GetObjectResponse (via its input stream or
SDK-provided stream), wrap it in an InputStreamResource or implement a
StreamingResponseBody, set the Content-Disposition and Content-Type as before,
and return ResponseEntity.ok().contentLength(...) (if available) .body(the
stream resource) so bytes are streamed to the client and the GetObjectResponse
stream is closed after the response is produced.
- Line 37: Sanitize and encode the filename used in the Content-Disposition
header to prevent header injection: replace the current inline header
construction that uses att.getFileName() directly with Spring's
ContentDisposition.builder("attachment").filename(...,
StandardCharsets.UTF_8).build().toString() (or otherwise RFC5987-encode the
filename), and add the necessary imports
(org.springframework.http.ContentDisposition and
java.nio.charset.StandardCharsets); ensure you reference att.getFileName() as
the source and apply the encoding/sanitization before passing it into the header
call in FileDownloadController (where the
header(HttpHeaders.CONTENT_DISPOSITION, ...) is set).
- Around line 29-31: The download method currently throws
IllegalArgumentException when attachmentRepository.findById(id) is empty, which
leads to a 500; change this to return a 404 by either throwing a
ResponseStatusException(HttpStatus.NOT_FOUND, "Attachment not found: "+id) or by
returning ResponseEntity.notFound().build() when the Optional is empty. Update
the download(...) method (where Attachment att is retrieved) to use
ResponseStatusException or an explicit ResponseEntity.notFound() path so the
REST API returns 404 instead of propagating IllegalArgumentException.
In `@src/main/java/org/example/alfs/dto/attachment/AttachmentViewDTO.java`:
- Around line 9-10: AttachmentViewDTO currently exposes the internal storage key
via the s3Key field; remove s3Key from the public DTO (AttachmentViewDTO) and
any other public DTOs that expose storage keys, replace with a public-safe
property such as downloadUrl or attachmentId if callers need a reference, and
ensure any mapping code (constructors, builders, mappers) uses the internal
s3Key only server-side when generating presigned URLs or resolving content; keep
the raw s3Key strictly internal (e.g., in the persistence/model layer) and
update usages that serialized s3Key to instead return the safe field.
In `@src/main/java/org/example/alfs/service/AttachmentService.java`:
- Around line 33-34: The current AttachmentService uses
ticketRepository.findById(...).orElseThrow(() -> new
IllegalArgumentException(...)) which will surface a 500; replace the
IllegalArgumentException with a HTTP-aware exception (e.g., throw new
ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found: " + ticketId))
or throw a custom NotFoundException that is handled by your `@ControllerAdvice`;
update the code around the Ticket ticket assignment in AttachmentService (and
similarly align FileDownloadController if present) to use
ResponseStatusException or the custom exception so missing tickets return a 404.
- Around line 31-46: The uploadToTicket method currently calls
storageService.upload(...) before saving DB state, risking orphaned objects if
attachmentRepository.save(...) or auditService.log(...) fails; modify
uploadToTicket to add compensation: after calling
storageService.upload(objectKey) keep the returned objectKey, wrap the DB save
and audit calls in a try/catch, and if any exception occurs call a new
MinioStorageService.delete(objectKey) (implement delete in your storage service)
before rethrowing the exception; alternatively you can implement a two-phase
flow by creating the Attachment (e.g., status PENDING), saving it first via
attachmentRepository.save(...), then calling storageService.upload(...) and
updating the Attachment with setS3Key(...) and status COMPLETE to avoid orphaned
uploads.
In `@src/main/java/org/example/alfs/service/storage/MinioStorageService.java`:
- Around line 36-41: PutObjectArgs is using file.getContentType() which can be
null for a MultipartFile; update the PutObjectArgs.builder() call so the content
type is replaced with a safe default when null (e.g., use a helper or inline
ternary to use "application/octet-stream" if file.getContentType() == null)
before building the args for PutObjectArgs in MinioStorageService (referencing
PutObjectArgs.builder(), props.getBucket(), objectKey, and
file.getContentType()) so MinIO always receives a non-null contentType.
In `@src/main/resources/application.properties`:
- Line 4: Replace the encoding artifact "milj�er" in the application.properties
comment with the correct Swedish word "miljöer" and scan the same file for
similar mojibake occurrences (notably the other commented line referenced) to
correct them so all comments use proper UTF-8 characters.
- Around line 5-7: The file currently hardcodes active S3 credentials
(storage.s3.accessKey, storage.s3.secretKey) and the endpoint; replace these
plaintext defaults with environment-backed placeholders (e.g., use property
placeholders that read from env vars or system properties) and remove real
secrets — for example make storage.s3.accessKey and storage.s3.secretKey
reference environment variables with safe non-secret dev fallbacks (or
empty/CHANGE_ME) and do the same for storage.s3.endpoint so credentials are not
committed; update any README or config docs to instruct setting the
corresponding env vars before deployment and remove the real values from the
tracked file.
---
Nitpick comments:
In `@src/main/java/org/example/alfs/controllers/FileController.java`:
- Around line 50-51: The `@GetMapping` annotation is used via its fully-qualified
name in the FileController.listByTicket method; change it to the imported form
for consistency with other mappings by adding an import for
org.springframework.web.bind.annotation.GetMapping and replacing the
fully-qualified annotation on the listByTicket method with `@GetMapping` (leave
the method signature and RequestParam as-is).
- Around line 39-45: The Attachment→AttachmentViewDTO construction is duplicated
in FileController; extract the mapping into a single reusable method or DTO
factory to avoid repetition. Add either a private method in FileController
(e.g., private AttachmentViewDTO mapAttachmentToDTO(Attachment att)) that
returns new AttachmentViewDTO(att.getId(), att.getTicket() != null ?
att.getTicket().getId() : null, att.getFileName(), att.getS3Key(),
att.getUploadedAt()), or add a static factory on AttachmentViewDTO (e.g.,
AttachmentViewDTO.fromAttachment(Attachment att)) that encapsulates the same
logic, and replace both verbatim constructions (the one at the shown diff and
the one at lines 60-66) with a call to that new method/factory.
In `@src/main/java/org/example/alfs/service/AuditService.java`:
- Around line 20-28: The current AuditService.log method creates an AuditLog but
never sets the acting user; add an overloaded method public void log(AuditAction
action, String fieldName, String oldValue, String newValue, Ticket ticket, User
actor) that mirrors the existing AuditService.log implementation but calls
log.setUser(actor) before auditLogRepository.save(log), and keep the existing
log(...) signature to preserve anonymous usage; update any callers that can
provide a User to call the new overload.
In `@src/main/java/org/example/alfs/service/storage/MinioStorageService.java`:
- Around line 57-59: The sanitize method in MinioStorageService only replaces
backslashes and slashes, leaving other problematic characters (e.g., null bytes,
quotes, control chars, and path traversal sequences) that can cause issues
downstream; update sanitize(String name) to normalize and strip or replace a
broader set of unsafe characters — e.g., remove control characters (chars <=
0x1F and 0x7F), strip NULs, replace quotes and whitespace like CR/LF, and
collapse sequences like "../" — while preserving the existing UUID-prefixing
logic for objectKey generation so behavior remains stable; ensure the method is
referenced where object keys are composed so all callers use the hardened
sanitizer.
🪄 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: 86731485-b630-4eea-a6cd-24f433750261
📒 Files selected for processing (12)
README.mdpom.xmlsrc/main/java/org/example/alfs/config/S3Properties.javasrc/main/java/org/example/alfs/config/StorageConfig.javasrc/main/java/org/example/alfs/controllers/FileController.javasrc/main/java/org/example/alfs/controllers/FileDownloadController.javasrc/main/java/org/example/alfs/dto/attachment/AttachmentViewDTO.javasrc/main/java/org/example/alfs/enums/AuditAction.javasrc/main/java/org/example/alfs/service/AttachmentService.javasrc/main/java/org/example/alfs/service/AuditService.javasrc/main/java/org/example/alfs/service/storage/MinioStorageService.javasrc/main/resources/application.properties
Comment on lines
+5
to
+7
| storage.s3.endpoint=http://localhost:9000 | ||
| storage.s3.accessKey=minioadmin | ||
| storage.s3.secretKey=minioadmin |
There was a problem hiding this comment.
Avoid committing active credentials in application defaults.
Keeping storage.s3.accessKey/storage.s3.secretKey as plaintext defaults in tracked config risks accidental reuse in non-dev deployments. Prefer env-backed properties with safe dev fallbacks.
Suggested hardening
-storage.s3.accessKey=minioadmin
-storage.s3.secretKey=minioadmin
+storage.s3.accessKey=${STORAGE_S3_ACCESS_KEY:minioadmin}
+storage.s3.secretKey=${STORAGE_S3_SECRET_KEY:minioadmin}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/application.properties` around lines 5 - 7, The file
currently hardcodes active S3 credentials (storage.s3.accessKey,
storage.s3.secretKey) and the endpoint; replace these plaintext defaults with
environment-backed placeholders (e.g., use property placeholders that read from
env vars or system properties) and remove real secrets — for example make
storage.s3.accessKey and storage.s3.secretKey reference environment variables
with safe non-secret dev fallbacks (or empty/CHANGE_ME) and do the same for
storage.s3.endpoint so credentials are not committed; update any README or
config docs to instruct setting the corresponding env vars before deployment and
remove the real values from the tracked file.
Contributor
There was a problem hiding this comment.
Good point, but we are intentionally keeping this as-is for now.
There was a problem hiding this comment.
@addee1, understood — noted for future reviews. These are default MinIO dev credentials and keeping them inline is a reasonable trade-off for a development/demo setup.
✏️ Learnings added
Learnt from: addee1
Repo: ithsjava25/project-backend-alfs PR: 8
File: src/main/resources/application.properties:5-7
Timestamp: 2026-04-11T16:56:53.425Z
Learning: In the project `ithsjava25/project-backend-alfs`, the MinIO credentials (`storage.s3.accessKey=minioadmin` and `storage.s3.secretKey=minioadmin`) are intentionally hardcoded in `src/main/resources/application.properties`. This is by design for development/demo purposes only — the team is aware these should be externalized via environment variables or a secrets manager in a production setup. Do not flag these as security issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: addee1
Repo: ithsjava25/project-backend-alfs PR: 9
File: src/main/resources/application.properties:19-20
Timestamp: 2026-04-09T10:02:51.794Z
Learning: In the project `ithsjava25/project-backend-alfs`, the JWT secret is intentionally hardcoded in `src/main/resources/application.properties` (`jwt.secret=bXlTdXBlclNlY3JldEtleU15U3VwZXJTZWNyZXRLZXk=`). This is by design for development/demo purposes only — the team is aware it should be externalized via environment variables or a secrets manager in a production setup. Do not flag this as a security issue.
Learnt from: addee1
Repo: ithsjava25/project-backend-alfs PR: 9
File: src/main/java/org/example/alfs/config/SecurityConfig.java:27-29
Timestamp: 2026-04-09T09:49:11.596Z
Learning: In the project `ithsjava25/project-backend-alfs`, the Spring Security configuration (`src/main/java/org/example/alfs/config/SecurityConfig.java`) intentionally disables CSRF and frame options, and permits the H2 console without authentication. This is by design for development-only use — the project is a stateless JWT-based REST API not targeting production deployment. Do not flag these settings as security issues.
…Disposition header
…n AttachmentService
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/alfs/service/storage/MinioStorageService.java (1)
37-41:⚠️ Potential issue | 🟡 MinorDefault unknown MIME type before building
PutObjectArgs.At Line 40,
file.getContentType()can benull/blank for multipart uploads. Use a safe fallback (application/octet-stream) before calling.contentType(...).Proposed fix
try (InputStream is = file.getInputStream()) { + String contentType = file.getContentType(); + if (contentType == null || contentType.isBlank()) { + contentType = "application/octet-stream"; + } PutObjectArgs args = PutObjectArgs.builder() .bucket(props.getBucket()) .object(objectKey) - .contentType(file.getContentType()) + .contentType(contentType) .stream(is, file.getSize(), -1) .build(); minioClient.putObject(args); }In the MinIO Java SDK, does PutObjectArgs.Builder.contentType(String) accept null or blank values, and what is the recommended default MIME type for unknown uploads?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/service/storage/MinioStorageService.java` around lines 37 - 41, The PutObjectArgs builder is receiving a possibly null/blank MIME from file.getContentType(), so update MinioStorageService to normalize the content type before building PutObjectArgs (e.g. String contentType = file.getContentType(); if blank/null set to "application/octet-stream"), then use .contentType(contentType) when constructing PutObjectArgs (symbols: MinioStorageService, file.getContentType(), PutObjectArgs.builder(), .contentType(...), objectKey, .stream(...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/alfs/service/storage/MinioStorageService.java`:
- Around line 37-41: The PutObjectArgs builder is receiving a possibly
null/blank MIME from file.getContentType(), so update MinioStorageService to
normalize the content type before building PutObjectArgs (e.g. String
contentType = file.getContentType(); if blank/null set to
"application/octet-stream"), then use .contentType(contentType) when
constructing PutObjectArgs (symbols: MinioStorageService, file.getContentType(),
PutObjectArgs.builder(), .contentType(...), objectKey, .stream(...)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1877c5e3-f0f0-44ef-894a-d04cbd3c8f60
📒 Files selected for processing (4)
src/main/java/org/example/alfs/controllers/FileDownloadController.javasrc/main/java/org/example/alfs/enums/AuditAction.javasrc/main/java/org/example/alfs/service/AttachmentService.javasrc/main/java/org/example/alfs/service/storage/MinioStorageService.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/example/alfs/enums/AuditAction.java
- src/main/java/org/example/alfs/controllers/FileDownloadController.java
- src/main/java/org/example/alfs/service/AttachmentService.java
| } | ||
|
|
||
| // List attachments for a ticket | ||
| @org.springframework.web.bind.annotation.GetMapping |
Contributor
There was a problem hiding this comment.
Import @GetMapping instead of using fully qualified name
Contributor
|
The conflict in application.properties is resolved! There are a few CodeRabbit comments left to address. Once those are handled, this should be ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/alfs/service/AttachmentService.java (1)
33-34:⚠️ Potential issue | 🟡 MinorUse a 404-friendly exception for missing ticket.
Line 34 still throws
IllegalArgumentException; if not globally mapped, this typically surfaces as 500 instead of 404 for a not-found resource.Suggested fix
+import org.springframework.http.HttpStatus; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; import org.springframework.web.multipart.MultipartFile; +import org.springframework.web.server.ResponseStatusException; ... Ticket ticket = ticketRepository.findById(ticketId) - .orElseThrow(() -> new IllegalArgumentException("Ticket not found: " + ticketId)); + .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found: " + ticketId));#!/bin/bash # Verify whether IllegalArgumentException is already mapped to 404 globally. rg -n --type=java -C3 '@ControllerAdvice|@ExceptionHandler|IllegalArgumentException|HttpStatus\.NOT_FOUND|ResponseStatusException'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/service/AttachmentService.java` around lines 33 - 34, The current orElseThrow in AttachmentService uses IllegalArgumentException when ticket isn't found (Ticket ticket = ticketRepository.findById(ticketId).orElseThrow(() -> new IllegalArgumentException(...))); replace that with a 404-friendly exception: either throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found: " + ticketId) or throw a custom exception annotated with `@ResponseStatus`(HttpStatus.NOT_FOUND) (e.g., ResourceNotFoundException) and update the orElseThrow to use it; target the orElseThrow expression in AttachmentService (and any similar repository.findById usages) so missing resources map to HTTP 404.
🤖 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/alfs/service/AttachmentService.java`:
- Line 40: AttachmentService currently sets attachment filename only when
file.getOriginalFilename() != null, which allows empty/blank names to be
persisted; update the logic in AttachmentService (the att.setFileName(...) call)
to normalize both null and blank/whitespace filenames to "file" (same behavior
as MinioStorageService.upload()), e.g., check StringUtils.isBlank or
trim+isEmpty and assign "file" when blank, otherwise use the original filename.
---
Duplicate comments:
In `@src/main/java/org/example/alfs/service/AttachmentService.java`:
- Around line 33-34: The current orElseThrow in AttachmentService uses
IllegalArgumentException when ticket isn't found (Ticket ticket =
ticketRepository.findById(ticketId).orElseThrow(() -> new
IllegalArgumentException(...))); replace that with a 404-friendly exception:
either throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not
found: " + ticketId) or throw a custom exception annotated with
`@ResponseStatus`(HttpStatus.NOT_FOUND) (e.g., ResourceNotFoundException) and
update the orElseThrow to use it; target the orElseThrow expression in
AttachmentService (and any similar repository.findById usages) so missing
resources map to HTTP 404.
🪄 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: da6ff2f0-ed77-47f3-9e61-5949429a4bb6
📒 Files selected for processing (3)
pom.xmlsrc/main/java/org/example/alfs/service/AttachmentService.javasrc/main/resources/application.properties
✅ Files skipped from review due to trivial changes (2)
- pom.xml
- src/main/resources/application.properties
…f `FileController`
…date FileController accordingly
…ication/octet-stream`
…ication/octet-stream`
|
|
||
| @RestController | ||
| @RequestMapping("/api/files") | ||
| public class FileController { |
Contributor
There was a problem hiding this comment.
Maybe rename AttachmentController for consistency?
|
|
||
| @RestController | ||
| @RequestMapping("/api/files") | ||
| public class FileDownloadController { |
Contributor
There was a problem hiding this comment.
Maybe rename AttachmentDownloadController for consistency?
…ntDownloadController
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/example/alfs/controllers/AttachmentController.java (1)
39-45: Extract duplicatedAttachment -> AttachmentViewDTOmapping.The DTO-construction block is duplicated in both methods, which increases drift risk. Pull it into a private mapper helper and reuse it.
Refactor sketch
- AttachmentViewDTO dto = new AttachmentViewDTO( - att.getId(), - att.getTicket() != null ? att.getTicket().getId() : null, - att.getFileName(), - "/api/files/" + att.getId() + "/download", - att.getUploadedAt() - ); + AttachmentViewDTO dto = toDto(att); ... - var dtoList = attachments.stream().map(att -> new AttachmentViewDTO( - att.getId(), - att.getTicket() != null ? att.getTicket().getId() : null, - att.getFileName(), - "/api/files/" + att.getId() + "/download", - att.getUploadedAt() - )).toList(); + var dtoList = attachments.stream().map(this::toDto).toList(); return ResponseEntity.ok(dtoList); } + + private AttachmentViewDTO toDto(Attachment att) { + return new AttachmentViewDTO( + att.getId(), + att.getTicket() != null ? att.getTicket().getId() : null, + att.getFileName(), + "/api/files/" + att.getId() + "/download", + att.getUploadedAt() + ); + }Also applies to: 60-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/AttachmentController.java` around lines 39 - 45, Extract the duplicated DTO construction into a private mapper in AttachmentController: create a private method (e.g., mapToViewDto(Attachment att) : AttachmentViewDTO) that constructs and returns the AttachmentViewDTO using att.getId(), att.getTicket() != null ? att.getTicket().getId() : null, att.getFileName(), "/api/files/" + att.getId() + "/download", and att.getUploadedAt(); then replace the duplicated blocks in both locations (the two places building AttachmentViewDTO) to call mapToViewDto(att) instead.
🤖 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/alfs/controllers/AttachmentController.java`:
- Around line 29-38: The upload method in AttachmentController currently
forwards invalid ticketId values to attachmentService.uploadToTicket; add the
same positive-ID guard used in listByTicket to validate ticketId > 0 at the
start of upload, and return ResponseEntity.badRequest() with a clear error body
when ticketId is null or <= 0 so the controller consistently rejects
non-positive IDs instead of calling AttachmentService.uploadToTicket with
invalid input.
In
`@src/main/java/org/example/alfs/controllers/AttachmentDownloadController.java`:
- Around line 36-41: The download method in AttachmentDownloadController
currently declares throws Exception and lets storageService.download(...)
exceptions bubble up causing opaque 500s; wrap the call to
storageService.download(att.getS3Key()) in a try/catch, remove or narrow the
throws Exception signature on download(), and map storage errors to explicit
ResponseStatusException responses (e.g., throw new
ResponseStatusException(HttpStatus.NOT_FOUND, ...) when the S3 object is missing
and throw new ResponseStatusException(HttpStatus.BAD_GATEWAY, ...) or
HttpStatus.SERVICE_UNAVAILABLE for storage/backend failures), ensuring the
method returns a ResponseEntity<Resource> or rethrows the mapped
ResponseStatusException accordingly.
---
Nitpick comments:
In `@src/main/java/org/example/alfs/controllers/AttachmentController.java`:
- Around line 39-45: Extract the duplicated DTO construction into a private
mapper in AttachmentController: create a private method (e.g.,
mapToViewDto(Attachment att) : AttachmentViewDTO) that constructs and returns
the AttachmentViewDTO using att.getId(), att.getTicket() != null ?
att.getTicket().getId() : null, att.getFileName(), "/api/files/" + att.getId() +
"/download", and att.getUploadedAt(); then replace the duplicated blocks in both
locations (the two places building AttachmentViewDTO) to call mapToViewDto(att)
instead.
🪄 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: f20550c2-90f4-44d9-b98c-934da6f85a1a
📒 Files selected for processing (5)
src/main/java/org/example/alfs/controllers/AttachmentController.javasrc/main/java/org/example/alfs/controllers/AttachmentDownloadController.javasrc/main/java/org/example/alfs/dto/attachment/AttachmentViewDTO.javasrc/main/java/org/example/alfs/service/AttachmentService.javasrc/main/java/org/example/alfs/service/storage/MinioStorageService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/alfs/service/AttachmentService.java
- src/main/java/org/example/alfs/dto/attachment/AttachmentViewDTO.java
This was referenced Apr 20, 2026
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.
Lägger till MinIO‑integration (SDK, konfiguration, MinioClient‑bean).
Implementerar filuppladdning kopplad till Ticket: sparar i MinIO, lagrar Attachment‑metadata i DB, skriver AuditLog (DOCUMENT_UPLOADED).
Endpoints:
POST /api/files/upload (multipart: ticketId, file) – validerar att fil inte är tom.
GET /api/files?ticketId=... – listar bilagor för ett ärende.
GET /api/files/{id}/download – laddar ner filen.
README uppdaterad med hur man startar MinIO och testar upload/download.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores