perf: avoid full table scan in toggle-favourite endpoint. Fixes #1223#1228
perf: avoid full table scan in toggle-favourite endpoint. Fixes #1223#1228ajithhraj wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe changes add a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested labels
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)
📝 Coding Plan
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/routes/images.py (1)
109-114:⚠️ Potential issue | 🟠 MajorGuard
db_get_image_by_id()before dereferencing and preserve HTTP exceptions
db_get_image_by_id()can returnNone; Line 113 can crash on.get(...). Also, anyHTTPExceptionraised in this block is currently converted to 500 by the broadexcept.Suggested fix
`@router.post`("/toggle-favourite") def toggle_favourite(req: ToggleFavouriteRequest): image_id = req.image_id try: success = db_toggle_image_favourite_status(image_id) if not success: raise HTTPException( status_code=404, detail="Image not found or failed to toggle" ) # Fetch updated status to return image = db_get_image_by_id(image_id) + if image is None: + raise HTTPException( + status_code=500, + detail="Unable to fetch updated favourite status", + ) return { "success": True, "image_id": image_id, "isFavourite": image.get("isFavourite", False), } + except HTTPException: + raise except Exception as e: logger.error(f"error in /toggle-favourite route: {e}") raise HTTPException(status_code=500, detail=f"Internal server error: {e}")Also applies to: 116-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/routes/images.py` around lines 109 - 114, Guard the result of db_get_image_by_id(image_id) before calling .get by checking for None and returning an appropriate HTTP error (e.g., raise HTTPException(status_code=404, detail="Image not found")) instead of letting a None dereference crash; also when catching exceptions in the surrounding try/except, re-raise any caught fastapi.HTTPException (or avoid catching it) so HTTP errors aren't converted to 500s. Update both the block that builds the response using image.get("isFavourite", False) and the other similar block that uses db_get_image_by_id to apply the same None check and HTTPException preservation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/database/images.py`:
- Around line 299-300: The code serializes folder_id using str(first_row[2])
which converts NULL to the literal "None"; update the image record construction
(the dict containing "folder_id" and "thumbnailPath") to set "folder_id" to None
when first_row[2] is None (e.g., use a conditional: folder_id =
str(first_row[2]) if first_row[2] is not None else None) so the JSON contains a
null value instead of the string "None".
In `@backend/tests/test_images.py`:
- Around line 16-36: Add a failing-path unit test alongside
test_toggle_favourite_uses_single_image_lookup that simulates
mock_toggle_favourite returning True while mock_get_image_by_id returns None:
call client.post("/images/toggle-favourite", json={"image_id": image_id}) using
the same image_id, assert response.status_code == 200, assert
response.json()["success"] is True and response.json()["image_id"] == image_id,
assert that the route handles the missing image lookup by exposing an
isFavourite field (or explicit null) — e.g., assert "isFavourite" in
response.json() and response.json()["isFavourite"] is None — and finally verify
mock_toggle_favourite.assert_called_once_with(image_id) and
mock_get_image_by_id.assert_called_once_with(image_id).
---
Outside diff comments:
In `@backend/app/routes/images.py`:
- Around line 109-114: Guard the result of db_get_image_by_id(image_id) before
calling .get by checking for None and returning an appropriate HTTP error (e.g.,
raise HTTPException(status_code=404, detail="Image not found")) instead of
letting a None dereference crash; also when catching exceptions in the
surrounding try/except, re-raise any caught fastapi.HTTPException (or avoid
catching it) so HTTP errors aren't converted to 500s. Update both the block that
builds the response using image.get("isFavourite", False) and the other similar
block that uses db_get_image_by_id to apply the same None check and
HTTPException preservation.
🪄 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: b47f25ca-0a13-4d50-b795-b153ab31d3ee
📒 Files selected for processing (3)
backend/app/database/images.pybackend/app/routes/images.pybackend/tests/test_images.py
|
Closing this as unreviewed issue |
2 participants
Addressed Issues:
Fixes #1223
Screenshots/Recordings:
Not applicable.
Additional Notes:
This PR avoids a full table scan in the
/toggle-favouriteendpoint by replacing thedb_get_all_images()scan with a targeteddb_get_image_by_id(image_id)lookup.Changes made:
db_get_image_by_id(image_id)inbackend/app/database/images.py/toggle-favouriteinbackend/app/routes/images.pyto fetch only the toggled imageVerified locally with:
$env:GITHUB_ACTIONS='true'; python -m pytest backend\tests\test_images.pypython -m py_compile backend\app\database\images.py backend\app\routes\images.pyAI Usage Disclosure:
I have used the following AI models and tools: OpenAI Codex (GPT-5)
Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests