Skip to content

42 feat attachmentservicejava - #112

Merged
johanbriger merged 9 commits into
mainfrom
42-feat-attachmentservicejava
Apr 3, 2026
Merged

42 feat attachmentservicejava#112
johanbriger merged 9 commits into
mainfrom
42-feat-attachmentservicejava

Conversation

@johanbriger

@johanbriger johanbriger commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features
    • Add attachments for medical records: upload with metadata capture (filename, type, size), permission checks, transactional handling, presigned download links, and deletion that removes stored files and records.
  • Bug Fixes
    • Uploads now reject empty files and enforce stricter file-size validation.

@johanbriger johanbriger linked an issue Apr 3, 2026 that may be closed by this pull request
@coderabbitai

coderabbitai Bot commented Apr 3, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds a new Spring AttachmentService for S3-backed attachments (upload, fetch with presigned URL, delete) with MedicalRecord lookup, policy checks, transactional semantics, S3 upload/delete via FileStorageService, and cleanup on persistence failures. Also tightens AttachmentPolicy.canUpload to reject empty files.

Changes

Cohort / File(s) Summary
Attachment Service Implementation
src/main/java/org/example/vet1177/services/AttachmentService.java
New service with constructor DI and methods: uploadAttachment(User, MultipartFile, AttachmentRequest) (validates record; enforces AttachmentPolicy.canUpload; uploads to S3; persists Attachment; attempts S3 cleanup on DB failure; returns AttachmentResponse with presigned URL), getAttachment(User, UUID) (fetches, enforces canDownload, returns presigned URL), and deleteAttachment(User, UUID) (enforces canDelete, deletes DB row and deletes or schedules S3 deletion depending on transaction synchronization).
Policy change
src/main/java/org/example/vet1177/policy/AttachmentPolicy.java
canUpload(...) now explicitly rejects zero-length uploads by throwing IllegalArgumentException when fileSize <= 0 (in addition to existing type/size validation and delegation to medicalRecordPolicy.canUpdate).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant AttachmentService
    participant MedicalRecordRepository
    participant AttachmentPolicy
    participant FileStorageService
    participant AttachmentRepository
    participant S3

    User->>AttachmentService: uploadAttachment(file, request)
    AttachmentService->>MedicalRecordRepository: findById(request.recordId)
    MedicalRecordRepository-->>AttachmentService: MedicalRecord / not found -> throw
    AttachmentService->>AttachmentPolicy: canUpload(user, record, contentType, size)
    AttachmentPolicy-->>AttachmentService: allow/deny
    AttachmentService->>FileStorageService: uploadFile(bucket, s3Key, stream)
    FileStorageService->>S3: putObject(stream)
    S3-->>FileStorageService: success
    AttachmentService->>AttachmentRepository: saveAndFlush(Attachment)
    AttachmentRepository-->>AttachmentService: saved / DB error -> propagate
    alt DB error after S3 upload
        AttachmentService->>S3: deleteObject(bucket, s3Key)
    end
    AttachmentService->>S3: generatePresignedUrl(bucket, s3Key)
    S3-->>AttachmentService: presigned URL
    AttachmentService-->>User: AttachmentResponse(presigned URL)
Loading
sequenceDiagram
    participant User
    participant AttachmentService
    participant AttachmentRepository
    participant AttachmentPolicy
    participant FileStorageService
    participant S3
    participant TransactionSync

    User->>AttachmentService: deleteAttachment(attachmentId)
    AttachmentService->>AttachmentRepository: findById(attachmentId)
    AttachmentRepository-->>AttachmentService: Attachment / not found -> throw
    AttachmentService->>AttachmentPolicy: canDelete(user, attachment)
    AttachmentPolicy-->>AttachmentService: allow/deny
    alt Transaction synchronization active
        AttachmentService->>TransactionSync: registerAfterCommit(delete S3 key)
        AttachmentService->>AttachmentRepository: delete(attachment)
        TransactionSync->>S3: deleteObject(s3Key) (after commit)
    else No transaction sync
        AttachmentService->>FileStorageService: deleteFile(bucket, s3Key)
        FileStorageService->>S3: deleteObject(s3Key)
        AttachmentService->>AttachmentRepository: delete(attachment)
    end
    AttachmentService-->>User: void (deleted)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • annikaholmqvist94
  • lindaeskilsson
  • TatjanaTrajkovic

Poem

🐰 I hopped with bytes and tiny keys,
I tucked files into S3 trees,
Policies checked each curious paw,
DB saved notes—no hiccup, no flaw,
Now attachments rest, snug as a breeze. 🥕📎

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title '42 feat attachmentservicejava' is vague and non-descriptive. It includes a ticket number prefix and generic terms ('feat', filename) without conveying the actual purpose or change. Revise the title to clearly describe the primary change, e.g., 'Add AttachmentService for S3-backed file upload and download management' or 'Implement attachment management with S3 storage and access control'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 42-feat-attachmentservicejava

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/main/java/org/example/vet1177/services/AttachmentService.java (1)

100-104: Consider reordering: delete DB record before S3 object.

If the S3 delete succeeds (line 101) but the subsequent DB delete (line 104) fails, the transaction rolls back leaving a DB record pointing to a non-existent S3 object.

Deleting the DB record first is safer—once the DB record is gone, the S3 object becomes unreachable anyway, and a failed S3 cleanup can be retried or handled by a background job.

♻️ Proposed reordering
         attachmentPolicy.canDelete(currentUser, attachment);
 
-        // Radera objekt i S3
-        fileStorageService.delete(attachment.getS3Key());
-
-        // Radera rad i DB
+        String s3Key = attachment.getS3Key();
+        
+        // Radera rad i DB först
         attachmentRepository.delete(attachment);
+        attachmentRepository.flush(); // Force DB delete within transaction
+        
+        // Radera objekt i S3 (best-effort after DB commit)
+        try {
+            fileStorageService.delete(s3Key);
+        } catch (Exception e) {
+            log.warn("Failed to delete S3 object {} after DB deletion. Manual cleanup may be required.", s3Key, e);
+        }
 
         log.info("Attachment {} deleted from storage and database", attachmentId);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/vet1177/services/AttachmentService.java` around
lines 100 - 104, In AttachmentService, swap the order of deletion so the DB row
is removed before the S3 object: call attachmentRepository.delete(attachment)
first, then call fileStorageService.delete(attachment.getS3Key()); ensure this
change is applied in the method that currently invokes
fileStorageService.delete(...) and attachmentRepository.delete(...), and
consider wrapping the S3 delete in its own try/catch or enqueueing a
retry/background job if the remote delete fails after the DB deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/org/example/vet1177/services/AttachmentService.java`:
- Around line 55-58: Sanitize the original filename returned by
file.getOriginalFilename() before building the S3 key in AttachmentService:
create a helper (e.g., sanitizeFilename(String name)) and use it where s3Key is
composed (the String s3Key = String.format("records/%s/%s_%s", record.getId(),
UUID.randomUUID(), file.getOriginalFilename()) and the other occurrence). The
sanitizer should treat null as empty, trim and strip any path separators or
traversal segments (../, /, \), remove control characters, and reduce to a safe
whitelist (e.g., alphanumerics, dot, underscore, dash); if the result is empty,
fall back to a stable fallback like a timestamp or UUID. Replace direct uses of
file.getOriginalFilename() with the sanitized value when constructing s3Key and
any other storage keys.
- Around line 61-74: The S3 upload (fileStorageService.upload with
s3Key/bucketName) happens before persisting Attachment via
attachmentRepository.save, risking orphaned S3 objects if the DB save fails;
either reorder to create and save an Attachment with a "PENDING" status (e.g.,
set a status field on Attachment), then call fileStorageService.upload and
finally update the Attachment to "COMPLETE", or wrap the current upload+save
sequence in a try/catch and in the catch call fileStorageService.delete(s3Key,
bucketName) (or equivalent) to remove the uploaded object before rethrowing the
exception; adjust Attachment creation/fields (Attachment.setStatus or similar)
and ensure error handling around fileStorageService.upload and
attachmentRepository.save covers both paths.

---

Nitpick comments:
In `@src/main/java/org/example/vet1177/services/AttachmentService.java`:
- Around line 100-104: In AttachmentService, swap the order of deletion so the
DB row is removed before the S3 object: call
attachmentRepository.delete(attachment) first, then call
fileStorageService.delete(attachment.getS3Key()); ensure this change is applied
in the method that currently invokes fileStorageService.delete(...) and
attachmentRepository.delete(...), and consider wrapping the S3 delete in its own
try/catch or enqueueing a retry/background job if the remote delete fails after
the DB deletion.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: abb382d2-9cd8-4ab4-aaa4-175e2bd7c065

