Bugfix/orphaned rows cascade delete#1407
Conversation
WalkthroughDatabase helpers now use ChangesDatabase connection migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 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
| monkeypatch.setattr(images_module, "DATABASE_PATH", db_path) | ||
| monkeypatch.setattr(folders_module, "DATABASE_PATH", db_path) | ||
| monkeypatch.setattr(yolo_mapping_module, "DATABASE_PATH", db_path) | ||
| monkeypatch.setattr(connection_module, "DATABASE_PATH", db_path) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Exercise the cascade through an FK-enabled delete connection.
The cascade test still deletes via _connect() (Lines 167-170), which the PR identifies as foreign-key-disabled. SQLite applies cascades on the connection executing DELETE, so this test cannot validate the intended behavior—and the production delete path remains unaffected if it also uses _connect(). Use get_db_connection() for the delete and migrate the application image-deletion connection as well.
🤖 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/tests/test_image_embeddings.py` at 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.
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
Bug Fixes
Tests