Bugfix/orphaned rows cascade delete - #1407
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDatabase helpers now use ChangesDatabase connection migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/tests/test_image_embeddings.py`:
- Line 40: The cascade test should execute DELETE through get_db_connection()
rather than _connect(), and the application image-deletion path should use
get_db_connection() as well. Update the relevant test delete block and
production image-deletion method, preserving the existing cascade behavior and
connection lifecycle.
🪄 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: b4d90432-c4fe-4078-8c32-3755a3d450ef
📒 Files selected for processing (4)
backend/app/database/image_embeddings.pybackend/app/database/semantic_labels.pybackend/tests/test_image_embeddings.pybackend/tests/test_semantic_labels.py
…d cascading delete test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/database/images.py (1)
470-479: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for cascade deletion.
Verify that deleting an image also removes its related
image_embeddingsandimage_semantic_labelsrows. The supplied coverage shows schema setup but not this end-to-end deletion contract. As per path instructions, critical functionality should be covered by automated 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/images.py` around lines 470 - 479, Add a regression test covering the image deletion flow that executes the relevant deletion operation and verifies the associated image_embeddings and image_semantic_labels rows are also removed. Reuse the existing database/schema setup and image deletion symbols exposed by the test suite, and assert both related tables are empty for the deleted image while preserving unrelated records.Source: Path instructions
🤖 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/images.py`:
- Line 13: Update _connect and the image helper operations to use the same
shared connection path as db_delete_images_by_ids via get_db_connection,
removing reliance on the module-level DATABASE_PATH and direct sqlite3.connect
calls. Ensure setup, reads, and deletes all target the manager-selected
database, including when tests patch the shared manager.
---
Nitpick comments:
In `@backend/app/database/images.py`:
- Around line 470-479: Add a regression test covering the image deletion flow
that executes the relevant deletion operation and verifies the associated
image_embeddings and image_semantic_labels rows are also removed. Reuse the
existing database/schema setup and image deletion symbols exposed by the test
suite, and assert both related tables are empty for the deleted image while
preserving unrelated records.
🪄 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 Plus
Run ID: a1a90b1a-ccfe-4c62-b2f0-098f382f319c
📒 Files selected for processing (2)
backend/app/database/images.pybackend/tests/test_image_embeddings.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/test_image_embeddings.py
|
@rohan-pandeyy Could u pls review this pr? |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/database/images.py (1)
24-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWire
SQLITE_ID_CHUNKintodb_delete_images_by_ids.
db_delete_images_by_ids()buildsDELETE FROM images WHERE id IN (...)with one placeholder per ID, so calls with 999+ IDs can exceed SQLite’s pre-3.32 host parameter limit even thoughSQLITE_ID_CHUNK = 500is declared but unused. Splitimage_idsinto chunks before constructing theINclause.🤖 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/images.py` around lines 24 - 27, Update db_delete_images_by_ids to split image_ids into SQLITE_ID_CHUNK-sized batches before constructing the DELETE IN clauses, executing the deletion for each batch while preserving existing behavior for smaller inputs.
🧹 Nitpick comments (1)
backend/app/database/connection.py (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the long docstring line.
Line 14 exceeds PEP 8’s recommended 79-character limit. Reflow the docstring without changing behavior.
Proposed fix
def get_database_path() -> str: - """Resolve the active database path dynamically, supporting various test patching styles.""" + """Resolve the active database path dynamically. + + Supports various test patching styles. + """As per path instructions, Python files must follow PEP 8.
🤖 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` at line 14, Wrap the long module docstring near the database-path resolution code across multiple lines to stay within PEP 8’s 79-character limit, preserving its wording and behavior.Source: Path instructions
🤖 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.
Outside diff comments:
In `@backend/app/database/images.py`:
- Around line 24-27: Update db_delete_images_by_ids to split image_ids into
SQLITE_ID_CHUNK-sized batches before constructing the DELETE IN clauses,
executing the deletion for each batch while preserving existing behavior for
smaller inputs.
---
Nitpick comments:
In `@backend/app/database/connection.py`:
- Line 14: Wrap the long module docstring near the database-path resolution code
across multiple lines to stay within PEP 8’s 79-character limit, preserving its
wording and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fcd4dd05-f2ad-4855-a6c0-036ae668b3ba
📒 Files selected for processing (4)
backend/app/database/connection.pybackend/app/database/image_embeddings.pybackend/app/database/images.pybackend/tests/test_image_embeddings.py
1 participant
Addressed Issues:
Fixes #1405
Screenshots/Recordings:
N/A (This is a database integrity update).
Additional Notes:
Both the
image_embeddingsandimage_semantic_labelstables define foreign key constraints withON DELETE CASCADEreferencingimages(id). This configuration is intended to automatically delete embeddings and semantic labels when an image is deleted.However, both modules established database connections using the private
_connect()helper fromapp.database.images, which does not runPRAGMA foreign_keys = ON;. Because SQLite disables foreign key constraints by default, image deletions left orphaned records in these tables.Changes made in this PR:
backend/app/database/image_embeddings.pyto use theget_db_connection()context manager fromapp.database.connection.backend/app/database/semantic_labels.pyto use theget_db_connection()context manager.backend/tests/test_image_embeddings.py's_isolated_dbtest fixture to patchapp.database.connection.DATABASE_PATH, redirecting the context manager to the test database and ensuring all tests pass cleanly.backend/tests/test_semantic_labels.py's_isolated_dbtest fixture to patchapp.database.connection.DATABASE_PATH, ensuring the new semantic search tests pass cleanly.AI Usage Disclosure:
I have used the following AI models and tools: Gemini 3.5 Flash .
Checklist
Summary by CodeRabbit