45 add attachment dto - #104
Conversation
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
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/schema.sql (1)
70-81:⚠️ Potential issue | 🟠 MajorAdd an explicit migration path for existing databases.
The
attachmenttable usesCREATE TABLE IF NOT EXISTSat line 70, which means the newdescriptioncolumn added at line 75 will only be created for fresh deployments. On production systems with pre-existingattachmenttables, the column won't be added, causing runtime failures when the JPA entity (which includes thedescriptionfield) attempts to read or write to it.Since the project has no migration framework and relies solely on
schema.sqlinitialization, an explicitALTER TABLEstatement is needed to backfill the column on existing databases.Suggested fix
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 REFERENCES users(id) ON DELETE SET NULL, file_name VARCHAR(500) NOT NULL, description VARCHAR(500), s3_key VARCHAR(1000) NOT NULL UNIQUE, s3_bucket VARCHAR(255) NOT NULL, file_type VARCHAR(100), file_size_bytes BIGINT, uploaded_at TIMESTAMPTZ NOT NULL DEFAULT NOW() ); + +ALTER TABLE attachment + ADD COLUMN IF NOT EXISTS description VARCHAR(500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/schema.sql` around lines 70 - 81, The schema uses CREATE TABLE IF NOT EXISTS for the attachment table so adding the new description column won't affect existing DBs; add an explicit migration ALTER TABLE statement to add the description column to existing attachment rows (e.g., ALTER TABLE attachment ADD COLUMN IF NOT EXISTS description VARCHAR(500);) so the JPA entity's description field exists on pre-existing databases—target the attachment table and the description column to ensure backward compatibility.
🧹 Nitpick comments (1)
src/main/java/org/example/vet1177/dto/request/attachment/AttachmentRequest.java (1)
13-15: Align requiredness across API and persistence fordescription.Line 13 makes
descriptionmandatory at API level, while the entity/schema currently allow null. Consider enforcing the same invariant in persistence (nullable = false/NOT NULL) or relaxing request validation if optional is intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/dto/request/attachment/AttachmentRequest.java` around lines 13 - 15, AttachmentRequest declares description as required (`@NotBlank`) but the persistence/schema allow null; pick one invariant and align both: if description must be required, update the persistence model (the entity field for description, e.g., AttachmentEntity.description or equivalent) to `@Column`(nullable = false) and add a DB migration to set the column NOT NULL; if description should be optional, relax the DTO by removing `@NotBlank` (or replace with `@Nullable` and keep `@Size`(max=500) if desired) and adjust any callers/tests. Ensure the same max length is enforced in the entity mapping (e.g., length=500) and update tests/migrations accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/resources/schema.sql`:
- Around line 70-81: The schema uses CREATE TABLE IF NOT EXISTS for the
attachment table so adding the new description column won't affect existing DBs;
add an explicit migration ALTER TABLE statement to add the description column to
existing attachment rows (e.g., ALTER TABLE attachment ADD COLUMN IF NOT EXISTS
description VARCHAR(500);) so the JPA entity's description field exists on
pre-existing databases—target the attachment table and the description column to
ensure backward compatibility.
---
Nitpick comments:
In
`@src/main/java/org/example/vet1177/dto/request/attachment/AttachmentRequest.java`:
- Around line 13-15: AttachmentRequest declares description as required
(`@NotBlank`) but the persistence/schema allow null; pick one invariant and align
both: if description must be required, update the persistence model (the entity
field for description, e.g., AttachmentEntity.description or equivalent) to
`@Column`(nullable = false) and add a DB migration to set the column NOT NULL; if
description should be optional, relax the DTO by removing `@NotBlank` (or replace
with `@Nullable` and keep `@Size`(max=500) if desired) and adjust any callers/tests.
Ensure the same max length is enforced in the entity mapping (e.g., length=500)
and update tests/migrations accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c1558e5-630a-4891-9ee7-ce9a4969e01d
📒 Files selected for processing (4)
src/main/java/org/example/vet1177/dto/request/attachment/AttachmentRequest.javasrc/main/java/org/example/vet1177/dto/response/attachment/AttachmentResponse.javasrc/main/java/org/example/vet1177/entities/Attachment.javasrc/main/resources/schema.sql
annikaholmqvist94
left a comment
annikaholmqvist94
left a comment
There was a problem hiding this comment.
Snyggt med DTOs och annotationerna!! Bara lite tankar som vanligt :P haha
| @NotNull(message = "Journal-ID får inte vara tomt") | ||
| UUID recordId, | ||
|
|
||
| @NotBlank(message = "Beskrivning av bilagan krävs") |
There was a problem hiding this comment.
så om bilagan krävs, ska vi då sätta även NOT NULL I schemat och se till att @column i entiteten är nullable=false. Så vi säkrar upp att det finns värden?
There was a problem hiding this comment.
Tagit bort not null då beskrivning bör vara valfritt
| String fileType, | ||
| Long fileSizeBytes, | ||
| Instant uploadedAt, | ||
| String uploadedBy, |
There was a problem hiding this comment.
Hur kommer det bli om detta är en String och i databasen har vi UUID? hmm..
There was a problem hiding this comment.
Snyggt fångat! Vi vill ju inte skicka en UUID till frontend. Genom att ha det som en String i DTO:n kan vi mappa om UUID:t till personens riktiga namn innan vi skickar iväg svaret. Fixar det i servicen när den är klar!
2 participants
closes #45
Changes:
DTOs: Created AttachmentRequestDTO (for uploads) and AttachmentResponseDTO (for API responses).
AttachmentRequestDTO now includes a description field to allow users to label their uploads (e.g., "X-ray of left paw").
Entity: Added the description field to the Attachment entity.
Database: Updated schema.sql to include the description column in the attachment table