S3 hardening#83
Merged
Merged
Conversation
|
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 50 minutes and 56 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 (2)
📝 WalkthroughWalkthroughSpring retry mechanisms are integrated for S3 operations with a persistent queue for failed deletions, scheduled recovery processing, and exponential backoff. GitHub username normalization is centralized and applied across employee and security layers. Application scheduling is enabled, audit transaction propagation is adjusted to use new transactions, and configuration properties for retry and failed-deletion behavior are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DocumentService
participant S3RetryExecutor
participant S3
participant FailedS3DeletionService
participant Database
Client->>DocumentService: delete(documentId)
DocumentService->>S3RetryExecutor: execute("delete", callback)
alt S3 Deletion Succeeds
S3RetryExecutor->>S3: delete object
S3-->>S3RetryExecutor: success
S3RetryExecutor-->>DocumentService: return
DocumentService->>Database: delete document entity
Database-->>DocumentService: success
else S3 Deletion Fails (Retryable)
S3RetryExecutor->>S3: attempt 1 (fails)
S3-->>S3RetryExecutor: retryable exception
S3RetryExecutor->>S3: attempt 2 (fails)
S3-->>S3RetryExecutor: retryable exception
S3RetryExecutor->>S3: attempt N (exhausted)
S3-->>S3RetryExecutor: AppException(S3_SERVICE_DEGRADED)
S3RetryExecutor-->>DocumentService: throw AppException
DocumentService->>FailedS3DeletionService: enqueue(bucket, key, exception)
FailedS3DeletionService->>Database: persist FailedS3DeletionEntity
Database-->>FailedS3DeletionService: success
FailedS3DeletionService-->>DocumentService: return
DocumentService->>Database: delete document entity
Database-->>DocumentService: success
end
DocumentService-->>Client: result
Note over FailedS3DeletionService,S3: Scheduled Recovery (async)
loop processPendingDeletions() scheduled
FailedS3DeletionService->>Database: query overdue entries
Database-->>FailedS3DeletionService: list FailedS3DeletionEntity
alt Deletion Succeeds
FailedS3DeletionService->>S3RetryExecutor: execute("delete", callback)
S3RetryExecutor->>S3: delete object
S3-->>S3RetryExecutor: success
S3RetryExecutor-->>FailedS3DeletionService: return
FailedS3DeletionService->>Database: delete queue entry
else Deletion Fails (Below Max Attempts)
FailedS3DeletionService->>S3RetryExecutor: execute("delete", callback)
S3RetryExecutor->>S3: delete object (retry logic)
S3-->>S3RetryExecutor: AppException
S3RetryExecutor-->>FailedS3DeletionService: throw AppException
FailedS3DeletionService->>Database: update entity (increment attemptCount, reschedule nextAttemptAt)
else Max Attempts Exceeded
FailedS3DeletionService->>Database: dead-letter entity
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/application.properties (1)
14-16:⚠️ Potential issue | 🟠 MajorRotate the committed GitHub OAuth client-secret and externalize it.
Line 15 contains what looks like a real OAuth client secret checked into the repo in plaintext. The S3 credentials above already use
${S3_ACCESS_KEY}/${S3_SECRET_KEY}placeholders — apply the same pattern here. If this secret is/was valid, it should be considered compromised: rotate it in the GitHub OAuth app and purge it from git history.Out of scope for this PR's changed lines, but worth addressing urgently since the file is being edited.
🛡️ Suggested change
-spring.security.oauth2.client.registration.github.client-id=Ov23liH2WD75Mu6yIMRq -spring.security.oauth2.client.registration.github.client-secret=63c07ed3f40021ed9a877c06ca09be0a6ef55883 +spring.security.oauth2.client.registration.github.client-id=${GITHUB_CLIENT_ID} +spring.security.oauth2.client.registration.github.client-secret=${GITHUB_CLIENT_SECRET}🤖 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 14 - 16, The GitHub OAuth client secret is committed in plaintext (spring.security.oauth2.client.registration.github.client-secret) and must be rotated and externalized: immediately rotate the secret in the GitHub OAuth app, remove the exposed secret from the repo, and replace the hard-coded value in application.properties with an environment variable placeholder (e.g., use ${GITHUB_OAUTH_CLIENT_SECRET} and similarly externalize spring.security.oauth2.client.registration.github.client-id to ${GITHUB_OAUTH_CLIENT_ID}); also purge the secret from git history (git filter-repo/BFG) and update deployment/config to provide the new env vars securely.
🧹 Nitpick comments (4)
src/main/java/org/example/projektarendehantering/ProjektArendehanteringApplication.java (1)
8-8: Scheduled task will overlap across instances in multi-replica deployments.Enabling scheduling at the application level activates
FailedS3DeletionService.processPendingDeletions()on every JVM instance. Since the batch fetch (findByNextAttemptAtBeforeOrderByCreatedAtAsc) is not protected by a row-level lock (e.g.,@Lock(PESSIMISTIC_WRITE)/SELECT … FOR UPDATE SKIP LOCKED) or a distributed lock (ShedLock, Quartz clustered), multiple replicas will concurrently pick up the same rows, issue redundantDeleteObjectcalls to S3, and emit duplicateDOCUMENT_S3_DELETE_RECOVERED/DOCUMENT_S3_DELETE_RETRY_FAILEDaudit events.If you already run a single replica, this is fine for now; otherwise consider ShedLock or pessimistic locking on the fetch query before scaling out.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/ProjektArendehanteringApplication.java` at line 8, EnableScheduling at application level causes FailedS3DeletionService.processPendingDeletions() to run on every replica and the repository method findByNextAttemptAtBeforeOrderByCreatedAtAsc can be picked up concurrently; fix by adding a locking strategy: either integrate a distributed lock (e.g., add ShedLock around FailedS3DeletionService.processPendingDeletions()) or enforce row-locking on the fetch (annotate the repository method or its query with a pessimistic lock / SELECT ... FOR UPDATE SKIP LOCKED or use `@Lock`(PESSIMISTIC_WRITE) on the JPA query) so only one instance claims each row; alternatively disable `@EnableScheduling` and run the job from a single leader instance. Ensure you modify the specific symbols: `@EnableScheduling`, FailedS3DeletionService.processPendingDeletions(), and the repository method findByNextAttemptAtBeforeOrderByCreatedAtAsc.src/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.java (1)
35-69: Optional: add per-attempt observability; minor double-compute ofisRetryable.
mapToAppExceptioncallsisRetryable(ex)twice (lines 60 and 67), which re-walks the cause chain. Cache it in a local.- Today you only log on the final mapped failure. Attaching a
RetryListener(or callingretryTemplate.registerListener(...)) to log each attempt (onError) with attempt number and backoff would make transient-vs-permanent behavior much easier to diagnose in production, and emit metrics.♻️ Sketch
private AppException mapToAppException(String operationName, RuntimeException ex) { boolean retryable = isRetryable(ex); String code = retryable ? "S3_SERVICE_DEGRADED" : "S3_" + operationName.toUpperCase() + "_FAILED"; String message = retryable ? "Temporary S3 issue while trying to " + operationName : "Failed to " + operationName + " file in S3"; log.error("S3 {} failed. code={}, retryable={}, error={}", operationName, code, retryable, ex.getMessage(), ex); return new AppException(code, message, 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/S3RetryExecutor.java` around lines 35 - 69, The mapToAppException method currently calls isRetryable(ex) twice; compute boolean retryable = isRetryable(ex) once, use it to set code/message and in the log; additionally, attach a RetryListener to the retryTemplate (e.g. via retryTemplate.registerListener(...) or the RetryTemplate builder) to log each onError attempt with attempt number, exception, and backoff/wait time so per-attempt observability is emitted (update S3RetryExecutor to register the listener during initialization and reference retryTemplate and mapToAppException accordingly).src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java (1)
83-114: Optional: callback bodies never execute in these stubs.
when(s3RetryExecutor.execute(eq("upload"), any())).thenReturn(null)short-circuits the lambda, so theObjectMetadataconstruction,file.getInputStream()call, and theIOException → AppException("S3_UPLOAD_STREAM_FAILED")wrapping inDocumentServiceare never exercised. Consider usingthenAnswerto invoke theRetryCallbackwith a stubRetryContext, which would also let you assert the stream-failure branch.♻️ Sketch
when(s3RetryExecutor.execute(eq("upload"), any())).thenAnswer(inv -> { RetryCallback<Object, RuntimeException> cb = inv.getArgument(1); return cb.doWithRetry(mock(RetryContext.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 83 - 114, The stub for s3RetryExecutor.execute currently uses thenReturn(null) which short-circuits the RetryCallback in DocumentService.uploadDocument so the ObjectMetadata/file.getInputStream()/IOException branch never runs; change the test to use thenAnswer and invoke the RetryCallback passed as the second argument (call cb.doWithRetry(mock(RetryContext.class)) or an equivalent mock RetryContext) so the lambda body in s3RetryExecutor.execute is executed during the test and you can assert the stream-failure branch and IOException → AppException("S3_UPLOAD_STREAM_FAILED") behavior.src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (1)
116-126: Minor: retry-with-backoff runs on the request thread during compensation.
s3RetryExecutor.execute("delete", ...)here can block the HTTP request thread for up tomaxAttempts × maxBackoffMs(3 × 2s = ~6s by default) while the user is already waiting for a failed upload. Under load or a broader S3 outage this amplifies thread exhaustion. Consider fire-and-forget compensation (submit to an executor / enqueue immediately and letprocessPendingDeletionshandle it) rather than retrying inline on the request path.🤖 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 116 - 126, The inline retry call using s3RetryExecutor.execute("delete", ...) in DocumentService blocks the request thread; change to fire-and-forget by removing the synchronous s3RetryExecutor.execute call and instead immediately enqueue the deletion via failedS3DeletionService.enqueue(bucket, s3Key, cleanupEx) (or submit a short Runnable to a dedicated executor) and call recordS3Audit(...) to record the compensation; let the existing processPendingDeletions background worker perform retries/backoff asynchronously. Ensure s3RetryExecutor.execute is not invoked on the request path and that enqueue/async submit carries the exception/context so processPendingDeletions can retry.
🤖 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 115-128: The compensating enqueue/audit calls inside
uploadDocument are executed within the caller's `@Transactional` scope so their
inserts are rolled back; update the compensation path so the enqueue and audit
persist even if the outer tx is rolled back by either (a) annotate
FailedS3DeletionService.enqueue (and the audit sink used by recordS3Audit /
AuditService.record) with `@Transactional`(propagation = Propagation.REQUIRES_NEW)
so they execute in their own transaction, or (b) move the cleanup/enqueue/audit
calls out of the uploadDocument transaction (e.g. make uploadDocument
non-transactional and run the DB save in a TransactionTemplate) ensuring
failedS3DeletionService.enqueue and recordS3Audit are invoked in a separate
transaction context.
In
`@src/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.java`:
- Around line 61-98: The batch method processPendingDeletions currently runs the
entire loop in one `@Transactional`, causing any exception to roll back all
per-item work and allowing poison messages to be retried forever; fix by
extracting per-item handling into a new transactional boundary (e.g. add a
method processOne(FailedS3DeletionEntity) annotated with
`@Transactional`(REQUIRES_NEW) or use TransactionTemplate inside the loop) and
call that from a non-transactional processPendingDeletions loop so each
delete/save/audit is committed/rolled back per item; additionally add a
configurable max attempts property (e.g. app.s3.failed-delete.max-attempts) and
in processOne check attemptCount against the cap to move the row to a
dead-letter state/table (or mark it inactive so
findByNextAttemptAtBeforeOrderByCreatedAtAsc no longer returns it) instead of
infinitely retrying, and consider scaling nextAttemptAt by attemptCount (capped)
using retryDelaySeconds to implement backoff.
- Around line 35-59: The enqueue method in FailedS3DeletionService joins the
caller transaction so its save can be lost on caller rollback; change
`@Transactional` on enqueue to `@Transactional`(propagation =
Propagation.REQUIRES_NEW) so the queue entry is persisted in its own
transaction, and also avoid calling trimError(e) twice by computing String
trimmed = trimError(e) once and using trimmed when building the
FailedS3DeletionEntity and when calling auditService.record (referencing
FailedS3DeletionService.enqueue, FailedS3DeletionEntity builder,
auditService.record, and trimError).
In
`@src/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.java`:
- Around line 43-55: The isRetryable method currently treats any
SdkClientException as retryable; change it to only treat client exceptions as
retryable when they explicitly indicate retryability: check for instance of
RetryableException, or if the root is an SdkClientException call its
retryable()/isRetryable() property (and explicitly exclude
NonRetryableException) instead of blanket instanceof SdkClientException; update
isRetryable(Throwable) to use these narrower checks (referencing the isRetryable
method, SdkClientException, RetryableException, NonRetryableException and the
retryable()/isRetryable() property).
In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/FailedS3DeletionEntity.java`:
- Around line 15-41: Add a DB-level unique constraint on (bucket, s3Key) and an
index on nextAttemptAt by updating the FailedS3DeletionEntity class-level `@Table`
annotation: add uniqueConstraints = `@UniqueConstraint`(columnNames =
{"bucket","s3Key"}) and an indexes entry (e.g.
`@Index`(name="idx_failed_s3_next_attempt", columnList="nextAttemptAt") or a
composite columnList="nextAttemptAt,createdAt" for better ordering). This
ensures duplicates cannot be inserted concurrently (protecting enqueue flow that
uses existsByBucketAndS3Key + save) and speeds up
findByNextAttemptAtBeforeOrderByCreatedAtAsc.
In
`@src/test/java/org/example/projektarendehantering/application/service/FailedS3DeletionServiceTest.java`:
- Around line 73-80: The test currently stubs s3RetryExecutor.execute(...) to
return null so the RetryCallback is never invoked; change the stub in
FailedS3DeletionServiceTest to use thenAnswer and call the provided
RetryCallback (invoke its execute() or appropriate method) with the passed args
so the lambda that calls s3Template.deleteObject(bucket, key) runs, then add
verify(s3Template).deleteObject("bucket-a","key-a") to assert the actual delete
call happened and keep verify(failedS3DeletionRepository).delete(pending) as
before.
---
Outside diff comments:
In `@src/main/resources/application.properties`:
- Around line 14-16: The GitHub OAuth client secret is committed in plaintext
(spring.security.oauth2.client.registration.github.client-secret) and must be
rotated and externalized: immediately rotate the secret in the GitHub OAuth app,
remove the exposed secret from the repo, and replace the hard-coded value in
application.properties with an environment variable placeholder (e.g., use
${GITHUB_OAUTH_CLIENT_SECRET} and similarly externalize
spring.security.oauth2.client.registration.github.client-id to
${GITHUB_OAUTH_CLIENT_ID}); also purge the secret from git history (git
filter-repo/BFG) and update deployment/config to provide the new env vars
securely.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 116-126: The inline retry call using
s3RetryExecutor.execute("delete", ...) in DocumentService blocks the request
thread; change to fire-and-forget by removing the synchronous
s3RetryExecutor.execute call and instead immediately enqueue the deletion via
failedS3DeletionService.enqueue(bucket, s3Key, cleanupEx) (or submit a short
Runnable to a dedicated executor) and call recordS3Audit(...) to record the
compensation; let the existing processPendingDeletions background worker perform
retries/backoff asynchronously. Ensure s3RetryExecutor.execute is not invoked on
the request path and that enqueue/async submit carries the exception/context so
processPendingDeletions can retry.
In
`@src/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.java`:
- Around line 35-69: The mapToAppException method currently calls
isRetryable(ex) twice; compute boolean retryable = isRetryable(ex) once, use it
to set code/message and in the log; additionally, attach a RetryListener to the
retryTemplate (e.g. via retryTemplate.registerListener(...) or the RetryTemplate
builder) to log each onError attempt with attempt number, exception, and
backoff/wait time so per-attempt observability is emitted (update
S3RetryExecutor to register the listener during initialization and reference
retryTemplate and mapToAppException accordingly).
In
`@src/main/java/org/example/projektarendehantering/ProjektArendehanteringApplication.java`:
- Line 8: EnableScheduling at application level causes
FailedS3DeletionService.processPendingDeletions() to run on every replica and
the repository method findByNextAttemptAtBeforeOrderByCreatedAtAsc can be picked
up concurrently; fix by adding a locking strategy: either integrate a
distributed lock (e.g., add ShedLock around
FailedS3DeletionService.processPendingDeletions()) or enforce row-locking on the
fetch (annotate the repository method or its query with a pessimistic lock /
SELECT ... FOR UPDATE SKIP LOCKED or use `@Lock`(PESSIMISTIC_WRITE) on the JPA
query) so only one instance claims each row; alternatively disable
`@EnableScheduling` and run the job from a single leader instance. Ensure you
modify the specific symbols: `@EnableScheduling`,
FailedS3DeletionService.processPendingDeletions(), and the repository method
findByNextAttemptAtBeforeOrderByCreatedAtAsc.
In
`@src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java`:
- Around line 83-114: The stub for s3RetryExecutor.execute currently uses
thenReturn(null) which short-circuits the RetryCallback in
DocumentService.uploadDocument so the
ObjectMetadata/file.getInputStream()/IOException branch never runs; change the
test to use thenAnswer and invoke the RetryCallback passed as the second
argument (call cb.doWithRetry(mock(RetryContext.class)) or an equivalent mock
RetryContext) so the lambda body in s3RetryExecutor.execute is executed during
the test and you can assert the stream-failure branch and IOException →
AppException("S3_UPLOAD_STREAM_FAILED") behavior.
🪄 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: a40110aa-8af1-4c87-8063-0d3f6af20f46
📒 Files selected for processing (12)
pom.xmlsrc/main/java/org/example/projektarendehantering/ProjektArendehanteringApplication.javasrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.javasrc/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/FailedS3DeletionEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/FailedS3DeletionRepository.javasrc/main/resources/application.propertiessrc/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.javasrc/test/java/org/example/projektarendehantering/application/service/FailedS3DeletionServiceTest.javasrc/test/java/org/example/projektarendehantering/application/service/S3RetryExecutorTest.javasrc/test/resources/application.properties
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/main/resources/templates/fragments/head.html (2)
7-7:th:inline="css"is unused here.The inlined block contains no Thymeleaf expressions (
[[...]]/[(...)]), soth:inline="css"has no effect and can be dropped. If future theming via server-side variables is intended, leaving it is fine — otherwise prefer a plain<style>tag to avoid confusion.Proposed change
- <style th:inline="css"> + <style>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/head.html` at line 7, Remove the unused Thymeleaf inline attribute from the style tag: locate the <style th:inline="css"> element in the head fragment and delete the th:inline="css" attribute so it becomes a plain <style> tag (unless you intentionally plan to use Thymeleaf CSS expressions later, in which case leave it). Ensure you update the single occurrence of "th:inline="css"" in the head.html fragment.
283-294: Media query order is non-idiomatic.The
max-width: 700pxblock is declared beforemax-width: 980px. For viewports ≤700px both queries match, so the 980px rules win on any overlapping selector due to source order. Today the two blocks don't overlap on the same properties so behavior is correct, but this is fragile — any future addition that targets the same selector in both blocks will silently break the small-screen layout.Conventionally, mobile-first uses
min-widthascending, or desktop-first usesmax-widthdescending. Recommend reordering the 980px block before the 700px block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/head.html` around lines 283 - 294, Reorder the two media-query blocks so the `@media` (max-width: 980px) block appears before the `@media` (max-width: 700px) block; move the rules for .header-row, .cards, .auth-grid into the earlier 980px block and keep .container, .page, .page-title, .auth-label in the 700px block so the narrower (700px) rules take precedence; if you prefer a mobile-first approach instead, convert to ascending min-width queries (using min-width: 700px and min-width: 980px) and adjust selectors accordingly.src/test/resources/application.properties (1)
38-41: Consider disabling the scheduler in tests rather than running it every second.
app.s3.failed-delete.scheduler-delay-ms=1000keeps the@Scheduled processPendingDeletions()firing every second during any test that boots the full context. If@EnableSchedulingis active in the test profile, this background thread can race with integration tests (e.g., asserting onfailed_s3_deletionsrows right after an enqueue, or hitting the mockedS3Templatefrom the scheduler thread). A safer test default is either a very large fixed delay (e.g.3600000) or disabling the scheduler entirely for the test profile and drivingprocessPendingDeletions()directly from tests that need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/resources/application.properties` around lines 38 - 41, The test config's scheduler delay app.s3.failed-delete.scheduler-delay-ms=1000 causes processPendingDeletions() to run every second and race with tests; update the test profile to either disable scheduling (remove/@DisableScheduling or ensure `@EnableScheduling` is not active in tests) or set app.s3.failed-delete.scheduler-delay-ms to a very large value (e.g., 3600000) so the `@Scheduled` processPendingDeletions() won’t fire during tests, and update tests to call the S3 deletion handler directly when they need to exercise processPendingDeletions() (referencing the processPendingDeletions() method and failed_s3_deletions assertions).src/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.java (1)
102-127: Minor: redundanttrimError(ex)calls and a redundant pre-check.Two small cleanups in the failure path:
trimError(ex)is called three times (lines 106, 109, 124) and once more insidemarkDeadLetter(...). Cache it into a local and reuse — matches the style already used inenqueue(...)(line 49).- The early
if (item.getAttemptCount() >= maxAttempts)guard at line 85 can never fire under normal flow, because any row that reachesmaxAttemptsin the catch block below is immediatelydelete(item)‑ed (viamarkDeadLetter), so it won't be re-selected byfindByNextAttemptAtBeforeOrderByCreatedAtAsc. It's a reasonable defense against externally-seeded rows, but worth noting the comment isn't self‑evident from the code.♻️ Suggested cache of trimmed error
} catch (Exception ex) { + String trimmedError = trimError(ex); int nextAttemptCount = item.getAttemptCount() + 1; item.setAttemptCount(nextAttemptCount); item.setUpdatedAt(Instant.now()); - item.setLastError(trimError(ex)); + item.setLastError(trimmedError); if (nextAttemptCount >= maxAttempts) { - markDeadLetter(item, trimError(ex)); + markDeadLetter(item, trimmedError); log.warn("Giving up failed S3 deletion after max attempts. bucket={}, key={}, attempts={}", item.getBucket(), item.getS3Key(), item.getAttemptCount(), ex); return; } long delaySeconds = computeRetryDelaySeconds(nextAttemptCount); item.setNextAttemptAt(Instant.now().plus(delaySeconds, ChronoUnit.SECONDS)); failedS3DeletionRepository.save(item); log.warn("Failed retrying S3 deletion. bucket={}, key={}, attempts={}", item.getBucket(), item.getS3Key(), item.getAttemptCount(), ex); auditService.record(AuditEventEntity.builder() .eventName("DOCUMENT_S3_DELETE_RETRY_FAILED") .description("Retry failed for queued S3 deletion") .queryString("bucket=" + item.getBucket() + "&s3Key=" + item.getS3Key() - + "&attempts=" + item.getAttemptCount() + "&error=" + trimError(ex)) + + "&attempts=" + item.getAttemptCount() + "&error=" + trimmedError) .occurredAt(Instant.now()) .build()); }🤖 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/FailedS3DeletionService.java` around lines 102 - 127, Cache the trimmed error once in the catch block (e.g., String trimmed = trimError(ex)) and replace the repeated trimError(ex) calls used in item.setLastError(...), markDeadLetter(...), log.warn(...), and auditService.record(...) to avoid redundant work (refer to trimError, markDeadLetter, auditService.record, and the catch block in FailedS3DeletionService). Also either remove or document the earlier defensive pre-check that tests item.getAttemptCount() >= maxAttempts before processing (the query findByNextAttemptAtBeforeOrderByCreatedAtAsc will normally exclude dead-lettered rows) — if you keep it, add a clarifying comment explaining it defends against externally-seeded rows; if you remove it, ensure tests still cover that scenario.
🤖 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/FailedS3DeletionService.java`:
- Around line 70-77: In FailedS3DeletionService.processPendingDeletions, wrap
the per-item invocation of selfProvider.getObject().processOne(item.getId()) in
a try/catch that catches Exception (or Throwable if you prefer) so a
RuntimeException from processOne (which is REQUIRES_NEW) doesn't abort the for
loop; log the error including the item id and any exception details and continue
to the next item so one poison item cannot prevent remaining items from being
processed.
In
`@src/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.java`:
- Around line 37-43: The execute(...) method currently catches all
RuntimeException and always re-wraps them via mapToAppException(...), which
loses any AppException.code thrown by the callback; modify execute(String
operationName, RetryCallback<T, RuntimeException> callback) to detect
already-typed AppException and rethrow it directly (i.e. if ex instanceof
AppException throw (AppException) ex), and additionally if the caught
RuntimeException’s cause is an AppException, unwrap and rethrow that cause to
preserve the original code; otherwise continue to map other exceptions with
mapToAppException(operationName, ex).
- Around line 45-66: In S3RetryExecutor.isRetryable extract the shared "HTTP
status or AWS error code" logic into a helper and call it from both the
S3Exception and AwsServiceException branches: create a private method (e.g.
isRetryableStatusOrAwsError(int statusCode, String awsErrorCode) or
isRetryableAwsServiceException(AwsServiceException ex)) that returns
RETRYABLE_HTTP_STATUS.contains(statusCode) ||
isRetryableAwsErrorCode(awsErrorCode), then replace the existing S3Exception
check and the AwsServiceException branch to invoke that helper (for S3Exception
pass s3Exception.statusCode() and s3Exception.awsErrorDetails() != null ?
s3Exception.awsErrorDetails().errorCode() : null; for AwsServiceException pass
awsServiceException.statusCode() and awsServiceException.awsErrorDetails() !=
null ? awsServiceException.awsErrorDetails().errorCode() : null).
In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/FailedS3DeletionEntity.java`:
- Around line 15-19: The table constraint/index column names in
FailedS3DeletionEntity use camelCase literals that won't match the physical
snake_case columns; update the `@Table` annotation so the UniqueConstraint uses
columnNames = {"bucket","s3_key"} and the `@Index` uses columnList =
"next_attempt_at,created_at" to match SpringPhysicalNamingStrategy-generated
columns, or alternatively add explicit `@Column`(name="s3Key") /
`@Column`(name="nextAttemptAt") / `@Column`(name="createdAt") on the corresponding
fields (s3Key, nextAttemptAt, createdAt) to preserve camelCase names.
In `@src/main/resources/templates/fragments/head.html`:
- Around line 7-295: The PR inlines the stylesheet in
templates/fragments/head.html but leaves the static file and config references,
creating a duplicate source of truth; either (A) remove
src/main/resources/static/app.css and delete the /app.css whitelist entries from
SecurityConfig.securityFilterChain and AuditWebMvcConfig.addInterceptors so the
app uses only the inlined CSS in head.html, or (B) revert head.html to reference
the external /app.css (restore the <link> to /app.css) so the single canonical
stylesheet remains in static and keep the existing
SecurityConfig.securityFilterChain and AuditWebMvcConfig.addInterceptors
entries; choose one approach and apply it consistently.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.java`:
- Around line 102-127: Cache the trimmed error once in the catch block (e.g.,
String trimmed = trimError(ex)) and replace the repeated trimError(ex) calls
used in item.setLastError(...), markDeadLetter(...), log.warn(...), and
auditService.record(...) to avoid redundant work (refer to trimError,
markDeadLetter, auditService.record, and the catch block in
FailedS3DeletionService). Also either remove or document the earlier defensive
pre-check that tests item.getAttemptCount() >= maxAttempts before processing
(the query findByNextAttemptAtBeforeOrderByCreatedAtAsc will normally exclude
dead-lettered rows) — if you keep it, add a clarifying comment explaining it
defends against externally-seeded rows; if you remove it, ensure tests still
cover that scenario.
In `@src/main/resources/templates/fragments/head.html`:
- Line 7: Remove the unused Thymeleaf inline attribute from the style tag:
locate the <style th:inline="css"> element in the head fragment and delete the
th:inline="css" attribute so it becomes a plain <style> tag (unless you
intentionally plan to use Thymeleaf CSS expressions later, in which case leave
it). Ensure you update the single occurrence of "th:inline="css"" in the
head.html fragment.
- Around line 283-294: Reorder the two media-query blocks so the `@media`
(max-width: 980px) block appears before the `@media` (max-width: 700px) block;
move the rules for .header-row, .cards, .auth-grid into the earlier 980px block
and keep .container, .page, .page-title, .auth-label in the 700px block so the
narrower (700px) rules take precedence; if you prefer a mobile-first approach
instead, convert to ascending min-width queries (using min-width: 700px and
min-width: 980px) and adjust selectors accordingly.
In `@src/test/resources/application.properties`:
- Around line 38-41: The test config's scheduler delay
app.s3.failed-delete.scheduler-delay-ms=1000 causes processPendingDeletions() to
run every second and race with tests; update the test profile to either disable
scheduling (remove/@DisableScheduling or ensure `@EnableScheduling` is not active
in tests) or set app.s3.failed-delete.scheduler-delay-ms to a very large value
(e.g., 3600000) so the `@Scheduled` processPendingDeletions() won’t fire during
tests, and update tests to call the S3 deletion handler directly when they need
to exercise processPendingDeletions() (referencing the processPendingDeletions()
method and failed_s3_deletions assertions).
🪄 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: f5d2f750-0464-4258-9629-8c17b2044707
📒 Files selected for processing (8)
src/main/java/org/example/projektarendehantering/application/service/AuditService.javasrc/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.javasrc/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/FailedS3DeletionEntity.javasrc/main/resources/application.propertiessrc/main/resources/templates/fragments/head.htmlsrc/test/java/org/example/projektarendehantering/application/service/FailedS3DeletionServiceTest.javasrc/test/resources/application.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/application.properties
Run upload callbacks through the mocked retry executor and verify upload via executor invocation so patient upload tests match the current DocumentService behavior. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (2)
38-127:⚠️ Potential issue | 🟠 MajorMove S3 compensation outside the proxy-managed transaction boundary to catch commit-time failures.
Line 62 uploads the S3 object before the
@Transactionalmethod commits. Thecatchblock at line 110 only catches exceptions thrown before thereturnstatement at line 108. If the database commit fails during the proxy's commit phase (after the method returns), the exception occurs outside this try-catch scope, leaving the S3 object orphaned. Consider removing the method-level@Transactionalannotation and wrapping the database operations inTransactionTemplateor a separate transactional method, so commit failures trigger the compensation logic before control returns to the caller.🤖 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 38 - 127, The uploadDocument method currently uploads to S3 (s3Template.upload via s3RetryExecutor) inside a method annotated with `@Transactional`, so commit-time failures can leave orphaned S3 objects; remove the method-level `@Transactional` and instead perform the DB work (documentRepository.save, caseRepository.save, and auditService.record logic in uploadDocument) inside an explicit transactional boundary using TransactionTemplate or by calling a separate `@Transactional` helper (e.g., saveDocumentTransactionally). Keep the S3 upload before invoking that transactional block, and wrap the transactional call in a try/catch that on any exception runs the S3 compensation (s3RetryExecutor.execute -> s3Template.deleteObject) and enqueues cleanup (failedS3DeletionService.enqueue) / records S3 audit (recordS3Audit) before rethrowing the exception.
155-175:⚠️ Potential issue | 🟠 MajorDefer S3 deletion until after the database delete commits.
The S3 deletion at lines 168–171 executes synchronously within the transaction, before the database delete at line 166 commits. If an exception occurs after the S3 deletion but before the transaction commits, the database delete rolls back while the S3 object is already gone.
While
failedS3DeletionService.enqueue()provides recovery via a retry queue, it's a reactive fallback, not a preventive measure. The codebase demonstrates the correct pattern inAuditServiceusingTransactionSynchronizationManager.registerSynchronization()withafterCommit(). Apply the same pattern here to defer S3 cleanup until after the database transaction successfully commits.🤖 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 155 - 175, The deleteDocument method currently performs S3 deletion inside the DB transaction; change it to delete the DB entity first (documentRepository.delete(entity)), capture the s3Key, and then register a TransactionSynchronizationManager.registerSynchronization(...) with an afterCommit() callback that runs s3RetryExecutor.execute("delete", ...) calling s3Template.deleteObject(bucket, s3Key) and on exception calls failedS3DeletionService.enqueue(bucket, s3Key, e); ensure no S3 calls run before commit and keep existing logging behavior in the afterCommit handler.
🧹 Nitpick comments (1)
src/test/java/org/example/projektarendehantering/infrastructure/security/CustomOAuth2UserServiceTest.java (1)
51-51: Consider asserting normalization more explicitly.The fixture change implicitly covers normalization (the
findByGithubUsername("testuser")stub would no longer match if normalization were removed), but an explicitverify(employeeRepository).findByGithubUsername("testuser")would make the intent obvious and catch regressions where normalization is bypassed but a broader matcher still passes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/infrastructure/security/CustomOAuth2UserServiceTest.java` at line 51, Add an explicit verification that the GitHub username is normalized by asserting employeeRepository.findByGithubUsername("testuser") is called in CustomOAuth2UserServiceTest (after the OAuth2 user processing where the fixture Map.of("login", " TestUser ", "id", 123) is used); this makes normalization intent explicit and ensures regressions are caught even if broader matchers still pass.
🤖 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/EmployeeService.java`:
- Around line 58-61: The immutability check in EmployeeService uses
entity.getGithubUsername().equals(normalizedGithubUsername), which can NPE if
the stored username is null; change it to a null-safe equality check (for
example use Objects.equals(entity.getGithubUsername(), normalizedGithubUsername)
or compare normalizedGithubUsername.equals(entity.getGithubUsername()) after
guarding null) so the condition correctly detects inequality without throwing
and still throws BadRequestException("EMPLOYEE_USERNAME_IMMUTABLE", ...) when
usernames differ.
In
`@src/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.java`:
- Around line 42-67: The enqueue method currently persists the
FailedS3DeletionEntity and then calls auditService.record(...) inside the same
REQUIRES_NEW transaction so any audit failure can roll back the queued state;
fix this by persisting the queue state first (keep
failedS3DeletionRepository.save(...) as-is) and then invoking audit in an
isolated best-effort transaction or guarded call — e.g., add a new method
recordAuditBestEffort(...) on the audit service (or a private helper) that is
annotated with its own transactional boundary (REQUIRES_NEW or NOT_SUPPORTED) or
wrap the auditService.record(...) call in a try/catch that logs but does not
rethrow, then replace direct auditService.record(...) calls in enqueue (and the
other affected places like processOne) with this isolated best-effort call so
audit failures cannot roll back the queue mutation.
In
`@src/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.java`:
- Around line 60-84: The isRetryable method currently calls rootCause(throwable)
and only inspects the deepest cause, which skips wrapper metadata (e.g.
SdkClientException.retryable()); change it to walk the throwable chain top-down
(start at the original throwable and iterate through getCause()) and at each
level check the same cases used now—S3Exception, AwsServiceException,
NonRetryableException, RetryableException, and SdkClientException (calling
retryable() and falling back to isLikelyTransientClientError on the
message)—returning as soon as any level indicates retryability or
non-retryability; keep the same helper isRetryableStatusOrAwsError and preserve
existing return semantics when no level matches.
---
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 38-127: The uploadDocument method currently uploads to S3
(s3Template.upload via s3RetryExecutor) inside a method annotated with
`@Transactional`, so commit-time failures can leave orphaned S3 objects; remove
the method-level `@Transactional` and instead perform the DB work
(documentRepository.save, caseRepository.save, and auditService.record logic in
uploadDocument) inside an explicit transactional boundary using
TransactionTemplate or by calling a separate `@Transactional` helper (e.g.,
saveDocumentTransactionally). Keep the S3 upload before invoking that
transactional block, and wrap the transactional call in a try/catch that on any
exception runs the S3 compensation (s3RetryExecutor.execute ->
s3Template.deleteObject) and enqueues cleanup (failedS3DeletionService.enqueue)
/ records S3 audit (recordS3Audit) before rethrowing the exception.
- Around line 155-175: The deleteDocument method currently performs S3 deletion
inside the DB transaction; change it to delete the DB entity first
(documentRepository.delete(entity)), capture the s3Key, and then register a
TransactionSynchronizationManager.registerSynchronization(...) with an
afterCommit() callback that runs s3RetryExecutor.execute("delete", ...) calling
s3Template.deleteObject(bucket, s3Key) and on exception calls
failedS3DeletionService.enqueue(bucket, s3Key, e); ensure no S3 calls run before
commit and keep existing logging behavior in the afterCommit handler.
---
Nitpick comments:
In
`@src/test/java/org/example/projektarendehantering/infrastructure/security/CustomOAuth2UserServiceTest.java`:
- Line 51: Add an explicit verification that the GitHub username is normalized
by asserting employeeRepository.findByGithubUsername("testuser") is called in
CustomOAuth2UserServiceTest (after the OAuth2 user processing where the fixture
Map.of("login", " TestUser ", "id", 123) is used); this makes normalization
intent explicit and ensures regressions are caught even if broader matchers
still pass.
🪄 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: 2ace1cbc-55de-49af-bff1-30e507c0d319
📒 Files selected for processing (18)
AUDIT.mdsrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeMapper.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeService.javasrc/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.javasrc/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.javasrc/main/java/org/example/projektarendehantering/common/GithubUsernameNormalizer.javasrc/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.javasrc/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/FailedS3DeletionEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/security/CustomOAuth2UserService.javasrc/main/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapter.javasrc/main/resources/static/app.csssrc/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.javasrc/test/java/org/example/projektarendehantering/application/service/EmployeeMapperTest.javasrc/test/java/org/example/projektarendehantering/application/service/EmployeeServiceTest.javasrc/test/java/org/example/projektarendehantering/infrastructure/security/CustomOAuth2UserServiceTest.javasrc/test/java/org/example/projektarendehantering/infrastructure/security/SecurityActorAdapterTest.java
💤 Files with no reviewable changes (2)
- src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java
- src/main/resources/static/app.css
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/projektarendehantering/common/GithubUsernameNormalizer.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/projektarendehantering/infrastructure/persistence/FailedS3DeletionEntity.java
- src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
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/projektarendehantering/application/service/DocumentServiceTest.java (1)
143-162:⚠️ Potential issue | 🔴 Critical
uploadDocument_shouldAllowDocxtest will not compile — missingObjectMetadataimport and inconsistent pattern.The test on line 161 references
any(ObjectMetadata.class)butObjectMetadatais not imported. Additionally, this is the only upload test that still verifiess3Template.upload(...)directly; all others (lines 99, 111, 119, 130, 246) have been migrated toverify(s3RetryExecutor).execute(...). Fix the missing import and align the assertion with the refactored pattern:♻️ Suggested change
- verify(s3Template).upload(eq("test-bucket"), anyString(), any(InputStream.class), any(ObjectMetadata.class)); + verify(s3RetryExecutor).execute(eq("upload"), any());🤖 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 143 - 162, The test uploadDocument_shouldAllowDocx fails to compile because ObjectMetadata is not imported and it still verifies s3Template.upload(...) instead of the refactored s3RetryExecutor.execute(...). Add the missing import for com.amazonaws.services.s3.model.ObjectMetadata (or the correct ObjectMetadata class used elsewhere) and change the verification from verify(s3Template).upload(...) to verify(s3RetryExecutor).execute(...) using the same argument matchers (bucket name, any key/string, any(InputStream.class), any(ObjectMetadata.class)) so it matches the other migrated tests.
🧹 Nitpick comments (1)
src/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.java (1)
140-149: Dead-lettered deletions are dropped on the floor — consider preserving them.
markDeadLetterdeletes the row outright, so the only lasting record of a permanently-failed S3 cleanup is a single audit event. That makes on-call investigation ("which keys are orphaned?") and manual replay ("retry everything that went DLQ yesterday") much harder than they need to be, and silently defeats much of the point of the retry queue for the tail of pathological cases (bad bucket policy, permanent 403, lifecycle-deleted keys, etc.).Two lightweight options:
- Add a
statuscolumn (PENDING/DEAD_LETTERED) toFailedS3DeletionEntity, flip it here instead of deleting, and filterfindByNextAttemptAtBeforeOrderByCreatedAtAsctoPENDINGonly.- Mirror the row into a
failed_s3_deletions_dlqtable before deleting.Either keeps the forensic trail queryable without re-introducing the "retried forever" failure mode.
🤖 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/FailedS3DeletionService.java` around lines 140 - 149, markDeadLetter currently deletes the FailedS3DeletionEntity row, losing forensic data; instead preserve the record by either (A) adding a status field to FailedS3DeletionEntity (e.g., PENDING / DEAD_LETTERED), update markDeadLetter to set status=DEAD_LETTERED and save via failedS3DeletionRepository.save(...), and change the consumer query (findByNextAttemptAtBeforeOrderByCreatedAtAsc) to filter only PENDING records; or (B) insert the entity into a separate failed_s3_deletions_dlq table before deleting (create a DLQ entity/repository and persist the mirrored row in markDeadLetter) so the original can be removed but forensic data is retained; update markDeadLetter and any repository/query logic accordingly.
🤖 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 100-107: The catch(IOException) inside the lambda passed to
s3RetryExecutor.execute is unreachable because s3Template.upload(...) and
ByteArrayInputStream do not throw checked IOExceptions; remove the try/catch
around s3Template.upload(bucket, s3Key, new ByteArrayInputStream(fileBytes),
metadata) and let s3RetryExecutor handle failures, or if you need to translate
runtime transport errors catch RuntimeException instead and rethrow an
AppException; also consider moving any error handling for file.getBytes() (the
only checked IO source) to the code before invoking the s3RetryExecutor.lambda
so S3_UPLOAD_STREAM_FAILED can still be raised there if needed.
---
Outside diff comments:
In
`@src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java`:
- Around line 143-162: The test uploadDocument_shouldAllowDocx fails to compile
because ObjectMetadata is not imported and it still verifies
s3Template.upload(...) instead of the refactored s3RetryExecutor.execute(...).
Add the missing import for com.amazonaws.services.s3.model.ObjectMetadata (or
the correct ObjectMetadata class used elsewhere) and change the verification
from verify(s3Template).upload(...) to verify(s3RetryExecutor).execute(...)
using the same argument matchers (bucket name, any key/string,
any(InputStream.class), any(ObjectMetadata.class)) so it matches the other
migrated tests.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.java`:
- Around line 140-149: markDeadLetter currently deletes the
FailedS3DeletionEntity row, losing forensic data; instead preserve the record by
either (A) adding a status field to FailedS3DeletionEntity (e.g., PENDING /
DEAD_LETTERED), update markDeadLetter to set status=DEAD_LETTERED and save via
failedS3DeletionRepository.save(...), and change the consumer query
(findByNextAttemptAtBeforeOrderByCreatedAtAsc) to filter only PENDING records;
or (B) insert the entity into a separate failed_s3_deletions_dlq table before
deleting (create a DLQ entity/repository and persist the mirrored row in
markDeadLetter) so the original can be removed but forensic data is retained;
update markDeadLetter and any repository/query logic accordingly.
🪄 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: ff6998b0-3d53-468e-857f-bc3d9035bc0c
📒 Files selected for processing (8)
pom.xmlsrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeService.javasrc/main/java/org/example/projektarendehantering/application/service/FailedS3DeletionService.javasrc/main/java/org/example/projektarendehantering/application/service/S3RetryExecutor.javasrc/main/resources/application.propertiessrc/main/resources/templates/fragments/head.htmlsrc/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- pom.xml
- src/main/resources/templates/fragments/head.html
- src/main/resources/application.properties
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #79
Closes #81
Summary by CodeRabbit
Release Notes
New Features
Chores