fix: add error handling to GET /bugs/{id} and GET /bugs/search endpoints#44
fix: add error handling to GET /bugs/{id} and GET /bugs/search endpoints#44SatishKumar620 wants to merge 4 commits intoOWASP-BLT:mainfrom
Conversation
📊 Monthly LeaderboardHi @SatishKumar620! Here's how you rank for March 2026:
Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run |
owasp-blt
bot
added
unresolved-conversations: 0
needs-peer-review
|
👋 Hi @SatishKumar620! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
|
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:
WalkthroughGET /bugs/search and GET /bugs/{id} in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14272
✅ Merge Readiness AssessmentGoal AlignmentThe PR fully addresses issue What's Done Well
Minor Pre-existing Observations (non-blocking, out of this PR's scope)
Verdict✅ The PR is complete and merge-ready. The core objective — consistent DB error handling across all bug endpoints — is fully achieved with no blocking issues. The observations above are pre-existing patterns not introduced by this PR and can be addressed in follow-up issues if desired. |
owasp-blt
bot
added
has-peer-review
|
@Nachiket-Roy Thanks |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/handlers/bugs.py (2)
62-96: Error handling implementation looks good.The try/except block correctly wraps the database operations, returns a generic error message to clients, and logs the full exception for debugging. This achieves the PR objective of consistent error handling.
One minor style improvement from static analysis:
💅 Minor: simplify f-string on line 95
except Exception as e: - logger.error(f"Error searching bugs: {str(e)}") + logger.error(f"Error searching bugs: {e}") return error_response("Failed to search bugs. Please try again later.", status=500)The
str(e)call is redundant inside an f-string since it implicitly converts to string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/bugs.py` around lines 62 - 96, The logger.error call in the exception handler of the search routine uses an unnecessary str() inside an f-string; update the exception logging in the except block (the logger.error in the try/except around db.prepare in this file) to interpolate the exception directly (e.g., use f"Error searching bugs: {e}") to simplify the f-string while keeping the same message.
105-184: Error handling for GET /bugs/{id} is well implemented.The try/except block properly covers all three database queries (main bug, screenshots, and tags). The approach of using a single try block is reasonable here - a partial response would add complexity, and a clean 500 error is appropriate when any part of the data retrieval fails.
Same minor style suggestion as the search endpoint:
💅 Minor: simplify f-string on line 183
except Exception as e: - logger.error(f"Error fetching bug {bug_id}: {str(e)}") + logger.error(f"Error fetching bug {bug_id}: {e}") return error_response("Failed to fetch bug. Please try again later.", status=500)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/bugs.py` around lines 105 - 184, The except block uses an f-string in logger.error; replace the f-string with structured logging parameters to avoid string interpolation at call time—update the logger.error call in the except Exception as e handler (the logger.error line inside the try/except that references bug_id and e) to use logger.error("Error fetching bug %s: %s", bug_id, e) or logger.exception("Error fetching bug %s", bug_id) so the logger handles formatting and stack trace properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/handlers/bugs.py`:
- Around line 62-96: The logger.error call in the exception handler of the
search routine uses an unnecessary str() inside an f-string; update the
exception logging in the except block (the logger.error in the try/except around
db.prepare in this file) to interpolate the exception directly (e.g., use
f"Error searching bugs: {e}") to simplify the f-string while keeping the same
message.
- Around line 105-184: The except block uses an f-string in logger.error;
replace the f-string with structured logging parameters to avoid string
interpolation at call time—update the logger.error call in the except Exception
as e handler (the logger.error line inside the try/except that references bug_id
and e) to use logger.error("Error fetching bug %s: %s", bug_id, e) or
logger.exception("Error fetching bug %s", bug_id) so the logger handles
formatting and stack trace properly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 28abf3b4-6833-4cab-8cef-af4dd6fd6a97
📒 Files selected for processing (1)
src/handlers/bugs.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_bugs.py (2)
110-116: Make the queryset mock chain more realistic.
filter()always returning the samemock_qscan hide chain-related bugs in pagination/filter composition. Returning a new queryset object perfilter()call makes these tests stricter and closer to real ORM behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bugs.py` around lines 110 - 116, The queryset mock in _make_mock_bug_class currently returns the same mock_qs from filter(), which hides chaining bugs; change filter on mock_qs to return a new queryset mock per call (e.g., set mock_qs.filter.side_effect to a callable that creates and returns a fresh MagicMock/qs with an AsyncMock count returning the same count), and ensure mock_bug.objects still returns the initial mock_qs so callers can start a chain while each subsequent .filter() yields an independent queryset object.
289-295: Assert exact status filter arguments, not just “called”.
assert_called()can pass even if the wrong field/value is used. Assert the expected call payload to prevent false positives.Precise assertion
- mock_qs.filter.assert_called() + mock_qs.filter.assert_any_call(status="open")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bugs.py` around lines 289 - 295, The test test_status_filter_applied currently only checks mock_qs.filter.assert_called(), which can miss incorrect arguments; update it to assert the exact filter payload used by handle_bugs by replacing the call assertion with an exact argument assertion on mock_qs.filter (e.g., mock_qs.filter.assert_called_once_with(status="open") or mock_qs.filter.assert_called_once_with({"status": "open"}) depending on whether handle_bugs calls filter with kwargs or a dict) so the test fails if the wrong field or value is passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_bugs.py`:
- Around line 297-303: The test declares mock_qs but doesn't use it, causing
RUF059; update test_non_digit_domain_ignored to use mock_qs to assert that
passing a non-digit "domain" does not add ORM filters: when patching
handlers.bugs.Bug with mock_bug and using mock_qs from
_make_mock_bug_class(count=0), assert that mock_qs.filter (or the equivalent
query-builder method exposed by mock_qs) was not called (or call_count == 0)
after calling handle_bugs with {"domain": "not-a-number"}, thereby both removing
the unused-variable warning and strengthening the assertion that handle_bugs
ignored the non-digit domain.
- Around line 12-30: The test currently mutates global interpreter state at
import time by inserting into sys.path and setting sys.modules (_MockResponse,
_mock_workers, Response entries) before importing handlers.bugs; change this to
a fixture-scoped setup that uses pytest's monkeypatch or
unittest.mock.patch.dict to temporarily set sys.path and sys.modules (including
providing a MockResponse class and a fake "workers", "libs", "libs.db", and
"models") and import handle_bugs inside that fixture so the patching is undone
after the test; update the test to reference the fixture when calling
handle_bugs and remove the top-level sys.path/sys.modules mutations from
tests/test_bugs.py.
---
Nitpick comments:
In `@tests/test_bugs.py`:
- Around line 110-116: The queryset mock in _make_mock_bug_class currently
returns the same mock_qs from filter(), which hides chaining bugs; change filter
on mock_qs to return a new queryset mock per call (e.g., set
mock_qs.filter.side_effect to a callable that creates and returns a fresh
MagicMock/qs with an AsyncMock count returning the same count), and ensure
mock_bug.objects still returns the initial mock_qs so callers can start a chain
while each subsequent .filter() yields an independent queryset object.
- Around line 289-295: The test test_status_filter_applied currently only checks
mock_qs.filter.assert_called(), which can miss incorrect arguments; update it to
assert the exact filter payload used by handle_bugs by replacing the call
assertion with an exact argument assertion on mock_qs.filter (e.g.,
mock_qs.filter.assert_called_once_with(status="open") or
mock_qs.filter.assert_called_once_with({"status": "open"}) depending on whether
handle_bugs calls filter with kwargs or a dict) so the test fails if the wrong
field or value is passed.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b1a40c7a-afd7-4b6b-ae40-c7f55f446f65
📒 Files selected for processing (1)
tests/test_bugs.py
SatishKumar620
force-pushed
the
fix/error-handling-bugs-get-endpoints
branch
from
c2c17c4 to
4b68eb4
Compare
|
@coderabbitai Please assess the PR's completeness and merge readiness. It is ready to merged? |
|
Tip For best results, initiate chat on the files or code changes.
To summarize the verdict:
The three minor observations noted (error detail leakage, hardcoded |
|
@coderabbitai review again |
|
✅ Actions performedFull review triggered. |
|
@DonnieBLT I checked the ORM and it currently only supports single-table queries, no JOIN support yet. The GET /bugs/{id} and search endpoints both need a LEFT JOIN with the domains table to return domain info. |
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14300
✅ Merge Readiness Assessment — PR
|
| Area | Status |
|---|---|
GET /bugs/search wrapped in try/except |
✅ |
GET /bugs/{id} wrapped in try/except |
✅ |
get_db_safe has its own isolated try/except |
✅ |
bug_id ValueError handled before DB try block |
✅ |
Error logging via logger.error(...) |
✅ |
500 error_response returned on failure |
✅ |
Pattern consistent with POST /bugs and GET /bugs list |
✅ |
| No regressions to existing endpoints | ✅ |
Improvement Since Last Review
A previously noted concern — error detail leakage via str(e) in the response body — has been resolved for the two endpoints in scope:
GET /bugs/searchnow returns"Failed to search bugs. Please try again later."✅GET /bugs/{id}now returns"Failed to fetch bug. Please try again later."✅
Regarding the ORM JOIN Discussion
Your note about extending src/libs/orm.py to support JOINs is a good idea for the future, but it is not a blocker here — both GET /bugs/search and GET /bugs/{id} already use raw SQL with LEFT JOIN domains d ON b.domain = d.id directly, so domain data is already returned correctly by this PR. The ORM extension would be a separate, independent improvement.
Verdict
✅ PR #44 is complete and merge-ready. All objectives from issue #43 are fully met. No blocking issues remain.
|
@coderabbitai requested by maintainer to use orm so review pr again |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 30784
🔍 Review: ORM Migration FeasibilityThe Core BlockerThe current SELECT ... FROM {table} [WHERE ...] [ORDER BY ...] [LIMIT ?] [OFFSET ?]It has no JOIN support. This is the fundamental blocker — three of the four DB queries in the affected endpoints require a JOIN:
What Can Be Migrated Right Now (Partial)Only the screenshots query is single-table and can use the ORM today: # Before (raw SQL)
screenshots_result = await db.prepare('''
SELECT id, image, created
FROM bug_screenshots
WHERE bug = ?
ORDER BY created DESC
''').bind(bug_id).all()
screenshots_data = convert_d1_results(screenshots_result.results if hasattr(screenshots_result, 'results') else [])
# After (ORM) — requires a BugScreenshot model with table_name = "bug_screenshots"
screenshots_data = await BugScreenshot.objects(db).filter(bug=bug_id).order_by('-created').values('id', 'image', 'created').all()But the main bug, domain, and tags queries cannot be migrated without ORM extension. Path ForwardYour earlier suggestion was exactly right. The recommended approach is:
Note that Current PR StatusThe error-handling objective (issue Recommended next steps:
Would you like me to open a follow-up issue to track the ORM JOIN extension work?
|
|
Yes, please update it to support joins. |
- Wrap bare db.prepare() calls in try/except for GET /bugs/{id} and
GET /bugs/search, consistent with POST /bugs error handling pattern
- Return generic messages in 500 responses instead of raw str(e)
Closes OWASP-BLT#43
…ition
- Replace GET /bugs/{id} main query with ORM JOIN on domains table
- Replace screenshots fetch with BugScreenshot.objects(db) ORM call
- Keep search as raw SQL — ORM does not support OR conditions
- Add comment explaining why raw SQL is retained for search
Addresses DonnieBLT feedback on PR OWASP-BLT#44
SatishKumar620
force-pushed
the
fix/error-handling-bugs-get-endpoints
branch
from
4b68eb4 to
f25ac88
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/bugs.py`:
- Around line 91-143: Define and validate bug_id before entering the DB/try
block and return a 400 error for invalid values: extract the path parameter into
bug_id (e.g., convert to int or validate UUID as appropriate) at the start of
the handler, check it's present and valid and call error_response("Invalid bug
id", status=400) if not; then proceed to use that validated bug_id in
Bug.objects, BugScreenshot.objects, the raw SQL bind, and in the except block
(so the except no longer risks referencing an undefined variable).
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 50a26756-814f-4759-a65a-c0ac23612720
📒 Files selected for processing (1)
src/handlers/bugs.py
|
@coderabbitai Please assess the PR's completeness and merge readiness. It is ready to merged? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 33670
🚧 Blocking Issue: Ambiguous Column in
|
| Area | Status |
|---|---|
ORM .join() support implemented |
✅ |
GET /bugs/{id} main fetch uses ORM with JOIN |
✅ |
GET /bugs/{id} screenshots use ORM |
✅ |
GET /bugs/search raw SQL — justified (ORM lacks OR support) |
✅ |
| Tags raw SQL — justified (no BugTag model) | ✅ |
All error handling with try/except + logger.error |
✅ |
Generic 500 error messages (no str(e) leakage) |
✅ |
filter(id=bug_id) ambiguous column with JOIN |
❌ Blocking |
Once that single line is updated to filter(**{"bugs.id": bug_id}), the PR will be ready to merge.
|
@coderabbitai Please assess the PR's completeness and merge readiness. It is ready to merged? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 33682
🚧 New Blocking Issue:
|
| Area | Status |
|---|---|
GET /bugs/search wrapped in try/except |
✅ |
GET /bugs/{id} wrapped in try/except |
✅ |
bug_id ValueError handled before DB block |
✅ |
Error logging via logger.error(...) |
✅ |
Generic 500 error messages (no str(e) leakage) |
✅ |
| ORM JOIN support implemented | ✅ |
filter(id=bug_id) ambiguous column — previously fixed |
✅ |
values() with AS aliases raises ValueError at runtime |
❌ Blocking |
Once the aliased column issue is resolved (by extending _validate_identifier in the ORM to accept col AS alias syntax, or restructuring the values() call), the PR will be ready to merge.
4 participants
Problem
In
src/handlers/bugs.py, theGET /bugs/{id}andGET /bugs/searchendpoints had bare
await db.prepare()calls with no try/except block.If D1 is unavailable or returns an unexpected error, these endpoints
would throw an unhandled exception and crash the worker instead of
returning a proper error response.
The
POST /bugshandler already had correct error handling — this PRmakes
GET /bugs/{id}andGET /bugs/searchconsistent with that pattern.Fix
Wrapped all DB calls in both endpoints with try/except blocks that log
the error and return a
500 error_response, matching the existingpattern in
POST /bugsandhandle_stats.Closes #43
Summary by CodeRabbit
New Features
Bug Fixes