📥 Commits

Reviewing files that changed from the base of the PR and between b945254 and 1426599.

📒 Files selected for processing (1)
  • src/main/java/org/example/vet1177/services/AttachmentService.java

Comment thread src/main/java/org/example/vet1177/services/AttachmentService.java
Comment thread src/main/java/org/example/vet1177/services/AttachmentService.java Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/main/java/org/example/vet1177/services/AttachmentService.java (1)

54-61: ⚠️ Potential issue | 🟠 Major

Still building the S3 key from the raw filename.

Lines 54-55 compute sanitizedName, but Line 61 still formats the S3 key with file.getOriginalFilename(). The previous key-sanitization bug is still present, and sanitizedName is currently dead code.

🛡️ Minimal fix
         String s3Key = String.format("records/%s/%s_%s",
                 record.getId(),
                 UUID.randomUUID(),
-                file.getOriginalFilename());
+                sanitizedName);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/vet1177/services/AttachmentService.java` around
lines 54 - 61, The S3 key is still built from file.getOriginalFilename() while
sanitizeFilename(...) produces sanitizedName; update the s3Key construction (the
String s3Key = ... assignment) to use sanitizedName instead of
file.getOriginalFilename(), ensuring you keep the same prefix format
(records/%s/%s_%s) with record.getId() and UUID.randomUUID() and drop any unused
dead code or variables if no longer needed; verify sanitizeFilename(...) is
called before s3Key and referenced directly.
🤖 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/vet1177/services/AttachmentService.java`:
- Around line 84-88: The attachment save uses
attachmentRepository.save(attachment) inside the `@Transactional` method
(AttachmentService) which defers DB write until commit and can leave uploaded S3
objects orphaned if commit fails; replace the save call with
attachmentRepository.saveAndFlush(attachment) so the DB write happens inside the
try/catch block before mapToResponse(...) and before the S3 cleanup logic so
exceptions will be caught and the S3 delete path will run if needed.
- Around line 138-142: The code currently calls
fileStorageService.delete(attachment.getS3Key()) before removing the DB row via
attachmentRepository.delete inside the transaction, which risks S3 deletion on
rollback; change the flow so the S3 deletion happens only after the DB
transaction commits — for example, in AttachmentService (the method doing the
delete) remove the direct fileStorageService.delete call and instead register a
TransactionSynchronization
(TransactionSynchronizationManager.registerSynchronization) or publish a
transactional event/ use an outbox that runs
fileStorageService.delete(attachment.getS3Key()) in afterCommit; keep
attachmentRepository.delete(attachment) inside the transaction and perform the
S3 deletion only in the afterCommit handler to ensure durability.

---

Duplicate comments:
In `@src/main/java/org/example/vet1177/services/AttachmentService.java`:
- Around line 54-61: The S3 key is still built from file.getOriginalFilename()
while sanitizeFilename(...) produces sanitizedName; update the s3Key
construction (the String s3Key = ... assignment) to use sanitizedName instead of
file.getOriginalFilename(), ensuring you keep the same prefix format
(records/%s/%s_%s) with record.getId() and UUID.randomUUID() and drop any unused
dead code or variables if no longer needed; verify sanitizeFilename(...) is
called before s3Key and referenced directly.
🪄 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: 8e26ecb4-d6d4-4fec-bf48-ec0bdbc8d481

📥 Commits

Reviewing files that changed from the base of the PR and between 1426599 and 0c2cde6.

📒 Files selected for processing (1)
  • src/main/java/org/example/vet1177/services/AttachmentService.java

Comment thread src/main/java/org/example/vet1177/services/AttachmentService.java Outdated
Comment thread src/main/java/org/example/vet1177/services/AttachmentService.java Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/main/java/org/example/vet1177/services/AttachmentService.java (1)

56-63: ⚠️ Potential issue | 🟠 Major

Use sanitizedName here instead of the raw multipart filename.

sanitizeFilename() is computed on Lines 56-57 but never used. Line 63 still builds the S3 key from file.getOriginalFilename(), and Line 78 can still persist null/blank into the non-null file_name column. Reuse the sanitized fallback for storage, and only keep the original name if it is actually present.

