Test corrupted db #5470
Test corrupted db #5470
Conversation
Reviewer's Guide by SourceryThis PR implements changes to handle corrupted database scenarios and modifies datetime handling in SQLite operations. The changes primarily focus on adding debug logging, improving error handling during database migrations, and implementing a new datetime adaptation mechanism for SQLite operations. Class diagram for AutoRetryCursor modificationsclassDiagram
class AutoRetryCursor {
+adapt_datetime_iso(val)
+execute(sql: str, parameters: Iterable[Any])
}
note for AutoRetryCursor "Added adapt_datetime_iso method and modified execute method to handle datetime adaptation for SQLite operations."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @gitofanindya - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The test appears to be intentionally failing with an invalid assertion (link)
Overall Comments:
- Remove all debug print statements and use the existing logging mechanism instead. Print statements are not appropriate for production code.
- The test modification
assert 1 == 0will always fail and should be removed. If this is a test for corrupted database handling, it should have meaningful assertions. - Exception handling should use specific exception types and proper error handling rather than broad Exception catches with print statements.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🔴 Testing: 1 blocking issue
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import sqlite3 | ||
| # return super().execute(sql, parameters) | ||
| # new_param = tuple( datetime.fromtimestamp(param, tz=timezone.utc) if isinstance(param, datetime) else param for param in parameters ) | ||
| new_param = tuple( sqlite3.register_adapter(param, self.adapt_datetime_iso) if isinstance(param, datetime) else param for param in parameters ) |
There was a problem hiding this comment.
issue (bug_risk): Registering SQLite adapters inside execute() is unsafe and inefficient
The register_adapter call modifies global state and should be done once at module level. This could cause thread-safety issues and adds unnecessary overhead on every query. Consider moving the adapter registration to module initialization.
| assert len(cols) == 3 | ||
| assert 1 == 0 |
There was a problem hiding this comment.
issue (testing): The test appears to be intentionally failing with an invalid assertion
The addition of assert 1 == 0 seems to be a debugging artifact that will always fail. This should be removed as it doesn't contribute to testing the database corruption scenario.
| class AutoRetryCursor(Cursor): | ||
| def adapt_datetime_iso(self, val): | ||
| return datetime.fromtimestamp(val.strftime('%s'), tz=timezone.utc) | ||
| def execute(self, sql: str, parameters: Iterable[Any] = ()) -> Cursor: |
There was a problem hiding this comment.
issue (complexity): Consider moving datetime adaptation logic out of the execute() method into a connection-level adapter setup.
The datetime handling in execute() adds unnecessary complexity by mixing adaptation concerns with execution logic. Instead of registering adapters per-parameter during execution, use SQLite's built-in adapter system:
# In _create_main_conn or __init__
def _setup_adapters(self, connection):
def adapt_datetime_iso(val):
return val.strftime('%s')
sqlite3.register_adapter(datetime, adapt_datetime_iso)
return connection
# Simplify execute() back to:
def execute(self, sql: str, parameters: Iterable[Any] = ()) -> Cursor:
count = 1
while True:
count += 1
try:
return super().execute(sql, parameters)
except OperationalError as exc:
log.info(
f"Retry locked database #{count}, {sql=}, {parameters=}",
exc_info=True,
)
if count > 5:
raise excThis approach:
- Registers the adapter once at connection time
- Keeps the cursor focused on its core responsibility
- Uses SQLite's built-in type adaptation system as intended
| self._create_table(cursor, name, force=True) | ||
| print(">>>> called _create_table") |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5470 +/- ##
=======================================
Coverage 52.08% 52.08%
=======================================
Files 96 96
Lines 16094 16144 +50
=======================================
+ Hits 8382 8409 +27
- Misses 7712 7735 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
Enhance logging in database operations and simulate a test failure scenario for corrupted database handling.
Enhancements:
Tests: