From e564fdb69c1c9963d9fc8b4d47a7e58f641ca6af Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Wed, 24 Jun 2026 22:08:06 +0530 Subject: [PATCH 01/15] fix(albums): improve SQLite error handling and logging in database layer and routes --- backend/app/database/albums.py | 372 +++++++++++++------------- backend/app/routes/albums.py | 460 ++++++++++++++++++-------------- backend/tests/test_albums.py | 28 +- backend/tests/test_albums_db.py | 21 +- 4 files changed, 478 insertions(+), 403 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 2db8df4e5..0dd848356 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -1,87 +1,85 @@ import sqlite3 + import bcrypt -from app.config.settings import DATABASE_PATH + from app.database.connection import get_db_connection +from app.logging.setup_logging import get_logger + +logger = get_logger(__name__) def db_create_albums_table() -> None: - conn = None try: - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() - cursor.execute( - """ - CREATE TABLE IF NOT EXISTS albums ( - album_id TEXT PRIMARY KEY, - album_name TEXT UNIQUE, - description TEXT, - is_hidden BOOLEAN DEFAULT 0, - password_hash TEXT - ) - """ - ) - conn.commit() - finally: - if conn is not None: - conn.close() + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute(""" + CREATE TABLE IF NOT EXISTS albums ( + album_id TEXT PRIMARY KEY, + album_name TEXT UNIQUE, + description TEXT, + is_hidden BOOLEAN DEFAULT 0, + password_hash TEXT + ) + """) + except sqlite3.Error as e: + logger.error(f"Error creating albums table: {e}") + raise def db_create_album_images_table() -> None: - conn = None try: - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() - cursor.execute( - """ - CREATE TABLE IF NOT EXISTS album_images ( - album_id TEXT, - image_id TEXT, - PRIMARY KEY (album_id, image_id), - FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, - FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE - ) - """ - ) - conn.commit() - finally: - if conn is not None: - conn.close() - - -def db_get_all_albums(show_hidden: bool = False): - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute(""" + CREATE TABLE IF NOT EXISTS album_images ( + album_id TEXT, + image_id TEXT, + PRIMARY KEY (album_id, image_id), + FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, + FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE + ) + """) + except sqlite3.Error as e: + logger.error(f"Error creating album_images table: {e}") + raise + + +def db_get_all_albums(show_hidden: bool = False) -> list[tuple]: try: - if show_hidden: - cursor.execute("SELECT * FROM albums") - else: - cursor.execute("SELECT * FROM albums WHERE is_hidden = 0") - albums = cursor.fetchall() - return albums - finally: - conn.close() - - -def db_get_album_by_name(name: str): - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() + with get_db_connection() as conn: + cursor = conn.cursor() + if show_hidden: + cursor.execute("SELECT * FROM albums") + else: + cursor.execute("SELECT * FROM albums WHERE is_hidden = 0") + return cursor.fetchall() + except sqlite3.Error as e: + logger.error(f"Error getting all albums: {e}") + raise + + +def db_get_album_by_name(name: str) -> tuple | None: try: - cursor.execute("SELECT * FROM albums WHERE album_name = ?", (name,)) - album = cursor.fetchone() - return album if album else None - finally: - conn.close() + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute("SELECT * FROM albums WHERE album_name = ?", (name,)) + album = cursor.fetchone() + return album if album else None + except sqlite3.Error as e: + logger.error(f"Error getting album by name '{name}': {e}") + raise -def db_get_album(album_id: str): - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() +def db_get_album(album_id: str) -> tuple | None: try: - cursor.execute("SELECT * FROM albums WHERE album_id = ?", (album_id,)) - album = cursor.fetchone() - return album if album else None - finally: - conn.close() + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute("SELECT * FROM albums WHERE album_id = ?", (album_id,)) + album = cursor.fetchone() + return album if album else None + except sqlite3.Error as e: + logger.error(f"Error getting album '{album_id}': {e}") + raise def db_insert_album( @@ -89,26 +87,26 @@ def db_insert_album( album_name: str, description: str = "", is_hidden: bool = False, - password: str = None, + password: str | None = None, ): - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() try: - password_hash = None - if password: - password_hash = bcrypt.hashpw( - password.encode("utf-8"), bcrypt.gensalt() - ).decode("utf-8") - cursor.execute( - """ - INSERT INTO albums (album_id, album_name, description, is_hidden, password_hash) - VALUES (?, ?, ?, ?, ?) - """, - (album_id, album_name, description, int(is_hidden), password_hash), - ) - conn.commit() - finally: - conn.close() + with get_db_connection() as conn: + cursor = conn.cursor() + password_hash = None + if password: + password_hash = bcrypt.hashpw( + password.encode("utf-8"), bcrypt.gensalt() + ).decode("utf-8") + cursor.execute( + """ + INSERT INTO albums (album_id, album_name, description, is_hidden, password_hash) + VALUES (?, ?, ?, ?, ?) + """, + (album_id, album_name, description, int(is_hidden), password_hash), + ) + except sqlite3.Error as e: + logger.error(f"Error inserting album '{album_name}': {e}") + raise def db_update_album( @@ -116,139 +114,143 @@ def db_update_album( album_name: str, description: str, is_hidden: bool, - password: str = None, + password: str | None = None, ): - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() try: - if password is not None: - # Update with new password - password_hash = bcrypt.hashpw( - password.encode("utf-8"), bcrypt.gensalt() - ).decode("utf-8") - cursor.execute( - """ - UPDATE albums - SET album_name = ?, description = ?, is_hidden = ?, password_hash = ? - WHERE album_id = ? - """, - (album_name, description, int(is_hidden), password_hash, album_id), - ) - else: - # Update without changing password - cursor.execute( - """ - UPDATE albums - SET album_name = ?, description = ?, is_hidden = ? - WHERE album_id = ? - """, - (album_name, description, int(is_hidden), album_id), - ) - conn.commit() - finally: - conn.close() + with get_db_connection() as conn: + cursor = conn.cursor() + if password is not None: + password_hash = bcrypt.hashpw( + password.encode("utf-8"), bcrypt.gensalt() + ).decode("utf-8") + cursor.execute( + """ + UPDATE albums + SET album_name = ?, description = ?, is_hidden = ?, password_hash = ? + WHERE album_id = ? + """, + (album_name, description, int(is_hidden), password_hash, album_id), + ) + else: + cursor.execute( + """ + UPDATE albums + SET album_name = ?, description = ?, is_hidden = ? + WHERE album_id = ? + """, + (album_name, description, int(is_hidden), album_id), + ) + except sqlite3.Error as e: + logger.error(f"Error updating album '{album_id}': {e}") + raise def db_delete_album(album_id: str): - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.execute("DELETE FROM albums WHERE album_id = ?", (album_id,)) + try: + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute("DELETE FROM albums WHERE album_id = ?", (album_id,)) + except sqlite3.Error as e: + logger.error(f"Error deleting album '{album_id}': {e}") + raise def db_get_album_images(album_id: str): - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() try: - cursor.execute( - "SELECT image_id FROM album_images WHERE album_id = ?", (album_id,) - ) - images = cursor.fetchall() - return [img[0] for img in images] - finally: - conn.close() + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT image_id FROM album_images WHERE album_id = ?", (album_id,) + ) + images = cursor.fetchall() + return [img[0] for img in images] + except sqlite3.Error as e: + logger.error(f"Error getting images for album '{album_id}': {e}") + raise def db_add_images_to_album(album_id: str, image_ids: list[str]): - """ - Safely adds images to an album using parameterized queries. - Maintains UUID support and uses efficient single queries. - """ - # Validate input type if not isinstance(image_ids, list): - raise ValueError("image_ids must be a list of IDs") + raise TypeError("image_ids must be a list of IDs") - # Remove integer conversion - keep IDs as strings for UUID support sanitized_ids = [] for img_id in image_ids: - # Basic validation - ensure it's a non-empty string if isinstance(img_id, str) and img_id.strip(): sanitized_ids.append(img_id.strip()) if not sanitized_ids: raise ValueError("No valid image IDs provided") - with get_db_connection() as conn: - cursor = conn.cursor() - - # Generate placeholders safely based on list length - placeholders = ",".join(["?"] * len(sanitized_ids)) - query = f"SELECT id FROM images WHERE id IN ({placeholders})" - cursor.execute(query, sanitized_ids) # Pass string IDs directly - valid_images = [row[0] for row in cursor.fetchall()] - - if not valid_images: - raise ValueError("None of the provided image IDs exist in the database.") - - # Insert into album_images using executemany - cursor.executemany( - "INSERT OR IGNORE INTO album_images (album_id, image_id) VALUES (?, ?)", - [(album_id, img_id) for img_id in valid_images], - ) - conn.commit() + try: + with get_db_connection() as conn: + cursor = conn.cursor() + + placeholders = ",".join(["?"] * len(sanitized_ids)) + query = f"SELECT id FROM images WHERE id IN ({placeholders})" + cursor.execute(query, sanitized_ids) + valid_images = [row[0] for row in cursor.fetchall()] + + if not valid_images: + raise ValueError( + "None of the provided image IDs exist in the database." + ) + + cursor.executemany( + "INSERT OR IGNORE INTO album_images (album_id, image_id) VALUES (?, ?)", + [(album_id, img_id) for img_id in valid_images], + ) + except sqlite3.Error as e: + logger.error(f"Error adding images to album '{album_id}': {e}") + raise def db_remove_image_from_album(album_id: str, image_id: str): - with get_db_connection() as conn: - cursor = conn.cursor() - - cursor.execute( - "SELECT 1 FROM album_images WHERE album_id = ? AND image_id = ?", - (album_id, image_id), - ) - exists = cursor.fetchone() + try: + with get_db_connection() as conn: + cursor = conn.cursor() - if exists: cursor.execute( - "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", + "SELECT 1 FROM album_images WHERE album_id = ? AND image_id = ?", (album_id, image_id), ) - else: - raise ValueError("Image not found in the specified album") + exists = cursor.fetchone() + + if exists: + cursor.execute( + "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", + (album_id, image_id), + ) + else: + raise ValueError("Image not found in the specified album") + except sqlite3.Error as e: + logger.error(f"Error removing image '{image_id}' from album '{album_id}': {e}") + raise def db_remove_images_from_album(album_id: str, image_ids: list[str]): - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() try: - cursor.executemany( - "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", - [(album_id, img_id) for img_id in image_ids], - ) - conn.commit() - finally: - conn.close() + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.executemany( + "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", + [(album_id, img_id) for img_id in image_ids], + ) + except sqlite3.Error as e: + logger.error(f"Error removing images from album '{album_id}': {e}") + raise def verify_album_password(album_id: str, password: str) -> bool: - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() try: - cursor.execute( - "SELECT password_hash FROM albums WHERE album_id = ?", (album_id,) - ) - row = cursor.fetchone() - if not row or not row[0]: - return False - return bcrypt.checkpw(password.encode("utf-8"), row[0].encode("utf-8")) - finally: - conn.close() + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT password_hash FROM albums WHERE album_id = ?", (album_id,) + ) + row = cursor.fetchone() + if not row or not row[0]: + return False + return bcrypt.checkpw(password.encode("utf-8"), row[0].encode("utf-8")) + except sqlite3.Error as e: + logger.error(f"Error verifying password for album '{album_id}': {e}") + raise diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index ae0408613..067528857 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -1,31 +1,36 @@ -from fastapi import APIRouter, HTTPException, status, Query, Body, Path import uuid -from app.schemas.album import ( - GetAlbumsResponse, - CreateAlbumRequest, - CreateAlbumResponse, - GetAlbumResponse, - GetAlbumImagesRequest, - GetAlbumImagesResponse, - UpdateAlbumRequest, - SuccessResponse, - ErrorResponse, - ImageIdsRequest, - Album, -) + +from fastapi import APIRouter, Body, HTTPException, Path, Query, status + from app.database.albums import ( - db_get_all_albums, - db_get_album_by_name, - db_get_album, - db_insert_album, - db_update_album, + db_add_images_to_album, db_delete_album, + db_get_album, + db_get_album_by_name, db_get_album_images, - db_add_images_to_album, + db_get_all_albums, + db_insert_album, db_remove_image_from_album, db_remove_images_from_album, + db_update_album, verify_album_password, ) +from app.logging.setup_logging import get_logger +from app.schemas.album import ( + Album, + CreateAlbumRequest, + CreateAlbumResponse, + ErrorResponse, + GetAlbumImagesRequest, + GetAlbumImagesResponse, + GetAlbumResponse, + GetAlbumsResponse, + ImageIdsRequest, + SuccessResponse, + UpdateAlbumRequest, +) + +logger = get_logger(__name__) router = APIRouter() @@ -33,47 +38,63 @@ # GET /albums/ - Get all albums @router.get("/", response_model=GetAlbumsResponse) def get_albums(show_hidden: bool = Query(False)): - albums = db_get_all_albums(show_hidden) - album_list = [] - for album in albums: - album_list.append( - Album( - album_id=album[0], - album_name=album[1], - description=album[2] or "", - is_hidden=bool(album[3]), + try: + albums = db_get_all_albums(show_hidden) + album_list = [] + for album in albums: + album_list.append( + Album( + album_id=album[0], + album_name=album[1], + description=album[2] or "", + is_hidden=bool(album[3]), + ) ) - ) - return GetAlbumsResponse(success=True, albums=album_list) - - -# POST /albums/ - Create a new album -@router.post("/", response_model=CreateAlbumResponse) -def create_album(body: CreateAlbumRequest): - existing_album = db_get_album_by_name(body.name) - if existing_album: + return GetAlbumsResponse(success=True, albums=album_list) + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in get_albums route: {e}") raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( success=False, - error="Album Already Exists", - message=f"Album '{body.name}' is already in the database.", + error="Internal Server Error", + message="An unexpected error occurred while fetching albums.", ).model_dump(), ) - album_id = str(uuid.uuid4()) + +# POST /albums/ - Create a new album +@router.post("/", response_model=CreateAlbumResponse) +def create_album(body: CreateAlbumRequest): try: + existing_album = db_get_album_by_name(body.name) + if existing_album: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=ErrorResponse( + success=False, + error="Album Already Exists", + message=f"Album '{body.name}' is already in the database.", + ).model_dump(), + ) + + album_id = str(uuid.uuid4()) db_insert_album( album_id, body.name, body.description, body.is_hidden, body.password ) return CreateAlbumResponse(success=True, album_id=album_id) - except Exception as e: + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in create_album route: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( success=False, error="Internal Server Error", - message=f"Failed to create album: {str(e)}", + message="An unexpected error occurred while creating the album.", ).model_dump(), ) @@ -81,16 +102,16 @@ def create_album(body: CreateAlbumRequest): # GET /albums/{album_id} - Get specific album details @router.get("/{album_id}", response_model=GetAlbumResponse) def get_album(album_id: str = Path(...)): - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, error="Album Not Found", message="Album not found" - ).model_dump(), - ) - try: + album = db_get_album(album_id) + if not album: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, error="Album Not Found", message="Album not found" + ).model_dump(), + ) + album_obj = Album( album_id=album[0], album_name=album[1], @@ -98,70 +119,80 @@ def get_album(album_id: str = Path(...)): is_hidden=bool(album[3]), ) return GetAlbumResponse(success=True, data=album_obj) - except Exception as e: + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in get_album route: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( success=False, error="Internal Server Error", - message=f"Failed to fetch album: {str(e)}", + message="An unexpected error occurred while fetching the album.", ).model_dump(), ) # PUT /albums/{album_id} - Update Album @router.put("/{album_id}", response_model=SuccessResponse) -def update_album(album_id: str = Path(...), body: UpdateAlbumRequest = Body(...)): - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the given ID.", - ).model_dump(), - ) - - album_dict = { - "album_id": album[0], - "album_name": album[1], - "description": album[2], - "is_hidden": bool(album[3]), - "password_hash": album[4], - } - - if album_dict["password_hash"]: - if not body.current_password: +def update_album( + album_id: str = Path(...), body: UpdateAlbumRequest = Body(...) +): # noqa: B008 + try: + album = db_get_album(album_id) + if not album: raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, + status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( success=False, - error="Missing Password", - message="Current password is required to update this album.", + error="Album Not Found", + message="No album exists with the given ID.", ).model_dump(), ) - if not verify_album_password(album_id, body.current_password): - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail=ErrorResponse( - success=False, - error="Incorrect Password", - message="The current password is incorrect.", - ).model_dump(), - ) + album_dict = { + "album_id": album[0], + "album_name": album[1], + "description": album[2], + "is_hidden": bool(album[3]), + "password_hash": album[4], + } + + if album_dict["password_hash"]: + if not body.current_password: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail=ErrorResponse( + success=False, + error="Missing Password", + message="Current password is required to update this album.", + ).model_dump(), + ) + + if not verify_album_password(album_id, body.current_password): + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail=ErrorResponse( + success=False, + error="Incorrect Password", + message="The current password is incorrect.", + ).model_dump(), + ) - try: db_update_album( album_id, body.name, body.description, body.is_hidden, body.password ) return SuccessResponse(success=True, msg="Album updated successfully") - except Exception as e: + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in update_album route: {e}") raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( - success=False, error="Failed to Update Album", message=str(e) + success=False, + error="Failed to Update Album", + message="An unexpected error occurred while updating the album.", ).model_dump(), ) @@ -169,25 +200,30 @@ def update_album(album_id: str = Path(...), body: UpdateAlbumRequest = Body(...) # DELETE /albums/{album_id} - Delete an album @router.delete("/{album_id}", response_model=SuccessResponse) def delete_album(album_id: str = Path(...)): - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the provided ID.", - ).model_dump(), - ) - try: + album = db_get_album(album_id) + if not album: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, + error="Album Not Found", + message="No album exists with the provided ID.", + ).model_dump(), + ) + db_delete_album(album_id) return SuccessResponse(success=True, msg="Album deleted successfully") - except Exception as e: + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in delete_album route: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( - success=False, error="Failed to Delete Album", message=str(e) + success=False, + error="Failed to Delete Album", + message="An unexpected error occurred while deleting the album.", ).model_dump(), ) @@ -198,93 +234,105 @@ def delete_album(album_id: str = Path(...)): # Since we need to send a password securely, switching this to POST -- necessary. # Open to suggestions if better approach possible. def get_album_images( - album_id: str = Path(...), body: GetAlbumImagesRequest = Body(...) + album_id: str = Path(...), body: GetAlbumImagesRequest = Body(...) # noqa: B008 ): - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the provided ID.", - ).model_dump(), - ) - - album_dict = { - "album_id": album[0], - "album_name": album[1], - "description": album[2], - "is_hidden": bool(album[3]), - "password_hash": album[4], - } - - if album_dict["is_hidden"]: - if not body.password: - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail=ErrorResponse( - success=False, - error="Password Required", - message="Password is required to access this hidden album.", - ).model_dump(), - ) - if not verify_album_password(album_id, body.password): + try: + album = db_get_album(album_id) + if not album: raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, + status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( success=False, - error="Invalid Password", - message="The password provided is incorrect.", + error="Album Not Found", + message="No album exists with the provided ID.", ).model_dump(), ) - try: + album_dict = { + "album_id": album[0], + "album_name": album[1], + "description": album[2], + "is_hidden": bool(album[3]), + "password_hash": album[4], + } + + if album_dict["is_hidden"]: + if not body.password: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail=ErrorResponse( + success=False, + error="Password Required", + message="Password is required to access this hidden album.", + ).model_dump(), + ) + if not verify_album_password(album_id, body.password): + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail=ErrorResponse( + success=False, + error="Invalid Password", + message="The password provided is incorrect.", + ).model_dump(), + ) + image_ids = db_get_album_images(album_id) return GetAlbumImagesResponse(success=True, image_ids=image_ids) - except Exception as e: + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in get_album_images route: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( - success=False, error="Failed to Retrieve Images", message=str(e) + success=False, + error="Failed to Retrieve Images", + message="An unexpected error occurred while retrieving images.", ).model_dump(), ) # POST /albums/{album_id}/images - Add images to an album @router.post("/{album_id}/images", response_model=SuccessResponse) -def add_images_to_album(album_id: str = Path(...), body: ImageIdsRequest = Body(...)): - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the provided ID.", - ).model_dump(), - ) +def add_images_to_album( + album_id: str = Path(...), body: ImageIdsRequest = Body(...) +): # noqa: B008 + try: + album = db_get_album(album_id) + if not album: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, + error="Album Not Found", + message="No album exists with the provided ID.", + ).model_dump(), + ) - if not body.image_ids: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=ErrorResponse( - success=False, - error="No Image IDs", - message="You must provide a list of image IDs to add.", - ).model_dump(), - ) + if not body.image_ids: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="No Image IDs", + message="You must provide a list of image IDs to add.", + ).model_dump(), + ) - try: db_add_images_to_album(album_id, body.image_ids) return SuccessResponse( success=True, msg=f"Added {len(body.image_ids)} images to album" ) - except Exception as e: + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in add_images_to_album route: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( - success=False, error="Failed to Add Images", message=str(e) + success=False, + error="Failed to Add Images", + message="An unexpected error occurred while adding images.", ).model_dump(), ) @@ -292,27 +340,32 @@ def add_images_to_album(album_id: str = Path(...), body: ImageIdsRequest = Body( # DELETE /albums/{album_id}/images/{image_id} - Remove image from album @router.delete("/{album_id}/images/{image_id}", response_model=SuccessResponse) def remove_image_from_album(album_id: str = Path(...), image_id: str = Path(...)): - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the provided ID.", - ).model_dump(), - ) - try: + album = db_get_album(album_id) + if not album: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, + error="Album Not Found", + message="No album exists with the provided ID.", + ).model_dump(), + ) + db_remove_image_from_album(album_id, image_id) return SuccessResponse( success=True, msg="Image removed from album successfully" ) - except Exception as e: + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in remove_image_from_album route: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( - success=False, error="Failed to Remove Image", message=str(e) + success=False, + error="Failed to Remove Image", + message="An unexpected error occurred while removing the image.", ).model_dump(), ) @@ -320,38 +373,43 @@ def remove_image_from_album(album_id: str = Path(...), image_id: str = Path(...) # DELETE /albums/{album_id}/images - Remove multiple images from album @router.delete("/{album_id}/images", response_model=SuccessResponse) def remove_images_from_album( - album_id: str = Path(...), body: ImageIdsRequest = Body(...) + album_id: str = Path(...), body: ImageIdsRequest = Body(...) # noqa: B008 ): - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the provided ID.", - ).model_dump(), - ) + try: + album = db_get_album(album_id) + if not album: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, + error="Album Not Found", + message="No album exists with the provided ID.", + ).model_dump(), + ) - if not body.image_ids: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=ErrorResponse( - success=False, - error="No Image IDs Provided", - message="You must provide at least one image ID to remove.", - ).model_dump(), - ) + if not body.image_ids: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, + error="No Image IDs Provided", + message="You must provide at least one image ID to remove.", + ).model_dump(), + ) - try: db_remove_images_from_album(album_id, body.image_ids) return SuccessResponse( success=True, msg=f"Removed {len(body.image_ids)} images from album" ) - except Exception as e: + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in remove_images_from_album route: {e}") raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=ErrorResponse( - success=False, error="Failed to Remove Images", message=str(e) + success=False, + error="Failed to Remove Images", + message="An unexpected error occurred while removing the images.", ).model_dump(), ) diff --git a/backend/tests/test_albums.py b/backend/tests/test_albums.py index cec9f670e..49683a851 100644 --- a/backend/tests/test_albums.py +++ b/backend/tests/test_albums.py @@ -1,11 +1,13 @@ -import sys import os -import pytest +import sys +import uuid +from unittest.mock import patch + import bcrypt +import pytest from fastapi import FastAPI from fastapi.testclient import TestClient -from unittest.mock import patch -import uuid + from app.routes import albums as albums_router sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) @@ -258,7 +260,7 @@ def test_get_album_by_id_not_found(self): "Hidden Album", "Secret", 1, - bcrypt.hashpw("oldpass".encode(), bcrypt.gensalt()).decode(), + bcrypt.hashpw(b"oldpass", bcrypt.gensalt()).decode(), ), { "name": "Updated Hidden Album", @@ -277,7 +279,7 @@ def test_get_album_by_id_not_found(self): "Hidden Album", "Secret", 1, - bcrypt.hashpw("correctpass".encode(), bcrypt.gensalt()).decode(), + bcrypt.hashpw(b"correctpass", bcrypt.gensalt()).decode(), ), { "name": "Invalid Attempt", @@ -472,3 +474,17 @@ def test_remove_multiple_images_from_album(self, mock_db_album): mock_remove_bulk.assert_called_once_with( album_id, image_ids_to_remove["image_ids"] ) + + +class TestAlbumRouteErrors: + """Test suite for verifying that album routes correctly handle exceptions.""" + + def test_unexpected_exceptions_mapped_to_500(self): + """Verify that an unexpected exception returns a 500 error.""" + with patch("app.routes.albums.db_get_all_albums") as mock_get_all: + mock_get_all.side_effect = Exception("Database explosion") + response = client.get("/albums/") + assert response.status_code == 500 + json_resp = response.json() + assert json_resp["detail"]["success"] is False + assert "unexpected error" in json_resp["detail"]["message"].lower() diff --git a/backend/tests/test_albums_db.py b/backend/tests/test_albums_db.py index d10d308e8..79042e402 100644 --- a/backend/tests/test_albums_db.py +++ b/backend/tests/test_albums_db.py @@ -1,23 +1,23 @@ import os import sqlite3 import tempfile -from typing import Iterator, List, Optional +from collections.abc import Iterator from unittest.mock import MagicMock, patch import bcrypt import pytest from app.database.albums import ( - 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_create_albums_table, db_delete_album, + db_get_album, + db_get_album_by_name, db_get_album_images, + db_get_all_albums, + db_insert_album, db_remove_images_from_album, + db_update_album, verify_album_password, ) from app.database.images import db_create_images_table @@ -34,7 +34,6 @@ def test_db(monkeypatch: pytest.MonkeyPatch) -> Iterator[str]: os.close(db_fd) monkeypatch.setattr("app.config.settings.DATABASE_PATH", db_path) - monkeypatch.setattr("app.database.albums.DATABASE_PATH", db_path) monkeypatch.setattr("app.database.images.DATABASE_PATH", db_path) # db_delete_album goes through the shared get_db_connection helper monkeypatch.setattr("app.database.connection.DATABASE_PATH", db_path) @@ -55,14 +54,14 @@ def make_album( name: str = "Trip", description: str = "", hidden: bool = False, - password: Optional[str] = None, + password: str | None = None, ) -> str: """Insert an album and return its id.""" db_insert_album(album_id, name, description, hidden, password) return album_id -def link_images(db_path: str, album_id: str, image_ids: List[str]) -> None: +def link_images(db_path: str, album_id: str, image_ids: list[str]) -> None: """Seed album_images rows directly -- these reads don't need real images.""" conn = sqlite3.connect(db_path) conn.executemany( @@ -73,7 +72,7 @@ def link_images(db_path: str, album_id: str, image_ids: List[str]) -> None: conn.close() -def stored_hash(db_path: str, album_id: str) -> Optional[str]: +def stored_hash(db_path: str, album_id: str) -> str | None: """Read an album's raw password_hash straight from the table.""" conn = sqlite3.connect(db_path) row = conn.execute( From 22bcd659e91aace43562276338cf5d78eba7e701 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sat, 25 Jul 2026 00:33:05 +0530 Subject: [PATCH 02/15] refactor(albums): use decorators for route errors and context managers for db errors --- backend/app/database/albums.py | 19 ++ backend/app/routes/albums.py | 537 ++++++++++++++++----------------- 2 files changed, 279 insertions(+), 277 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 0dd848356..8e524be51 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -1,4 +1,7 @@ +from __future__ import annotations + import sqlite3 +from contextlib import contextmanager import bcrypt @@ -8,6 +11,22 @@ logger = get_logger(__name__) +@contextmanager +def logged_db_connection(action: str): + try: + with get_db_connection() as conn: + yield conn + except sqlite3.IntegrityError as e: + logger.error(f"Integrity Error {action}: {e}") + raise + except sqlite3.OperationalError as e: + logger.error(f"Operational Error {action}: {e}") + raise + except sqlite3.Error as e: + logger.error(f"Database Error {action}: {e}") + raise + + def db_create_albums_table() -> None: try: with get_db_connection() as conn: diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index 067528857..2b7a892ac 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -1,4 +1,6 @@ +import sqlite3 import uuid +from functools import wraps from fastapi import APIRouter, Body, HTTPException, Path, Query, status @@ -32,384 +34,365 @@ logger = get_logger(__name__) -router = APIRouter() + +def handle_route_exceptions(error_title: str, error_message: str): + def decorator(func): + @wraps(func) + def wrapper(*args, **kwargs): + try: + return func(*args, **kwargs) + except HTTPException: + raise + except Exception as e: # noqa: BLE001 + logger.error(f"Error in {func.__name__} route: {e}") + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=ErrorResponse( + success=False, + error=error_title, + message=error_message, + ).model_dump(), + ) + + return wrapper + + return decorator +router = APIRouter() + # GET /albums/ - Get all albums -@router.get("/", response_model=GetAlbumsResponse) + +router.get("/", response_model=GetAlbumsResponse) + + +@handle_route_exceptions( + "Internal Server Error", "An unexpected error occurred while fetching albums." +) def get_albums(show_hidden: bool = Query(False)): - try: - albums = db_get_all_albums(show_hidden) - album_list = [] - for album in albums: - album_list.append( - Album( - album_id=album[0], - album_name=album[1], - description=album[2] or "", - is_hidden=bool(album[3]), - ) + albums = db_get_all_albums(show_hidden) + album_list = [] + for album in albums: + album_list.append( + Album( + album_id=album[0], + album_name=album[1], + description=album[2] or "", + is_hidden=bool(album[3]), ) - return GetAlbumsResponse(success=True, albums=album_list) - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in get_albums route: {e}") + ) + return GetAlbumsResponse(success=True, albums=album_list) + + +# POST /albums/ - Create a new album + +router.post("/", response_model=CreateAlbumResponse) + + +@handle_route_exceptions( + "Internal Server Error", "An unexpected error occurred while creating the album." +) +def create_album(body: CreateAlbumRequest): + existing_album = db_get_album_by_name(body.name) + if existing_album: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + status_code=status.HTTP_409_CONFLICT, detail=ErrorResponse( success=False, - error="Internal Server Error", - message="An unexpected error occurred while fetching albums.", + error="Album Already Exists", + message=f"Album '{body.name}' is already in the database.", ).model_dump(), ) - -# POST /albums/ - Create a new album -@router.post("/", response_model=CreateAlbumResponse) -def create_album(body: CreateAlbumRequest): + album_id = str(uuid.uuid4()) try: - existing_album = db_get_album_by_name(body.name) - if existing_album: - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=ErrorResponse( - success=False, - error="Album Already Exists", - message=f"Album '{body.name}' is already in the database.", - ).model_dump(), - ) - - album_id = str(uuid.uuid4()) db_insert_album( album_id, body.name, body.description, body.is_hidden, body.password ) - return CreateAlbumResponse(success=True, album_id=album_id) - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in create_album route: {e}") + except sqlite3.IntegrityError: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + status_code=status.HTTP_409_CONFLICT, detail=ErrorResponse( success=False, - error="Internal Server Error", - message="An unexpected error occurred while creating the album.", + error="Album Already Exists", + message=f"Album '{body.name}' is already in the database.", ).model_dump(), ) + return CreateAlbumResponse(success=True, album_id=album_id) # GET /albums/{album_id} - Get specific album details -@router.get("/{album_id}", response_model=GetAlbumResponse) -def get_album(album_id: str = Path(...)): - try: - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, error="Album Not Found", message="Album not found" - ).model_dump(), - ) +router.get("/{album_id}", response_model=GetAlbumResponse) - album_obj = Album( - album_id=album[0], - album_name=album[1], - description=album[2] or "", - is_hidden=bool(album[3]), - ) - return GetAlbumResponse(success=True, data=album_obj) - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in get_album route: {e}") + +@handle_route_exceptions( + "Internal Server Error", "An unexpected error occurred while fetching the album." +) +def get_album(album_id: str = Path(...)): + album = db_get_album(album_id) + if not album: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( - success=False, - error="Internal Server Error", - message="An unexpected error occurred while fetching the album.", + success=False, error="Album Not Found", message="Album not found" ).model_dump(), ) + album_obj = Album( + album_id=album[0], + album_name=album[1], + description=album[2] or "", + is_hidden=bool(album[3]), + ) + return GetAlbumResponse(success=True, data=album_obj) -# PUT /albums/{album_id} - Update Album -@router.put("/{album_id}", response_model=SuccessResponse) -def update_album( - album_id: str = Path(...), body: UpdateAlbumRequest = Body(...) -): # noqa: B008 - try: - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the given ID.", - ).model_dump(), - ) - album_dict = { - "album_id": album[0], - "album_name": album[1], - "description": album[2], - "is_hidden": bool(album[3]), - "password_hash": album[4], - } +# PUT /albums/{album_id} - Update Album - if album_dict["password_hash"]: - if not body.current_password: - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail=ErrorResponse( - success=False, - error="Missing Password", - message="Current password is required to update this album.", - ).model_dump(), - ) +router.put("/{album_id}", response_model=SuccessResponse) - if not verify_album_password(album_id, body.current_password): - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail=ErrorResponse( - success=False, - error="Incorrect Password", - message="The current password is incorrect.", - ).model_dump(), - ) - db_update_album( - album_id, body.name, body.description, body.is_hidden, body.password - ) - return SuccessResponse(success=True, msg="Album updated successfully") - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in update_album route: {e}") +@handle_route_exceptions( + "Failed to Update Album", "An unexpected error occurred while updating the album." +) +def update_album( + album_id: str = Path(...), body: UpdateAlbumRequest = Body(...) +): # noqa: B008 + album = db_get_album(album_id) + if not album: raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( success=False, - error="Failed to Update Album", - message="An unexpected error occurred while updating the album.", + error="Album Not Found", + message="No album exists with the given ID.", ).model_dump(), ) + album_dict = { + "album_id": album[0], + "album_name": album[1], + "description": album[2], + "is_hidden": bool(album[3]), + "password_hash": album[4], + } -# DELETE /albums/{album_id} - Delete an album -@router.delete("/{album_id}", response_model=SuccessResponse) -def delete_album(album_id: str = Path(...)): - try: - album = db_get_album(album_id) - if not album: + if album_dict["password_hash"]: + if not body.current_password: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail=ErrorResponse( + success=False, + error="Missing Password", + message="Current password is required to update this album.", + ).model_dump(), + ) + + if not verify_album_password(album_id, body.current_password): raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, + status_code=status.HTTP_401_UNAUTHORIZED, detail=ErrorResponse( success=False, - error="Album Not Found", - message="No album exists with the provided ID.", + error="Incorrect Password", + message="The current password is incorrect.", ).model_dump(), ) - db_delete_album(album_id) - return SuccessResponse(success=True, msg="Album deleted successfully") - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in delete_album route: {e}") + db_update_album( + album_id, body.name, body.description, body.is_hidden, body.password + ) + return SuccessResponse(success=True, msg="Album updated successfully") + + +# DELETE /albums/{album_id} - Delete an album +router.delete("/{album_id}", response_model=SuccessResponse) + + +@handle_route_exceptions( + "Failed to Delete Album", "An unexpected error occurred while deleting the album." +) +def delete_album(album_id: str = Path(...)): + album = db_get_album(album_id) + if not album: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( success=False, - error="Failed to Delete Album", - message="An unexpected error occurred while deleting the album.", + error="Album Not Found", + message="No album exists with the provided ID.", ).model_dump(), ) + db_delete_album(album_id) + return SuccessResponse(success=True, msg="Album deleted successfully") + # GET /albums/{album_id}/images - Get all images in an album -@router.post("/{album_id}/images/get", response_model=GetAlbumImagesResponse) -# GET requests do not accept a body by default. -# Since we need to send a password securely, switching this to POST -- necessary. -# Open to suggestions if better approach possible. +router.post("/{album_id}/images/get", response_model=GetAlbumImagesResponse) + + +@handle_route_exceptions( + "Failed to Retrieve Images", "An unexpected error occurred while retrieving images." +) def get_album_images( album_id: str = Path(...), body: GetAlbumImagesRequest = Body(...) # noqa: B008 ): - try: - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the provided ID.", - ).model_dump(), - ) - - album_dict = { - "album_id": album[0], - "album_name": album[1], - "description": album[2], - "is_hidden": bool(album[3]), - "password_hash": album[4], - } - - if album_dict["is_hidden"]: - if not body.password: - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail=ErrorResponse( - success=False, - error="Password Required", - message="Password is required to access this hidden album.", - ).model_dump(), - ) - if not verify_album_password(album_id, body.password): - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail=ErrorResponse( - success=False, - error="Invalid Password", - message="The password provided is incorrect.", - ).model_dump(), - ) - - image_ids = db_get_album_images(album_id) - return GetAlbumImagesResponse(success=True, image_ids=image_ids) - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in get_album_images route: {e}") + album = db_get_album(album_id) + if not album: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( success=False, - error="Failed to Retrieve Images", - message="An unexpected error occurred while retrieving images.", + error="Album Not Found", + message="No album exists with the provided ID.", ).model_dump(), ) + album_dict = { + "album_id": album[0], + "album_name": album[1], + "description": album[2], + "is_hidden": bool(album[3]), + "password_hash": album[4], + } -# POST /albums/{album_id}/images - Add images to an album -@router.post("/{album_id}/images", response_model=SuccessResponse) -def add_images_to_album( - album_id: str = Path(...), body: ImageIdsRequest = Body(...) -): # noqa: B008 - try: - album = db_get_album(album_id) - if not album: + if album_dict["is_hidden"]: + if not body.password: raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, + status_code=status.HTTP_401_UNAUTHORIZED, detail=ErrorResponse( success=False, - error="Album Not Found", - message="No album exists with the provided ID.", + error="Password Required", + message="Password is required to access this hidden album.", ).model_dump(), ) - - if not body.image_ids: + if not verify_album_password(album_id, body.password): raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_401_UNAUTHORIZED, detail=ErrorResponse( success=False, - error="No Image IDs", - message="You must provide a list of image IDs to add.", + error="Invalid Password", + message="The password provided is incorrect.", ).model_dump(), ) - db_add_images_to_album(album_id, body.image_ids) - return SuccessResponse( - success=True, msg=f"Added {len(body.image_ids)} images to album" + image_ids = db_get_album_images(album_id) + return GetAlbumImagesResponse(success=True, image_ids=image_ids) + + +# POST /albums/{album_id}/images - Add images to an album + +router.post("/{album_id}/images", response_model=SuccessResponse) + + +@handle_route_exceptions( + "Failed to Add Images", "An unexpected error occurred while adding images." +) +def add_images_to_album( + album_id: str = Path(...), body: ImageIdsRequest = Body(...) +): # noqa: B008 + album = db_get_album(album_id) + if not album: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, + error="Album Not Found", + message="No album exists with the provided ID.", + ).model_dump(), ) - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in add_images_to_album route: {e}") + + if not body.image_ids: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( success=False, - error="Failed to Add Images", - message="An unexpected error occurred while adding images.", + error="No Image IDs", + message="You must provide a list of image IDs to add.", + ).model_dump(), + ) + + try: + db_add_images_to_album(album_id, body.image_ids) + except (ValueError, TypeError) as e: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=ErrorResponse( + success=False, error="Invalid Image IDs", message=str(e) ).model_dump(), ) + return SuccessResponse( + success=True, msg=f"Added {len(body.image_ids)} images to album" + ) + # DELETE /albums/{album_id}/images/{image_id} - Remove image from album -@router.delete("/{album_id}/images/{image_id}", response_model=SuccessResponse) + +router.delete("/{album_id}/images/{image_id}", response_model=SuccessResponse) + + +@handle_route_exceptions( + "Failed to Remove Image", "An unexpected error occurred while removing the image." +) def remove_image_from_album(album_id: str = Path(...), image_id: str = Path(...)): - try: - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the provided ID.", - ).model_dump(), - ) + album = db_get_album(album_id) + if not album: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, + error="Album Not Found", + message="No album exists with the provided ID.", + ).model_dump(), + ) + try: db_remove_image_from_album(album_id, image_id) - return SuccessResponse( - success=True, msg="Image removed from album successfully" - ) - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in remove_image_from_album route: {e}") + except ValueError as e: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( - success=False, - error="Failed to Remove Image", - message="An unexpected error occurred while removing the image.", + success=False, error="Image Not Found", message=str(e) ).model_dump(), ) + return SuccessResponse(success=True, msg="Image removed from album successfully") + # DELETE /albums/{album_id}/images - Remove multiple images from album -@router.delete("/{album_id}/images", response_model=SuccessResponse) + +router.delete("/{album_id}/images", response_model=SuccessResponse) + + +@handle_route_exceptions( + "Failed to Remove Images", "An unexpected error occurred while removing the images." +) def remove_images_from_album( album_id: str = Path(...), body: ImageIdsRequest = Body(...) # noqa: B008 ): - try: - album = db_get_album(album_id) - if not album: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=ErrorResponse( - success=False, - error="Album Not Found", - message="No album exists with the provided ID.", - ).model_dump(), - ) - - if not body.image_ids: - raise HTTPException( - status_code=status.HTTP_400_BAD_REQUEST, - detail=ErrorResponse( - success=False, - error="No Image IDs Provided", - message="You must provide at least one image ID to remove.", - ).model_dump(), - ) - - db_remove_images_from_album(album_id, body.image_ids) - return SuccessResponse( - success=True, msg=f"Removed {len(body.image_ids)} images from album" + album = db_get_album(album_id) + if not album: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=ErrorResponse( + success=False, + error="Album Not Found", + message="No album exists with the provided ID.", + ).model_dump(), ) - except HTTPException: - raise - except Exception as e: # noqa: BLE001 - logger.error(f"Error in remove_images_from_album route: {e}") + + if not body.image_ids: raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( success=False, - error="Failed to Remove Images", - message="An unexpected error occurred while removing the images.", + error="No Image IDs Provided", + message="You must provide at least one image ID to remove.", ).model_dump(), ) + + db_remove_images_from_album(album_id, body.image_ids) + return SuccessResponse( + success=True, msg=f"Removed {len(body.image_ids)} images from album" + ) From 863c8afa293262c6105c6c0abe76f53c2fc7ffaa Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sat, 25 Jul 2026 00:40:15 +0530 Subject: [PATCH 03/15] fix: resolve linting and backend test issues --- backend/app/routes/albums.py | 8 ++++---- backend/tests/test_albums_db.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index 2b7a892ac..f4910dc18 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -156,8 +156,8 @@ def get_album(album_id: str = Path(...)): "Failed to Update Album", "An unexpected error occurred while updating the album." ) def update_album( - album_id: str = Path(...), body: UpdateAlbumRequest = Body(...) -): # noqa: B008 + album_id: str = Path(...), body: UpdateAlbumRequest = Body(...) # noqa: B008 +): album = db_get_album(album_id) if not album: raise HTTPException( @@ -289,8 +289,8 @@ def get_album_images( "Failed to Add Images", "An unexpected error occurred while adding images." ) def add_images_to_album( - album_id: str = Path(...), body: ImageIdsRequest = Body(...) -): # noqa: B008 + album_id: str = Path(...), body: ImageIdsRequest = Body(...) # noqa: B008 +): album = db_get_album(album_id) if not album: raise HTTPException( diff --git a/backend/tests/test_albums_db.py b/backend/tests/test_albums_db.py index 79042e402..c8cdc0fbd 100644 --- a/backend/tests/test_albums_db.py +++ b/backend/tests/test_albums_db.py @@ -111,7 +111,7 @@ def test_closes_the_connection_when_create_fails(self, create_table): Mocked deliberately: a real CREATE can't be made to fail while leaving the connection observable. """ - with patch("app.database.albums.sqlite3.connect") as mock_connect: + with patch("app.database.connection.sqlite3.connect") as mock_connect: conn = MagicMock() conn.cursor.return_value.execute.side_effect = sqlite3.Error("fail") mock_connect.return_value = conn From a8ae4c1ee90538a4ea3fee352c81d0878f824d3a Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sat, 25 Jul 2026 00:55:12 +0530 Subject: [PATCH 04/15] fix(albums): correctly apply context manager to database functions --- backend/app/database/albums.py | 310 ++++++++++++++------------------- 1 file changed, 128 insertions(+), 182 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 8e524be51..bf7c0c79b 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -1,5 +1,3 @@ -from __future__ import annotations - import sqlite3 from contextlib import contextmanager @@ -28,77 +26,57 @@ def logged_db_connection(action: str): def db_create_albums_table() -> None: - try: - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.execute(""" - CREATE TABLE IF NOT EXISTS albums ( - album_id TEXT PRIMARY KEY, - album_name TEXT UNIQUE, - description TEXT, - is_hidden BOOLEAN DEFAULT 0, - password_hash TEXT - ) - """) - except sqlite3.Error as e: - logger.error(f"Error creating albums table: {e}") - raise + with logged_db_connection("creating albums table") as conn: + cursor = conn.cursor() + cursor.execute(""" + CREATE TABLE IF NOT EXISTS albums ( + album_id TEXT PRIMARY KEY, + album_name TEXT UNIQUE, + description TEXT, + is_hidden BOOLEAN DEFAULT 0, + password_hash TEXT + ) + """) def db_create_album_images_table() -> None: - try: - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.execute(""" - CREATE TABLE IF NOT EXISTS album_images ( - album_id TEXT, - image_id TEXT, - PRIMARY KEY (album_id, image_id), - FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, - FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE - ) - """) - except sqlite3.Error as e: - logger.error(f"Error creating album_images table: {e}") - raise + with logged_db_connection("creating album_images table") as conn: + cursor = conn.cursor() + cursor.execute(""" + CREATE TABLE IF NOT EXISTS album_images ( + album_id TEXT, + image_id TEXT, + PRIMARY KEY (album_id, image_id), + FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, + FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE + ) + """) def db_get_all_albums(show_hidden: bool = False) -> list[tuple]: - try: - with get_db_connection() as conn: - cursor = conn.cursor() - if show_hidden: - cursor.execute("SELECT * FROM albums") - else: - cursor.execute("SELECT * FROM albums WHERE is_hidden = 0") - return cursor.fetchall() - except sqlite3.Error as e: - logger.error(f"Error getting all albums: {e}") - raise + with logged_db_connection("getting all albums") as conn: + cursor = conn.cursor() + if show_hidden: + cursor.execute("SELECT * FROM albums") + else: + cursor.execute("SELECT * FROM albums WHERE is_hidden = 0") + return cursor.fetchall() def db_get_album_by_name(name: str) -> tuple | None: - try: - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.execute("SELECT * FROM albums WHERE album_name = ?", (name,)) - album = cursor.fetchone() - return album if album else None - except sqlite3.Error as e: - logger.error(f"Error getting album by name '{name}': {e}") - raise + with logged_db_connection(f"getting album by name '{name}'") as conn: + cursor = conn.cursor() + cursor.execute("SELECT * FROM albums WHERE album_name = ?", (name,)) + album = cursor.fetchone() + return album if album else None def db_get_album(album_id: str) -> tuple | None: - try: - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.execute("SELECT * FROM albums WHERE album_id = ?", (album_id,)) - album = cursor.fetchone() - return album if album else None - except sqlite3.Error as e: - logger.error(f"Error getting album '{album_id}': {e}") - raise + with logged_db_connection(f"getting album '{album_id}'") as conn: + cursor = conn.cursor() + cursor.execute("SELECT * FROM albums WHERE album_id = ?", (album_id)) + album = cursor.fetchone() + return album if album else None def db_insert_album( @@ -108,24 +86,20 @@ def db_insert_album( is_hidden: bool = False, password: str | None = None, ): - try: - with get_db_connection() as conn: - cursor = conn.cursor() - password_hash = None - if password: - password_hash = bcrypt.hashpw( - password.encode("utf-8"), bcrypt.gensalt() - ).decode("utf-8") - cursor.execute( - """ - INSERT INTO albums (album_id, album_name, description, is_hidden, password_hash) - VALUES (?, ?, ?, ?, ?) - """, - (album_id, album_name, description, int(is_hidden), password_hash), - ) - except sqlite3.Error as e: - logger.error(f"Error inserting album '{album_name}': {e}") - raise + with logged_db_connection(f"inserting album '{album_name}'") as conn: + cursor = conn.cursor() + password_hash = None + if password: + password_hash = bcrypt.hashpw( + password.encode("utf-8"), bcrypt.gensalt() + ).decode("utf-8") + cursor.execute( + """ + INSERT INTO albums (album_id, album_name, description, is_hidden, password_hash) + VALUES (?, ?, ?, ?, ?) + """, + (album_id, album_name, description, int(is_hidden), password_hash), + ) def db_update_album( @@ -135,57 +109,45 @@ def db_update_album( is_hidden: bool, password: str | None = None, ): - try: - with get_db_connection() as conn: - cursor = conn.cursor() - if password is not None: - password_hash = bcrypt.hashpw( - password.encode("utf-8"), bcrypt.gensalt() - ).decode("utf-8") - cursor.execute( - """ - UPDATE albums - SET album_name = ?, description = ?, is_hidden = ?, password_hash = ? - WHERE album_id = ? - """, - (album_name, description, int(is_hidden), password_hash, album_id), - ) - else: - cursor.execute( - """ - UPDATE albums - SET album_name = ?, description = ?, is_hidden = ? - WHERE album_id = ? - """, - (album_name, description, int(is_hidden), album_id), - ) - except sqlite3.Error as e: - logger.error(f"Error updating album '{album_id}': {e}") - raise + with logged_db_connection(f"updating album '{album_id}'") as conn: + cursor = conn.cursor() + if password is not None: + password_hash = bcrypt.hashpw( + password.encode("utf-8"), bcrypt.gensalt() + ).decode("utf-8") + cursor.execute( + """ + UPDATE albums + SET album_name = ?, description = ?, is_hidden = ?, password_hash = ? + WHERE album_id = ? + """, + (album_name, description, int(is_hidden), password_hash, album_id), + ) + else: + cursor.execute( + """ + UPDATE albums + SET album_name = ?, description = ?, is_hidden = ? + WHERE album_id = ? + """, + (album_name, description, int(is_hidden), album_id), + ) def db_delete_album(album_id: str): - try: - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.execute("DELETE FROM albums WHERE album_id = ?", (album_id,)) - except sqlite3.Error as e: - logger.error(f"Error deleting album '{album_id}': {e}") - raise + with logged_db_connection(f"deleting album '{album_id}'") as conn: + cursor = conn.cursor() + cursor.execute("DELETE FROM albums WHERE album_id = ?", (album_id,)) def db_get_album_images(album_id: str): - try: - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.execute( - "SELECT image_id FROM album_images WHERE album_id = ?", (album_id,) - ) - images = cursor.fetchall() - return [img[0] for img in images] - except sqlite3.Error as e: - logger.error(f"Error getting images for album '{album_id}': {e}") - raise + with logged_db_connection(f"getting images for album '{album_id}'") as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT image_id FROM album_images WHERE album_id = ?", (album_id,) + ) + images = cursor.fetchall() + return [img[0] for img in images] def db_add_images_to_album(album_id: str, image_ids: list[str]): @@ -200,76 +162,60 @@ def db_add_images_to_album(album_id: str, image_ids: list[str]): if not sanitized_ids: raise ValueError("No valid image IDs provided") - try: - with get_db_connection() as conn: - cursor = conn.cursor() + with logged_db_connection(f"adding images to album '{album_id}'") as conn: + cursor = conn.cursor() - placeholders = ",".join(["?"] * len(sanitized_ids)) - query = f"SELECT id FROM images WHERE id IN ({placeholders})" - cursor.execute(query, sanitized_ids) - valid_images = [row[0] for row in cursor.fetchall()] + placeholders = ",".join(["?"] * len(sanitized_ids)) + query = f"SELECT id FROM images WHERE id IN ({placeholders})" + cursor.execute(query, sanitized_ids) + valid_images = [row[0] for row in cursor.fetchall()] - if not valid_images: - raise ValueError( - "None of the provided image IDs exist in the database." - ) + if not valid_images: + raise ValueError("None of the provided image IDs exist in the database.") - cursor.executemany( - "INSERT OR IGNORE INTO album_images (album_id, image_id) VALUES (?, ?)", - [(album_id, img_id) for img_id in valid_images], - ) - except sqlite3.Error as e: - logger.error(f"Error adding images to album '{album_id}': {e}") - raise + cursor.executemany( + "INSERT OR IGNORE INTO album_images (album_id, image_id) VALUES (?, ?)", + [(album_id, img_id) for img_id in valid_images], + ) def db_remove_image_from_album(album_id: str, image_id: str): - try: - with get_db_connection() as conn: - cursor = conn.cursor() - + with logged_db_connection( + f"removing image '{image_id}' from album '{album_id}'" + ) as conn: + cursor = conn.cursor() + + cursor.execute( + "SELECT 1 FROM album_images WHERE album_id = ? AND image_id = ?", + (album_id, image_id), + ) + exists = cursor.fetchone() + + if exists: cursor.execute( - "SELECT 1 FROM album_images WHERE album_id = ? AND image_id = ?", + "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", (album_id, image_id), ) - exists = cursor.fetchone() - - if exists: - cursor.execute( - "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", - (album_id, image_id), - ) - else: - raise ValueError("Image not found in the specified album") - except sqlite3.Error as e: - logger.error(f"Error removing image '{image_id}' from album '{album_id}': {e}") - raise + else: + raise ValueError("[Mapped 404] Image not found in the specified album") def db_remove_images_from_album(album_id: str, image_ids: list[str]): - try: - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.executemany( - "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", - [(album_id, img_id) for img_id in image_ids], - ) - except sqlite3.Error as e: - logger.error(f"Error removing images from album '{album_id}': {e}") - raise + with logged_db_connection(f"removing images from album '{album_id}'") as conn: + cursor = conn.cursor() + cursor.executemany( + "DELETE FROM album_images WHERE album_id = ? AND image_id = ?", + [(album_id, img_id) for img_id in image_ids], + ) def verify_album_password(album_id: str, password: str) -> bool: - try: - with get_db_connection() as conn: - cursor = conn.cursor() - cursor.execute( - "SELECT password_hash FROM albums WHERE album_id = ?", (album_id,) - ) - row = cursor.fetchone() - if not row or not row[0]: - return False - return bcrypt.checkpw(password.encode("utf-8"), row[0].encode("utf-8")) - except sqlite3.Error as e: - logger.error(f"Error verifying password for album '{album_id}': {e}") - raise + with logged_db_connection(f"verifying password for album '{album_id}'") as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT password_hash FROM albums WHERE album_id = ?", (album_id,) + ) + row = cursor.fetchone() + if not row or not row[0]: + return False + return bcrypt.checkpw(password.encode("utf-8"), row[0].encode("utf-8")) From 6dfcfab5ff8edbd16a967d3af54399316ded5d5e Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sat, 25 Jul 2026 00:58:48 +0530 Subject: [PATCH 05/15] fix(albums): match exact error title requested by CodeRabbit --- backend/app/routes/albums.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index f4910dc18..75dd92321 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -318,7 +318,7 @@ def add_images_to_album( raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=ErrorResponse( - success=False, error="Invalid Image IDs", message=str(e) + success=False, error="Failed to Add Images", message=str(e) ).model_dump(), ) @@ -353,7 +353,7 @@ def remove_image_from_album(album_id: str = Path(...), image_id: str = Path(...) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=ErrorResponse( - success=False, error="Image Not Found", message=str(e) + success=False, error="Failed to Remove Image", message=str(e) ).model_dump(), ) From 3cd72c9ca9d05619aa5168e69a3ada587f4131e4 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sat, 25 Jul 2026 01:02:53 +0530 Subject: [PATCH 06/15] chore: format files --- backend/app/database/albums.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index bf7c0c79b..9d4f68050 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -28,7 +28,8 @@ def logged_db_connection(action: str): def db_create_albums_table() -> None: with logged_db_connection("creating albums table") as conn: cursor = conn.cursor() - cursor.execute(""" + cursor.execute( + """ CREATE TABLE IF NOT EXISTS albums ( album_id TEXT PRIMARY KEY, album_name TEXT UNIQUE, @@ -36,13 +37,15 @@ def db_create_albums_table() -> None: is_hidden BOOLEAN DEFAULT 0, password_hash TEXT ) - """) + """ + ) def db_create_album_images_table() -> None: with logged_db_connection("creating album_images table") as conn: cursor = conn.cursor() - cursor.execute(""" + cursor.execute( + """ CREATE TABLE IF NOT EXISTS album_images ( album_id TEXT, image_id TEXT, @@ -50,7 +53,8 @@ def db_create_album_images_table() -> None: FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE ) - """) + """ + ) def db_get_all_albums(show_hidden: bool = False) -> list[tuple]: From 86271c9786aee1b5d07b9111051bc2496c213b0f Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sat, 25 Jul 2026 01:10:07 +0530 Subject: [PATCH 07/15] fix(albums): correct sqlite3 binding syntax in db_get_album --- backend/app/database/albums.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 9d4f68050..48f32a1f1 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -28,8 +28,7 @@ def logged_db_connection(action: str): def db_create_albums_table() -> None: with logged_db_connection("creating albums table") as conn: cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS albums ( album_id TEXT PRIMARY KEY, album_name TEXT UNIQUE, @@ -37,15 +36,13 @@ def db_create_albums_table() -> None: is_hidden BOOLEAN DEFAULT 0, password_hash TEXT ) - """ - ) + """) def db_create_album_images_table() -> None: with logged_db_connection("creating album_images table") as conn: cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS album_images ( album_id TEXT, image_id TEXT, @@ -53,8 +50,7 @@ def db_create_album_images_table() -> None: FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE ) - """ - ) + """) def db_get_all_albums(show_hidden: bool = False) -> list[tuple]: @@ -78,7 +74,7 @@ def db_get_album_by_name(name: str) -> tuple | None: def db_get_album(album_id: str) -> tuple | None: with logged_db_connection(f"getting album '{album_id}'") as conn: cursor = conn.cursor() - cursor.execute("SELECT * FROM albums WHERE album_id = ?", (album_id)) + cursor.execute("SELECT * FROM albums WHERE album_id = ?", (album_id,)) album = cursor.fetchone() return album if album else None From ce8e3912b62e2af844d76283261584c1af259b0c Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sat, 25 Jul 2026 01:22:00 +0530 Subject: [PATCH 08/15] fix(albums): map duplicate album names during update to 409 --- backend/app/database/albums.py | 12 ++++++++---- backend/app/routes/albums.py | 16 +++++++++++++--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index 48f32a1f1..d04c97af8 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -28,7 +28,8 @@ def logged_db_connection(action: str): def db_create_albums_table() -> None: with logged_db_connection("creating albums table") as conn: cursor = conn.cursor() - cursor.execute(""" + cursor.execute( + """ CREATE TABLE IF NOT EXISTS albums ( album_id TEXT PRIMARY KEY, album_name TEXT UNIQUE, @@ -36,13 +37,15 @@ def db_create_albums_table() -> None: is_hidden BOOLEAN DEFAULT 0, password_hash TEXT ) - """) + """ + ) def db_create_album_images_table() -> None: with logged_db_connection("creating album_images table") as conn: cursor = conn.cursor() - cursor.execute(""" + cursor.execute( + """ CREATE TABLE IF NOT EXISTS album_images ( album_id TEXT, image_id TEXT, @@ -50,7 +53,8 @@ def db_create_album_images_table() -> None: FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE ) - """) + """ + ) def db_get_all_albums(show_hidden: bool = False) -> list[tuple]: diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index 75dd92321..72f239902 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -198,9 +198,19 @@ def update_album( ).model_dump(), ) - db_update_album( - album_id, body.name, body.description, body.is_hidden, body.password - ) + try: + db_update_album( + album_id, body.name, body.description, body.is_hidden, body.password + ) + except sqlite3.IntegrityError: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=ErrorResponse( + success=False, + error="Album Already Exists", + message=f"Album '{body.name}' is already in the database.", + ).model_dump(), + ) return SuccessResponse(success=True, msg="Album updated successfully") From 92ceb392ea97ef908d1dd23859e6ac7413bff360 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Sat, 25 Jul 2026 01:28:58 +0530 Subject: [PATCH 09/15] fix(albums): restore missing @ decorator for all routes --- backend/app/routes/albums.py | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index 72f239902..930778603 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -63,9 +63,8 @@ def wrapper(*args, **kwargs): # GET /albums/ - Get all albums -router.get("/", response_model=GetAlbumsResponse) - +@router.get("/", response_model=GetAlbumsResponse) @handle_route_exceptions( "Internal Server Error", "An unexpected error occurred while fetching albums." ) @@ -86,9 +85,8 @@ def get_albums(show_hidden: bool = Query(False)): # POST /albums/ - Create a new album -router.post("/", response_model=CreateAlbumResponse) - +@router.post("/", response_model=CreateAlbumResponse) @handle_route_exceptions( "Internal Server Error", "An unexpected error occurred while creating the album." ) @@ -122,9 +120,7 @@ def create_album(body: CreateAlbumRequest): # GET /albums/{album_id} - Get specific album details -router.get("/{album_id}", response_model=GetAlbumResponse) - - +@router.get("/{album_id}", response_model=GetAlbumResponse) @handle_route_exceptions( "Internal Server Error", "An unexpected error occurred while fetching the album." ) @@ -149,9 +145,8 @@ def get_album(album_id: str = Path(...)): # PUT /albums/{album_id} - Update Album -router.put("/{album_id}", response_model=SuccessResponse) - +@router.put("/{album_id}", response_model=SuccessResponse) @handle_route_exceptions( "Failed to Update Album", "An unexpected error occurred while updating the album." ) @@ -215,9 +210,7 @@ def update_album( # DELETE /albums/{album_id} - Delete an album -router.delete("/{album_id}", response_model=SuccessResponse) - - +@router.delete("/{album_id}", response_model=SuccessResponse) @handle_route_exceptions( "Failed to Delete Album", "An unexpected error occurred while deleting the album." ) @@ -238,9 +231,7 @@ def delete_album(album_id: str = Path(...)): # GET /albums/{album_id}/images - Get all images in an album -router.post("/{album_id}/images/get", response_model=GetAlbumImagesResponse) - - +@router.post("/{album_id}/images/get", response_model=GetAlbumImagesResponse) @handle_route_exceptions( "Failed to Retrieve Images", "An unexpected error occurred while retrieving images." ) @@ -292,9 +283,8 @@ def get_album_images( # POST /albums/{album_id}/images - Add images to an album -router.post("/{album_id}/images", response_model=SuccessResponse) - +@router.post("/{album_id}/images", response_model=SuccessResponse) @handle_route_exceptions( "Failed to Add Images", "An unexpected error occurred while adding images." ) @@ -339,9 +329,8 @@ def add_images_to_album( # DELETE /albums/{album_id}/images/{image_id} - Remove image from album -router.delete("/{album_id}/images/{image_id}", response_model=SuccessResponse) - +@router.delete("/{album_id}/images/{image_id}", response_model=SuccessResponse) @handle_route_exceptions( "Failed to Remove Image", "An unexpected error occurred while removing the image." ) @@ -372,9 +361,8 @@ def remove_image_from_album(album_id: str = Path(...), image_id: str = Path(...) # DELETE /albums/{album_id}/images - Remove multiple images from album -router.delete("/{album_id}/images", response_model=SuccessResponse) - +@router.delete("/{album_id}/images", response_model=SuccessResponse) @handle_route_exceptions( "Failed to Remove Images", "An unexpected error occurred while removing the images." ) From bd6a2b8121bdb0e1cfc37ac6ad7a99bdc8891819 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 31 Jul 2026 02:30:31 +0530 Subject: [PATCH 10/15] test: fix AttributeError by mocking connection instead of albums --- backend/tests/test_memory_signals_db.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/tests/test_memory_signals_db.py b/backend/tests/test_memory_signals_db.py index 9a712d976..2db1a20db 100644 --- a/backend/tests/test_memory_signals_db.py +++ b/backend/tests/test_memory_signals_db.py @@ -75,7 +75,7 @@ def test_db(monkeypatch: pytest.MonkeyPatch) -> Iterator[str]: "app.database.images", "app.database.folders", "app.database.yolo_mapping", - "app.database.albums", + "app.database.connection", "app.database.faces", "app.database.face_clusters", "app.database.videos", From a7efbaf0dba3fe212ad4b27a08e924226b6f1346 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Mon, 3 Aug 2026 15:37:13 +0530 Subject: [PATCH 11/15] fix(albums): remove undefined DATABASE_PATH reference and format --- backend/app/database/albums.py | 21 +++++++-------------- backend/app/routes/albums.py | 12 ++++++------ 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index c02f200c3..e48980532 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -28,8 +28,7 @@ def logged_db_connection(action: str): def db_create_albums_table() -> None: with logged_db_connection("creating albums table") as conn: cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS albums ( album_id TEXT PRIMARY KEY, album_name TEXT UNIQUE, @@ -38,8 +37,7 @@ def db_create_albums_table() -> None: password_hash TEXT, cover_image_path TEXT ) - """ - ) + """) # Shipped databases predate the is_hidden -> is_locked rename and the # cover_image_path column, and CREATE IF NOT EXISTS won't add either. cursor.execute("PRAGMA table_info(albums)") @@ -53,8 +51,7 @@ def db_create_albums_table() -> None: def db_create_album_images_table() -> None: with logged_db_connection("creating album_images table") as conn: cursor = conn.cursor() - cursor.execute( - """ + cursor.execute(""" CREATE TABLE IF NOT EXISTS album_images ( album_id TEXT, image_id TEXT, @@ -62,8 +59,7 @@ def db_create_album_images_table() -> None: FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE ) - """ - ) + """) # The PK leads with album_id, so lookup by image alone needs its own # index. The memory scorer does exactly that, per image. cursor.execute( @@ -167,16 +163,12 @@ def db_delete_album(album_id: str): def db_update_album_cover_image(album_id: str, cover_image_path: str): """Update the cover image path for an album""" - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() - try: + with logged_db_connection(f"updating cover image for album '{album_id}'") as conn: + cursor = conn.cursor() cursor.execute( "UPDATE albums SET cover_image_path = ? WHERE album_id = ?", (cover_image_path, album_id), ) - conn.commit() - finally: - conn.close() def db_get_album_images(album_id: str): @@ -259,6 +251,7 @@ def verify_album_password(album_id: str, password: str) -> bool: return False return bcrypt.checkpw(password.encode("utf-8"), row[0].encode("utf-8")) + def db_get_image_path(image_id: str) -> str | None: """Get the path of an image by its ID.""" with logged_db_connection(f"getting path for image '{image_id}'") as conn: diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index c2d2d371a..35a8d41c9 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -2,7 +2,7 @@ import uuid from functools import wraps -from fastapi import APIRouter, Body, HTTPException, Path, Query, status +from fastapi import APIRouter, Body, HTTPException, Path, status from app.database.albums import ( db_add_images_to_album, @@ -11,13 +11,13 @@ db_get_album_by_name, db_get_album_images, db_get_all_albums, + db_get_image_path, db_insert_album, db_remove_image_from_album, db_remove_images_from_album, db_update_album, db_update_album_cover_image, verify_album_password, - db_get_image_path, ) from app.logging.setup_logging import get_logger from app.schemas.album import ( @@ -64,6 +64,7 @@ def wrapper(*args, **kwargs): router = APIRouter() + # GET /albums/ - Get all albums (including locked ones) @router.get("/", response_model=GetAlbumsResponse) @handle_route_exceptions( @@ -413,7 +414,8 @@ def remove_images_from_album( # PUT /albums/{album_id}/cover - Set album cover image @router.put("/{album_id}/cover", response_model=SuccessResponse) @handle_route_exceptions( - "Failed to Set Cover Image", "An unexpected error occurred while setting the cover image." + "Failed to Set Cover Image", + "An unexpected error occurred while setting the cover image.", ) def set_album_cover_image( album_id: str = Path(...), body: SetCoverImageRequest = Body(...) @@ -457,6 +459,4 @@ def set_album_cover_image( db_update_album_cover_image(album_id, image_path) - return SuccessResponse( - success=True, msg="Album cover image updated successfully" - ) + return SuccessResponse(success=True, msg="Album cover image updated successfully") From 880de80273ed8184ca17ad24341520a16e335f07 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Mon, 3 Aug 2026 15:37:36 +0530 Subject: [PATCH 12/15] fix(albums): add noqa B008 to set_album_cover_image --- backend/app/routes/albums.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index 35a8d41c9..0fd0b0477 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -418,7 +418,7 @@ def remove_images_from_album( "An unexpected error occurred while setting the cover image.", ) def set_album_cover_image( - album_id: str = Path(...), body: SetCoverImageRequest = Body(...) + album_id: str = Path(...), body: SetCoverImageRequest = Body(...) # noqa: B008 ): """Set or update the cover image for an album""" album = db_get_album(album_id) From 63b874c710c785d902ce6115ec3e9d381720a09e Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Mon, 3 Aug 2026 15:45:50 +0530 Subject: [PATCH 13/15] chore(albums): format albums.py with pre-commit black --- backend/app/database/albums.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/backend/app/database/albums.py b/backend/app/database/albums.py index e48980532..eccb39740 100644 --- a/backend/app/database/albums.py +++ b/backend/app/database/albums.py @@ -28,7 +28,8 @@ def logged_db_connection(action: str): def db_create_albums_table() -> None: with logged_db_connection("creating albums table") as conn: cursor = conn.cursor() - cursor.execute(""" + cursor.execute( + """ CREATE TABLE IF NOT EXISTS albums ( album_id TEXT PRIMARY KEY, album_name TEXT UNIQUE, @@ -37,7 +38,8 @@ def db_create_albums_table() -> None: password_hash TEXT, cover_image_path TEXT ) - """) + """ + ) # Shipped databases predate the is_hidden -> is_locked rename and the # cover_image_path column, and CREATE IF NOT EXISTS won't add either. cursor.execute("PRAGMA table_info(albums)") @@ -51,7 +53,8 @@ def db_create_albums_table() -> None: def db_create_album_images_table() -> None: with logged_db_connection("creating album_images table") as conn: cursor = conn.cursor() - cursor.execute(""" + cursor.execute( + """ CREATE TABLE IF NOT EXISTS album_images ( album_id TEXT, image_id TEXT, @@ -59,7 +62,8 @@ def db_create_album_images_table() -> None: FOREIGN KEY (album_id) REFERENCES albums(album_id) ON DELETE CASCADE, FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE ) - """) + """ + ) # The PK leads with album_id, so lookup by image alone needs its own # index. The memory scorer does exactly that, per image. cursor.execute( From 943245296c46bee5ecc435fd2ad3f867b6ddacde Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Tue, 11 Aug 2026 20:29:56 +0530 Subject: [PATCH 14/15] style: fix linting errors from import order and formatting --- backend/app/routes/albums.py | 96 ++++++++++++++++++++++-------------- backend/tests/test_albums.py | 7 ++- 2 files changed, 65 insertions(+), 38 deletions(-) diff --git a/backend/app/routes/albums.py b/backend/app/routes/albums.py index 73c5738e8..9d66f8102 100644 --- a/backend/app/routes/albums.py +++ b/backend/app/routes/albums.py @@ -6,33 +6,6 @@ from functools import wraps import sqlite3 from app.logging.setup_logging import get_logger - -logger = get_logger(__name__) - -P = ParamSpec("P") -R = TypeVar("R") - -def handle_route_exceptions(error_title: str, error_message: str) -> Callable[[Callable[P, R]], Callable[P, R]]: - def decorator(func: Callable[P, R]) -> Callable[P, R]: - @wraps(func) - def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: - try: - return func(*args, **kwargs) - except HTTPException: - raise - except Exception as e: - logger.error(f"Error in {func.__name__} route: {e}") - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=ErrorResponse( - success=False, - error=error_title, - message=error_message, - ).model_dump(), - ) - return wrapper - return decorator - from app.schemas.album import ( GetAlbumsResponse, CreateAlbumRequest, @@ -71,6 +44,38 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: album_util_create_from_memory, ) +logger = get_logger(__name__) + +P = ParamSpec("P") +R = TypeVar("R") + + +def handle_route_exceptions( + error_title: str, error_message: str +) -> Callable[[Callable[P, R]], Callable[P, R]]: + def decorator(func: Callable[P, R]) -> Callable[P, R]: + @wraps(func) + def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: + try: + return func(*args, **kwargs) + except HTTPException: + raise + except Exception as e: + logger.error(f"Error in {func.__name__} route: {e}") + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail=ErrorResponse( + success=False, + error=error_title, + message=error_message, + ).model_dump(), + ) + + return wrapper + + return decorator + + router = APIRouter() @@ -96,7 +101,9 @@ def _internal_error(message: str) -> HTTPException: # GET /albums/ - Get all albums (including locked ones) @router.get("/", response_model=GetAlbumsResponse) -@handle_route_exceptions("Internal Server Error", "An unexpected error occurred while fetching albums.") +@handle_route_exceptions( + "Internal Server Error", "An unexpected error occurred while fetching albums." +) def get_albums(): """Get all albums. Always returns both locked and unlocked albums.""" albums = db_get_all_albums() @@ -128,7 +135,9 @@ def get_albums(): # POST /albums/ - Create a new album @router.post("/", response_model=CreateAlbumResponse) -@handle_route_exceptions("Internal Server Error", "An unexpected error occurred while creating the album.") +@handle_route_exceptions( + "Internal Server Error", "An unexpected error occurred while creating the album." +) def create_album(body: CreateAlbumRequest): existing_album = db_get_album_by_name(body.name) if existing_album: @@ -183,7 +192,6 @@ def create_album_from_memory( except AlbumNameTakenError as e: raise _album_exists(body.name) from e - return CreateAlbumFromMemoryResponse( success=True, message=f"Created album '{body.name}' with {result['image_count']} photos", @@ -193,7 +201,9 @@ def create_album_from_memory( # GET /albums/{album_id} - Get specific album details @router.get("/{album_id}", response_model=GetAlbumResponse) -@handle_route_exceptions("Internal Server Error", "An unexpected error occurred while fetching the album.") +@handle_route_exceptions( + "Internal Server Error", "An unexpected error occurred while fetching the album." +) def get_album(album_id: str = Path(...)): album = db_get_album(album_id) if not album: @@ -224,7 +234,9 @@ def get_album(album_id: str = Path(...)): # PUT /albums/{album_id} - Update Album @router.put("/{album_id}", response_model=SuccessResponse) -@handle_route_exceptions("Failed to Update Album", "An unexpected error occurred while updating the album.") +@handle_route_exceptions( + "Failed to Update Album", "An unexpected error occurred while updating the album." +) def update_album(album_id: str = Path(...), body: UpdateAlbumRequest = Body(...)): album = db_get_album(album_id) @@ -270,7 +282,9 @@ def update_album(album_id: str = Path(...), body: UpdateAlbumRequest = Body(...) # DELETE /albums/{album_id} - Delete an album @router.delete("/{album_id}", response_model=SuccessResponse) -@handle_route_exceptions("Failed to Delete Album", "An unexpected error occurred while deleting the album.") +@handle_route_exceptions( + "Failed to Delete Album", "An unexpected error occurred while deleting the album." +) def delete_album(album_id: str = Path(...)): album = db_get_album(album_id) @@ -290,7 +304,9 @@ def delete_album(album_id: str = Path(...)): # GET /albums/{album_id}/images - Get all images in an album @router.post("/{album_id}/images/get", response_model=GetAlbumImagesResponse) -@handle_route_exceptions("Failed to Retrieve Images", "An unexpected error occurred while retrieving images.") +@handle_route_exceptions( + "Failed to Retrieve Images", "An unexpected error occurred while retrieving images." +) # GET requests do not accept a body by default. # Since we need to send a password securely, switching this to POST -- necessary. # Open to suggestions if better approach possible. @@ -335,7 +351,9 @@ def get_album_images( # POST /albums/{album_id}/images - Add images to an album @router.post("/{album_id}/images", response_model=SuccessResponse) -@handle_route_exceptions("Failed to Add Images", "An unexpected error occurred while adding images.") +@handle_route_exceptions( + "Failed to Add Images", "An unexpected error occurred while adding images." +) def add_images_to_album(album_id: str = Path(...), body: ImageIdsRequest = Body(...)): album = db_get_album(album_id) @@ -375,7 +393,9 @@ def add_images_to_album(album_id: str = Path(...), body: ImageIdsRequest = Body( # DELETE /albums/{album_id}/images/{image_id} - Remove image from album @router.delete("/{album_id}/images/{image_id}", response_model=SuccessResponse) -@handle_route_exceptions("Failed to Remove Image", "An unexpected error occurred while removing the image.") +@handle_route_exceptions( + "Failed to Remove Image", "An unexpected error occurred while removing the image." +) def remove_image_from_album(album_id: str = Path(...), image_id: str = Path(...)): album = db_get_album(album_id) @@ -405,7 +425,9 @@ def remove_image_from_album(album_id: str = Path(...), image_id: str = Path(...) # DELETE /albums/{album_id}/images - Remove multiple images from album @router.delete("/{album_id}/images", response_model=SuccessResponse) -@handle_route_exceptions("Failed to Remove Images", "An unexpected error occurred while removing the images.") +@handle_route_exceptions( + "Failed to Remove Images", "An unexpected error occurred while removing the images." +) def remove_images_from_album( album_id: str = Path(...), body: ImageIdsRequest = Body(...) ): diff --git a/backend/tests/test_albums.py b/backend/tests/test_albums.py index b257a00c4..1d0d35c60 100644 --- a/backend/tests/test_albums.py +++ b/backend/tests/test_albums.py @@ -702,6 +702,7 @@ class TestAlbumRouteErrors: def test_unexpected_exceptions_mapped_to_500(self): """Verify that an unexpected exception returns a 500 error.""" from unittest.mock import patch + with patch("app.routes.albums.db_get_all_albums") as mock_get_all: mock_get_all.side_effect = Exception("Database explosion") response = client.get("/albums/") @@ -714,8 +715,12 @@ def test_duplicate_album_integrity_error(self): """Verify that a database IntegrityError returns a 409 conflict.""" import sqlite3 from unittest.mock import patch + with patch("app.routes.albums.db_get_album_by_name", return_value=None): - with patch("app.routes.albums.db_insert_album", side_effect=sqlite3.IntegrityError("Unique constraint failed")): + with patch( + "app.routes.albums.db_insert_album", + side_effect=sqlite3.IntegrityError("Unique constraint failed"), + ): response = client.post( "/albums/", json={"name": "Duplicate", "description": "This should fail"}, From 220601331d2836714bce4e798040aa29a0885d70 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Tue, 11 Aug 2026 20:36:28 +0530 Subject: [PATCH 15/15] fix: restore albums module in test database monkeypatch to fix failing backend tests --- backend/tests/test_memory_signals_db.py | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/tests/test_memory_signals_db.py b/backend/tests/test_memory_signals_db.py index 2db1a20db..d6bd453e0 100644 --- a/backend/tests/test_memory_signals_db.py +++ b/backend/tests/test_memory_signals_db.py @@ -75,6 +75,7 @@ def test_db(monkeypatch: pytest.MonkeyPatch) -> Iterator[str]: "app.database.images", "app.database.folders", "app.database.yolo_mapping", + "app.database.albums", "app.database.connection", "app.database.faces", "app.database.face_clusters",