feat: implement Attachment entity and repository#29
Open
johanbriger wants to merge 7 commits intomainfrom
Open
feat: implement Attachment entity and repository#29johanbriger wants to merge 7 commits intomainfrom
johanbriger wants to merge 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new JPA entity Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
johanbriger
requested review from
TatjanaTrajkovic,
annikaholmqvist94 and
lindaeskilsson
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/example/vet1177/repository/AttachmentRepository.java (1)
14-16: Add DB indexes for these query paths.
findByMedicalRecordIdandfindByUploadedByIdwill degrade with table growth without indexes onattachment(record_id)andattachment(uploaded_by).Proposed migration snippet
+CREATE INDEX IF NOT EXISTS idx_attachment_record_id ON attachment(record_id); +CREATE INDEX IF NOT EXISTS idx_attachment_uploaded_by ON attachment(uploaded_by);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/repository/AttachmentRepository.java` around lines 14 - 16, The repository methods findByMedicalRecordId and findByUploadedById will become slow without DB indexes; add explicit non-unique indexes on attachment(record_id) and attachment(uploaded_by) via your schema migration system (e.g., add a Flyway/Liquibase migration) so queries used by AttachmentRepository are indexed, include a clear index name (e.g., idx_attachment_record_id and idx_attachment_uploaded_by) and provide a matching rollback that drops those indexes; alternatively, if you prefer schema-by-entity, add `@Table`(indexes = {`@Index`(name="idx_attachment_record_id", columnList="record_id"), `@Index`(name="idx_attachment_uploaded_by", columnList="uploaded_by")}) to the Attachment entity so the DDL includes the indexes.
🤖 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/entities/Attachment.java`:
- Around line 15-21: In the Attachment entity, the `@ManyToOne` relationships for
fields medicalRecord and uploadedBy lack cascade settings and will block parent
deletions; update the two `@ManyToOne` annotations in class Attachment to include
cascade = CascadeType.ALL (or appropriate explicit CascadeType values) on the
medicalRecord and uploadedBy mappings and then update the database schema for
the corresponding foreign keys to include ON DELETE CASCADE so that deleting a
MedicalRecord or User correctly cascades to Attachment rows.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/repository/AttachmentRepository.java`:
- Around line 14-16: The repository methods findByMedicalRecordId and
findByUploadedById will become slow without DB indexes; add explicit non-unique
indexes on attachment(record_id) and attachment(uploaded_by) via your schema
migration system (e.g., add a Flyway/Liquibase migration) so queries used by
AttachmentRepository are indexed, include a clear index name (e.g.,
idx_attachment_record_id and idx_attachment_uploaded_by) and provide a matching
rollback that drops those indexes; alternatively, if you prefer
schema-by-entity, add `@Table`(indexes = {`@Index`(name="idx_attachment_record_id",
columnList="record_id"), `@Index`(name="idx_attachment_uploaded_by",
columnList="uploaded_by")}) to the Attachment entity so the DDL includes the
indexes.
🪄 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: 33ab0162-fc1f-44b2-8a65-81b79835af81
📒 Files selected for processing (2)
src/main/java/org/example/vet1177/entities/Attachment.javasrc/main/java/org/example/vet1177/repository/AttachmentRepository.java
annikaholmqvist94
approved these changes
Mar 27, 2026
Contributor
annikaholmqvist94
left a comment
annikaholmqvist94
left a comment
There was a problem hiding this comment.
Snyggt! Bara att kika på coderabbits kommentar sen merge:)
TatjanaTrajkovic
approved these changes
Mar 27, 2026
Contributor
TatjanaTrajkovic
left a comment
TatjanaTrajkovic
left a comment
There was a problem hiding this comment.
IImplement CodeRabbit's suggested changes and then merge.
Updated Attachment entity with @onDelete(action = OnDeleteAction.CASCADE) to ensure attachments are removed when their parent MedicalRecord is deleted.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/vet1177/entities/Attachment.java (1)
18-25:⚠️ Potential issue | 🟠 MajorAlign FK delete behavior across both relations and schema
Line 20 applies cascade semantics only on
medicalRecord, whileuploadedByhas none, and the table FKs are currently non-cascading insrc/main/resources/schema.sql(record_id,uploaded_by). This can break parent deletes with FK violations and creates inconsistent lifecycle behavior.Suggested patch
diff --git a/src/main/java/org/example/vet1177/entities/Attachment.java b/src/main/java/org/example/vet1177/entities/Attachment.java @@ `@ManyToOne`(fetch = FetchType.LAZY) `@JoinColumn`(name = "uploaded_by", nullable = false) + `@OnDelete`(action = OnDeleteAction.CASCADE) private User uploadedBy;diff --git a/src/main/resources/schema.sql b/src/main/resources/schema.sql @@ -CREATE TABLE IF NOT EXISTS attachment ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - record_id UUID NOT NULL REFERENCES medical_record(id), - uploaded_by UUID NOT NULL REFERENCES users(id), +CREATE TABLE IF NOT EXISTS attachment ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + record_id UUID NOT NULL REFERENCES medical_record(id) ON DELETE CASCADE, + uploaded_by UUID NOT NULL REFERENCES users(id) ON DELETE CASCADE, file_name VARCHAR(500) NOT NULL, s3_key VARCHAR(1000) NOT NULL UNIQUE, s3_bucket VARCHAR(255) NOT NULL,#!/bin/bash set -euo pipefail echo "== Attachment entity delete annotations ==" rg -n -C2 '@ManyToOne|@JoinColumn|@OnDelete' src/main/java/org/example/vet1177/entities/Attachment.java echo echo "== attachment FK definitions in schema ==" rg -n -C2 'CREATE TABLE IF NOT EXISTS attachment|record_id|uploaded_by|ON DELETE' src/main/resources/schema.sqlExpected: both
record_idanduploaded_byFKs show explicit intended delete behavior, and entity mapping is consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/entities/Attachment.java` around lines 18 - 25, The Attachment entity currently has `@OnDelete`(action = OnDeleteAction.CASCADE) only for the medicalRecord relation (field medicalRecord) while uploadedBy lacks delete semantics, and the DB FKs (record_id, uploaded_by) in schema.sql are non-cascading; make behavior consistent by adding the same delete-on-parent policy to the uploadedBy relation (add `@OnDelete`(action = OnDeleteAction.CASCADE) to the uploadedBy field in class Attachment) and update the foreign key definitions in src/main/resources/schema.sql so both record_id and uploaded_by FKs include ON DELETE CASCADE (or choose and apply the inverse policy to both places if you prefer NO ACTION), keeping nullable settings as-is and ensuring mappings (JoinColumn names) match the schema.
🤖 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/entities/Attachment.java`:
- Around line 18-25: The Attachment entity currently has `@OnDelete`(action =
OnDeleteAction.CASCADE) only for the medicalRecord relation (field
medicalRecord) while uploadedBy lacks delete semantics, and the DB FKs
(record_id, uploaded_by) in schema.sql are non-cascading; make behavior
consistent by adding the same delete-on-parent policy to the uploadedBy relation
(add `@OnDelete`(action = OnDeleteAction.CASCADE) to the uploadedBy field in class
Attachment) and update the foreign key definitions in
src/main/resources/schema.sql so both record_id and uploaded_by FKs include ON
DELETE CASCADE (or choose and apply the inverse policy to both places if you
prefer NO ACTION), keeping nullable settings as-is and ensuring mappings
(JoinColumn names) match the schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e517f09-8170-4c0c-9170-013434d8d80e
📒 Files selected for processing (1)
src/main/java/org/example/vet1177/entities/Attachment.java
Ensures that attachments are persisted, updated, and deleted automatically when their parent MedicalRecord is modified or removed.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/example/vet1177/entities/MedicalRecord.java (2)
52-53: Redundant fully-qualified class name.
Listis already imported on line 6, but line 53 usesjava.util.List<Attachment>instead of justList<Attachment>. This is inconsistent with line 118 which correctly uses the short form.♻️ Suggested fix
`@OneToMany`(mappedBy = "medicalRecord", cascade = CascadeType.ALL, orphanRemoval = true) -private java.util.List<Attachment> attachments = new ArrayList<>(); +private List<Attachment> attachments = new ArrayList<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/entities/MedicalRecord.java` around lines 52 - 53, The field declaration in class MedicalRecord uses a redundant fully-qualified type name "java.util.List<Attachment>" for the attachments field; replace it with the imported short form "List<Attachment>" to match other usages (e.g., the attachments field in MedicalRecord) and maintain consistency with the existing import and line 118 usage.
118-119: Returning mutable internal list breaks encapsulation.
getAttachments()returns the internal list directly. Callers can bypassaddAttachment()/removeAttachment()and mutate the collection without maintaining bidirectional relationship consistency. Consider returning an unmodifiable view.Additionally,
setAttachments()replaces the list without synchronizing the bidirectional relationship on old or new attachments.♻️ Suggested improvements
-public List<Attachment> getAttachments() { return attachments; } -public void setAttachments(List<Attachment> attachments) { this.attachments = attachments; } +public List<Attachment> getAttachments() { + return java.util.Collections.unmodifiableList(attachments); +} + +public void setAttachments(List<Attachment> attachments) { + this.attachments.forEach(a -> a.setMedicalRecord(null)); + this.attachments.clear(); + if (attachments != null) { + attachments.forEach(this::addAttachment); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/entities/MedicalRecord.java` around lines 118 - 119, getAttachments() exposes the internal mutable attachments list and setAttachments(...) replaces it without keeping the bidirectional links in sync; change getAttachments() to return an unmodifiable view or a defensive copy of the attachments list (e.g., Collections.unmodifiableList or new ArrayList<>(attachments)) and update setAttachments(List<Attachment> attachments) to (1) null-check and detach existing attachments by clearing their back-reference (e.g., existing.setMedicalRecord(null) or via removeAttachment()), (2) assign or wrap the new list, and (3) attach each new Attachment by setting its back-reference to this or calling addAttachment(attachment) so the bidirectional relationship is consistently maintained; handle nulls safely to avoid NPEs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/vet1177/entities/MedicalRecord.java`:
- Around line 52-53: The field declaration in class MedicalRecord uses a
redundant fully-qualified type name "java.util.List<Attachment>" for the
attachments field; replace it with the imported short form "List<Attachment>" to
match other usages (e.g., the attachments field in MedicalRecord) and maintain
consistency with the existing import and line 118 usage.
- Around line 118-119: getAttachments() exposes the internal mutable attachments
list and setAttachments(...) replaces it without keeping the bidirectional links
in sync; change getAttachments() to return an unmodifiable view or a defensive
copy of the attachments list (e.g., Collections.unmodifiableList or new
ArrayList<>(attachments)) and update setAttachments(List<Attachment>
attachments) to (1) null-check and detach existing attachments by clearing their
back-reference (e.g., existing.setMedicalRecord(null) or via
removeAttachment()), (2) assign or wrap the new list, and (3) attach each new
Attachment by setting its back-reference to this or calling
addAttachment(attachment) so the bidirectional relationship is consistently
maintained; handle nulls safely to avoid NPEs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f60ebcd-dd97-4f7b-b10a-0309002030cd
📒 Files selected for processing (1)
src/main/java/org/example/vet1177/entities/MedicalRecord.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/example/vet1177/entities/User.java (1)
89-92: Handle re-assignment inaddAttachmentto keep both sides consistent.Line 89-92 sets the new owner but does not detach from a previous owner list, which can leave stale in-memory state.
Recommended refactor
public void addAttachment(Attachment attachment) { - uploadedAttachments.add(attachment); - attachment.setUploadedBy(this); + if (attachment == null) return; + + User currentOwner = attachment.getUploadedBy(); + if (currentOwner != null && currentOwner != this) { + currentOwner.getUploadedAttachments().remove(attachment); + } + + if (!uploadedAttachments.contains(attachment)) { + uploadedAttachments.add(attachment); + } + attachment.setUploadedBy(this); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/entities/User.java` around lines 89 - 92, The addAttachment(Attachment attachment) method on User sets the new owner but doesn't remove the attachment from a prior owner's uploadedAttachments, leaving stale state; update addAttachment to check attachment.getUploadedBy() and if it's non-null and not this, remove the attachment from that previous owner's uploadedAttachments list before calling attachment.setUploadedBy(this) and uploadedAttachments.add(attachment); include null checks and avoid duplicate adds so both sides (User.uploadedAttachments and Attachment.uploadedBy) remain consistent.
🤖 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/entities/User.java`:
- Around line 29-32: The current User entity (uploadedAttachments field) omits
CascadeType.REMOVE but the Attachment.uploadedBy FK is NOT NULL without ON
DELETE action, causing hard-delete FK failures; make deletion semantics explicit
by either implementing a soft-delete on User (add a deleted flag/softDelete
logic in the User entity and update repository/queries to exclude deleted users
so attachments remain), or alter the Attachment mapping/schema so uploadedBy is
nullable and the DB FK uses ON DELETE SET NULL (and update the Attachment entity
mapping accordingly); locate the User class/uploadedAttachments and
Attachment.uploadedBy to apply the chosen fix and update any repository/service
delete flows to use soft-delete or the new DB behavior.
- Around line 81-87: The current getters/setters on User (getUploadedAttachments
/ setUploadedAttachments) allow external code to replace or mutate the list
without maintaining the bidirectional invariant on Attachment.uploadedBy; change
to return an unmodifiable copy or defensive copy from getUploadedAttachments and
remove or deprecate the plain setUploadedAttachments, instead provide
addUploadedAttachment(Attachment) and removeUploadedAttachment(Attachment)
methods that set attachment.setUploadedBy(this) (and clear it on removal) and
internally update the backing collection; if you must keep
setUploadedAttachments, make it iterate the incoming list and call
attachment.setUploadedBy(this) for each element and copy into a new internal
list to avoid reference leaks.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/entities/User.java`:
- Around line 89-92: The addAttachment(Attachment attachment) method on User
sets the new owner but doesn't remove the attachment from a prior owner's
uploadedAttachments, leaving stale state; update addAttachment to check
attachment.getUploadedBy() and if it's non-null and not this, remove the
attachment from that previous owner's uploadedAttachments list before calling
attachment.setUploadedBy(this) and uploadedAttachments.add(attachment); include
null checks and avoid duplicate adds so both sides (User.uploadedAttachments and
Attachment.uploadedBy) remain consistent.
🪄 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: 63e8b802-06e7-4229-87cc-f16f70e361bd
📒 Files selected for processing (1)
src/main/java/org/example/vet1177/entities/User.java
Read Summary to se all changes/fixes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/entities/Attachment.java`:
- Around line 23-26: In Attachment change the `@JoinColumn` on the uploadedBy
field to allow nulls: update the annotation on the uploadedBy field in class
Attachment (the field name uploadedBy, annotations `@ManyToOne` and
`@OnDelete`(action = OnDeleteAction.SET_NULL)) so the `@JoinColumn` uses nullable =
true (or removes the nullable attribute) to match the OnDeleteAction.SET_NULL
behavior and the DB ON DELETE SET NULL semantic; ensure the JPA mapping and
database schema remain consistent so deleting a User can set
Attachment.uploadedBy to null without validation errors.
In `@src/main/java/org/example/vet1177/entities/MedicalRecord.java`:
- Around line 122-129: The setter setAttachments clears the attachments
collection directly which leaves detached children with their owning side
(attachment.medicalRecord) still pointing to this MedicalRecord; instead iterate
over a copy of the current attachments and call removeAttachment(...) for each
to properly update the owning side and trigger orphanRemoval, then if
newAttachments is non-null call addAttachment(...) for each new item; ensure you
don't modify the collection while iterating (use a new ArrayList<>(attachments))
and keep addAttachment/removeAttachment as the single place that mutates both
sides.
🪄 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: f1131ded-723c-4ae6-9cbc-a0a746550a51
📒 Files selected for processing (4)
src/main/java/org/example/vet1177/entities/Attachment.javasrc/main/java/org/example/vet1177/entities/MedicalRecord.javasrc/main/java/org/example/vet1177/entities/User.javasrc/main/resources/schema.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/vet1177/entities/User.java
Read Summary to se all changes/fixes.
lindaeskilsson
approved these changes
Mar 29, 2026
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
4 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 #22 **
Summary:
1.
schema.sql(Database)uploaded_byin theattachmenttable to be nullable and addedON DELETE SET NULL. This prevents Foreign Key errors when a user is deleted.ON DELETE CASCADEtorecord_idin theattachmenttable to ensure attachments are removed when a medical record is deleted.2.
Attachment.java(Entity)@JoinColumnforuploadedBytonullable = true. This aligns the Java code with the database'sON DELETE SET NULLbehavior, allowing users to be deleted without validation errors.3.
MedicalRecord.java(Entity)setAttachmentsto properly detach children by iterating over a defensive copy (new ArrayList<>(attachments)). This ensuresorphanRemovalis triggered correctly and preventsConcurrentModificationException.addAttachmentandremoveAttachmenthelper methods to ensure the "back-reference" (attachment.setMedicalRecord(this)) is always updated automatically.getAttachmentsto returnCollections.unmodifiableList()to protect the internal state from external mutation.4.
User.java(Entity)PERSISTandMERGEfor theuploadedAttachmentslist. This ensures that deleting aUserdoes not delete their uploaded files, preserving medical history.addUploadedAttachment) to maintain the bidirectional relationship.Summary by CodeRabbit