From 829cc77bacbfe54e3d4b26f73e590272cfc4ede0 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Mon, 22 Jun 2026 01:38:23 +0530 Subject: [PATCH 1/3] refactor: use specific sqlite3 exceptions and logger in database modules - Replaced broad except Exception blocks with specific sqlite3.Error in metadata, faces, and face_clusters database helper functions. - Captured transaction errors and logged rollbacks in connection context manager. - Replaced basic print statements with project's standard get_logger logging. --- backend/app/database/connection.py | 6 +++++- backend/app/database/face_clusters.py | 10 +++++++--- backend/app/database/faces.py | 7 +++++-- backend/app/database/metadata.py | 7 +++++-- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/backend/app/database/connection.py b/backend/app/database/connection.py index 599526dc1..d82ad6950 100644 --- a/backend/app/database/connection.py +++ b/backend/app/database/connection.py @@ -2,6 +2,9 @@ from contextlib import contextmanager from typing import Generator from app.config.settings import DATABASE_PATH +from app.logging.setup_logging import get_logger + +logger = get_logger(__name__) @contextmanager @@ -25,7 +28,8 @@ def get_db_connection() -> Generator[sqlite3.Connection, None, None]: try: yield conn conn.commit() - except Exception: + except Exception as e: + logger.error(f"Database transaction failed, rolling back: {e}") conn.rollback() raise finally: diff --git a/backend/app/database/face_clusters.py b/backend/app/database/face_clusters.py index ba9f4ba4a..9ba273263 100644 --- a/backend/app/database/face_clusters.py +++ b/backend/app/database/face_clusters.py @@ -1,6 +1,9 @@ import sqlite3 from typing import Optional, List, Dict, TypedDict, Union from app.config.settings import DATABASE_PATH +from app.logging.setup_logging import get_logger + +logger = get_logger(__name__) # Type definitions ClusterId = str @@ -60,10 +63,10 @@ def db_delete_all_clusters(cursor: Optional[sqlite3.Cursor] = None) -> int: if own_connection: conn.commit() return deleted_count - except Exception: + except sqlite3.Error as e: if own_connection: conn.rollback() - print("Error deleting all clusters.") + logger.error(f"Error deleting all clusters: {e}") raise finally: if own_connection: @@ -114,9 +117,10 @@ def db_insert_clusters_batch( if own_connection: conn.commit() return cluster_ids - except Exception: + except sqlite3.Error as e: if own_connection: conn.rollback() + logger.error(f"Error inserting clusters batch: {e}") raise finally: if own_connection: diff --git a/backend/app/database/faces.py b/backend/app/database/faces.py index 0e43f7117..3262e5a85 100644 --- a/backend/app/database/faces.py +++ b/backend/app/database/faces.py @@ -3,6 +3,9 @@ import numpy as np from typing import Optional, List, Dict, Union, TypedDict from app.config.settings import DATABASE_PATH +from app.logging.setup_logging import get_logger + +logger = get_logger(__name__) # Type definitions FaceId = int @@ -331,10 +334,10 @@ def db_update_face_cluster_ids_batch( if own_connection: conn.commit() - except Exception: + except sqlite3.Error as e: if own_connection: conn.rollback() - print("Error updating face cluster IDs in batch.") + logger.error(f"Error updating face cluster IDs in batch: {e}") raise finally: if own_connection: diff --git a/backend/app/database/metadata.py b/backend/app/database/metadata.py index d431f6e2b..c24723fae 100644 --- a/backend/app/database/metadata.py +++ b/backend/app/database/metadata.py @@ -3,6 +3,9 @@ import json from typing import Optional, Dict, Any from app.config.settings import DATABASE_PATH +from app.logging.setup_logging import get_logger + +logger = get_logger(__name__) def db_create_metadata_table() -> None: @@ -84,11 +87,11 @@ def db_update_metadata( if own_connection: conn.commit() return success - except Exception as e: + except sqlite3.Error as e: if own_connection: conn.rollback() - print(f"Error updating metadata: {e}") + logger.error(f"Error updating metadata: {e}") raise finally: if own_connection: From 41f5c28a48457e3474861574f901b4178c3ac9d3 Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 26 Jun 2026 01:04:40 +0530 Subject: [PATCH 2/3] fix(database): separate data preparation from DB transactions in metadata and faces Per Shevill's review on PR #1335: - In metadata.py (db_update_metadata): move json.dumps() outside the DB try block into its own try/except (TypeError, ValueError) block with logger.error. This ensures non-serializable metadata is caught and logged before the sqlite3.Error transaction block. - In faces.py (db_update_face_cluster_ids_batch): move list comprehension for update_data outside the DB try block into its own try/except (AttributeError, KeyError, TypeError) block with logger.error. This ensures malformed mapping dicts are caught and logged before the sqlite3.Error transaction block. Both changes ensure sqlite3.Error remains the correct and only exception to catch in the DB transaction, while non-DB Python errors are logged explicitly. --- backend/app/database/faces.py | 20 ++++++++++++++------ backend/app/database/metadata.py | 13 ++++++++++--- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/backend/app/database/faces.py b/backend/app/database/faces.py index 3262e5a85..4099e7aec 100644 --- a/backend/app/database/faces.py +++ b/backend/app/database/faces.py @@ -315,14 +315,22 @@ def db_update_face_cluster_ids_batch( conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() + # 1. Prepare update data outside the DB transaction. + # mapping.get() calls could raise AttributeError if mappings are malformed; + # these are not sqlite3 errors and must be caught before the transaction. try: - # Prepare update data as tuples (cluster_id, face_id) - update_data = [] - for mapping in face_cluster_mapping: - face_id = mapping.get("face_id") - cluster_id = mapping.get("cluster_id") - update_data.append((cluster_id, face_id)) + update_data = [ + (mapping.get("cluster_id"), mapping.get("face_id")) + for mapping in face_cluster_mapping + ] + except (AttributeError, KeyError, TypeError) as e: + if own_connection: + conn.close() + logger.error(f"Failed to prepare face cluster update data: {e}") + raise + # 2. Database transaction — only pure DB operations here, so sqlite3.Error is safe. + try: cursor.executemany( """ UPDATE faces diff --git a/backend/app/database/metadata.py b/backend/app/database/metadata.py index c24723fae..7049f58b4 100644 --- a/backend/app/database/metadata.py +++ b/backend/app/database/metadata.py @@ -71,14 +71,22 @@ def db_update_metadata( Returns: True if the metadata was updated, False otherwise """ + # 1. Prepare/serialize data outside the DB transaction. + # json.dumps can raise TypeError/ValueError for non-serializable objects; + # these are not sqlite3 errors and must be caught separately. + try: + metadata_json = json.dumps(metadata) + except (TypeError, ValueError) as e: + logger.error(f"Failed to serialize metadata: {e}") + raise + own_connection = cursor is None if own_connection: conn = sqlite3.connect(DATABASE_PATH) cursor = conn.cursor() + # 2. Database transaction — only pure DB operations here, so sqlite3.Error is safe. try: - metadata_json = json.dumps(metadata) - # Delete all existing rows and insert new one cursor.execute("DELETE FROM metadata") cursor.execute("INSERT INTO metadata (metadata) VALUES (?)", (metadata_json,)) @@ -90,7 +98,6 @@ def db_update_metadata( except sqlite3.Error as e: if own_connection: conn.rollback() - logger.error(f"Error updating metadata: {e}") raise finally: From 8c06399eded97aa3dc41cf26d0c675d1ba4b560e Mon Sep 17 00:00:00 2001 From: Dushyant Acharya Date: Fri, 26 Jun 2026 14:07:33 +0530 Subject: [PATCH 3/3] fix(metadata): handle owned connection setup inside sqlite error path --- backend/app/database/metadata.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/backend/app/database/metadata.py b/backend/app/database/metadata.py index 7049f58b4..7e208ae15 100644 --- a/backend/app/database/metadata.py +++ b/backend/app/database/metadata.py @@ -81,12 +81,14 @@ def db_update_metadata( raise own_connection = cursor is None - if own_connection: - conn = sqlite3.connect(DATABASE_PATH) - cursor = conn.cursor() + conn = None - # 2. Database transaction — only pure DB operations here, so sqlite3.Error is safe. + # 2. Database transaction / owned connection setup. try: + if own_connection: + conn = sqlite3.connect(DATABASE_PATH) + cursor = conn.cursor() + # Delete all existing rows and insert new one cursor.execute("DELETE FROM metadata") cursor.execute("INSERT INTO metadata (metadata) VALUES (?)", (metadata_json,)) @@ -97,9 +99,10 @@ def db_update_metadata( return success except sqlite3.Error as e: if own_connection: - conn.rollback() + if conn is not None: + conn.rollback() logger.error(f"Error updating metadata: {e}") raise finally: - if own_connection: + if own_connection and conn is not None: conn.close()