refactor(hogql): defer printer boundary loads and guard the full compile pipeline - #72352
Conversation
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/hogql/test/test_no_django_imports.py:44-45
**Import Guard Uses Warmed Package**
The loop imports `posthog.hogql.printer` before `clickhouse_property_resolution`, so the latter is checked only after its cyclic printer dependencies are initialized. A standalone process that imports `clickhouse_property_resolution` first can still fail during this cycle while the guard passes, leaving the claimed direct import path uncovered.
Reviews (1): Last reviewed commit: "refactor(hogql): defer printer boundary ..." | Re-trigger Greptile |
15cb459 to
5ef1f08
Compare
🤖 CI report✅ Backend coverage — all changed backend lines covered🧪 Backend test coveragePatch coverage — changed backend lines (products + core): All changed backend lines are covered ✅ Per-product line coverage (touched products)
Report-only. Patch coverage = changed backend lines covered vs |
…ile pipeline Generated-By: PostHog Code Task-Id: e47d9107-4fc1-40d6-8ec6-6cd1be8ea6b0
Generated-By: PostHog Code Task-Id: e47d9107-4fc1-40d6-8ec6-6cd1be8ea6b0
… printer import Generated-By: PostHog Code Task-Id: e47d9107-4fc1-40d6-8ec6-6cd1be8ea6b0
345023a to
3e1c58e
Compare
There was a problem hiding this comment.
Pull request overview
This PR completes the “Seam 1” work to make the HogQL printer layer importable without requiring django.setup() by deferring the last remaining Django-coupled imports in printer/utils.py to the existing “prepare” boundary, and strengthens the Django-free guard test with an end-to-end compile in a bare interpreter.
Changes:
- Deferred Django-side loads in
prepare_ast_for_printing(restricted property access control + events retention floor), and deferredclickhouse_property_resolutionto break the import cycle and keep the printer package importable without Django. - Ensured access-control cache invalidation signal receivers are always connected via
AppConfig.ready()(no longer relying on incidental imports). - Expanded
test_no_django_importsto validate a full parse → resolve → prepare → print pipeline in a subprocess with Django settings unset, stubbing only declared loader boundaries.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
products/access_control/backend/apps.py |
Hooks access-control receiver registration into AppConfig.ready() so cache invalidation continues to work even when printer imports are deferred. |
posthog/temporal/messaging/test/test_hogql_compile.py |
Updates patch target to the defining module, since the printer now imports the function at call time. |
posthog/hogql/test/test_no_django_imports.py |
Extends the Django-free guard to cover cold-start import order and a full end-to-end compile in a bare interpreter. |
posthog/hogql/printer/utils.py |
Moves remaining module-level Django imports (and the cpr import) behind the prepare boundary to keep printer imports Django-free. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Problem
Final step of making the HogQL compile pipeline importable without
django.setup()(previously: #71112, #71568, #71913, #71960, #72271). After #72271 freed the transform layer, exactly two module-level Django imports remained in the whole printer package, both inprinter/utils.py: the property-level access-control load (get_restricted_properties_for_team) and the events-retention floor (events_retention_months_for_team). Becauseprinter/__init__.pyimportsutils, those two lines kept every printer module - and transitivelyclickhouse_property_resolution- coupled to Django at import time.Changes
printer/utils.pydefers both imports to their call sites insideprepare_ast_for_printing, with the same "Deferred:" boundary comments used byDatabase.create_forandload_property_metadata. Both loads were already conditional (restricted_properties is Noneguard;apply_events_retention_floorflag), so the function-local imports change nothing about when they run.printer/__init__pulls in every dialect printer plus utils) andclickhouse_property_resolutionimport Django-free and joinedDJANGO_FREE_MODULESin the guard test. The list orders the printer beforeclickhouse_property_resolutionbecause of a pre-existing import-order cycle (cpr importsprinter.base/printer.clickhouse, which triggers the package init, which imports utils, which imports cpr back) - it resolves whenever the printer package initializes first, which every production path does. Untangling that cycle is a separable cleanup, not attempted here.load_property_metadata). The query uses an unaliasedcount(), which exercises the resolver's lazy printer import for implicit-alias derivation - the last known "needs Django until Seam 1 lands" caveat, now covered Django-free.Note
This PR is stacked on #72271 and should merge after it.
How did you test this code?
Automated only (no manual testing):
test_no_django_imports.py- the extended guard. The new end-to-end compile assertion catches a regression no existing test does: every printer test runs under pytest-django, so a reintroduced module-level Django import (or an ORM call on the compile path outside the declared loader boundaries) would previously only surface in production tooling contexts; now it fails this test withAppRegistryNotReady. It reuses the existing child subprocess - no new process, no DB, and the only mock is the declared Django boundary.posthog/hogql/printer/test/,test_property_types.py,test_property_types_dmat.py,test_property_skip_indexes.py, andproducts/access_control/backend/tests/test_property_access_control.py(exercises the deferred access-control load through the real path) - 1172 passed, all snapshots unchanged.uv run mypy --cache-fine-grained .- clean apart from the known pre-existingpersonhog_client/converters.pyunused-ignore that reproduces on clean master.Automatic notifications
Docs update
No user-facing, API, or config changes - internal refactor only.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with PostHog Code (Claude). Step 4 of 4 in the Seam 1 sequence: (1) relocate pure constants (#71913), (2) PropertyMetadata bundle + loader (#71960), (3) materialized-column map into the bundle (#72271), (4) this PR. Skills invoked: /writing-tests (for the guard-test extension). Decisions: deferred imports at the call sites rather than extracting a separate loader module, because
prepare_ast_for_printingalready is the Django boundary for these per-query loads (same rationale asDatabase.create_for); verified before editing that nomock.patchsite targets either function as aprinter.utilsattribute, so the deferral breaks no tests; left the cpr↔printer import-order cycle in place (pre-existing, resolves under production import order) and documented it in the guard list rather than restructuring the printer package in a byte-identical refactor series; in the child compile, runtimedjango.conf.settingsreads were confirmed unreachable (they only fire for channel-lookup and currency functions, not a plain property query).Created with PostHog Code