refactor(albums): improve SQLite error handling and logging - #1414
refactor(albums): improve SQLite error handling and logging#1414Dotify71 wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAlbum database helpers now use logged connection contexts for SQLite operations. Album routes centralize unexpected-exception handling while preserving explicit status mappings, and tests update database patching, bcrypt inputs, type annotations, and HTTP 500 coverage. ChangesAlbum Error Handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AlbumRoute
participant AlbumDatabase
Client->>AlbumRoute: send album or image request
AlbumRoute->>AlbumDatabase: invoke database helper
AlbumDatabase-->>AlbumRoute: return data or raise exception
AlbumRoute-->>Client: return structured HTTP response
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (2)
backend/app/database/albums.py (2)
7-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant logger comments.
backend/app/database/albums.py#L7-L8: remove# Initialize logger.backend/app/routes/albums.py#L31-L32: remove# Initialize logger.As per path instructions, “Point out redundant obvious comments that do not add clarity.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/albums.py` around lines 7 - 8, Remove the redundant “Initialize logger” comments above the logger declarations in backend/app/database/albums.py lines 7-8 and backend/app/routes/albums.py lines 31-32; leave the logger declarations unchanged.Source: Path instructions
51-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd concrete return annotations to the album lookup helpers.
get_db_connection()returns a default SQLite connection, sodb_get_all_albums()returnslist[tuple], whiledb_get_album_by_name()anddb_get_album()returntuple | Nonefor thealbumsrow shape. Annotate these explicitly instead of returning barefetchall()/fetchone()rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/albums.py` around lines 51 - 86, The album lookup helpers lack explicit return types. Update db_get_all_albums to return list[tuple], and annotate db_get_album_by_name and db_get_album as returning tuple | None, preserving their existing query and result behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/database/albums.py`:
- Around line 26-28: Add automated route-level tests around the albums handlers
in backend/app/routes/albums.py (lines 53-64) covering unexpected exceptions
returning structured 500 responses and explicit HTTPException cases such as 401,
404, and 409 preserving their original status and detail; retain the existing
sqlite3.Error cleanup behavior in backend/app/database/albums.py (lines 26-28),
which requires no direct change.
In `@backend/app/routes/albums.py`:
- Around line 55-64: Stop exposing str(e) in the ErrorResponse messages for all
listed exception handlers in backend/app/routes/albums.py:55-64, 87-90, 121-124,
183-186, 212-215, 274-277, 315-318, 346-349, and 389-392. Keep the detailed
exceptions in logger.error, but replace each client-facing message with its
fixed operation-specific fetch, create, update, delete, image-fetch, image-add,
image-removal, or bulk-removal failure message.
---
Nitpick comments:
In `@backend/app/database/albums.py`:
- Around line 7-8: Remove the redundant “Initialize logger” comments above the
logger declarations in backend/app/database/albums.py lines 7-8 and
backend/app/routes/albums.py lines 31-32; leave the logger declarations
unchanged.
- Around line 51-86: The album lookup helpers lack explicit return types. Update
db_get_all_albums to return list[tuple], and annotate db_get_album_by_name and
db_get_album as returning tuple | None, preserving their existing query and
result behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 48121a55-f156-48da-88ab-10ab0b70fff7
📒 Files selected for processing (2)
backend/app/database/albums.pybackend/app/routes/albums.py
e582865 to
d41adb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
backend/app/database/albums.py (1)
11-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated try/except/log/raise boilerplate.
All 13 functions repeat the identical
try: ... except sqlite3.Error as e: logger.error(...); raiseshape, differing only in the log message. A small decorator or a context manager wrappingget_db_connection()with logging could remove this duplication and make it trivial to apply the specific-exception fix above consistently.♻️ Sketch
from contextlib import contextmanager `@contextmanager` def logged_db_connection(action: str): try: with get_db_connection() as conn: yield conn except sqlite3.Error as e: logger.error(f"Error {action}: {e}") raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/albums.py` around lines 11 - 256, Extract the repeated SQLite error-handling in all database functions into a shared helper, preferably a context manager wrapping get_db_connection() that accepts the operation-specific action and logs sqlite3.Error before re-raising. Update db_create_albums_table, db_create_album_images_table, db_get_all_albums, db_get_album_by_name, db_get_album, db_insert_album, db_update_album, db_delete_album, db_get_album_images, db_add_images_to_album, db_remove_image_from_album, db_remove_images_from_album, and verify_album_password to use it while preserving their existing contextual log messages.backend/app/routes/albums.py (1)
41-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated
except HTTPException: raise / except Exception: ... 500boilerplate across 9 handlers.Every handler repeats the same two-clause exception boundary. A small decorator/dependency wrapping route bodies (log + convert to the appropriate
ErrorResponse) would remove this duplication and make it easier to enforce the exception-type distinctions raised above consistently.Also applies to: 71-99, 105-133, 141-197, 203-228, 239-292, 300-337, 343-370, 378-414
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/routes/albums.py` around lines 41 - 65, Introduce a shared route exception wrapper for get_albums and the other affected handlers that re-raises HTTPException unchanged, logs unexpected exceptions, and converts them to the existing 500 ErrorResponse structure. Apply the wrapper consistently to all nine handlers and remove their duplicated try/except boundaries, preserving each route’s existing success behavior and error message context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/database/albums.py`:
- Around line 24-26: Replace the generic sqlite3.Error handlers throughout this
file with specific sqlite3.IntegrityError and sqlite3.OperationalError handling,
including the album insertion flow in db_insert_album and every other listed
database operation. Preserve the existing logging and re-raise behavior while
ensuring UNIQUE/constraint violations remain distinguishable from operational
database failures.
- Around line 1-256: Add `from __future__ import annotations` at the beginning
of the albums database module, before imports and before signatures such as
`db_get_album_by_name`, `db_get_album`, `db_insert_album`, and `db_update_album`
that use PEP 604 annotations; leave the existing function behavior unchanged.
In `@backend/app/routes/albums.py`:
- Around line 295-338: Update add_images_to_album to catch the ValueError and
TypeError raised by db_add_images_to_album for invalid or nonexistent image IDs
and translate them into a 400 HTTPException using the route’s existing
ErrorResponse structure; leave unexpected exceptions on the generic 500 path.
- Around line 340-371: Update remove_image_from_album to catch the ValueError
raised by db_remove_image_from_album for a missing image and translate it into
the existing 404 response pattern, while preserving the generic 500 handling for
unexpected exceptions.
- Around line 68-100: Update create_album to catch the sqlite3.IntegrityError
raised by db_insert_album when the unique album-name constraint loses a
concurrent race, and map that case to the same 409 Conflict response used for an
existing album. Preserve the generic 500 handling for unrelated exceptions and
retain the existing pre-insert duplicate check.
- Around line 136-198: Update the unexpected-exception fallback in update_album
to raise HTTP_500_INTERNAL_SERVER_ERROR instead of HTTP_400_BAD_REQUEST, while
preserving the existing HTTPException handling, logging, and error response.
---
Nitpick comments:
In `@backend/app/database/albums.py`:
- Around line 11-256: Extract the repeated SQLite error-handling in all database
functions into a shared helper, preferably a context manager wrapping
get_db_connection() that accepts the operation-specific action and logs
sqlite3.Error before re-raising. Update db_create_albums_table,
db_create_album_images_table, db_get_all_albums, db_get_album_by_name,
db_get_album, db_insert_album, db_update_album, db_delete_album,
db_get_album_images, db_add_images_to_album, db_remove_image_from_album,
db_remove_images_from_album, and verify_album_password to use it while
preserving their existing contextual log messages.
In `@backend/app/routes/albums.py`:
- Around line 41-65: Introduce a shared route exception wrapper for get_albums
and the other affected handlers that re-raises HTTPException unchanged, logs
unexpected exceptions, and converts them to the existing 500 ErrorResponse
structure. Apply the wrapper consistently to all nine handlers and remove their
duplicated try/except boundaries, preserving each route’s existing success
behavior and error message context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0588f46-07df-42cc-884b-4d683dc91d38
📒 Files selected for processing (2)
backend/app/database/albums.pybackend/app/routes/albums.py
d41adb7 to
e564fdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/app/routes/albums.py (1)
38-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the decorator factory.
The new factory, nested decorator, and wrapper are untyped. Annotate the callable contract while preserving the wrapped route signature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/routes/albums.py` around lines 38 - 59, The handle_route_exceptions decorator factory and its nested decorator/wrapper are missing callable type annotations. Add appropriate generic ParamSpec and TypeVar annotations to decorator, func, args, kwargs, and the returned callable so the wrapped route’s signature is preserved.Source: Path instructions
backend/tests/test_albums.py (1)
477-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the database-enforced duplicate-create path.
This only tests generic 500 conversion. Add a test where
db_get_album_by_namereturnsNoneanddb_insert_albumraisessqlite3.IntegrityError, then assert the route returns 409. The existing duplicate test exits before insertion.As per path instructions, ensure critical functionality is covered by automated tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_albums.py` around lines 477 - 490, Extend TestAlbumRouteErrors with a test for the database-enforced duplicate-create path: patch db_get_album_by_name to return None, make db_insert_album raise sqlite3.IntegrityError, call the album creation route with valid input, and assert the response status is 409. Keep the existing generic exception test unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/routes/albums.py`:
- Around line 201-204: Update the album update handler around db_update_album to
catch sqlite3.IntegrityError caused by a duplicate album name and return the
same 409 response used by create_album, instead of allowing the decorator to
convert it to 500. Preserve the existing successful update response for
non-conflicting updates.
---
Nitpick comments:
In `@backend/app/routes/albums.py`:
- Around line 38-59: The handle_route_exceptions decorator factory and its
nested decorator/wrapper are missing callable type annotations. Add appropriate
generic ParamSpec and TypeVar annotations to decorator, func, args, kwargs, and
the returned callable so the wrapped route’s signature is preserved.
In `@backend/tests/test_albums.py`:
- Around line 477-490: Extend TestAlbumRouteErrors with a test for the
database-enforced duplicate-create path: patch db_get_album_by_name to return
None, make db_insert_album raise sqlite3.IntegrityError, call the album creation
route with valid input, and assert the response status is 409. Keep the existing
generic exception test unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d1df77d6-c282-4c18-87e3-183c93465509
📒 Files selected for processing (4)
backend/app/database/albums.pybackend/app/routes/albums.pybackend/tests/test_albums.pybackend/tests/test_albums_db.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/database/albums.py
|
Everything is set and the pr is ready to review or merge. :)) |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
1 participant
Description
This PR addresses issue #1340 by refactoring the SQLite error handling and integrating the standard logger within the albums database layer.
Changes made:
sqlite3.Errorcatching with specific exceptions (sqlite3.IntegrityError,sqlite3.OperationalError, etc.) inbackend/app/database/albums.py.backend/app/utils/logger.pyto appropriately log database errors, ensuring better traceability and consistency with the rest of the database modules.Closes #1340
Checklist
Summary by CodeRabbit
500.409 Conflictfor integrity/duplicate violations.400 Bad Requestfor invalid input; removing images returns404 Not Foundwhen the image isn’t in the album (with the mapped message).500.