Added method to delete images that are broken (mismatch between minio…#107
Conversation
… and database) and refactored test for DocumentController.
|
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 11 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 (3)
📝 WalkthroughWalkthroughThe PR refactors the document download flow by delegating file retrieval responsibility from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as DocumentController
participant Service as DocumentService
participant MinIO as MinIO Service
participant DB as DocumentRepository
Client->>Controller: GET /download/{fileKey}
Controller->>Service: downloadFile(fileKey)
Service->>MinIO: downloadFile(fileKey)
alt File exists
MinIO-->>Service: InputStream
Service-->>Controller: InputStream
Controller-->>Client: HTTP 200 + file content
else File missing
MinIO-->>Service: Exception
Service->>DB: findByFileKey(fileKey)
alt DB record exists
DB-->>Service: Document
Service->>DB: deleteByFileKey(fileKey)
DB-->>Service: deleted
else No DB record
DB-->>Service: null
end
Service-->>Controller: ResponseStatusException (404)
Controller-->>Client: HTTP 404 "File not found"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java (1)
78-100:⚠️ Potential issue | 🟠 MajorThese two 404 tests will fail with 500 errors:
@AuthenticationPrincipalresolves tonullwithout authentication.
DocumentController.getFileunconditionally callsuserDetails.getUser()at the start of the handler, before any 404 branches. Since the prior@BeforeEachsecurity setup was removed and these two tests do not include.with(authentication(...)), the@AuthenticationPrincipalparameter resolves tonull, causing aNullPointerExceptionwhich MockMvc surfaces as a 500 error — not the expected 404. (TheaddFilters = falsesetting disables the security filter chain but does not inject a principal.)Add the same authentication post-processor you used in
getFile_shouldReturnDocument:Diff
- mockMvc.perform(get("/documents/abc")).andExpect(status().isNotFound()); + mockMvc.perform(get("/documents/abc").with(authentication(auth))) + .andExpect(status().isNotFound());Apply this to both tests at lines 82 and 99.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java` around lines 78 - 100, Both failing tests call DocumentController.getFile which dereferences the `@AuthenticationPrincipal` user (userDetails.getUser()) before any 404 logic, so the MockMvc requests need an authentication principal; update the two tests getFile_shouldReturn404_whenDocumentNotFound and getFile_shouldReturn404_whenIncidentNotFound to add the same authentication post-processor used in getFile_shouldReturnDocument (i.e., chain the .with(authentication(...)) post-processor on the mockMvc.perform(get("/documents/abc")) calls) so the principal is injected and the controller can proceed to return 404 instead of throwing a NullPointerException.
🧹 Nitpick comments (2)
src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java (1)
75-75: Consider keeping theContent-Dispositionassertion.Only asserting
status().isOk()no longer verifies that the controller still sets the filename header and content type after delegating toDocumentService.downloadFile. A quickheader().string(HttpHeaders.CONTENT_DISPOSITION, containsString("test.pdf"))(andcontent().contentType(MediaType.APPLICATION_PDF)) would catch future regressions in the response-assembly code that is still controller-owned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java` at line 75, The test currently only asserts status().isOk() for the mockMvc call and should also verify the controller still sets response headers after delegating to DocumentService.downloadFile; update the mockMvc.perform(get("/documents/abc").with(authentication(auth))) assertion to include header().string(HttpHeaders.CONTENT_DISPOSITION, containsString("test.pdf")) and content().contentType(MediaType.APPLICATION_PDF) so the test checks the Content-Disposition filename and PDF content type that the controller is responsible for.src/main/java/org/example/team6backend/document/repository/DocumentRepository.java (1)
18-18:deleteByFileKeyis added but never called.
DocumentService.downloadFileperforms reconciliation withfindByFileKey(...)followed bydocumentRepository.delete(docOpt.get()), so this new derived-delete method is currently dead code. Either switch the service to calldeleteByFileKey(objectKey)directly (avoids the extra SELECT + managed-entity round-trip) or remove the declaration.Note: Spring Data derived
deleteBy...methods must run inside a transaction; sinceDocumentService.downloadFileis@Transactional, that's fine if you choose to use it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/repository/DocumentRepository.java` at line 18, The new repository method deleteByFileKey(String fileKey) is never used; update DocumentService.downloadFile to replace the findByFileKey(...) + documentRepository.delete(docOpt.get()) sequence with a single documentRepository.deleteByFileKey(objectKey) call to avoid the extra SELECT and managed-entity round-trip (DocumentService.downloadFile is already `@Transactional` so the derived delete will run correctly), or if you prefer not to change service logic, remove the unused deleteByFileKey declaration from DocumentRepository to avoid dead code.
🤖 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/team6backend/document/service/DocumentService.java`:
- Around line 63-64: In DocumentService inside the catch (Exception e) block
that currently calls log.warn("Missing file in Minio: {}", objectKey), change
the logging to include the caught exception so the stack trace and root cause
are preserved; update the log.warn invocation that references objectKey to also
pass the exception (e) as the throwable parameter so logs show both the message
and the exception details.
- Around line 61-75: MinioService.downloadFile currently wraps all errors into a
generic RuntimeException which makes DocumentService's catch(Exception) delete
DB rows on transient failures; change MinioService.downloadFile to catch MinIO
client ErrorResponseException (check errorCode == "NoSuchKey" or HTTP 404) and
rethrow a specific checked/unchecked FileNotFoundException (or custom
ObjectNotFoundException), leaving other exceptions unchanged, then update
DocumentService to catch only that FileNotFoundException around
minioService.downloadFile(objectKey) and perform the
documentRepository.findByFileKey(...) + delete only in that branch; for all
other exceptions let them propagate as 5xx (or rethrow as
ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, ...)) and include the
caught exception in the log.warn(...) calls so root causes are preserved.
In `@src/main/resources/static/viewincident.html`:
- Around line 101-105: The race comes from assigning img.onerror after inserting
HTML (where images start loading); instead set the error handler before the
browser can fetch or attach it inline when creating the markup rather than via a
separate querySelector pass. Update the code that builds the images (the
innerHTML insertion that produces elements with class ".incident-img") so each
<img> includes the onerror handler inline (or create elements via
document.createElement('img'), set img.onerror = (...) and then set img.src)
instead of relying on the later document.querySelectorAll(...).forEach(...)
block that currently assigns img.onerror.
- Line 87: Remove the debug console.log that prints the full incident payload:
delete the statement console.log("INCIDENT DATA: ", i) in viewincident.html (the
one referencing variable i) so PII is not exposed to the browser console; if you
need runtime tracing instead, replace it with a gated debug/logging mechanism or
a sanitized log that omits subject/description/comments/documents.
---
Outside diff comments:
In
`@src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java`:
- Around line 78-100: Both failing tests call DocumentController.getFile which
dereferences the `@AuthenticationPrincipal` user (userDetails.getUser()) before
any 404 logic, so the MockMvc requests need an authentication principal; update
the two tests getFile_shouldReturn404_whenDocumentNotFound and
getFile_shouldReturn404_whenIncidentNotFound to add the same authentication
post-processor used in getFile_shouldReturnDocument (i.e., chain the
.with(authentication(...)) post-processor on the
mockMvc.perform(get("/documents/abc")) calls) so the principal is injected and
the controller can proceed to return 404 instead of throwing a
NullPointerException.
---
Nitpick comments:
In
`@src/main/java/org/example/team6backend/document/repository/DocumentRepository.java`:
- Line 18: The new repository method deleteByFileKey(String fileKey) is never
used; update DocumentService.downloadFile to replace the findByFileKey(...) +
documentRepository.delete(docOpt.get()) sequence with a single
documentRepository.deleteByFileKey(objectKey) call to avoid the extra SELECT and
managed-entity round-trip (DocumentService.downloadFile is already
`@Transactional` so the derived delete will run correctly), or if you prefer not
to change service logic, remove the unused deleteByFileKey declaration from
DocumentRepository to avoid dead code.
In
`@src/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java`:
- Line 75: The test currently only asserts status().isOk() for the mockMvc call
and should also verify the controller still sets response headers after
delegating to DocumentService.downloadFile; update the
mockMvc.perform(get("/documents/abc").with(authentication(auth))) assertion to
include header().string(HttpHeaders.CONTENT_DISPOSITION,
containsString("test.pdf")) and content().contentType(MediaType.APPLICATION_PDF)
so the test checks the Content-Disposition filename and PDF content type that
the controller is responsible for.
🪄 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: b20c5c0d-9354-4c1b-977c-ab5c6f0999f3
📒 Files selected for processing (5)
src/main/java/org/example/team6backend/document/controller/DocumentController.javasrc/main/java/org/example/team6backend/document/repository/DocumentRepository.javasrc/main/java/org/example/team6backend/document/service/DocumentService.javasrc/main/resources/static/viewincident.htmlsrc/test/java/org/example/team6backend/document/controller/DocumentControllerTest.java
…ent race conditions -improve MinIO download handling to avoid deleting DB records on transient failures -only remove document records when file is confirmed missing in MinIO (NoSuchKey) -return proper 500 response for temporary MinIO errors instead of false 404 cleanup -improve logging for missing and failed file downloads
1 participant
… and database) and refactored test for DocumentController.
Summary by CodeRabbit