refactor: replace broad except blocks with specific exceptions and logger#1335
refactor: replace broad except blocks with specific exceptions and logger#1335Dotify71 wants to merge 2 commits into
Conversation
- 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.
WalkthroughFour backend database modules add module-level loggers and replace broad exception handling with sqlite-specific error handling and structured logging. ChangesDatabase Error Handling and Logging Refactor
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
backend/app/database/metadata.py (1)
90-94: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCapture traceback for metadata update failures.
Use
logger.exception(...)(orlogger.error(..., exc_info=True)) to retain full failure context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/metadata.py` around lines 90 - 94, In the sqlite3.Error exception handler where metadata updates fail, replace the current logger.error call that only logs the error message with logger.exception to capture the full traceback information. Alternatively, you can keep logger.error but add the exc_info=True parameter to include stack trace details. This will provide complete failure context for debugging issues in the metadata update operation.backend/app/database/face_clusters.py (1)
66-69: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winInclude traceback for SQLite failure logs.
Both handlers currently log only
str(e). Switch tologger.exception(...)(orexc_info=True) to keep actionable stack traces.Suggested patch
- except sqlite3.Error as e: + except sqlite3.Error: if own_connection: conn.rollback() - logger.error(f"Error deleting all clusters: {e}") + logger.exception("Error deleting all clusters") raise ... - except sqlite3.Error as e: + except sqlite3.Error: if own_connection: conn.rollback() - logger.error(f"Error inserting clusters batch: {e}") + logger.exception("Error inserting clusters batch") raiseAlso applies to: 120-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/face_clusters.py` around lines 66 - 69, The sqlite3.Error exception handlers in the delete_all_clusters function (around line 66-69) and the second location mentioned (around lines 120-123) are using logger.error() which only logs the error message without the traceback. Replace these logger.error() calls with logger.exception() to automatically capture and include the full stack trace in the logs, which will aid in debugging SQLite failures.backend/app/database/connection.py (2)
31-32: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPreserve traceback in error logs.
Use
logger.exception(...)(orlogger.error(..., exc_info=True)) so failures include stack traces, not just the exception string.Suggested patch
- except Exception as e: - logger.error(f"Database transaction failed, rolling back: {e}") + except Exception: + logger.exception("Database transaction failed, rolling back") conn.rollback() raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/connection.py` around lines 31 - 32, The error logging in the exception handler for the database transaction failure does not preserve the full stack trace. Replace the logger.error call with logger.exception to automatically include the complete traceback information, which will aid in debugging. Alternatively, if you prefer to keep logger.error, add the exc_info=True parameter to include the stack trace. The error message string in the "Database transaction failed, rolling back" log statement should be preserved while ensuring the exception details are properly captured in the logs.
31-34: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftAdd regression tests for rollback + re-raise behavior.
This branch is core transaction control flow and should be covered with automated tests for both DB and non-DB exceptions.
As per coding guidelines, “Ensure that test code is automated, comprehensive, and follows testing best practices” and “Verify that all critical functionality is covered by tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/connection.py` around lines 31 - 34, Add comprehensive automated regression tests for the exception handler in the database connection transaction block that catches Exception as e and calls rollback then re-raise. Create test cases that cover both database-specific exceptions and general exceptions to verify that the logger.error call properly logs the error message, the conn.rollback method is invoked, and the exception is properly re-raised to the caller. These tests should validate the critical transaction control flow behavior is working as expected.Source: Coding guidelines
backend/app/database/faces.py (1)
337-340: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winLog batch update failures with traceback.
Prefer
logger.exception("Error updating face cluster IDs in batch")so debugging includes stack context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/database/faces.py` around lines 337 - 340, In the sqlite3.Error exception handler where the error message is logged for the face cluster ID batch update operation, replace the logger.error call that includes the exception variable e in an f-string with logger.exception instead. The logger.exception method automatically captures and includes the full traceback and stack context in the log output, eliminating the need to manually append the error to the message string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/app/database/connection.py`:
- Around line 31-32: The error logging in the exception handler for the database
transaction failure does not preserve the full stack trace. Replace the
logger.error call with logger.exception to automatically include the complete
traceback information, which will aid in debugging. Alternatively, if you prefer
to keep logger.error, add the exc_info=True parameter to include the stack
trace. The error message string in the "Database transaction failed, rolling
back" log statement should be preserved while ensuring the exception details are
properly captured in the logs.
- Around line 31-34: Add comprehensive automated regression tests for the
exception handler in the database connection transaction block that catches
Exception as e and calls rollback then re-raise. Create test cases that cover
both database-specific exceptions and general exceptions to verify that the
logger.error call properly logs the error message, the conn.rollback method is
invoked, and the exception is properly re-raised to the caller. These tests
should validate the critical transaction control flow behavior is working as
expected.
In `@backend/app/database/face_clusters.py`:
- Around line 66-69: The sqlite3.Error exception handlers in the
delete_all_clusters function (around line 66-69) and the second location
mentioned (around lines 120-123) are using logger.error() which only logs the
error message without the traceback. Replace these logger.error() calls with
logger.exception() to automatically capture and include the full stack trace in
the logs, which will aid in debugging SQLite failures.
In `@backend/app/database/faces.py`:
- Around line 337-340: In the sqlite3.Error exception handler where the error
message is logged for the face cluster ID batch update operation, replace the
logger.error call that includes the exception variable e in an f-string with
logger.exception instead. The logger.exception method automatically captures and
includes the full traceback and stack context in the log output, eliminating the
need to manually append the error to the message string.
In `@backend/app/database/metadata.py`:
- Around line 90-94: In the sqlite3.Error exception handler where metadata
updates fail, replace the current logger.error call that only logs the error
message with logger.exception to capture the full traceback information.
Alternatively, you can keep logger.error but add the exc_info=True parameter to
include stack trace details. This will provide complete failure context for
debugging issues in the metadata update operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80b66d8d-f1a5-4f9b-b3c4-d68aa4d59744
📒 Files selected for processing (4)
backend/app/database/connection.pybackend/app/database/face_clusters.pybackend/app/database/faces.pybackend/app/database/metadata.py
|
@rohan-pandeyy can you give a quick review in this. |
Shevilll
left a comment
Shevilll
left a comment
There was a problem hiding this comment.
Peer Review: Refactoring Exception Handling and Logging
Thank you for addressing the broad exception blocks and replacing standard print statements with the project's standard logger (get_logger). This is a solid step toward production-grade backend diagnostics.
While the refactoring of print to logger.error is correct, there are critical transaction and exception safety implications introduced by replacing except Exception with except sqlite3.Error within these manual transaction blocks.
1. The Risk of Non-Database Exceptions in Transaction Blocks
By changing the exception filter to sqlite3.Error, any non-database Python exception (e.g., TypeError, KeyError, AttributeError) raised inside the try block will bypass the except sqlite3.Error block. Consequently, conn.rollback() will not be explicitly called in the except block.
Although SQLite automatically aborts/rolls back uncommitted transactions when a connection is closed (which happens in the finally block), relying on this implicit behavior is less robust than explicit rollbacks. Moreover, the error will bypass your structured logger and bubble up as an unlogged crash.
Examples of Potential Failures:
-
In
backend/app/database/metadata.py(db_update_metadata):try: metadata_json = json.dumps(metadata) # Can raise TypeError if metadata contains non-serializable objects cursor.execute("DELETE FROM metadata") cursor.execute("INSERT INTO metadata (metadata) VALUES (?)", (metadata_json,)) ... except sqlite3.Error as e: # Will NOT catch TypeError!
If
json.dumps()raises aTypeError, the connection is closed infinallywithout explicit rollback or logger recording. -
In
backend/app/database/faces.py(db_update_face_cluster_ids_batch):try: update_data = [] for mapping in face_cluster_mapping: # Can raise KeyError/AttributeError if mapping is malformed face_id = mapping.get("face_id") cluster_id = mapping.get("cluster_id") update_data.append((cluster_id, face_id)) cursor.executemany(...) ... except sqlite3.Error as e: # Will NOT catch KeyError or AttributeError!
2. Recommended Action Items
To resolve these safety concerns, you should adopt one of the following two patterns:
Option A: Separate Data Preparation from DB Transactions (Recommended)
Perform all Python-level data validation and serialization before opening the database transaction. This ensures the try block only wraps pure database operations, making sqlite3.Error the correct and safe exception to catch.
For metadata.py:
def db_update_metadata(metadata: Dict[str, Any], cursor: Optional[sqlite3.Cursor] = None) -> bool:
# 1. Prepare/serialize data outside transaction
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
try:
cursor.execute("DELETE FROM metadata")
cursor.execute("INSERT INTO metadata (metadata) VALUES (?)", (metadata_json,))
success = cursor.rowcount > 0
if own_connection:
conn.commit()
return success
except sqlite3.Error as e:
if own_connection:
conn.rollback()
logger.error(f"Error updating metadata: {e}")
raise
finally:
if own_connection:
conn.close()3. High-Level Architectural Improvement (Bonus)
The repository defines a database connection context manager in backend/app/database/connection.py called get_db_connection():
@contextmanager
def get_db_connection() -> Generator[sqlite3.Connection, None, None]:
conn = sqlite3.connect(DATABASE_PATH)
try:
yield conn
conn.commit()
except Exception as e:
logger.error(f"Database transaction failed, rolling back: {e}")
conn.rollback()
raise
finally:
conn.close()If the database modules are refactored to use this context manager, all of this boilerplate transaction code (try/except/finally/commit/rollback/close) is eliminated! It automatically handles rollbacks for all exception types, ensuring complete safety.
Using this context manager would unify the transaction strategy across the entire backend. While a full refactor might be out of scope for this specific PR, it is highly recommended to transition toward it in the future.
…data and faces Per Shevill's review on PR AOSSIE-Org#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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/database/metadata.py`:
- Around line 83-86: The owned-connection setup in metadata handling is
currently outside the sqlite3.Error handling, so failures from sqlite3.connect()
or conn.cursor() bypass the logged DB error path and can leak an open
connection. Move the own_connection branch in the database/metadata.py flow into
the existing try/except sqlite3.Error block around the cursor setup, and ensure
any partially opened conn is closed in the exception path before re-raising or
returning. Use the existing connection/cursor setup logic in that function to
keep the error logging and cleanup consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73d0ac19-a8f2-4f11-9433-8d6c2bb21960
📒 Files selected for processing (2)
backend/app/database/faces.pybackend/app/database/metadata.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/database/faces.py
| own_connection = cursor is None | ||
| if own_connection: | ||
| conn = sqlite3.connect(DATABASE_PATH) | ||
| cursor = conn.cursor() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap owned connection setup in the sqlite3.Error path.
sqlite3.connect() and conn.cursor() can both fail, but Line 83-86 sits outside the try/except sqlite3.Error block. That regresses the PR objective for logged DB failures, and if conn.cursor() raises after connect() succeeds, the owned connection is never closed.
Suggested fix
- 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:
+ own_connection = cursor is None
+ conn = None
+
+ # 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,))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| own_connection = cursor is None | |
| if own_connection: | |
| conn = sqlite3.connect(DATABASE_PATH) | |
| cursor = conn.cursor() | |
| own_connection = cursor is None | |
| conn = None | |
| # 2. Database transaction / owned connection setup. | |
| try: | |
| if own_connection: | |
| conn = sqlite3.connect(DATABASE_PATH) | |
| cursor = conn.cursor() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/database/metadata.py` around lines 83 - 86, The owned-connection
setup in metadata handling is currently outside the sqlite3.Error handling, so
failures from sqlite3.connect() or conn.cursor() bypass the logged DB error path
and can leak an open connection. Move the own_connection branch in the
database/metadata.py flow into the existing try/except sqlite3.Error block
around the cursor setup, and ensure any partially opened conn is closed in the
exception path before re-raising or returning. Use the existing
connection/cursor setup logic in that function to keep the error logging and
cleanup consistent.
2 participants
Description
This PR refactors the exception handling in the database modules under
backend/app/database/(specificallyconnection.py,metadata.py,faces.py, andface_clusters.py). Broadexcept Exceptionblocks have been replaced with specific database exceptions (sqlite3.Error), and standard logger logging has been introduced instead of print statements to improve error reporting and maintainability.Changes
connection.py:get_loggerto capture and log transaction rollback occurrences.get_db_connectionsince it yields control to client blocks which can raise any standard Python error, but captured the error, logged the event, and re-raised it.metadata.py:except Exceptionindb_update_metadatawithexcept sqlite3.Error.print().faces.py:except Exceptionindb_update_face_cluster_ids_batchwithexcept sqlite3.Error.print().face_clusters.py:except Exceptionindb_delete_all_clustersanddb_insert_clusters_batchwithexcept sqlite3.Error.print()with standard logger messages.Closes #1334
Summary by CodeRabbit