Fix: Standardize database connections using context manager in images.py #643
Conversation
WalkthroughA refactoring that replaces direct sqlite3 connection management with a centralized context manager ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (2)
backend/app/database/images.py (2)
201-241: Consider consistent error handling across read operations.Unlike other read functions (e.g.,
db_get_all_imagesanddb_get_images_by_folder_ids), this function lacks a try-except block to catch and log database errors. If an exception occurs, it will propagate to the caller instead of returning an empty list.Apply this diff to add consistent error handling:
def db_get_untagged_images() -> List[UntaggedImageRecord]: """ Find all images that need AI tagging. Returns images where: - The image's folder has AI_Tagging enabled (True) - The image has isTagged set to False Returns: List of dictionaries containing image data: id, path, folder_id, thumbnailPath, metadata """ + try: - with get_db_connection() as conn: - cursor = conn.cursor() - - cursor.execute( - """ - SELECT i.id, i.path, i.folder_id, i.thumbnailPath, i.metadata - FROM images i - JOIN folders f ON i.folder_id = f.folder_id - WHERE f.AI_Tagging = TRUE - AND i.isTagged = FALSE - """ - ) - - results = cursor.fetchall() - - untagged_images = [] - for image_id, path, folder_id, thumbnail_path, metadata in results: - from app.utils.images import image_util_parse_metadata - - md = image_util_parse_metadata(metadata) - untagged_images.append( - { - "id": image_id, - "path": path, - "folder_id": str(folder_id) if folder_id is not None else None, - "thumbnailPath": thumbnail_path, - "metadata": md, - } - ) - - return untagged_images + with get_db_connection() as conn: + cursor = conn.cursor() + + cursor.execute( + """ + SELECT i.id, i.path, i.folder_id, i.thumbnailPath, i.metadata + FROM images i + JOIN folders f ON i.folder_id = f.folder_id + WHERE f.AI_Tagging = TRUE + AND i.isTagged = FALSE + """ + ) + + results = cursor.fetchall() + + untagged_images = [] + for image_id, path, folder_id, thumbnail_path, metadata in results: + from app.utils.images import image_util_parse_metadata + + md = image_util_parse_metadata(metadata) + untagged_images.append( + { + "id": image_id, + "path": path, + "folder_id": str(folder_id) if folder_id is not None else None, + "thumbnailPath": thumbnail_path, + "metadata": md, + } + ) + + return untagged_images + except Exception as e: + logger.error(f"Error getting untagged images: {e}") + return []
363-381: Consider simplifying by removing the existence check.The SELECT at line 367 checks if the image exists before toggling, but the subsequent UPDATE will affect 0 rows if the image doesn't exist anyway. Both approaches return the same result, so the SELECT adds an extra database round-trip without changing behavior.
Apply this diff to simplify the function:
def db_toggle_image_favourite_status(image_id: str) -> bool: try: with get_db_connection() as conn: cursor = conn.cursor() - cursor.execute("SELECT id FROM images WHERE id = ?", (image_id,)) - if not cursor.fetchone(): - return False cursor.execute( """ UPDATE images SET isFavourite = CASE WHEN isFavourite = 1 THEN 0 ELSE 1 END WHERE id = ? """, (image_id,), ) return cursor.rowcount > 0 except Exception as e: logger.error(f"Database error: {e}") return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/database/images.py(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Hemil36
Repo: AOSSIE-Org/PictoPy PR: 570
File: backend/app/database/connection.py:16-24
Timestamp: 2025-10-31T17:00:50.132Z
Learning: In PictoPy backend, the user prefers not to use database connection retry logic or extended busy timeouts in the centralized get_db_connection() context manager, even though the app has concurrent access patterns via ProcessPoolExecutor and FastAPI.
🧬 Code graph analysis (1)
backend/app/database/images.py (2)
backend/app/database/connection.py (1)
get_db_connection(8-32)backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
🔇 Additional comments (8)
backend/app/database/images.py (8)
9-9: LGTM! Good refactoring foundation.The import of
get_db_connectionenables consistent database connection management with automatic transaction handling and constraint enforcement across all database operations.
46-78: LGTM! Table creation is transactionally sound.Both CREATE TABLE statements execute within a single transaction via the context manager, ensuring atomic schema creation. Foreign key constraints with CASCADE are properly defined.
80-106: LGTM! Bulk insert with UPSERT logic is correct.The context manager ensures the entire batch is committed atomically. The UPSERT logic properly handles conflicts, and the early return at line 103 still allows the context manager to commit the transaction.
109-198: LGTM! Query construction and result grouping are sound.The dynamic query building with parameterized WHERE clause is safe. Result grouping by image ID and tag aggregation logic correctly construct the final image dictionaries with associated tags.
244-265: LGTM! Simple UPDATE with appropriate rowcount check.The function correctly updates the tagged status and returns whether any rows were affected, providing feedback about whether the image existed.
268-294: LGTM! Batch insert with deduplication is correct.The
INSERT OR IGNOREstatement appropriately handles duplicate image-class pairs in the junction table. The context manager ensures atomic batch insertion.
297-329: LGTM! Dynamic IN clause construction is safe.The f-string usage at line 319 constructs the placeholder string (
?,?,?...) from the list length, not from user input. Actualfolder_idsvalues are safely passed as parameters toexecute(), preventing SQL injection.
332-360: LGTM! Batch deletion with CASCADE is correctly implemented.The dynamic IN clause construction is safe (same pattern as in
db_get_images_by_folder_ids). The context manager ensures atomic deletion, and CASCADE constraints will automatically clean up related records inimage_classes.
|
@rahulharpal1603 can you take a look |
|
Already addressed in PR #607 |
2 participants
Related issue #641
This pull request refactors the image database access layer to use a context-managed database connection, improving resource management and error handling. The main change is replacing the custom
_connect()function with theget_db_connection()context manager, ensuring connections are properly closed and transactions are handled more safely. Additionally, explicitcommit(),rollback(), andclose()calls are removed in favor of relying on the context manager's behavior.Database connection management:
_connect()function with theget_db_connection()context manager throughout all image database functions, ensuring connections are automatically closed and foreign key constraints are enforced. [1] [2]commit(),rollback(), andclose()calls in favor of context-managed transactions, simplifying error handling and reducing boilerplate code. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Error handling improvements:
These changes make the database layer more robust, maintainable, and less error-prone.
Summary by CodeRabbit