Suggested fix
         String originalName = file.getOriginalFilename();
         String sanitizedName = sanitizeFilename(originalName);

         // Skapa unik S3-nyckel
         String s3Key = String.format("records/%s/%s_%s",
                 record.getId(),
                 UUID.randomUUID(),
-                file.getOriginalFilename());
+                sanitizedName);
...
-        attachment.setFileName(file.getOriginalFilename());
+        attachment.setFileName(originalName == null || originalName.isBlank()
+                ? sanitizedName
+                : originalName.trim());

Also applies to: 78-78

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/vet1177/services/AttachmentService.java` around
lines 56 - 63, The code computes originalName and sanitizedName via
sanitizeFilename(originalName) but then still uses file.getOriginalFilename()
when building the S3 key and when persisting the filename; update the logic in
AttachmentService to use sanitizedName for the S3 key (replace
file.getOriginalFilename() in the s3Key construction) and when saving the
file_name persist sanitizedName (fall back to originalName only if sanitizedName
is null/blank), ensuring sanitizeFilename, originalName, sanitizedName and s3Key
are the referenced symbols you adjust.
🤖 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/vet1177/services/AttachmentService.java`:
- Around line 53-54: The attachment upload currently lets zero-byte files pass
attachmentPolicy.canUpload (which only rejects size < 0) but then fails in
FileStorageService.upload (which rejects <= 0); update the fast-fail in
AttachmentService by adding a check for file.getSize() <= 0 (or tighten
AttachmentPolicy.validateFileSize to reject <= 0) so that
AttachmentService.attachmentPolicy.canUpload(...) rejects empty files with a
business-rule error before delegating to FileStorageService.upload (also ensure
the same check/behavior is applied to the code path around the
FileStorageService.upload error handling at lines 66-70).
- Around line 141-152: The current afterCommit() in AttachmentService deletes
the S3 object but only logs failures, which creates permanent orphans; change it
to hand off failures to a durable retry path. Keep the immediate call to
fileStorageService.delete(s3Key) inside afterCommit, but in the catch block call
a durable retry enqueuer (e.g., S3DeletionRetryService.enqueue(s3Key) or
OrphanedS3ObjectRepository.save(new OrphanedS3Object(s3Key))) instead of only
logging; implement S3DeletionRetryService to persist the key (or publish to a
message queue) and have a background worker/process retry deletes until success,
and ensure the enqueue/save is idempotent and records error metadata for
debugging. Also ensure the enqueue/save call is used from the same afterCommit
registration (TransactionSynchronizationManager.registerSynchronization) so the
enqueue happens only after DB commit.

---

Duplicate comments:
In `@src/main/java/org/example/vet1177/services/AttachmentService.java`:
- Around line 56-63: The code computes originalName and sanitizedName via
sanitizeFilename(originalName) but then still uses file.getOriginalFilename()
when building the S3 key and when persisting the filename; update the logic in
AttachmentService to use sanitizedName for the S3 key (replace
file.getOriginalFilename() in the s3Key construction) and when saving the
file_name persist sanitizedName (fall back to originalName only if sanitizedName
is null/blank), ensuring sanitizeFilename, originalName, sanitizedName and s3Key
are the referenced symbols you adjust.
🪄 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: d1934514-060a-49de-96c5-55ef0001ae6f

📥 Commits

Reviewing files that changed from the base of the PR and between 0c2cde6 and 89588af.

📒 Files selected for processing (1)
  • src/main/java/org/example/vet1177/services/AttachmentService.java

Comment thread src/main/java/org/example/vet1177/services/AttachmentService.java
Comment on lines +141 to +152
attachmentRepository.delete(attachment);

