Feat/multi address - #8
Open
nex-code-c wants to merge 4 commits into
Open
Conversation
Optional `photo` on the contact, stored as a base64 data URL so it lives on the record with no file storage. On ContactBase, so create, replace, and read all carry it, and on ContactUpdate so PATCH can set or clear it. Tests pin the round trip and the PUT contract: a full replace that omits `photo` clears it, which is what the edit form has to carry through.
Addresses the review on David-Parry#6 and its frontend counterpart. The API accepted any string as a photo and echoed it back into an <img src>. It now takes only a base64 data URL for a PNG, JPEG, GIF, or WebP, capped at roughly 2 MB decoded, on both POST/PUT and PATCH. The web client rejects the wrong file type or an oversized file with a message naming the reason, and shrinks what it does accept to a 512px JPEG before submitting. That keeps a stored photo in the tens of KB, so photos no longer bloat every list response or approach the 1MB server action body limit — which is raised to 4mb anyway, as headroom for browsers that cannot repaint the image. Also fixes a race the review caught: a slow read of an earlier pick could land after a newer one and win. Reads now carry a sequence number and only the newest is allowed to apply, and removing a photo outranks any read still in flight. Image decoding is bounded by a timeout, and a canvas-less environment keeps the original data URL rather than losing the photo. README notes that create_all does not alter existing tables, so a file database made by an older build will not gain the photo column on its own.
A contact had exactly one address spread across five columns. Those are gone,
replaced by an `addresses` table: its own primary key, a foreign key back to
`contacts` with ON DELETE CASCADE, and a `type` constrained to Home/Work/Other.
A contact can now hold as many as it needs, and a fourth address is a row
rather than a schema change.
Addresses come back nested under the contact everywhere it is returned, and
there are two ways to write them: the contact's own POST/PUT take an
`addresses` array and replace the set in one request, while
/contacts/{id}/addresses gives per-address create, read, replace, and delete.
PATCH leaves addresses untouched unless the key is sent.
The relationship uses delete-orphan so the ORM cleans up children on both
SQLite and Postgres, not just where the FK constraint is enforced.
PR Summary by QodoAdd multi-address contacts and validated inline photos
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. Existing databases cannot upgrade
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| # Base64 data URL ("data:image/png;base64,..."), so a photo needs no blob | ||
| # store or static file route -- it travels with the contact JSON. | ||
| photo: Mapped[str | None] = mapped_column(Text) |
There was a problem hiding this comment.
1. Existing databases cannot upgrade 🐞 Bug ☼ Reliability
Adding the mapped Contact.photo column without a schema migration leaves older persistent contacts tables unchanged, so contact inserts or ORM reads fail on the missing column. create_all() only creates missing tables and cannot make this upgrade safe.
Agent Prompt
## Issue description
Persistent databases created before this PR do not receive the new `contacts.photo` column, causing inserts and contact queries to fail after upgrade.
## Issue Context
Startup only invokes SQLAlchemy `create_all`, which does not alter existing tables. Add a real migration mechanism and a migration that adds the nullable photo column before the new model is used.
## Fix Focus Areas
- app/models.py[37-39]
- app/database.py[48-52]
- app/main.py[67-70]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
-26
to
-29
| address: Mapped[str | None] = mapped_column(String(300)) | ||
| city: Mapped[str | None] = mapped_column(String(120)) | ||
| state: Mapped[str | None] = mapped_column(String(120)) | ||
| postal_code: Mapped[str | None] = mapped_column(String(20)) |
There was a problem hiding this comment.
2. Existing addresses become invisible 🐞 Bug ≡ Correctness
The PR removes the legacy scalar address mapping and reads only the new addresses relationship, but no migration copies existing address/city/state/postal_code/country values into address rows. After an upgrade, previously stored addresses disappear from every API response even if the schema is manually made queryable.
Agent Prompt
## Issue description
Existing contacts store address data in scalar columns, but the new API reads only rows from the `addresses` table, making legacy address data invisible.
## Issue Context
Add a data migration that creates an address row for each contact with legacy address content before retiring or ignoring the old columns. Preserve all five legacy fields and choose/document the migrated address type.
## Fix Focus Areas
- app/models.py[26-30]
- app/models.py[52-57]
- app/models.py[67-106]
- app/database.py[48-52]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+88
to
+90
| if len(value) > MAX_PHOTO_LENGTH: | ||
| raise ValueError("photo must be 2 MB or smaller once decoded") | ||
| if not _PHOTO_DATA_URL.match(value): |
There was a problem hiding this comment.
3. Invalid photos pass validation 🐞 Bug ≡ Correctness
validate_photo checks only the encoded string's length and alphabet, so values such as data:image/png;base64,AAAA are accepted despite not being PNG images, and malformed Base64 padding can also pass. Because it never decodes the payload, the 2,800,000-character threshold additionally permits about 2,099,982 decoded bytes, exceeding the documented 2 MB ceiling.
Agent Prompt
## Issue description
The photo validator accepts non-images and malformed Base64, and applies the size limit to encoded text rather than decoded bytes.
## Issue Context
Strictly decode Base64 with validation, reject decoding errors, check the decoded byte length against the chosen 2 MB definition, and verify that the bytes match the declared PNG/JPEG/GIF/WebP format. Add negative tests for valid Base64 containing non-image bytes, invalid padding, mismatched signatures, and the exact decoded boundary.
## Fix Focus Areas
- app/schemas.py[73-95]
- tests/test_contacts_api.py[168-181]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo review follow-up on the photo PR.
validate_photo only checked that the payload used the base64 alphabet, so
strings that cannot decode ("data:image/png;base64,A") were stored and
handed back to clients as an image. It also measured the size ceiling on
the encoded characters, which let a valid payload decode to ~2.1 MB while
the field documents 2 MB. Decode once, and check the decoded length.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 548a3ce |
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 548a3ce |
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
1 participant
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.
No description provided.