Feature/albums backend #786
Conversation
|
|
WalkthroughCentralized DB connections; added album_media junction table, cover_image and timestamps; converted album/image APIs to media-centric (media_id, media_type); updated DB CRUD, routes, schemas, tests, and OpenAPI accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as AlbumsRoute
participant DB as AlbumsDB
Client->>API: GET /albums/{album_id}/media?password=...
API->>DB: db_get_album(album_id)
DB-->>API: album dict / None
alt album exists & password ok
API->>DB: db_get_album_media(album_id)
DB-->>API: media_items list
API-->>Client: 200 GetAlbumMediaResponse
else not found / bad password
API-->>Client: 404 / 401 ErrorResponse
end
Client->>API: POST /albums/{album_id}/media (AddMediaRequest)
API->>DB: db_add_media_to_album(album_id, media_items)
DB-->>API: added count
API-->>Client: 200 SuccessResponse ("Added N items")
Client->>API: DELETE /albums/{album_id}/media/{media_id}
API->>DB: db_remove_media_from_album(album_id, media_id)
DB-->>API: confirmation
API-->>Client: 200 SuccessResponse ("removed")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
|
|
![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 (6)
backend/tests/test_albums.py (1)
94-95: Consider asserting the UUID validation result.The call to
uuid.UUID(album_id)verifies that the album_id is a valid UUID by checking it doesn't throw an exception, but it would be clearer to explicitly assert the result or add a comment explaining the intent.# Verify that the album_id is a valid UUID album_id = json_response["album_id"] - uuid.UUID(album_id) + assert uuid.UUID(album_id) # Validates UUID formatbackend/app/routes/albums.py (1)
141-145: Consider simplifying the conditional assignments.Since
UpdateAlbumRequestdeclaresnameandis_hiddenas required fields, checkingif body.name is not Noneandif body.is_hidden is not Noneis unnecessary. However, this defensive coding doesn't hurt and provides safety if the schema changes.If you prefer to simplify:
- current_name = body.name if body.name is not None else album["album_name"] - current_desc = body.description if body.description is not None else album["description"] - current_cover = body.cover_image_id if body.cover_image_id is not None else album["cover_image_id"] - current_hidden = body.is_hidden if body.is_hidden is not None else bool(album["is_hidden"]) + current_name = body.name # Required field + current_desc = body.description if body.description is not None else album["description"] + current_cover = body.cover_image_id if body.cover_image_id is not None else album["cover_image_id"] + current_hidden = body.is_hidden # Required fieldbackend/app/schemas/album.py (2)
16-18: Add validation for media_type field.The database enforces
media_type IN ('image', 'video')via a CHECK constraint, but the Pydantic model doesn't validate this. Consider adding a validator or using a Literal type to catch invalid values early and provide clearer error messages.+from typing import Literal class MediaItem(BaseModel): # <--- NEW (For mixed media) media_id: str - media_type: str # 'image' or 'video' + media_type: Literal["image", "video"]
78-81: Consider consolidating duplicate ErrorResponse models.Based on relevant code snippets,
ErrorResponseappears in multiple schema files (images.py, facetagging.py, test.py) with identical structure. Consider creating a shared base schema file to reduce duplication.docs/backend/backend_python/openapi.json (1)
1357-1372: Consider adding a maximum limit to the media_items array.The
media_itemsarray has nomaxItemsconstraint, which could allow clients to send arbitrarily large requests. Consider adding a reasonable limit (e.g., 100-1000 items per request) to prevent potential denial-of-service attacks or database performance issues.Based on static analysis, add to the schema:
"media_items": { "items": { "$ref": "#/components/schemas/MediaItem" }, "type": "array", "maxItems": 1000, "title": "Media Items" }backend/app/database/albums.py (1)
168-175: Consider returning bool for consistency.For consistency with
db_insert_albumanddb_update_album(which return bool), consider havingdb_delete_albumreturn a boolean indicating whether the album was found and deleted.-def db_delete_album(album_id: str) -> None: +def db_delete_album(album_id: str) -> bool: conn = _connect() try: cursor = conn.cursor() cursor.execute("DELETE FROM albums WHERE album_id = ?", (album_id,)) conn.commit() + return cursor.rowcount > 0 finally: conn.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/database/albums.py(2 hunks)backend/app/routes/albums.py(10 hunks)backend/app/schemas/album.py(3 hunks)backend/main.py(0 hunks)backend/tests/conftest.py(1 hunks)backend/tests/test_albums.py(14 hunks)docs/backend/backend_python/openapi.json(13 hunks)
💤 Files with no reviewable changes (1)
- backend/main.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/schemas/album.py (3)
backend/app/schemas/images.py (1)
ErrorResponse(59-62)backend/app/schemas/facetagging.py (1)
ErrorResponse(30-33)backend/app/schemas/test.py (1)
ErrorResponse(41-44)
backend/tests/conftest.py (1)
backend/app/database/albums.py (1)
db_create_albums_table(16-59)
backend/app/database/albums.py (1)
backend/app/database/images.py (1)
_connect(45-49)
🪛 Checkov (3.2.334)
docs/backend/backend_python/openapi.json
[medium] 1359-1365: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (36)
backend/tests/conftest.py (1)
9-9: LGTM! Correctly reflects the schema migration.The removal of
db_create_album_images_tablealigns with the refactor wheredb_create_albums_tablenow creates both thealbumsandalbum_mediatables.Also applies to: 28-28
backend/tests/test_albums.py (7)
23-48: LGTM! Mock fixtures properly updated.The fixtures now return dictionary-based records with the new schema fields (
cover_image_id,created_at,updated_at), matching the updated database layer.
107-108: LGTM! Correctly updated to dictionary-based mock.The mock now returns a dict as expected by the updated database layer.
117-168: LGTM! Get albums tests properly updated.All three test cases correctly use dictionary-based mocks and assertions, aligning with the new data layer returns.
170-200: LGTM! Album retrieval tests correctly updated.Tests properly use dictionary-based mocks and assertions for the new schema.
202-271: LGTM! Update album test properly refactored.The test correctly uses dictionary-based mocks with parametrized overrides and includes the new
cover_image_idfield across all test cases.
273-292: LGTM! Delete album test correctly updated.Test properly uses dictionary-based mock for the album data.
295-379: LGTM! Media management tests properly refactored.The test class has been correctly renamed and all tests updated to use media-centric endpoints (
/media), request bodies withmedia_items, and the newMediaItemstructure withmedia_idandmedia_typefields.backend/app/routes/albums.py (9)
1-31: LGTM! Imports correctly updated.All necessary imports for the media-centric refactor are properly included, including the new schemas (
AddMediaRequest,GetAlbumMediaResponse) and database functions.
36-53: LGTM! GET /albums/ correctly updated.The endpoint properly constructs
Albumobjects from dictionary-based database returns, including all new fields (cover_image_id,created_at,updated_at).
72-80: LGTM! POST /albums/ correctly passes all arguments.The fix correctly passes all 6 required arguments to
db_insert_album, including the newcover_image_idparameter, resolving the previous 500 error on album creation.
106-113: LGTM! GET /albums/{album_id} correctly updated.Album object construction properly uses dictionary access and includes all new schema fields.
169-177: LGTM! Update album correctly passes all arguments.The fix ensures all parameters, including
cover_image_id, are properly passed todb_update_album.
188-210: LGTM! Delete album correctly updated.The endpoint properly checks for album existence using the updated database layer.
213-257: Consider the security implications of password in query parameter.Passing the password as a query parameter (Line 217) is technically correct for GET requests, but passwords in URLs can be logged by web servers, proxies, and browser history. Consider these alternatives:
- Use a session/token-based authentication flow where the user authenticates once and receives a token
- Use a custom header (though this has its own trade-offs)
- Document this behavior and ensure appropriate logging configurations
The current implementation works functionally, but be aware of the logging risk.
260-296: LGTM! Add media to album correctly implemented.The endpoint properly extracts
media_idandmedia_typefromMediaItemobjects and passes them to the database layer, returning the count of items successfully added.
299-321: LGTM! Remove media from album correctly implemented.Good practice to check that the album exists before attempting to remove media.
backend/app/schemas/album.py (5)
7-14: LGTM! Album model correctly updated.The new fields (
cover_image_id,created_at,updated_at) are properly added as optional string fields, matching the database schema.
24-35: LGTM! CreateAlbumRequest correctly updated.The optional
cover_image_idfield is properly added to support cover images during album creation.
37-49: LGTM! UpdateAlbumRequest correctly updated.The optional
cover_image_idfield is properly added to support updating album cover images.
51-52: LGTM! AddMediaRequest correctly defined.The new request model properly uses a list of
MediaItemobjects for adding media to albums.
70-72: LGTM! GetAlbumMediaResponse correctly updated.The response model is properly renamed and updated to use
media_items: List[MediaItem]instead of image-specific fields.docs/backend/backend_python/openapi.json (4)
623-773: LGTM! Media endpoint definitions correctly updated.The OpenAPI specification properly reflects the new media-centric endpoints:
- GET
/albums/{album_id}/mediawith password query parameter- POST
/albums/{album_id}/mediawithAddMediaRequestbody- DELETE
/albums/{album_id}/media/{media_id}for removing individual media items
1373-1433: LGTM! Album schema correctly updated.The schema properly includes the new optional fields (
cover_image_id,created_at,updated_at) with correct type definitions.
2243-2260: MediaItem schema matches Pydantic model.The OpenAPI schema correctly reflects the
MediaItemmodel structure. Note: As mentioned in the schemas/album.py review, consider adding an enum constraint formedia_typeto match the database CHECK constraint.
1707-1727: LGTM! GetAlbumMediaResponse schema correctly updated.The response schema properly uses
media_itemswithMediaItemreferences, matching the refactored API.backend/app/database/albums.py (10)
10-14: LGTM! Centralized connection helper with FK enforcement.The
_connect()helper properly enables foreign key constraints and provides a consistent connection pattern, matching the approach inbackend/app/database/images.py.
16-59: LGTM! Table creation properly implements the new schema.The refactored function correctly creates both the
albumsandalbum_mediatables with:
- Appropriate foreign key constraints with CASCADE/SET NULL behaviors
- CHECK constraint for
media_typevalidation- UNIQUE constraint to prevent duplicate media entries per album
- Timestamps with automatic defaults
63-78: LGTM! Get all albums properly refactored.The function correctly:
- Uses
_connect()androw_factoryfor dictionary returns- Filters by
is_hiddenbased on theshow_hiddenparameter- Orders results by
created_at DESCfor newest-first display
80-89: LGTM! Get album by ID correctly implemented.Function properly fetches album by ID and returns a dictionary or None.
91-100: LGTM! Get album by name correctly implemented.Function properly fetches album by name and returns a dictionary or None.
102-132: LGTM! Album insertion correctly handles all new fields.The function properly:
- Hashes passwords using bcrypt when provided
- Includes
cover_image_idin the INSERT statement- Returns
Trueon success orFalseonIntegrityError(duplicate names)- Logs failures for debugging
134-166: LGTM! Album update correctly implements conditional password updates.The function properly:
- Auto-updates the
updated_attimestamp- Conditionally updates the password only when provided (allows updates without changing password)
- Includes
cover_image_idin the update- Returns a boolean indicating success
179-197: LGTM! Get album media correctly implemented.Function properly fetches media items with appropriate ordering by
sort_order(for manual ordering) andadded_at(fallback), returning dictionary-based results.
233-243: LGTM! Remove media correctly implemented.Function properly removes media items from an album using both
album_idandmedia_idin the WHERE clause.
245-257: LGTM! Password verification correctly implemented.Function properly uses bcrypt to verify passwords and handles cases where no password is set.
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/app/database/albums.py (5)
16-59: Consider adding an index for common album_media query patternThe albums and album_media DDL and logging/cleanup around table creation look good. Given that
db_get_album_mediafilters byalbum_idand orders bysort_order, consider adding an index such as:CREATE INDEX IF NOT EXISTS idx_album_media_album_sort ON album_media(album_id, sort_order);This will scale better if albums can have many media items.
102-131: Minor nit: broaden IntegrityError log message and consider Optional typingFunctionally
db_insert_albumlooks fine (hashing, commit, boolean conversion, andboolreturn). Two small polish points:
- The warning
"duplicate name?"might be misleading if the violation is onalbum_id; consider a more generic message like"Album creation failed (integrity error)".- For type hints,
cover_image_idandpasswordare nullable at call sites, soOptional[str]would better reflect usage.
177-196: Align media retrieval docstring with actual return shape
db_get_album_mediaquery and ordering look good, and returning a list of dicts is consistent with the rest of the module. The docstring currently says “(id, type)” but you actually returnmedia_id,media_type, andadded_at. Consider updating the docstring (or the selected columns) so they match exactly.
200-248: Tighten media_type validation and logging in db_add_media_to_albumThe improved checks for album existence and media existence, plus the duplicate handling, look solid. Two minor concerns:
media_typeis never validated before insert; if a caller passes an invalid value, the CHECK constraint will raiseIntegrityError, which is swallowed and treated like a duplicate. That can hide bad input. Consider explicitly validatingmedia_type in ('image', 'video')and either skipping with a log or surfacing a clearer error.- When you skip a media_id because it doesn’t exist in
images, there’s no log. A debug‑level log (with album_id and media_id) could be helpful when diagnosing mismatches between albums and the media table.These won’t break correctness but would improve observability and data hygiene.
250-262: Media removal helper is straightforward
db_remove_media_from_albumis a simple, parameterized delete wrapped in a commit/close block; looks fine. If you ever need to know whether anything was removed, you could returncursor.rowcount > 0, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/albums.py(2 hunks)
🔇 Additional comments (5)
backend/app/database/albums.py (5)
3-14: Centralized connection helper and logger look goodThe
_connecthelper withPRAGMA foreign_keys = ONand the module‑level logger setup are clean and consistent; this is a solid foundation for the rest of the DB helpers.
61-79: Album list retrieval API is consistent and dict‑based
db_get_all_albumscorrectly usesrow_factory = sqlite3.Row, optional hidden filtering, and returnsList[dict]ordered bycreated_at DESC. This matches the PR goal of returning dicts from the DB layer.
80-101: Single‑album getters are straightforward and safeBoth
db_get_albumanddb_get_album_by_nameare simple, parameterized queries returningOptional[dict], which is clear and avoids tuple indexing issues.
168-175: Delete helper is minimal and consistent
db_delete_albumuses the shared_connecthelper, a parameterized DELETE, and commits/cleans up correctly. No changes needed.
264-276: Password verification helper is consistent with new connection pattern
verify_album_passwordnow uses_connect, handles missing/empty hashes safely, and delegates to bcrypt for checking. This matches the schema and other helpers.
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/database/albums.py (1)
217-260: Previous concerns addressed; consider logging skipped media.The media operations are well-implemented with proper validation:
- Explicit album existence check prevents silent FK failures ✓
- Media validation ensures referential integrity ✓
- IntegrityError handling now strictly for duplicates ✓
Optional improvement: Line 238 silently skips invalid
media_idvalues. Consider logging this for debugging:cursor.execute("SELECT 1 FROM images WHERE id = ?", (media_id,)) if not cursor.fetchone(): + logger.warning(f"Skipping invalid media_id: {media_id}") continue # Skip invalid media IDs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/albums.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/database/albums.py (1)
backend/app/database/images.py (1)
_connect(45-49)
🔇 Additional comments (3)
backend/app/database/albums.py (3)
1-14: LGTM: Clean connection management.The centralized
_connect()helper with foreign key enforcement is a best practice that ensures referential integrity across the schema. Consistent with the pattern used inbackend/app/database/images.py.
16-59: LGTM: Robust schema design.The refactored schema properly supports mixed media with:
- CHECK constraint ensuring
media_type IN ('image', 'video')- UNIQUE constraint preventing duplicates
- Appropriate FK cascades (ON DELETE CASCADE for junction table, SET NULL for cover image)
- Comprehensive error handling and logging
63-193: LGTM: Well-designed CRUD operations.The enhanced CRUD functions demonstrate solid engineering:
- Consistent use of
row_factoryfor clean dict-based APIs- Comprehensive type hints with
Optional[dict]andList[dict]- Clear password update semantics (None/empty/value) as documented
- Safe dynamic SQL construction in
db_update_album(updates list is internal, user data parameterized)- Boolean returns for insert/update enable proper error propagation
|
|
2 similar comments
|
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/app/database/albums.py (3)
102-133: Consider explicit Optional type hint and cover_image_id validation.Two suggestions for clarity and robustness:
Line 106: The
cover_image_idparameter acceptsNoneas default but lacks an explicitOptional[str]type hint. While Python infers this, explicit typing improves code clarity.Lines 119-125: No validation that
cover_image_idexists in theimagestable before insertion. If it doesn't exist, the foreign key constraint will raise anIntegrityError, which is caught generically on line 128-130. Consider adding explicit validation to distinguish between duplicate album names and invalid cover image references for better error reporting.Apply this diff to add explicit type hint:
def db_insert_album( album_id: str, album_name: str, description: str = "", - cover_image_id: str = None, + cover_image_id: Optional[str] = None, is_hidden: bool = False, password: str = None, ) -> bool:For validation, you could add before the INSERT:
if cover_image_id: cursor.execute("SELECT 1 FROM images WHERE id = ?", (cover_image_id,)) if not cursor.fetchone(): logger.error(f"Invalid cover_image_id: {cover_image_id}") return False
134-184: Good password semantics; consider cover_image_id validation.The password update logic is well-documented and correctly handles all three cases (None, empty string, non-empty string). The automatic
updated_attimestamp is a good practice.Similar to
db_insert_album, consider validating thatcover_image_idexists in theimagestable before updating (lines 152-159). Currently, an invalidcover_image_idwill cause a foreign key constraint violation caught generically by the exception handler on lines 180-182.You could add validation before building the query:
if cover_image_id: cursor.execute("SELECT 1 FROM images WHERE id = ?", (cover_image_id,)) if not cursor.fetchone(): logger.error(f"Invalid cover_image_id: {cover_image_id}") return False
217-261: Good validation improvements; consider Python-level media_type validation.The fixes from previous reviews (album existence check, clarified comments, and explicit duplicate handling) are well-implemented. The function now provides robust validation and graceful error handling.
Optional enhancement (lines 234-246): Consider validating
media_typevalues in Python before insertion. While the database CHECK constraint (line 45) will catch invalid values, Python validation would provide clearer error messages and fail fast.You could add validation after line 234:
if media_type not in ('image', 'video'): logger.warning(f"Skipping invalid media_type: {media_type}") continue
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/albums.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/database/albums.py (1)
backend/app/database/images.py (1)
_connect(45-49)
🔇 Additional comments (6)
backend/app/database/albums.py (6)
1-14: LGTM: Clean setup and centralized connection handling.The imports, logger initialization, and
_connect()helper follow good patterns and match the approach used inbackend/app/database/images.py. Enabling foreign keys via PRAGMA is essential for referential integrity.
61-101: LGTM: Consistent query patterns with proper dict conversion.The get operations correctly use
row_factory = sqlite3.Rowto enable dictionary conversion, have well-defined return types, and include proper connection cleanup. The ordering bycreated_at DESCprovides sensible defaults for album listings.
186-194: LGTM: Clean delete operation with CASCADE handling.The delete operation is straightforward and benefits from the
ON DELETE CASCADEconstraint defined in thealbum_mediatable schema (line 48), which automatically removes related media entries.
197-216: LGTM: Well-structured media retrieval with sensible ordering.The function correctly returns dictionaries and uses a sensible ordering strategy (explicit
sort_orderfirst, then chronological). Returning an empty list for non-existent albums is acceptable behavior for read operations.
263-273: LGTM: Straightforward media removal.The function correctly removes media associations from the junction table. Silent success for non-existent entries is standard and acceptable behavior for delete operations.
275-287: LGTM: Secure password verification.The function correctly uses
bcrypt.checkpwand handles edge cases (non-existent albums and albums without passwords) appropriately.
|
|
2 similar comments
|
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/albums.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/database/albums.py (4)
backend/app/database/images.py (1)
_connect(45-49)backend/app/models/ObjectClassifier.py (1)
close(27-33)backend/app/models/FaceNet.py (1)
close(27-29)backend/app/models/YOLO.py (1)
close(34-36)
🔇 Additional comments (10)
backend/app/database/albums.py (10)
1-8: LGTM! Logger initialization follows project conventions.The centralized logger setup is consistent with the project pattern and will help with debugging album operations.
10-14: LGTM! Connection helper centralizes FK enforcement.The
_connect()helper ensures foreign key constraints (including ON DELETE CASCADE) are enforced consistently across all operations. This matches the pattern inbackend/app/database/images.py.
63-100: LGTM! Album retrieval functions are clean and consistent.The getter functions follow a consistent pattern: use
_connect(), setrow_factoryfor dict conversion, and ensure connections are closed infinallyblocks.
102-137: LGTM! Album creation validates FK references before insert.The validation of
cover_image_id(lines 113-117) before insertion prevents FK constraint violations and provides clear error logging. Password hashing is correctly applied only when a password is provided.
139-195: LGTM! Update function correctly handles password removal semantics.The three-case password handling (lines 173-181) addresses the past review feedback:
password == "": Clears protection by settingpassword_hash = NULLpasswordis truthy: Hashes and updates passwordpassword is None: Preserves existing valueThe
cover_image_idvalidation (lines 156-161) prevents invalid FK references.
197-204: LGTM! Delete function relies on CASCADE for cleanup.The ON DELETE CASCADE constraint on
album_mediaensures junction records are automatically removed when an album is deleted.
208-226: LGTM! Media retrieval query is well-ordered.The query orders by
sort_order ASC(user-defined sequence) and thenadded_at DESC(most recent first) as a secondary sort, which provides predictable and intuitive ordering.
228-272: LGTM! Media addition function addresses past review concerns.The upfront checks for album existence (lines 234-238) and media existence (lines 247-250) ensure that
IntegrityErrorexceptions (lines 260-263) now strictly represent duplicate entries (UNIQUE constraint violations). The clarified comment (line 246) correctly states that both media types are stored in the images table. The sort_order logic (lines 240-243, 257) ensures proper sequencing.
274-284: LGTM! Media removal is straightforward.Simple DELETE from the junction table with proper connection cleanup.
286-298: LGTM! Password verification uses bcrypt correctly.The function correctly handles albums without passwords (line 294 returns
Falseifpassword_hashis NULL) and uses bcrypt's secure comparison.
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/app/database/albums.py (3)
113-117: Consider validating cover image media type.The cover image validation checks existence in the
imagestable but doesn't verify thatmedia_type='image'. If videos are stored in the same table, a video ID could be set as a cover image, which may not be the intended behavior.Apply this diff if you want to enforce cover images to be images only:
if cover_image_id: - cursor.execute("SELECT 1 FROM images WHERE id = ?", (cover_image_id,)) + cursor.execute("SELECT 1 FROM images WHERE id = ? AND media_type = 'image'", (cover_image_id,)) if not cursor.fetchone(): - logger.error(f"Invalid cover_image_id: {cover_image_id}") + logger.error(f"Invalid cover_image_id or not an image: {cover_image_id}") return False
156-161: Same media type validation concern as in insert.Similar to Line 113-117, the cover image validation doesn't verify
media_type='image'. Consider the same fix as suggested fordb_insert_album.Apply this diff:
if cover_image_id: - cursor.execute("SELECT 1 FROM images WHERE id = ?", (cover_image_id,)) + cursor.execute("SELECT 1 FROM images WHERE id = ? AND media_type = 'image'", (cover_image_id,)) if not cursor.fetchone(): - logger.error(f"Cannot update album {album_id}: Invalid cover_image_id {cover_image_id}") + logger.error(f"Cannot update album {album_id}: Invalid cover_image_id or not an image {cover_image_id}") conn.close() return False
197-204: Consider consistent return type across CRUD operations.
db_insert_albumanddb_update_albumreturnboolto indicate success/failure, butdb_delete_albumreturnsNone. For consistency, consider returningboolhere as well, especially if callers need to distinguish between a successful delete and a no-op (album not found).If desired, apply this diff:
-def db_delete_album(album_id: str) -> None: +def db_delete_album(album_id: str) -> bool: conn = _connect() try: cursor = conn.cursor() cursor.execute("DELETE FROM albums WHERE album_id = ?", (album_id,)) + deleted = cursor.rowcount > 0 conn.commit() + return deleted finally: conn.close()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/albums.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/database/albums.py (4)
backend/app/database/images.py (1)
_connect(45-49)backend/app/models/ObjectClassifier.py (1)
close(27-33)backend/app/models/FaceNet.py (1)
close(27-29)backend/app/models/FaceDetector.py (1)
close(69-79)
🔇 Additional comments (5)
backend/app/database/albums.py (5)
10-14: LGTM: Centralized connection helper with foreign key enforcement.The
_connect()helper properly enables foreign keys and centralizes connection logic. This ensures referential integrity across all database operations.
63-100: LGTM: Consistent read operations with proper resource management.The read operations follow a consistent pattern with parameterized queries, proper connection cleanup, and clear return types. The use of
sqlite3.Rowand dictionary conversion provides a clean interface for consumers.
208-227: LGTM: Clean media retrieval with proper ordering.The function correctly retrieves album media with appropriate ordering (sort_order, then added_at) and returns a clean dict-based interface.
228-275: Excellent validation and error handling in batch media addition.The function now includes comprehensive validation:
- Explicit album existence check (Line 235-238)
- Media type validation (Line 246-248)
- Media ID existence check (Line 250-253)
- Graceful duplicate handling (Line 263-266)
The sequential sort_order assignment and deterministic junction ID generation are well-implemented.
289-301: LGTM: Secure password verification.The function uses bcrypt for secure password comparison and properly handles cases where the album doesn't exist or has no password protection.
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/app/database/albums.py (1)
280-290: LGTM: Straightforward media removal.The function correctly removes the media-album association. Consider returning a boolean to indicate success (e.g.,
return cursor.rowcount > 0) for consistency withdb_delete_album().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/albums.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/database/albums.py (1)
backend/app/database/images.py (1)
_connect(45-49)
🔇 Additional comments (7)
backend/app/database/albums.py (7)
1-8: LGTM: Clean imports and logger setup.The module dependencies and logger initialization follow good practices.
10-14: LGTM: Centralized connection management with FK enforcement.The
_connect()helper properly enables foreign key constraints, which is essential for maintaining referential integrity across the albums, album_media, and images tables.
63-100: LGTM: Consistent getter implementations.All three getter functions follow a consistent pattern: use
_connect(), setrow_factoryfor dictionary conversion, and return the appropriate type (List[dict]orOptional[dict]). The filtering and ordering indb_get_all_albums()is well-implemented.
102-138: LGTM: Robust album insertion with proper validation.The function properly validates the cover image exists and is of type 'image' before insertion, uses bcrypt for password hashing, and handles duplicate album names gracefully by catching
IntegrityErrorand returningFalse.
140-196: LGTM: Well-designed update logic with clear password semantics.The function properly handles the three password cases (None = no change, "" = remove protection, value = set/update), validates the cover image, and uses dynamic SQL with parameterized queries to prevent SQL injection. This addresses the concerns from the previous review.
211-229: LGTM: Clean media retrieval with proper ordering.The function correctly retrieves media items with appropriate ordering (sort_order ascending, then added_at descending) and returns dictionaries for API consistency.
292-304: LGTM: Secure password verification.The function correctly uses
bcrypt.checkpw()for secure password comparison and properly handles cases where no password is set.
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/database/albums.py (1)
3-3: Remove unusedUnionimport.The
Uniontype is imported but never used in this file. All type hints useOptionalor direct types instead.Apply this diff:
-from typing import List, Tuple, Optional, Union +from typing import List, Tuple, Optional
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/albums.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/database/albums.py (1)
backend/app/database/images.py (1)
_connect(45-49)
🔇 Additional comments (5)
backend/app/database/albums.py (5)
63-138: LGTM!The CRUD operations are well-structured with:
- Consistent use of
_connect()for connection management- Proper
row_factoryusage for dictionary returns- Appropriate type hints (
Optional[dict],bool)- Clean error handling for duplicate album names
- Cover image validation ensuring it references an actual image
140-196: LGTM!The update function correctly implements:
- Password semantics (None=keep, ""=remove, string=update) as documented
- Cover image validation matching insert logic
- Automatic timestamp updates
- Parameterized queries preventing SQL injection
- Appropriate error logging and boolean return
198-229: LGTM!Both functions are well-implemented:
db_delete_albumcorrectly returns a boolean indicating success (past type hint issue resolved)db_get_album_mediaproperly orders results bysort_orderfor consistent display- Connection management and cleanup are handled correctly
231-278: LGTM with a note on concurrent access.The media addition logic is well-structured and addresses all past review concerns:
- Album existence is validated upfront
- Media type matching prevents inconsistencies
- IntegrityError handling is now specific to duplicates
- Sort ordering maintains display sequence
Minor observation: The sort order calculation (line 263:
current_max_sort + i + 1) could theoretically produce gaps if concurrent calls interleave, but this is not a practical concern for SQLite's single-writer model in this context.
280-304: LGTM!Both functions are correctly implemented:
db_remove_media_from_albumperforms straightforward deletion with proper parameterizationverify_album_passworduses secure bcrypt comparison and handles missing/null passwords appropriately- Connection management follows the established pattern
|
|
Sanjayazhagan
force-pushed
the
feature/albums-backend
branch
from
dac7d84 to
a828ccb
Compare
|
|
1 similar comment
|
|
Sanjayazhagan
force-pushed
the
feature/albums-backend
branch
from
a828ccb to
9fa9a0e
Compare
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/routes/albums.py (2)
71-90: Handledb_insert_albumreturningFalse.
db_insert_albumreturnsFalsewhen thecover_image_idis invalid or insertion fails, but this isn't checked. The endpoint will return success even when album creation fails.# FIXED: Passing all 6 arguments correctly - db_insert_album( + success = db_insert_album( album_id=album_id, album_name=body.name, description=body.description, cover_image_id=body.cover_image_id, is_hidden=body.is_hidden, password=body.password ) + if not success: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Album Creation Failed", + message="Failed to create album. Cover image may be invalid or not found.", + ).model_dump(), + ) return CreateAlbumResponse(success=True, album_id=album_id)
168-185: Handledb_update_albumreturningFalse.Similar to album creation, if
db_update_albumreturnsFalse(e.g., invalid cover image), the response still indicates success.# FIXED: Passing all arguments correctly - db_update_album( + success = db_update_album( album_id=album_id, album_name=current_name, description=current_desc, cover_image_id=current_cover, is_hidden=current_hidden, password=body.password ) + if not success: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="Update Failed", + message="Failed to update album. Cover image may be invalid.", + ).model_dump(), + ) return SuccessResponse(success=True, msg="Album updated successfully")backend/app/database/images.py (1)
100-114: Addmedia_typeto image records before bulk insert.The bulk insert now includes
media_typein the INSERT statement (line 108), butimage_util_prepare_image_records()at backend/app/utils/images.py:169 does not include this field when creating records. This will causecursor.executemany()to raise aKeyErrorfor the missing:media_typeparameter. Update the record dict to includemedia_type(e.g., derived from file extension or detected from file content).
🧹 Nitpick comments (8)
backend/app/schemas/album.py (1)
16-18: Consider validatingmedia_typevalues.The
media_typefield accepts any string, but the database constrains it to'image'or'video'. Adding validation here would catch invalid values earlier.+from typing import Literal + class MediaItem(BaseModel): # <--- NEW (For mixed media) media_id: str - media_type: str # 'image' or 'video' + media_type: Literal['image', 'video']backend/app/routes/albums.py (2)
141-145: Fallback logic is unnecessary for required fields.
body.nameandbody.is_hiddenare required inUpdateAlbumRequest(no Optional type, no default), so theis not Nonechecks are redundant. This could mask schema changes if fields become optional later.- # Use existing values if not provided in body - current_name = body.name if body.name is not None else album["album_name"] - current_desc = body.description if body.description is not None else album["description"] - current_cover = body.cover_image_id if body.cover_image_id is not None else album["cover_image_id"] - current_hidden = body.is_hidden if body.is_hidden is not None else bool(album["is_hidden"]) + # body.name and body.is_hidden are required; description and cover_image_id have defaults + current_name = body.name + current_desc = body.description if body.description is not None else album["description"] + current_cover = body.cover_image_id # None is valid (clears cover image) + current_hidden = body.is_hidden
299-321: Consider providing feedback when media item doesn't exist.
db_remove_media_from_albumdoesn't return whether any row was deleted. The endpoint returns success even ifmedia_idwas never in the album. This may be intentional for idempotency, but could also mask issues.docs/backend/backend_python/openapi.json (1)
1357-1372: Consider addingmaxItemsconstraint tomedia_itemsarray.The
media_itemsarray inAddMediaRequesthas no upper bound. For API safety and DoS prevention, consider adding a reasonablemaxItemslimit.This is flagged by static analysis (CKV_OPENAPI_21). While not critical, adding bounds improves API robustness:
"media_items": { "items": { "$ref": "#/components/schemas/MediaItem" }, "type": "array", - "title": "Media Items" + "title": "Media Items", + "maxItems": 1000 }Note: The limit should also be enforced in the route handler, not just in the OpenAPI spec.
backend/app/database/albums.py (4)
3-3: Remove unused import.
Unionis imported but not used anywhere in this file.Apply this diff:
-from typing import List, Tuple, Optional, Union +from typing import List, Tuple, Optional
113-118: Consider extracting duplicate validation logic.The cover image validation logic (checking if cover_image_id exists and is of type 'image') is duplicated in both
db_insert_album(lines 113-118) anddb_update_album(lines 157-162). Extract this into a helper function to improve maintainability.Add a helper function:
def _validate_cover_image(cursor: sqlite3.Cursor, cover_image_id: str) -> bool: """Validates that cover_image_id exists and is of type 'image'.""" cursor.execute("SELECT 1 FROM images WHERE id = ? AND media_type = 'image'", (cover_image_id,)) return cursor.fetchone() is not NoneThen use it in both functions:
if cover_image_id: - # We add "AND media_type = 'image'" to ensure it's not a video - cursor.execute("SELECT 1 FROM images WHERE id = ? AND media_type = 'image'", (cover_image_id,)) - if not cursor.fetchone(): + if not _validate_cover_image(cursor, cover_image_id): logger.error(f"Invalid cover_image_id (or not an image): {cover_image_id}") return False
173-182: Consider extracting password update logic.The password handling logic (lines 173-182) implements three distinct cases and is well-documented, but it adds complexity to the update function. Consider extracting this into a helper function that returns the SQL fragment and parameters to append.
Example helper:
def _build_password_update(password: Optional[str]) -> Tuple[List[str], List[Any]]: """Returns (sql_fragments, params) for password update logic.""" if password == "": return (["password_hash = NULL"], []) elif password: hashed = bcrypt.hashpw(password.encode('utf-8'), bcrypt.gensalt()).decode('utf-8') return (["password_hash = ?"], [hashed]) return ([], []) # password is None, no changeThis would simplify the main function and make the logic more testable.
280-290: Consider returning a boolean for consistency.Unlike other CRUD operations that return
bool, this function returnsNone. Consider returningbool(based onrowcount > 0) to allow callers to verify if the media was actually removed, matching the pattern used indb_delete_album.Apply this diff:
-def db_remove_media_from_album(album_id: str, media_id: str) -> None: +def db_remove_media_from_album(album_id: str, media_id: str) -> bool: conn = _connect() try: cursor = conn.cursor() cursor.execute( "DELETE FROM album_media WHERE album_id = ? AND media_id = ?", (album_id, media_id), ) + rows_affected = cursor.rowcount conn.commit() + return rows_affected > 0 finally: conn.close()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/database/albums.py(2 hunks)backend/app/database/images.py(7 hunks)backend/app/routes/albums.py(10 hunks)backend/app/schemas/album.py(3 hunks)backend/main.py(0 hunks)backend/tests/conftest.py(1 hunks)backend/tests/test_albums.py(14 hunks)docs/backend/backend_python/openapi.json(13 hunks)
💤 Files with no reviewable changes (1)
- backend/main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/routes/albums.py (2)
backend/app/schemas/album.py (8)
GetAlbumsResponse(58-60)CreateAlbumRequest(24-35)UpdateAlbumRequest(37-49)SuccessResponse(74-76)ErrorResponse(78-81)Album(7-14)AddMediaRequest(51-52)GetAlbumMediaResponse(70-72)backend/app/database/albums.py (10)
db_get_all_albums(63-78)db_get_album_by_name(91-100)db_get_album(80-89)db_insert_album(102-138)db_update_album(140-196)db_delete_album(198-207)db_get_album_media(211-229)db_add_media_to_album(231-278)db_remove_media_from_album(280-290)verify_album_password(292-304)
backend/app/database/albums.py (1)
backend/app/database/images.py (1)
_connect(46-50)
🪛 Checkov (3.2.334)
docs/backend/backend_python/openapi.json
[medium] 1359-1365: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (21)
backend/app/database/images.py (4)
21-31: LGTM! TypedDict correctly extended with media_type field.The
ImageRecordTypedDict properly includes the newmedia_type: strfield to match the database schema changes.
67-69: LGTM! Schema change with appropriate constraint.The
media_typecolumn with default'image'and CHECK constraint ensures data integrity and backward compatibility with existing records.
143-145: LGTM! Media type correctly propagated through query results.The
media_typefield is properly selected, unpacked, and included in the returned image dictionaries.Also applies to: 163-191
398-420: No functional changes detected.The
db_toggle_image_favourite_statusfunction has only a minor formatting change with no impact on behavior.backend/tests/test_albums.py (5)
23-34: LGTM! Fixtures correctly updated with new album fields.The
mock_db_albumfixture now includescover_image_id,created_at, andupdated_atfields matching the updated database schema.
59-95: LGTM! Album creation tests properly updated.Test parametrization correctly includes
cover_image_idand validates UUID format via exception on invalid values.
97-115: LGTM! Duplicate name test updated for dict-based returns.The mock correctly returns a dictionary to match the updated
db_get_album_by_namebehavior.
249-271: LGTM! Update album tests well-structured with parameterization.The approach of copying the fixture and applying overrides cleanly handles the three test scenarios for album updates.
295-379: LGTM! Media management tests correctly refactored.Tests properly cover the new media-centric endpoints (
/mediainstead of/images) with updated request bodies (media_itemswithmedia_idandmedia_type) and response assertions.backend/app/schemas/album.py (2)
7-14: LGTM! Album model properly extended.New optional fields
cover_image_id,created_at, andupdated_atmaintain backward compatibility while supporting the enhanced functionality.
51-81: LGTM! Request/response schemas properly defined.
AddMediaRequestandGetAlbumMediaResponsecorrectly model the media-centric API payloads.backend/app/routes/albums.py (2)
36-53: LGTM! Get albums correctly maps dict results to Album models.The route properly handles dict-based DB returns and maps all fields including the new
cover_image_id,created_at, andupdated_at.
213-257: LGTM! Password-protected media retrieval properly implemented.Using query parameters for password in GET requests is the correct approach. The password validation flow correctly protects hidden albums.
docs/backend/backend_python/openapi.json (2)
623-772: LGTM! OpenAPI paths correctly updated for media endpoints.The endpoint definitions for
/albums/{album_id}/media(GET, POST) and/albums/{album_id}/media/{media_id}(DELETE) are properly documented with correct parameters, request bodies, and response schemas.
1373-1432: LGTM! New schemas properly defined.The
Album,MediaItem, and related schemas correctly reflect the new fields and structure for the media-centric API.Also applies to: 2243-2260
backend/app/database/albums.py (6)
16-59: LGTM! Well-designed schema migration.The table definitions properly implement the migration from
album_imagestoalbum_mediawith:
- Appropriate foreign key constraints and cascade behaviors
- CHECK constraint ensuring valid media types
- UNIQUE constraint preventing duplicate album-media associations
- Proper timestamp columns with defaults
The error handling and logging provide good observability during startup.
63-100: LGTM! Clean retrieval API.The album retrieval functions correctly:
- Use row_factory for convenient dict-based access
- Include proper type hints
- Handle resource cleanup in finally blocks
- Sort by created_at for user-friendly ordering
198-207: LGTM!The delete function correctly returns a boolean indicating whether a row was deleted, and relies on CASCADE constraints to clean up related album_media entries.
211-229: LGTM! Appropriate media retrieval logic.The function correctly retrieves media items with proper ordering (sort_order takes precedence, then chronological by added_at).
231-278: LGTM! Robust media addition with proper validation.The function correctly implements all the fixes from previous reviews:
- Explicit album existence check (lines 238-241) prevents FK violations
- Media type validation ensures media_id exists AND type matches (lines 252-256)
- IntegrityError handling is now strictly for duplicates after validation
The sort_order management and error handling are appropriate.
292-304: LGTM! Secure password verification.The function correctly uses bcrypt for password verification and handles both public albums (no password_hash) and missing albums appropriately.
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/app/schemas/album.py (2)
78-81: Consider consolidating duplicate ErrorResponse across schema modules.The
ErrorResponsemodel is duplicated inbackend/app/schemas/images.py,backend/app/schemas/facetagging.py, andbackend/app/schemas/test.py. Centralizing this in a shared schema module would reduce duplication and ease maintenance.
16-18: Add validation for media_type field.The
media_typefield accepts any string, but the database and routes expect only'image'or'video'. Adding a field validator or using a Literal type would catch invalid values at request time rather than during database operations.Apply this diff to add validation:
class MediaItem(BaseModel): # <--- NEW (For mixed media) media_id: str - media_type: str # 'image' or 'video' + media_type: str + + @field_validator("media_type") + def validate_media_type(cls, value): + if value not in ('image', 'video'): + raise ValueError("media_type must be 'image' or 'video'") + return value
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/database/albums.py(2 hunks)backend/app/database/images.py(7 hunks)backend/app/routes/albums.py(10 hunks)backend/app/schemas/album.py(3 hunks)backend/main.py(0 hunks)backend/tests/conftest.py(1 hunks)backend/tests/test_albums.py(14 hunks)docs/backend/backend_python/openapi.json(13 hunks)
💤 Files with no reviewable changes (1)
- backend/main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/tests/test_albums.py (1)
backend/tests/test_folders.py (1)
client(76-78)
backend/app/routes/albums.py (2)
backend/app/schemas/album.py (7)
CreateAlbumRequest(24-35)CreateAlbumResponse(62-64)GetAlbumResponse(66-68)UpdateAlbumRequest(37-49)Album(7-14)AddMediaRequest(51-52)GetAlbumMediaResponse(70-72)backend/app/database/albums.py (8)
db_get_all_albums(63-78)db_get_album(80-89)db_insert_album(102-138)db_update_album(140-196)db_get_album_media(211-229)db_add_media_to_album(231-278)db_remove_media_from_album(280-290)verify_album_password(292-304)
backend/app/schemas/album.py (3)
backend/app/schemas/images.py (1)
ErrorResponse(59-62)backend/app/schemas/facetagging.py (1)
ErrorResponse(30-33)backend/app/schemas/test.py (1)
ErrorResponse(41-44)
backend/app/database/albums.py (1)
backend/app/database/images.py (1)
_connect(46-50)
🪛 Checkov (3.2.334)
docs/backend/backend_python/openapi.json
[medium] 1359-1365: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (23)
backend/app/database/images.py (3)
21-31: LGTM! TypedDict updated correctly.The addition of
media_type: strtoImageRecordaligns with the new database schema and supports the mixed media functionality.
68-68: Good use of CHECK constraint and default value.The
media_typecolumn correctly usesDEFAULT 'image'for backward compatibility and aCHECKconstraint to enforce valid values.
126-213: LGTM! Query and result handling updated consistently.The
media_typefield is correctly selected, unpacked, and included in the result dictionaries throughout the function.backend/tests/test_albums.py (3)
23-48: LGTM! Fixtures updated to match new schema.Both
mock_db_albumandmock_db_hidden_albumcorrectly includecover_image_id,created_at, andupdated_atfields consistent with the database changes.
78-95: LGTM! UUID validation ensures proper ID generation.The test correctly verifies that the returned
album_idis a valid UUID, confirming proper ID generation in the create flow.
295-379: LGTM! Media management tests properly cover new endpoints.The test suite correctly validates adding, retrieving, and removing media items using the new media-centric API structure.
backend/app/routes/albums.py (4)
36-53: LGTM! Endpoint correctly uses dict-based album data.The
get_albumsfunction properly constructsAlbumobjects from dictionary responses, including all new fields.
57-90: LGTM! Album creation correctly passes all required arguments.The fix ensures all six database arguments are passed, addressing the 500 error mentioned in the PR summary.
128-185: LGTM! Update endpoint uses dict access consistently.The function correctly retrieves current album values from the dictionary and passes all arguments to the database update function.
213-321: LGTM! Media endpoints correctly implement the new API.The endpoints properly:
- Use query parameters for password on GET requests (appropriate for GET)
- Convert
MediaItemobjects to tuples for database operations- Return media-centric responses with
media_itemsstructuredocs/backend/backend_python/openapi.json (4)
1357-1372: LGTM! OpenAPI schemas correctly define media structures.The
AddMediaRequestandMediaItemschemas properly reflect the media-centric API changes.Also applies to: 2243-2260
1373-1433: LGTM! Album schema correctly includes new fields.The
cover_image_id,created_at, andupdated_atfields are properly defined as optional strings.
623-773: LGTM! OpenAPI paths correctly reflect media endpoints.The endpoint definitions properly document the media-centric API with appropriate operation IDs and parameter specifications.
1357-1372: Static analysis hint about maxItems is a false positive.The Checkov warning suggests adding a maximum number of items to the
media_itemsarray. However, there's no business requirement to artificially limit the number of media items in an album. This warning can be safely ignored.backend/app/database/albums.py (9)
10-14: LGTM! Centralized connection helper with foreign keys enabled.The
_connect()function properly enables foreign key constraints and provides a consistent connection pattern across all database operations.
16-59: LGTM! Table schemas correctly support mixed media and timestamps.The table definitions properly:
- Add cover image support with FK constraint
- Include created_at/updated_at timestamps
- Create album_media junction table with UNIQUE constraint
- Use CHECK constraint for valid media_type values
63-100: LGTM! Retrieval functions correctly return dictionaries.The functions properly use
sqlite3.Rowfor column-name access and return dictionary structures consistent with the refactored API.
102-138: LGTM! Album insertion with proper validation.The function correctly:
- Validates that cover_image_id exists and is an image (not video)
- Hashes passwords securely with bcrypt
- Returns boolean to indicate success/failure
- Logs integrity errors for diagnostics
140-196: LGTM! Update operation with clear password semantics.The password handling logic is well-documented and correct:
None: preserve existing password"": remove password protection- Non-empty string: hash and set new password
The cover image validation ensures consistency with the images table.
198-207: LGTM! Delete operation correctly returns boolean.The function properly returns
Truewhen an album was deleted andFalseotherwise.
211-229: LGTM! Media retrieval with proper sorting.The function correctly returns media items sorted by
sort_orderASC andadded_atDESC, maintaining user-defined ordering.
231-278: LGTM! Media addition with comprehensive validation.The function properly:
- Checks album existence before processing (addresses previous error handling issue)
- Validates media_type is 'image' or 'video'
- Verifies media exists in images table with matching media_type (addresses previous validation issue)
- Handles duplicates gracefully via UNIQUE constraint
- Maintains sort order correctly
- Returns count of successfully added items
280-290: LGTM! Media removal is straightforward.The function correctly deletes the junction table entry for the specified album and media item.
2 participants
Title
Feat(backend): Upgrade Albums to support Videos & Cover Images progress on #786
Description
This PR upgrades the Albums feature to support mixed media (Videos + Images) and adds cover image functionality, resolving previous schema limitations. It includes a complete refactor of the album database tables, API routes, Pydantic schemas, and unit tests.
Key Changes
Database Schema
album_imagestable toalbum_mediato support mixed content.media_typecolumn to support'video'and'image'.cover_image_id,created_at, andupdated_atcolumns to thealbumstable.API Router (
routes/albums.py)500 Internal Server Erroron creation by ensuring all 6 DB arguments are passed correctly.GETrequests to use Query Parameters for passwords instead of Request Body (adhering to REST best practices).Schemas (
schemas/album.py)MediaItemmodel to handle mixed media types.Testing (
tests/test_albums.py)conftest.pyandtest_albums.pyto match the new dictionary-based database returns.Testing Strategy
POST /albums/(Creation) with and without cover images.GET /albums/{id}/mediafor password-protected albums.pytest.Notes for Reviewers
album_imagestable was renamed toalbum_media, existingdatabase.dbfiles will need to be deleted/reset for the changes to apply correctly in local development environments.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.