if (TransactionSynchronizationManager.isSynchronizationActive()) {
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
@Override
public void afterCommit() {
try {
fileStorageService.delete(s3Key);
log.info("S3 object {} deleted after successful DB commit", s3Key);
} catch (Exception e) {
log.error("CRITICAL: Failed to delete S3 object {} after DB commit!", s3Key, e);
}

@coderabbitai coderabbitai Bot Apr 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

A failed post-commit S3 delete becomes a permanent orphan.

Once the row is removed on Line 141, the afterCommit() block only logs if fileStorageService.delete(s3Key) fails. At that point the database no longer has anything to retry from, so a transient S3 issue can leave the blob undeleted indefinitely. Please hand this off to a durable retry path instead of dropping the failure in logs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/vet1177/services/AttachmentService.java` around
lines 141 - 152, The current afterCommit() in AttachmentService deletes the S3
object but only logs failures, which creates permanent orphans; change it to
hand off failures to a durable retry path. Keep the immediate call to
fileStorageService.delete(s3Key) inside afterCommit, but in the catch block call
a durable retry enqueuer (e.g., S3DeletionRetryService.enqueue(s3Key) or
OrphanedS3ObjectRepository.save(new OrphanedS3Object(s3Key))) instead of only
logging; implement S3DeletionRetryService to persist the key (or publish to a
message queue) and have a background worker/process retry deletes until success,
and ensure the enqueue/save is idempotent and records error metadata for
debugging. Also ensure the enqueue/save call is used from the same afterCommit
registration (TransactionSynchronizationManager.registerSynchronization) so the
enqueue happens only after DB commit.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo ithsjava25/project-backend-org-random-coders \
  --assignee johanbriger \
  --title "feat: Implement durable retry logic for failed S3/MinIO deletions" \
  --body "## Summary

When a post-commit S3/MinIO deletion fails in \`AttachmentService.deleteAttachment()\`, the failure is currently only logged. This means the blob remains orphaned indefinitely with no automated recovery path.

This issue tracks the work required to implement a durable retry mechanism so that no files are left orphaned in MinIO if a post-commit deletion fails.

## Background

Discussed in PR `#112` (comment: https://github.com/ithsjava25/project-backend-org-random-coders/pull/112#discussion_r3032969422).

In \`AttachmentService\`, the \`afterCommit()\` hook attempts to delete the S3 object after the database row has been removed. If that deletion fails, there is currently no mechanism to retry it — the S3 key is lost from the database and the blob becomes a permanent orphan.

## Tasks

### 1. Create \`OrphanedS3Object\` Entity
- Create a JPA entity \`OrphanedS3Object\` with fields:
  - \`id\` (UUID, primary key)
  - \`s3Key\` (String, unique, not null)
  - \`s3Bucket\` (String, not null)
  - \`createdAt\` (Instant, auto-set on persist)
  - \`retryCount\` (int, default 0)
  - \`lastAttemptAt\` (Instant, nullable)
  - \`lastError\` (String, nullable — truncated error message for debugging)

### 2. Create \`OrphanedS3ObjectRepository\`
- Extend \`JpaRepository<OrphanedS3Object, UUID>\`
- Add a query method to fetch records eligible for retry (e.g., ordered by \`lastAttemptAt\` ascending, with a max retry cap)
- Ensure upsert/save is idempotent (unique constraint on \`s3Key\`)

### 3. Update \`AttachmentService\`
- In the \`afterCommit()\` catch block (inside \`TransactionSynchronizationManager.registerSynchronization\`), call \`OrphanedS3ObjectRepository.save(new OrphanedS3Object(s3Key, bucketName))\` instead of only logging
- The enqueue/save must happen inside the same \`afterCommit\` registration so it only fires after a successful DB commit

### 4. Create a Scheduled Background Worker
- Create a \`@Component\` (e.g., \`OrphanedS3CleanupWorker\`) annotated with \`@Scheduled\`
- Periodically query \`OrphanedS3ObjectRepository\` for pending orphaned objects
- For each, attempt \`fileStorageService.delete(s3Key)\`
- On success: delete the \`OrphanedS3Object\` row
- On failure: increment \`retryCount\`, update \`lastAttemptAt\` and \`lastError\`, save
- Implement a maximum retry cap (e.g., 10 attempts) after which the entry is flagged/alerted rather than retried endlessly

## Acceptance Criteria

- [ ] \`OrphanedS3Object\` entity and migration script created
- [ ] \`OrphanedS3ObjectRepository\` created with idempotent save
- [ ] \`AttachmentService.afterCommit()\` enqueues failed deletions to \`OrphanedS3ObjectRepository\` instead of only logging
- [ ] Background worker retries deletions on a schedule and removes entries on success
- [ ] Retry count is bounded; entries exceeding the cap are logged/alerted
- [ ] Solution is covered by unit and/or integration tests

## References
- Raised by: `@johanbriger`
- Related PR: `#112`"

Length of output: 74


🏁 Script executed:

#!/bin/bash
gh issue list --repo ithsjava25/project-backend-org-random-coders --limit 5 --json number,title,url

Length of output: 792


@johanbriger, the issue has been created and assigned to you: #114 — feat: Implement durable retry logic for failed S3/MinIO deletions

It covers all the areas discussed:

  1. OrphanedS3Object entity — with fields for s3Key, s3Bucket, retryCount, lastAttemptAt, and lastError for debugging.
  2. OrphanedS3ObjectRepository — idempotent save backed by a unique constraint on s3Key.
  3. AttachmentService update — enqueue failed deletions to the repository inside afterCommit() instead of only logging.
  4. OrphanedS3CleanupWorker — a @Scheduled background worker that retries pending deletions, removes entries on success, and enforces a retry cap to prevent endless loops.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
src/main/java/org/example/vet1177/services/AttachmentService.java (2)

60-67: ⚠️ Potential issue | 🟠 Major

Use the sanitized filename where it actually matters.

sanitizedName is computed but not used in Line 67 or Line 82. This leaves key construction and persisted metadata exposed to unsafe/null original names.

🔧 Proposed fix
-        String s3Key = String.format("records/%s/%s_%s",
+        String s3Key = String.format("records/%s/%s_%s",
                 record.getId(),
                 UUID.randomUUID(),
-                file.getOriginalFilename());
+                sanitizedName);
...
-        attachment.setFileName(file.getOriginalFilename());
+        attachment.setFileName(sanitizedName);

Also applies to: 82-82

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/vet1177/services/AttachmentService.java` around
lines 60 - 67, The code computes sanitizedName but continues to use
file.getOriginalFilename() when building the S3 key and when persisting
metadata; update the AttachmentService code to use sanitizedName (the output of
sanitizeFilename(originalName)) in the s3Key creation and in any persisted
filename/metadata storage path so null/unsafe characters are avoided, keeping
the UUID and record.getId() usage intact; verify sanitizeFilename(...) is called
early (where originalName and sanitizedName are defined) and replace all usages
of file.getOriginalFilename() in this upload/persist flow with sanitizedName.

150-156: ⚠️ Potential issue | 🟠 Major

Post-commit S3 delete failures still have no durable retry path.

If deletion fails here, the code only logs and drops the key, so orphans can remain indefinitely.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/vet1177/services/AttachmentService.java` around
lines 150 - 156, The afterCommit() in AttachmentService currently calls
fileStorageService.delete(s3Key) and only logs failures, so orphaned S3 objects
may remain; change the catch block to persist the s3Key and metadata to a
durable retry mechanism (e.g., an S3DeletionRetry entity/repository or push onto
a reliable queue/topic) instead of dropping it, and return/exit so the exception
is not swallowed; ensure the retry worker/process reads that store and
re-invokes fileStorageService.delete(idempotently) with exponential backoff and
alerts after repeated failures so deletions are eventually completed.
🤖 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/vet1177/services/AttachmentService.java`:
- Around line 60-67: The code computes sanitizedName but continues to use
file.getOriginalFilename() when building the S3 key and when persisting
metadata; update the AttachmentService code to use sanitizedName (the output of
sanitizeFilename(originalName)) in the s3Key creation and in any persisted
filename/metadata storage path so null/unsafe characters are avoided, keeping
the UUID and record.getId() usage intact; verify sanitizeFilename(...) is called
early (where originalName and sanitizedName are defined) and replace all usages
of file.getOriginalFilename() in this upload/persist flow with sanitizedName.
- Around line 150-156: The afterCommit() in AttachmentService currently calls
fileStorageService.delete(s3Key) and only logs failures, so orphaned S3 objects
may remain; change the catch block to persist the s3Key and metadata to a
durable retry mechanism (e.g., an S3DeletionRetry entity/repository or push onto
a reliable queue/topic) instead of dropping it, and return/exit so the exception
is not swallowed; ensure the retry worker/process reads that store and
re-invokes fileStorageService.delete(idempotently) with exponential backoff and
alerts after repeated failures so deletions are eventually completed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d7a722e3-7aff-4d18-8b5e-4bfb62c6a476

📥 Commits

Reviewing files that changed from the base of the PR and between 89588af and fab0049.

📒 Files selected for processing (2)
  • src/main/java/org/example/vet1177/policy/AttachmentPolicy.java
  • src/main/java/org/example/vet1177/services/AttachmentService.java

@johanbriger
johanbriger merged commit 290f116 into main Apr 3, 2026
1 check passed
@johanbriger
johanbriger deleted the 42-feat-attachmentservicejava branch April 3, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feat: AttachmentService.java

2 participants