[GIT-213] fix: return HTTP response from dispatch() exception handler#9179
[GIT-213] fix: return HTTP response from dispatch() exception handler#9179sriramveeraghanta wants to merge 1 commit into
Conversation
BaseAPIView.dispatch() and BaseViewSet.dispatch() built the proper error Response via handle_exception() but returned the raw exception object instead, causing Django to raise "TypeError: 'Exception' object is not a valid HTTP response". Fix all six occurrences across the api, app, license and space view bases, and add a regression test covering every affected base class. Fixes #9157
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR fixes a critical bug across Plane's API base view classes where ChangesDispatch Exception Handling Correction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Pull request overview
Fixes a copy-pasted bug in six dispatch() methods across four base view modules where the raw exception was returned instead of the HTTP Response produced by handle_exception(), causing Django's response pipeline to fail with TypeError: 'Exception' object is not a valid HTTP response.
Changes:
- Replace
return excwithreturn responsein the exception handler ofdispatch()forBaseAPIView/BaseViewSetacross api, app, license, and space view bases. - Add a parametrized regression unit test covering all affected base classes plus the already-correct
api.BaseViewSetas a control.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/api/views/base.py | Return Response from BaseAPIView.dispatch() exception path. |
| apps/api/plane/app/views/base.py | Return Response from BaseViewSet/BaseAPIView dispatch() exception paths. |
| apps/api/plane/license/api/views/base.py | Return Response from BaseAPIView.dispatch() exception path. |
| apps/api/plane/space/views/base.py | Return Response from BaseViewSet/BaseAPIView dispatch() exception paths. |
| apps/api/plane/tests/unit/views/test_base_dispatch.py | Parametrized regression test asserting dispatch() returns the Response, not the exception. |
| apps/api/plane/tests/unit/views/init.py | New empty package init for the test directory. |
Description
When an API request triggered an unhandled exception, the shared view bases caught it, built the proper error
Responseviahandle_exception(), but then returned the raw exception object instead of the response:Django's response pipeline then failed with
TypeError: 'Exception' object is not a valid HTTP response, so clients received a malformed 500 with no structured body instead of a clean JSON error.The same copy-pasted defect existed in six
dispatch()methods across four files, so this PR fixes all of them — not only the one reported in the issue — so the crash cannot surface from the external API, app, space, or license surfaces:apps/api/plane/api/views/base.py—BaseAPIViewapps/api/plane/app/views/base.py—BaseViewSet,BaseAPIViewapps/api/plane/license/api/views/base.py—BaseAPIViewapps/api/plane/space/views/base.py—BaseViewSet,BaseAPIView(
api.BaseViewSetalready returnedresponsecorrectly, which confirmed the intended fix.)A parametrized regression test covers every affected base class and asserts that, when
super().dispatch()raises,dispatch()returns an HTTPResponserather than the exception.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
apps/api/plane/tests/unit/views/test_base_dispatch.py: parametrized over all six affected base classes plusapi.BaseViewSetas a control; assertsdispatch()returns arest_framework.response.Response(not the exception) whensuper().dispatch()raises. Verified RED (6 failures) before the fix and GREEN (7 passing) after.docker compose -f docker-compose-test.yml up --build --abort-on-container-exit --exit-code-from api-tests: 236 passed (exit code 0), including DB-backed contract/model/serializer tests.ruff checkpasses on all changed files.{"error": ...}JSON body with a 500 status instead of a crashed response pipeline.References
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests