feat: add contact photo support

[shreya20singh]

Summary

• Add contact photo support.

Testing Done

• Local code review completed (not performed per request)
• Unit tests added/updated
• Integration tests pass
• Manual testing performed

🤖 Generated with GitHub Copilot CLI

[qodo-code-review]

PR Summary by Qodo

Add validated contact photo support

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Adds optional photos across contact creation, retrieval, replacement, and partial updates.
• Validates base64 JPG, PNG, WebP, and GIF data URLs up to 2 MB.
• Documents photo constraints and tests persistence, mutation, rejection, and OpenAPI metadata.

Diagram

graph TD
  Client["API Client"] --> API["Contact API"] --> Validate{"Photo valid?"}
  Validate -->|Yes| Schemas["Contact Schemas"] --> Model["Contact Model"] --> Store["SQLite Store"]
  Validate -->|No| Reject["422 Response"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Store photos in object storage

• ➕ Avoids base64 expansion in database rows and API responses
• ➕ Scales better for larger images and caching through URLs
• ➖ Adds external infrastructure and credential management
• ➖ Requires upload, deletion, and orphan-cleanup lifecycle handling

2. Use multipart binary uploads

• ➕ Avoids base64's approximate 33% transfer overhead
• ➕ Separates binary validation from contact JSON
• ➖ Complicates create and update client workflows
• ➖ Requires additional endpoint and OpenAPI design changes

Recommendation: Directly storing a tightly limited data URL is appropriate for this self-contained SQLite API and keeps contact CRUD atomic and simple. If photos become larger, frequently requested, or production-scale, move the binary content to object storage and retain only a reference on the contact.

Files changed (5) +124 / -5

Enhancement (2) +61 / -1

models.pyPersist contact photos in the contact record +1/-0

Persist contact photos in the contact record

• Adds an optional text column to the SQLAlchemy contact model for storing the validated photo data URL.

app/models.py

schemas.pyValidate and expose photo data URLs +60/-1

Validate and expose photo data URLs

• Adds a reusable optional photo type to contact create, replace, update, and read schemas. Validation restricts payloads to 2 MB JPG, PNG, WebP, or GIF data URLs and verifies decoded file signatures match declared media types.

app/schemas.py

Tests (2) +60 / -2

test_contacts_api.pyCover contact photo API behavior +59/-2

Cover contact photo API behavior

• Tests photo creation, retrieval, replacement, patch addition and removal, plus rejection of unsupported, mismatched, and oversized image payloads.

tests/test_contacts_api.py

test_openapi.pyGuard photo constraints in OpenAPI +1/-0

Guard photo constraints in OpenAPI

• Verifies the generated contact schema documents the photo field's 2 MB size limit.

tests/test_openapi.py

Documentation (1) +3 / -2

README.mdDocument the optional contact photo field +3/-2

Document the optional contact photo field

• Adds 'photo' to the contact field list and documents accepted base64 data URLs, image formats, and the 2 MB limit.

README.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Concurrent startup duplicates addresses ✓ Resolved 🐞 Bug ≡ Correctness

Description

Two application instances can select the same non-null legacy row before either clears it, and both
then insert an address because the migration has no lock or uniqueness guard. Concurrent deployments
can therefore create duplicate legacy addresses, often with the same position, before both instances
null the source fields.

Code

app/database.py[R109-110]

+        connection.execute(
+            insert_address,

Evidence

Every application process calls init_db() at startup. The migration first reads all rows, later
inserts a child row, and only afterward clears the source columns; Address has neither a
uniqueness constraint for a migrated row nor a uniqueness constraint on (contact_id, position), so
two transactions can both execute the insert for the same contact.

app/main.py[61-64]
app/database.py[75-76]
app/database.py[84-87]
app/database.py[93-121]
app/models.py[63-88]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Concurrent application startups can migrate the same legacy contact more than once and create duplicate address rows.
## Issue Context
Every process runs `init_db()` during lifespan startup. Make claiming/migrating each source row atomic, or acquire a database-level migration lock that works for the supported database backends before reading and inserting rows.
## Fix Focus Areas
- app/database.py[65-121]
- app/main.py[61-64]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Legacy addresses disappear ✓ Resolved 🐞 Bug ≡ Correctness

Description

The startup migration creates the new schema but only migrates the photo column, leaving existing
contacts.address, city, state, postal_code, and country values stranded in now-unmapped
columns. After upgrading, those contacts serialize with an empty addresses collection, making
persisted address data unavailable through the API.

Code

app/database.py[R59-61]

+    columns = {column["name"] for column in inspect(connection).get_columns("contacts")}
+    if "photo" not in columns:
+        connection.execute(text("ALTER TABLE contacts ADD COLUMN photo TEXT"))

Evidence

The upgrade fixture models an existing contacts table containing all five legacy address columns,
but _run_migrations inspects and changes only photo. The ORM and response schemas now expose
addresses exclusively through the new child relationship, so no code reads those retained legacy
values.

app/database.py[52-61]
tests/test_database.py[11-27]
app/models.py[20-42]
app/models.py[63-90]
app/schemas.py[290-293]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Existing contact address fields are not migrated into the new `addresses` child table, so address data disappears from API responses after upgrade.
## Issue Context
`create_all()` can create the new table but does not transform existing rows. The migration currently handles only `photo`; add an idempotent data migration that detects the legacy columns and creates ordered child rows while preserving partially populated legacy addresses.
## Fix Focus Areas
- app/database.py[53-61]
- app/models.py[63-88]
- tests/test_database.py[6-49]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Existing databases lack photo ✓ Resolved 🐞 Bug ☼ Reliability

Description

Adding Contact.photo only to SQLAlchemy metadata leaves every existing contacts table without
the column because startup uses create_all, which does not alter existing tables. Persistent
SQLite and Postgres deployments will then fail contact queries and writes with a missing-column
database error after upgrading.

Code

app/models.py[21]

+    photo: Mapped[str | None] = mapped_column(Text)

Evidence

The new ORM mapping expects photo on every full Contact query, while initialization only invokes
Base.metadata.create_all; the repository explicitly documents persistent SQLite and Postgres
configurations, and tests recreate tables instead of exercising upgrades.

app/models.py[13-22]
app/database.py[48-52]
app/config.py[13-15]
README.md[62-76]
tests/conftest.py[15-20]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Existing persistent databases do not receive the new `contacts.photo` column, so ORM operations fail after upgrade.
## Issue Context
Startup only calls SQLAlchemy `create_all`, which creates missing tables but does not alter an existing `contacts` table. Add and run a migration that creates a nullable text column while preserving existing rows.
## Fix Focus Areas
- app/models.py[21-21]
- app/database.py[48-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View action required (1)

4. List embeds every full photo ✓ Resolved 🐞 Bug ➹ Performance

Description

Because photo is inherited by ContactRead, the list endpoint fetches and serializes each
contact's full base64 image. A default 50-row page can approach 140 MiB and the allowed 200-row page
about 560 MiB, causing excessive database transfer, memory use, serialization work, and response
size.

Code

app/schemas.py[R75-77]

+    photo: PhotoDataUrl | None = Field(
+        default=None,
+        max_length=MAX_PHOTO_DATA_URL_LENGTH,

Evidence

Each accepted image may contain 2 MiB decoded data, ContactRead inherits the photo field, list
pagination permits 50–200 rows, and the list query selects full Contact entities before serializing
every result as ContactRead.

app/schemas.py[9-12]
app/schemas.py[75-82]
app/schemas.py[198-216]
app/models.py[13-22]
app/crud.py[27-59]
app/routers/contacts.py[73-106]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Contact list and search responses include every full photo, allowing a single page to reach hundreds of MiB.
## Issue Context
Introduce a lightweight list-item response without photo data and project/defer the photo column in list queries. Keep the full photo on the single-contact endpoint, or expose it through a dedicated photo endpoint.
## Fix Focus Areas
- app/schemas.py[75-82]
- app/schemas.py[198-216]
- app/crud.py[27-59]
- app/routers/contacts.py[73-108]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Migration scans every contact ✓ Resolved 🐞 Bug ➹ Performance

Description

_migrate_legacy_addresses materializes the entire contacts table on every startup, including after
all legacy values have already been cleared, because the legacy columns remain in the schema and the
SELECT has no value filter. Database size therefore directly increases restart time and startup
memory usage indefinitely.

Code

app/database.py[R75-76]

+    selected_columns = ", ".join(("id", *legacy_columns))
+    rows = connection.execute(text(f"SELECT {selected_columns} FROM contacts")).mappings().all()

Evidence

Startup invokes init_db() for every application lifespan, while the migration only clears legacy
values and never removes the legacy columns. Because the query has no WHERE clause and calls
.all(), every later startup still reads and materializes every contact regardless of whether
migration work remains.

app/main.py[61-64]
app/database.py[67-76]
app/database.py[91-121]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The startup migration loads every contact into memory on every process start, even after migration has completed.
## Issue Context
Legacy columns are retained and nulled, so detecting their existence does not indicate that any rows still require migration. Filter in SQL and avoid materializing all rows at once.
## Fix Focus Areas
- app/database.py[75-76]
- app/database.py[93-121]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Truncated images pass validation ✓ Resolved 🐞 Bug ≡ Correctness

Description

_matches_image_type accepts signature-only or otherwise truncated payloads as photos, such as the
three-byte JPEG data:image/jpeg;base64,/9j/. These malformed values are persisted even though they
are not decodable images, so clients can receive broken photos despite successful validation.

Code

app/schemas.py[R17-20]

+    if media_type == "image/jpeg":
+        return content.startswith(b"\xff\xd8\xff")
+    if media_type == "image/png":
+        return content.startswith(b"\x89PNG\r\n\x1a\n")

Evidence

After base64 decoding, validation delegates only to short prefix checks: three bytes for JPEG, eight
for PNG, six for GIF, and twelve positional bytes for WebP; no parser verifies that the remaining
image structure exists or is decodable.

app/schemas.py[16-28]
app/schemas.py[31-46]
app/crud.py[62-69]
app/routers/contacts.py[45-63]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Image validation checks only leading magic bytes, allowing truncated and structurally invalid payloads to be stored as photos.
## Issue Context
Use a maintained image decoder to verify the complete payload for each allowed format, reject truncated/corrupt files, and retain the existing media-type and size checks. Add tests for signature-only JPEG, PNG, GIF, and minimal invalid RIFF/WEBP payloads.
## Fix Focus Areas
- app/schemas.py[16-28]
- app/schemas.py[31-46]
- tests/test_contacts_api.py[39-59]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit c4bc7f2 ⚖️ Balanced

Results up to commit 8b2f5c7 ⚖️ Balanced

No changes from previous review

Results up to commit 1dd76e1 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Legacy addresses disappear ✓ Resolved 🐞 Bug ≡ Correctness

Description

The startup migration creates the new schema but only migrates the photo column, leaving existing
contacts.address, city, state, postal_code, and country values stranded in now-unmapped
columns. After upgrading, those contacts serialize with an empty addresses collection, making
persisted address data unavailable through the API.

Code

app/database.py[R59-61]

+    columns = {column["name"] for column in inspect(connection).get_columns("contacts")}
+    if "photo" not in columns:
+        connection.execute(text("ALTER TABLE contacts ADD COLUMN photo TEXT"))

Evidence

The upgrade fixture models an existing contacts table containing all five legacy address columns,
but _run_migrations inspects and changes only photo. The ORM and response schemas now expose
addresses exclusively through the new child relationship, so no code reads those retained legacy
values.

app/database.py[52-61]
tests/test_database.py[11-27]
app/models.py[20-42]
app/models.py[63-90]
app/schemas.py[290-293]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Existing contact address fields are not migrated into the new `addresses` child table, so address data disappears from API responses after upgrade.

## Issue Context
`create_all()` can create the new table but does not transform existing rows. The migration currently handles only `photo`; add an idempotent data migration that detects the legacy columns and creates ordered child rows while preserving partially populated legacy addresses.

## Fix Focus Areas
- app/database.py[53-61]
- app/models.py[63-88]
- tests/test_database.py[6-49]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 4af21e0 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Concurrent startup duplicates addresses ✓ Resolved 🐞 Bug ≡ Correctness

Description

Two application instances can select the same non-null legacy row before either clears it, and both
then insert an address because the migration has no lock or uniqueness guard. Concurrent deployments
can therefore create duplicate legacy addresses, often with the same position, before both instances
null the source fields.

Code

app/database.py[R109-110]

+        connection.execute(
+            insert_address,

Evidence

Every application process calls init_db() at startup. The migration first reads all rows, later
inserts a child row, and only afterward clears the source columns; Address has neither a
uniqueness constraint for a migrated row nor a uniqueness constraint on (contact_id, position), so
two transactions can both execute the insert for the same contact.

app/main.py[61-64]
app/database.py[75-76]
app/database.py[84-87]
app/database.py[93-121]
app/models.py[63-88]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Concurrent application startups can migrate the same legacy contact more than once and create duplicate address rows.

## Issue Context
Every process runs `init_db()` during lifespan startup. Make claiming/migrating each source row atomic, or acquire a database-level migration lock that works for the supported database backends before reading and inserting rows.

## Fix Focus Areas
- app/database.py[65-121]
- app/main.py[61-64]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Migration scans every contact ✓ Resolved 🐞 Bug ➹ Performance

Description

_migrate_legacy_addresses materializes the entire contacts table on every startup, including after
all legacy values have already been cleared, because the legacy columns remain in the schema and the
SELECT has no value filter. Database size therefore directly increases restart time and startup
memory usage indefinitely.

Code

app/database.py[R75-76]

+    selected_columns = ", ".join(("id", *legacy_columns))
+    rows = connection.execute(text(f"SELECT {selected_columns} FROM contacts")).mappings().all()

Evidence

Startup invokes init_db() for every application lifespan, while the migration only clears legacy
values and never removes the legacy columns. Because the query has no WHERE clause and calls
.all(), every later startup still reads and materializes every contact regardless of whether
migration work remains.

app/main.py[61-64]
app/database.py[67-76]
app/database.py[91-121]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The startup migration loads every contact into memory on every process start, even after migration has completed.

## Issue Context
Legacy columns are retained and nulled, so detecting their existence does not indicate that any rows still require migration. Filter in SQL and avoid materializing all rows at once.

## Fix Focus Areas
- app/database.py[75-76]
- app/database.py[93-121]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 3aa1ce7 ⚖️ Balanced

No changes from previous review

Results up to commit c4bc7f2 ⚖️ Balanced

No changes from previous review

[shreya20singh]

/improve

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8b2f5c7

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8b2f5c7

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8b2f5c7

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1dd76e1

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4af21e0

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3aa1ce7

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3aa1ce7

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit c4bc7f2

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit c4bc7f2

[shreya20singh]

/review

[qodo-code-review]

Code review by qodo was updated up to the latest commit c4bc7f2