-
Notifications
You must be signed in to change notification settings - Fork 48
fix(django): handle request.user in async middleware context #358
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
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.
2 files reviewed, 2 comments
f5731a1 to
2b4847e
Compare
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.
6b8fad0 to
5201389
Compare
5201389 to
00ce411
Compare
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.
Thank you! Tested the local project and PostHog's Django–works well.
.github/workflows/ci.yml
Outdated
| uses: astral-sh/setup-uv@0c5e2b8115b80b4c7c5ddf6ffdd634974642d182 # v5.4.1 | ||
| with: | ||
| enable-cache: true | ||
| pyproject-file: 'test_project_django5/pyproject.toml' |
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.
Should we put the integration tests under integration_tests? There might be more in the future.
Fixes SynchronousOnlyOperation when accessing request.user in ASGI deployments. Django's request.user is a lazy object that triggers DB access when touched. In async context, this raises SynchronousOnlyOperation. The middleware now uses request.auser() in async paths to avoid blocking calls. Changes: - Add aextract_tags() and aextract_request_user() methods - Update __acall__() to use async versions - Add test verifying user extraction works in async context - Follow Django's naming convention for async methods (auser, asave, etc.) The issue was introduced in v6.7.5 (PR #328) but only became apparent after v6.7.10 (PR #348) made ASGI functional enough for users to discover it. Fixes #355
Add 5 additional tests covering edge cases for async middleware: - Unauthenticated users - Requests without user attribute (no auth middleware) - extra_tags callbacks in async context - tag_map callbacks in async context - Full header extraction with authenticated user Ensures async middleware works correctly in all scenarios users might encounter. 24 middleware tests now pass (up from 19).
Extract common logic into helper methods to eliminate duplication: - Add _build_tags() helper used by both extract_tags and aextract_tags Eliminates ~50 lines of duplicated tag extraction logic - Add _resolve_user_details() helper for user info extraction Centralizes user ID/email extraction logic - Use defensive getattr() instead of try/except More readable and explicit about what can be None - Handle callable is_authenticated for legacy Django Supports Django versions where is_authenticated was a method Benefits: - Single source of truth for tag extraction logic - Easier maintenance (change once, applies to sync and async) - More robust user detail extraction - All 24 tests still pass Inspired by code review feedback while maintaining Django conventions (aextract_* naming pattern).
Add standalone Django 5.2.7 test project to validate middleware fixes work correctly with modern Django's async features: - Test views for async user access (uses request.auser()) - Test views for exception capture (both sync and async) - ASGI configuration with uvicorn - Separate virtualenv to avoid conflicts with main project dependencies Verified fixes work correctly: - Async user access: middleware uses await request.auser() without SynchronousOnlyOperation - Exception capture: process_exception called for both sync and async views
Add comprehensive test suite for PostHog Django middleware in async context: - Test async user access (unauthenticated) - Test async authenticated user access (triggers SynchronousOnlyOperation in v6.7.11) - Test sync user access - Test async exception capture - Test sync exception capture Tests run directly against ASGI application using httpx AsyncClient without needing a server. Uses pytest-asyncio for async test support. The authenticated user test demonstrates the bug fixed in this PR: v6.7.11 raises SynchronousOnlyOperation when accessing request.user in async middleware with authenticated users. The fix uses await request.auser() instead. Add test dependencies: pytest, pytest-asyncio, httpx
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.
Remove references to pending PRs and stacked branches from test comments to make the code ready to merge. Comments now only reference the implemented functionality and earlier versions (v6.7.11) for context.
Apply pytest best practices to Django 5 test suite: - Add session-scoped asgi_app fixture to share app instance across tests - Replace print() statements with assertions for cleaner test output - Use pytest.skip() instead of print+return for skipped tests - Add @pytest.mark.django_db(transaction=True) for authenticated user test - Remove if __name__ == "__main__" blocks in favor of pytest runner - Add pytest-django dependency to properly recognize django_db marker - Fix mock patch target to 'posthog.capture_exception' (module level) All 7 tests pass with proper pytest output.
…n warning Move cookie parameter from per-request to AsyncClient constructor. This aligns with httpx's preferred API and eliminates the deprecation warning.
Use compatible release operator (~=) to pin major.minor versions while allowing patch updates: - django~=5.2.7 - uvicorn[standard]~=0.38.0 - pytest~=8.4.2 - pytest-asyncio~=1.2.0 - pytest-django~=4.11.1 - httpx~=0.28.1 Prevents accidental behavior changes from minor version bumps while still getting security patches.
Add separate CI job for Django 5 integration tests that verify: - Async user access with request.auser() - Exception capture via process_exception() - Real ASGI application behavior with httpx AsyncClient Runs on Python 3.12 with pinned Django 5.2.7 dependencies in isolated test_project_django5 environment. Complements existing unit tests that run across Python 3.9-3.13.
- Add permissions: contents: read to CI workflow (security best practice) - Exclude test_project_django5 from main pytest run to avoid name collision - Fix ruff E712: use truth check instead of == True - Add noqa: E402 for unavoidable Django setup imports - Configure testpaths to only run posthog/test in main CI
Move test_project_django5 to integration_tests/django5 to organize integration tests by topic for future additions. Update CI workflow and pytest config to use new path. Add .claude/settings.local.json to .gitignore.
0c688ba to
600b168
Compare
Summary
Fixes
SynchronousOnlyOperationwhen accessingrequest.userin ASGI deployments with Django + Uvicorn.Note: This PR is stacked on #350 (process_exception fix). Both will be released together as v6.7.12.
Problem
Django's
request.useris aSimpleLazyObjectthat defers database access until the attribute is touched. In async context, accessing it triggers blocking DB operations, raisingSynchronousOnlyOperation.The middleware was calling synchronous
extract_tags()in__acall__(), which accessedrequest.userdirectly:This worked in sync (WSGI) but failed in async (ASGI).
Root Cause Timeline
Solution
Add async versions of extraction methods following Django's naming convention:
aextract_tags()- async version ofextract_tags()aextract_request_user()- usesawait request.auser()instead ofrequest.user__acall__()to call async versionsFollows Django's pattern for async methods (
auser(),asave(),aget(), etc.).Changes
aextract_tags()methodaextract_request_user()method usingawait request.auser()__acall__()to useawait self.aextract_tags(request)Django 5 Integration Tests
New
test_project_django5/with real Django 5.2.7 ASGI app:request.auser()process_exception()Async Safety Notes
The following operations in
aextract_tags()are async-safe:request.auser()- now uses async versionrequest.headers.get()- dict accessrequest.build_absolute_uri()- string buildingrequest.method,request.path- attribute accessNote on callbacks: If you configure
POSTHOG_MW_EXTRA_TAGSorPOSTHOG_MW_TAG_MAP, ensure these functions don't perform blocking I/O (database queries, file operations, etc.) as they're called in async context. Keep them lightweight or they may raiseSynchronousOnlyOperation.Test Results
User Workaround Validation
User's workaround in #355 used the same approach (async methods with
request.auser()), validating this solution.Fixes #355