- 
                Notifications
    You must be signed in to change notification settings 
- Fork 48
fix(django): restore process_exception to capture view exceptions #350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d125873    to
    a6358a6      
    Compare
  
    Restores the process_exception method that was removed in v6.7.5 (PR #328), which broke exception capture from Django views and downstream middleware. Django converts view exceptions into responses before they propagate through the middleware stack's __call__ method, so the context manager's exception handler never sees them. Django provides these exceptions via the process_exception hook instead. Changes: - Add process_exception method to capture exceptions from views and downstream middleware with proper request context and tags - Add tests verifying process_exception behavior and settings (capture_exceptions, request_filter) - Update version to 6.7.11 - Update CHANGELOG to accurately reflect the fix Context: - v6.7.4 and earlier: no process_exception - exceptions not captured (#286) - v6.7.4 fix: added process_exception - worked correctly - v6.7.5 (PR #328): removed process_exception when adding async - broke again (#329) - v6.7.10 (PR #348): attempted fix but didn't restore process_exception - still broken - v6.7.11 (this fix): restores process_exception - fixes #329 Fixes #329
a6358a6    to
    8d0d253      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 2 comments
| def test_process_exception_respects_capture_exceptions_false(self): | ||
| """Verify process_exception respects capture_exceptions=False setting""" | ||
| mock_client = Mock() | ||
|  | ||
| middleware = self.create_middleware(capture_exceptions=False) | ||
| middleware.client = mock_client | ||
|  | ||
| view_exception = ValueError("Should not be captured") | ||
|  | ||
| def mock_get_response(request): | ||
| if hasattr(middleware, "process_exception"): | ||
| middleware.process_exception(request, view_exception) | ||
| return Mock(status_code=500) | ||
|  | ||
| middleware._sync_get_response = mock_get_response | ||
|  | ||
| request = MockRequest() | ||
| middleware(request) | ||
|  | ||
| mock_client.capture_exception.assert_not_called() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: setting _sync_get_response on line 256 has no effect - the middleware uses self.get_response set during initialization. should pass mock_get_response to create_middleware(get_response=mock_get_response) like the first test does.
| def test_process_exception_respects_capture_exceptions_false(self): | |
| """Verify process_exception respects capture_exceptions=False setting""" | |
| mock_client = Mock() | |
| middleware = self.create_middleware(capture_exceptions=False) | |
| middleware.client = mock_client | |
| view_exception = ValueError("Should not be captured") | |
| def mock_get_response(request): | |
| if hasattr(middleware, "process_exception"): | |
| middleware.process_exception(request, view_exception) | |
| return Mock(status_code=500) | |
| middleware._sync_get_response = mock_get_response | |
| request = MockRequest() | |
| middleware(request) | |
| mock_client.capture_exception.assert_not_called() | |
| def test_process_exception_respects_capture_exceptions_false(self): | |
| """Verify process_exception respects capture_exceptions=False setting""" | |
| mock_client = Mock() | |
| view_exception = ValueError("Should not be captured") | |
| def mock_get_response(request): | |
| if hasattr(middleware, "process_exception"): | |
| middleware.process_exception(request, view_exception) | |
| return Mock(status_code=500) | |
| middleware = self.create_middleware( | |
| capture_exceptions=False, get_response=mock_get_response | |
| ) | |
| middleware.client = mock_client | |
| request = MockRequest() | |
| middleware(request) | |
| mock_client.capture_exception.assert_not_called() | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/integrations/test_middleware.py
Line: 242:261
Comment:
**logic:** setting `_sync_get_response` on line 256 has no effect - the middleware uses `self.get_response` set during initialization. should pass `mock_get_response` to `create_middleware(get_response=mock_get_response)` like the first test does.
```suggestion
    def test_process_exception_respects_capture_exceptions_false(self):
        """Verify process_exception respects capture_exceptions=False setting"""
        mock_client = Mock()
        view_exception = ValueError("Should not be captured")
        def mock_get_response(request):
            if hasattr(middleware, "process_exception"):
                middleware.process_exception(request, view_exception)
            return Mock(status_code=500)
        middleware = self.create_middleware(
            capture_exceptions=False, get_response=mock_get_response
        )
        middleware.client = mock_client
        request = MockRequest()
        middleware(request)
        mock_client.capture_exception.assert_not_called()
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! Looking more carefully at the code structure, I can see the issue now.
The problem is that mock_get_response is defined on line 251, but it's assigned to middleware._sync_get_response on line 256 after the middleware has already been created. However, the middleware was created on line 246 with capture_exceptions=False but without passing the mock_get_response to create_middleware().
When middleware(request) is called on line 259, it will use self.get_response (set during initialization), not _sync_get_response. The assignment on line 256 has no effect.
The fix is to pass mock_get_response to create_middleware() like the first test does:
| def test_process_exception_respects_capture_exceptions_false(self): | |
| """Verify process_exception respects capture_exceptions=False setting""" | |
| mock_client = Mock() | |
| middleware = self.create_middleware(capture_exceptions=False) | |
| middleware.client = mock_client | |
| view_exception = ValueError("Should not be captured") | |
| def mock_get_response(request): | |
| if hasattr(middleware, "process_exception"): | |
| middleware.process_exception(request, view_exception) | |
| return Mock(status_code=500) | |
| middleware._sync_get_response = mock_get_response | |
| request = MockRequest() | |
| middleware(request) | |
| mock_client.capture_exception.assert_not_called() | |
| def test_process_exception_respects_capture_exceptions_false(self): | |
| """Verify process_exception respects capture_exceptions=False setting""" | |
| mock_client = Mock() | |
| view_exception = ValueError("Should not be captured") | |
| def mock_get_response(request): | |
| if hasattr(middleware, "process_exception"): | |
| middleware.process_exception(request, view_exception) | |
| return Mock(status_code=500) | |
| middleware = self.create_middleware( | |
| capture_exceptions=False, get_response=mock_get_response | |
| ) | |
| middleware.client = mock_client | |
| request = MockRequest() | |
| middleware(request) | |
| mock_client.capture_exception.assert_not_called() | 
| def test_process_exception_respects_request_filter(self): | ||
| """Verify process_exception respects request_filter setting""" | ||
| mock_client = Mock() | ||
|  | ||
| middleware = self.create_middleware( | ||
| request_filter=lambda req: False, capture_exceptions=True | ||
| ) | ||
| middleware.client = mock_client | ||
|  | ||
| view_exception = ValueError("Should be filtered") | ||
|  | ||
| def mock_get_response(request): | ||
| if hasattr(middleware, "process_exception"): | ||
| middleware.process_exception(request, view_exception) | ||
| return Mock(status_code=500) | ||
|  | ||
| middleware._sync_get_response = mock_get_response | ||
|  | ||
| request = MockRequest() | ||
| middleware(request) | ||
|  | ||
| mock_client.capture_exception.assert_not_called() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: setting _sync_get_response on line 279 has no effect - the middleware uses self.get_response set during initialization. should pass mock_get_response to create_middleware(get_response=mock_get_response) like the first test does.
| def test_process_exception_respects_request_filter(self): | |
| """Verify process_exception respects request_filter setting""" | |
| mock_client = Mock() | |
| middleware = self.create_middleware( | |
| request_filter=lambda req: False, capture_exceptions=True | |
| ) | |
| middleware.client = mock_client | |
| view_exception = ValueError("Should be filtered") | |
| def mock_get_response(request): | |
| if hasattr(middleware, "process_exception"): | |
| middleware.process_exception(request, view_exception) | |
| return Mock(status_code=500) | |
| middleware._sync_get_response = mock_get_response | |
| request = MockRequest() | |
| middleware(request) | |
| mock_client.capture_exception.assert_not_called() | |
| def test_process_exception_respects_request_filter(self): | |
| """Verify process_exception respects request_filter setting""" | |
| mock_client = Mock() | |
| view_exception = ValueError("Should be filtered") | |
| def mock_get_response(request): | |
| if hasattr(middleware, "process_exception"): | |
| middleware.process_exception(request, view_exception) | |
| return Mock(status_code=500) | |
| middleware = self.create_middleware( | |
| request_filter=lambda req: False, | |
| capture_exceptions=True, | |
| get_response=mock_get_response, | |
| ) | |
| middleware.client = mock_client | |
| request = MockRequest() | |
| middleware(request) | |
| mock_client.capture_exception.assert_not_called() | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/integrations/test_middleware.py
Line: 263:284
Comment:
**logic:** setting `_sync_get_response` on line 279 has no effect - the middleware uses `self.get_response` set during initialization. should pass `mock_get_response` to `create_middleware(get_response=mock_get_response)` like the first test does.
```suggestion
    def test_process_exception_respects_request_filter(self):
        """Verify process_exception respects request_filter setting"""
        mock_client = Mock()
        view_exception = ValueError("Should be filtered")
        def mock_get_response(request):
            if hasattr(middleware, "process_exception"):
                middleware.process_exception(request, view_exception)
            return Mock(status_code=500)
        middleware = self.create_middleware(
            request_filter=lambda req: False,
            capture_exceptions=True,
            get_response=mock_get_response,
        )
        middleware.client = mock_client
        request = MockRequest()
        middleware(request)
        mock_client.capture_exception.assert_not_called()
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check again!
Previous tests didn't properly demonstrate the real Django flow where view exceptions are caught by Django and passed to process_exception. test_sync_middleware_exception_capture made get_response raise directly, which the context manager caught - doesn't test the real issue where Django catches view exceptions before they reach the middleware. test_process_exception_integration just called process_exception and asserted it was called - circular logic. New tests properly simulate Django's behavior: - test_view_exceptions_only_captured_via_process_exception: proves context manager can't see view exceptions, only process_exception captures them - test_process_exception_called_during_view_exception: simulates complete Django flow - test_process_exception_respects_capture_exceptions_false: verifies settings respected - test_process_exception_respects_request_filter: verifies request filter respected These tests would have caught the v6.7.5 regression where process_exception was removed.
82a1533    to
    974ee42      
    Compare
  
    Merge latest master changes (ai_framework feature) and move the process_exception fix to Unreleased section. Will be released together with async user access fix as 6.7.12.
Exception tests verify 500 responses but not actual PostHog capture. Exception capture requires process_exception() method from PR #350. Without process_exception(), Django converts view exceptions to responses before they propagate through the middleware context manager, so they're not captured to PostHog even though 500 is still returned.
Add test_exception_capture.py that demonstrates: - Without process_exception() (v6.7.11), view exceptions are NOT captured to PostHog - With process_exception() (PR #350), exceptions ARE captured Tests use mocking to verify posthog.capture_exception is called. Successfully validated both async and sync view exception capture. This branch is stacked on PR #350 to test both fixes together: - PR #350: Exception capture via process_exception() - PR #358: Async user access via request.auser() All 7 tests pass, demonstrating both bug fixes work correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Exception tests verify 500 responses but not actual PostHog capture. Exception capture requires process_exception() method from PR #350. Without process_exception(), Django converts view exceptions to responses before they propagate through the middleware context manager, so they're not captured to PostHog even though 500 is still returned.
Add test_exception_capture.py that demonstrates: - Without process_exception() (v6.7.11), view exceptions are NOT captured to PostHog - With process_exception() (PR #350), exceptions ARE captured Tests use mocking to verify posthog.capture_exception is called. Successfully validated both async and sync view exception capture. This branch is stacked on PR #350 to test both fixes together: - PR #350: Exception capture via process_exception() - PR #358: Async user access via request.auser() All 7 tests pass, demonstrating both bug fixes work correctly.
Summary
Restores
process_exceptionmethod removed in v6.7.5, fixing exception capture from Django views.Problem
Django converts view exceptions to responses before they reach middleware's
__call__, so the context manager never sees them. Django provides these viaprocess_exceptionhook instead.Timeline
process_exception- workedChanges
process_exceptionmatching v6.7.4 behaviorLearnings
Django calls
process_exceptionWHILE still in the__call__context, so tags are already set. Just need to capture the exception directly - no new context needed. First attempt overcomplicated this.Also: Django checks
hasattr()before callingprocess_exception, so missing it fails silently. Tests needed to simulate this properly (v6.7.5 doesn't raise an error, just doesn't capture).Tests
21 middleware tests pass. New tests verify:
process_exceptioncaptures exceptions when Django calls itcapture_exceptions,request_filter) are respectedTest Validation Across Versions
Validated that tests correctly detect the regression:
process_exception): All 4 exception tests PASSprocess_exception):test_view_exceptions_only_captured_via_process_exceptionFAILS with "process_exception missing - view exceptions will not be captured!"Proves tests properly demonstrate Django's exception flow and would have caught the v6.7.5 regression.
Fixes #329