Conversation
Code Review Agent Run #e54387Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44406 +/- ##
==========================================
+ Coverage 82.17% 82.23% +0.05%
==========================================
Files 2995 2995
Lines 184993 185112 +119
Branches 42818 42835 +17
==========================================
+ Hits 152025 152232 +207
+ Misses 30209 30116 -93
- Partials 2759 2764 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #66423b
Actionable Suggestions - 1
-
superset/common/query_context_processor.py - 1
- Narrow except escapes fail-closed · Line 546-546
Additional Suggestions - 2
-
tests/unit_tests/common/test_query_context_processor.py - 2
-
Weak sharing test · Line 147-163This test calls the same `processor` twice, so both calls share the same user identity — it would pass even under the old `get_user_id`-bound keying (the deleted test used `side_effect=[1,2]`). It therefore doesn't guard the sharing fix. Use two processors with differing user identity but identical `can_access` scope and assert equal keys.
-
Duplicated test setup · Line 182-259The four `test_annotation_source_scope_*` tests each repeat the same `ChartDAO.find_by_id` + `security_manager` (`new_callable=MagicMock`) patch block and `mock_chart` construction. A shared fixture would remove this duplication and keep the access-scope variants focused on their differing assertions.
-
Review Details
-
Files reviewed - 2 · Commit Range:
19954fd..a61fec5- superset/common/query_context_processor.py
- tests/unit_tests/common/test_query_context_processor.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #ef2648Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Pushed two small commits from review on #44546, which had been stacked on this branch by mistake: the chart/datasource lookup now sits inside the fail-closed |
Code Review Agent Run #ab0c8cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #846c90Actionable Suggestions - 0Additional Suggestions - 3
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
the
requires:rebase
604acb6 to
ff3c5d4
Compare
github-actions
Bot
removed
the
requires:rebase
There was a problem hiding this comment.
Code Review Agent Run #df4733
Actionable Suggestions - 2
-
tests/unit_tests/common/test_query_context_processor.py - 2
- Missing Return Type Hint · Line 2399-2399
- Missing type annotations · Line 2484-2484
Additional Suggestions - 6
-
tests/unit_tests/common/test_query_context_processor.py - 6
-
Duplicated cache-call argument block · Line 2977-2984All three tests repeat the identical six-kwarg `_get_annotation_data_cached(...)` call (2977-2984, 2998-3005, 3021-3028). A helper taking only `force_cached` would keep future signature changes single-site and shrink each test by eight lines.
-
Test name overstates coverage · Line 238-254The test name and docstring claim fail-closed behavior "on any derivation error", but only `get_query_context` is made to raise; `get_rls_cache_key` is configured to return `[]` (line 252), so the RLS-lookup and Jinja `get_extra_cache_keys()` failure paths the docstring cites are never exercised. A regression that re-narrows the except to `SupersetException` would still pass the RLS-path case. Please cover the RLS-path raise explicitly.
-
Missing return type annotations · Line 2970-3028Org rule BITO.md [7819]/[11810] requires explicit `-> None` return hints and typed fixture parameters on all test functions; the three new tests (defs at 2970, 2989, 3013) omit both. Adding them aligns the block with the mandated typing standard.
-
Missing test docstring · Line 2989-2989BITO.md [12148]/[15725] require a docstring on every new test function. The sibling tests in this block (`..._reads_from_cache`, `..._force_cached_raises_on_miss`) have one; `test_get_annotation_data_cached_computes_and_caches_on_miss` does not. Adding a one-liner keeps the section consistent.
-
Untyped mock variables · Line 223-224`mock_query_object` and `mock_query_context` are untyped mock locals. BITO.md adaptive rule 12787 requires explicit `: MagicMock` annotations on mock variables in test files; sibling tests in this module follow that convention. Adding the annotations keeps the new tests consistent with the project's typing standard.
-
Inconsistent fixture usage · Line 273-273Unlike the three sibling `_annotation_source_scope` tests (lines 217-219, 238-240, 257-259), this test omits the `mock_annotation_chart` fixture, so `ChartDAO.find_by_id` is patched only by its own inline `patch(...)` and the shared fixture's wiring is bypassed. Using the fixture like the siblings keeps the ChartDAO patching pattern uniform across the four tests this diff adds.
-
Review Details
-
Files reviewed - 2 · Commit Range:
3a5f2d2..ff3c5d4- superset/common/query_context_processor.py
- tests/unit_tests/common/test_query_context_processor.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Code Review Agent Run #3b37e7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| annotation_data: dict[str, Any] = {} | ||
| if query_obj and annotation_key and cache.status != QueryStatus.FAILED: | ||
| try: | ||
| annotation_data = self._get_annotation_data_cached( |
There was a problem hiding this comment.
Two viewers with different annotation RLS scopes can now join the same dataframe-keyed async task, but it only warms the executing viewer's annotation entry; the other viewer's post-success synchronous read-back then runs the annotation query in the HTTP request and can time out even though the task succeeded. Should each subscriber's scoped annotation data be populated asynchronously before its task completion is reported?
Code Review Agent Run #5b5e9bActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
the
requires:rebase
Annotation-layer data is fetched per requesting user and, for chart-backed layers, scoped by the referenced chart datasource's RLS -- a stricter requirement than the dataframe itself has. Binding that context into the dataframe's own cache key meant every distinct viewer of an annotated chart got their own full copy of the (potentially much larger) dataframe, instead of sharing one cache entry. Split the two: the dataframe cache key goes back to depending only on its own datasource/RLS/extra_cache_keys, while annotation data is resolved and cached under a separate, still user/RLS-scoped key. This also restores cross-user task dedup for annotated charts in the async (GTF) flow, which had the same coupling problem. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…auses Replaces the annotation cache key's plain user_id + RLS-clause-list material with an access-scope fingerprint: the can_read/Annotation permission for native layers, and for chart-backed layers, whether the requester can access the referenced chart's datasource plus that chart's own recursive query_cache_key (which already covers RLS and per-user Jinja/virtual-dataset context). Users with identical access now share the annotation cache entry even when their user IDs differ; users lacking base datasource access no longer risk sharing a key with an authorized user just because their RLS clause lists happened to coincide (get_rls_cache_key only reflects RLS filters, not base access). Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t just SupersetException Bito flagged that _annotation_source_scope's fail-closed fallback only caught SupersetException, but the RLS lookup is a live DB query and a virtual dataset's get_extra_cache_keys() renders Jinja -- both can raise driver/template errors that aren't SupersetException subclasses, escaping the fallback and 500ing the whole chart-data request. Widened to catch any exception, matching the broad-except convention already used for cache-key derivation elsewhere in this file (lines 206/243). While fixing that, found the fallback itself wasn't safe either: it unconditionally re-calls get_rls_cache_key(), which can fail the same way the primary derivation did (e.g. the same DB outage), re-raising out of the except block. Wrapped that call too, defaulting to a None data_key. Also addressed two test-quality nits from the same review pass: the same-scope sharing test reused one processor/query_obj across both calls, which would've passed trivially regardless of whether the key was scope-based or identity-based; rewrote it against two independent processor/query_obj instances. Deduped the four _annotation_source_scope tests' repeated ChartDAO.find_by_id patch into a shared fixture, and added coverage for the new fallback-also-fails case. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n cache scope The chart lookup and lazy `datasource` access in `_annotation_source_scope` ran outside the `try`/`except`, so a DB error during either could still propagate and abort the whole chart-data request, defeating the fail-closed behavior the surrounding except clause exists to guarantee. Also adds return-type hints to the new annotation-scope tests per review feedback. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… resolve datasource correctly
Two real gaps sadpandajoe flagged in review, verified by tracing the
code (neither was actually fixed despite an earlier reply claiming
so):
1. The forced-refresh idempotency marker is keyed on (nonce,
cache_key). The annotation cache reused the dataframe's force_query
flag, so once the dataframe's marker (keyed on its own cache_key)
suppressed force_query for one access scope, a different scope's
annotation entry -- which had never actually forced its own
refresh -- silently read its stale existing entry instead. The
annotation cache now resolves and records its own marker against
annotation_key.
2. Chart-backed annotation-layer access scoping used chart.datasource,
which is pinned to table-backed datasources and resolves to None
for a semantic-view-backed chart. Every requester collapsed onto
the same {access: None, data_key: None} key regardless of actual
access. Swapped to resolved_datasource, the resolver the model's
own docstring says authorization call sites must use for exactly
this reason.
Added regression coverage for both: a semantic-view chart now gets
distinct per-access scopes, and a forced refresh whose dataframe
marker is already set still forces a fresh annotation read.
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
can_access_datasource() is a context-free proxy: a requester whose access to the referenced chart only comes from a dashboard/viewer-promiscuous-mode bypass (which depends on the chart's own saved form_data) still fails it, collapsing onto the same denied scope as a genuinely unauthorized requester -- so the latter could read the former's warmed cache entry. Derive `access` from the referenced chart's own QueryContext.raise_for_access() instead, the same path get_viz_annotation_data() actually executes. Also apply a layer's time-grain/time-range overrides to the referenced query context before deriving its cache key (factored into a shared _apply_annotation_overrides helper), not just before executing it -- an override can introduce per-user Jinja/RLS material (e.g. a template that only calls current_user_id() at a finer grain) that the saved, un-overridden query's key would otherwise miss. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
d5fe17a to
6a06c5e
Compare
github-actions
Bot
removed
the
requires:rebase
| ): | ||
| continue | ||
| layer_value = layer.get("value") | ||
| source_scope[str(layer_value)] = self._annotation_source_scope(layer) |
There was a problem hiding this comment.
When two layers reference the same chart with different overrides, the later layer overwrites the earlier layer’s scope here; if only the earlier override activates per-user Jinja/RLS, the second viewer can receive the first viewer’s rows. Should the cache context retain every layer’s overridden scope rather than just one per chart ID?
| # scope below regardless of their actual access. | ||
| datasource = chart.resolved_datasource if chart else None | ||
| if chart is None or datasource is None: | ||
| return {"access": None, "data_key": None} |
There was a problem hiding this comment.
A chart can have its datasource_id cleared while its saved query context still targets a valid dataset, so this gives allowed and denied viewers the same unscoped annotation key; after an allowed viewer warms it, the denied viewer receives those rows without source authorization. Should unresolved scope, including derivation-error fallbacks, disable annotation-cache reuse rather than produce a shared identity?
| except SupersetSecurityException: | ||
| access = False | ||
| data_key: Any = [ | ||
| annotation_query_context.query_cache_key(query_object) |
There was a problem hiding this comment.
For a saved samples context, execution clears metrics and selects all columns after this key is derived; an RLS template that calls current_user_id() only when metrics are absent therefore produces user-scoped rows under a shared annotation key. Should the scope key use the result handler’s prepared query, matching what actually executes?
There was a problem hiding this comment.
Code Review Agent Run #a2d0a6
Actionable Suggestions - 7
-
tests/unit_tests/common/test_query_context_processor.py - 7
- Fixture params unannotated · Line 218-220
- Mock vars unannotated · Line 224-225
- Fixture params unannotated · Line 239-241
- Inline import unjustified · Line 250-250
- Mock vars unannotated · Line 252-253
- Missing return type hints · Line 3049-3049
- Missing test docstring · Line 3068-3068
Additional Suggestions - 14
-
tests/unit_tests/common/test_query_context_processor.py - 14
-
Ordering claim unasserted · Line 298-301The assertions verify `_apply_annotation_overrides` mutated `mock_query_object`, but never observe what `query_cache_key` saw. Since `query_cache_key` is a MagicMock returning a constant, these asserts pass even if `_annotation_source_scope` derived the key *before* applying overrides -- the exact regression the docstring warns about. Snapshot `extras`/`from_dttm` inside a `query_cache_key.side_effect` to pin the ordering.
-
Duplicate MockCache stub (3rd copy) · Line 2597-2617This is the third near-identical inline `MockCache` stub in this file (siblings at lines 2316 and 2509), and the copies already drift (`annotation_data`, `cache_timeout`). A shared module-level stub/fixture would keep the next `get_df_payload` test from copying ~20 lines again. Left as a follow-up since it also touches the two pre-existing tests.
-
Missing test type annotations · Line 276-345Org rule (BITO adaptive 11810/12787) requires type annotations on fixture-injected test parameters and on mock variables. The four new tests declare `processor, mock_annotation_chart` unannotated, and lines 284/286 bind `MagicMock()` without `: MagicMock`. Return types are correctly annotated `-> None`; add the parameter/mock annotations to match.
-
Missing fixture param type · Line 121-121The renamed test adds ``-> None`` but leaves the ``processor`` fixture parameter unannotated. Repo rules (BITO.md adaptive rules 11810/12101/12801-family) require explicit type annotations on fixture-injected parameters in test functions, and the sibling tests in this file follow the same gap — but this line is newly rewritten, so it should carry the annotation. Low-risk, one-line fix.
-
Misleading mock comment · Line 126-132This rationale doesn't hold on Python 3.11: `unittest.mock._is_async_obj` only yields `AsyncMock` when `iscoroutinefunction(obj)` or `inspect.isawaitable(obj)` is true, and a bound `LocalProxy` over the synchronous `SupersetSecurityManager` (zero `async def` members in `superset/security/manager.py`) satisfies neither — verified by direct probe. An unbound proxy instead raises `RuntimeError` from `patch()`'s `get_original` lookup rather than silently mocking. The `new_callable=MagicMock` pinning itself is fine and harmless; only the explanation is wrong and will mislead future maintainers.
-
Missing fixture param annotations · Line 170-170Fixture-injected parameters `processor` and `query_obj` lack type annotations, unlike the sibling `test_annotation_cache_key_binds_native_annotation_read_scope` pattern and the repo's typing standard for test functions using fixtures. Annotate `processor: QueryContextProcessor` and `query_obj: MagicMock` to match the neighboring typed tests.
-
Missing fixture param annotations · Line 199-201Same typing-standard gap as the sibling test: fixture-injected `processor` and `mock_annotation_chart` parameters carry no annotations, while the `mock_annotation_chart` fixture itself (line 187) is fully annotated. Annotate `processor: QueryContextProcessor` and `mock_annotation_chart: MagicMock` for consistency with the file's typed tests.
-
Duplicated test scaffolding · Line 252-256This test duplicates the scaffolding of `test_annotation_source_scope_reuses_referenced_chart_cache_key` (lines 224-236): same mock query object/context, `query_cache_key` stub, `get_query_context` wiring, and `security_manager` patch. The module already factors shared setup into fixtures (`mock_annotation_chart`); a shared fixture would keep the two authorization scenarios from drifting.
-
Untyped fixture params · Line 348-349Fixture-injected params on this new test are untyped, while BITO rules 11810/12101 require explicit annotations for fixture params in test functions (`processor: QueryContextProcessor`, `mock_annotation_chart: MagicMock`; both already imported). The file's 40 existing test defs are also untyped, so this extends the gap rather than following the mandated pattern.
-
Inline import violates rule · Line 2486-2486BITO adaptive rule 12745 requires module-level imports unless a documented circular dependency exists; none applies here (`superset.common.query_object` does not import this test module). The file has ~25 similar inline imports, but the org rule governs new code. Move `QueryObject` to the module-level import block and drop the inline import.
-
Mocks missing type annotations · Line 2488-2490BITO adaptive rule 12787 asks for explicit type annotations on mock variables (`variable: MagicMock`). `mock_query_context` and `mock_datasource` (plus the `set_query_result` attribute at line 2529) are unannotated. Annotating the new mocks keeps the org typing standard visible; pre-existing sibling tests are out of scope.
-
Repeated call boilerplate · Line 3056-3056The three tests repeat the identical seven-keyword `_get_annotation_data_cached(...)` call (lines 3056-3063, 3077-3084, 3100-3107), varying only `force_query`/`force_cached`. A small helper or parametrization would leave one call site to update if the signature changes. Self-contained tests are also a valid idiom, so this is a maintainability trade-off.
-
Missing return type annotation · Line 3110-3110New test omits the `-> None` return annotation that BITO.md adaptive rules 7819/14234 mandate for all test functions, including new ones. Sibling tests (`test_get_annotation_data_cached_reads_from_cache` etc.) share the omission, but those lines are unchanged; this def line is new and the fix is one token. Add `-> None` to satisfy the enforced typing standard.
-
Identity assert on mocked access flag · Line 214-215`scope_a["access"] is True` asserts identity, but the value is `security_manager.can_access_datasource(...)`'s return (`query_context_processor.py:591`). On this mocked path the side_effect yields literal bools so it holds, yet the real `can_access_datasource` contract is truthy, not necessarily `bool` — a non-bool truthy return would fail the test spuriously. Equality assertions would be more robust.
-
Review Details
-
Files reviewed - 2 · Commit Range:
8f4d8b8..6a06c5e- superset/common/query_context_processor.py
- tests/unit_tests/common/test_query_context_processor.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| annotation_data: dict[str, Any] = {} | ||
| if query_obj and annotation_key and cache.status != QueryStatus.FAILED: | ||
| try: | ||
| annotation_data = self._get_annotation_data_cached( |
There was a problem hiding this comment.
When the dataframe is cached but an annotation on another PostgreSQL server is not, the async task records that annotation cursor's PID against the host database, so abort/timeout targets the wrong server and can terminate an unrelated session. Should cancellation track the database of the query actually executing here?
| except SupersetSecurityException: | ||
| access = False | ||
| data_key: Any = [ | ||
| annotation_query_context.query_cache_key(query_object) |
There was a problem hiding this comment.
A valid RLS clause such as owner_id = {{ current_user_id () }} is missed by the referenced query key's macro detector, so two users with the same grants get the same annotation key and the second receives the first user's rows—even if the referenced chart disables caching. Should annotation reuse require a verified user-dependent identity rather than trusting this syntactic detector?
| access: Any = True | ||
| except SupersetSecurityException: | ||
| access = False | ||
| data_key: Any = [ |
There was a problem hiding this comment.
For a saved annotation query without time_range, a relative override such as 1 hour ago : now contributes newly resolved timestamps to this key on every request. That defeats annotation cache/nonce reuse and can rerun the same user's slow annotation query synchronously after an async task succeeds; should the overridden logical range be retained when deriving this scope?
Code Review Agent Run #f9e08cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This branch has not been deployed
2 participants
SUMMARY
A prior fix (#42930) closed a real cross-user data leak: annotation-layer
payloads are fetched per requesting user and, for chart-backed layers, scoped
by the RLS clauses of the referenced chart's datasource, so they need to be
isolated per viewer. That fix bound the requesting user's identity into the
cache key for the entire chart dataframe, since the dataframe and the
annotation payload shared one cache entry. The side effect: every distinct
viewer of an annotation-layer chart got their own full copy of the
(potentially much larger) dataframe cached separately, instead of sharing one
entry.
This PR splits the two apart. The dataframe's cache key goes back to
depending only on its own datasource/RLS/
extra_cache_keys(unscoped byrequesting user), so distinct viewers of the same chart share one dataframe
cache entry again, regardless of their annotation access. Annotation data is
resolved and cached under its own, separate entry.
Update: while this was in review, #44402 landed on the identical root
cause with a different (and in one respect more complete) mechanism — see the
discussion on that PR. This PR now also adopts its approach to what goes
into the annotation key: instead of
{user_id, source_rls}(wheresource_rlsonly reflects RLS filter clauses, not base datasource access —so two users could coincidentally share a key despite one of them lacking
underlying access to the referenced chart's datasource), the annotation key
now binds an access-scope fingerprint: the
can_read/Annotationpermission for native layers, and for chart-backed layers,
can_access_datasource(...)on the referenced chart's datasource plus thatchart's own recursive
query_cache_key(which already covers RLS and anyper-user Jinja/virtual-dataset material). Credit to #44402 for identifying
and closing that base-access gap.
Where this PR still differs from #44402: it keeps the dataframe and
annotation payload on two separate cache entries rather than one combined
entry re-scoped by access class. The dataframe entry is always shared
regardless of annotation-access differences; #44402's combined entry means a
chart viewed by several distinct access classes still gets one full dataframe
copy per class. For charts with many annotation-access variations this PR's
approach uses less cache memory; for the common case (most viewers share the
same access) the two are equivalent.
No cache migration is needed; keys are content-addressed and recomputed on
every request, so stale entries under the old key shape simply age out via
normal TTL.
TESTING INSTRUCTIONS
pytest tests/unit_tests/common/test_query_context_processor.py— includesnew coverage: the dataframe cache key no longer varies with annotation
access scope, the annotation cache key varies with access scope (native
can_readpermission, chart-backed datasource access + the referencedchart's own cache key) but is shared across requesters with identical
scope, a fail-closed case when scope derivation errors, and an end-to-end
case proving a dataframe cache hit skips recomputation while annotation
data still resolves through its own path into the payload.
pytest tests/unit_tests/tasks/test_async_queries.py— unaffected/still green.users with the same role/RLS/annotation access; confirm both see correctly
scoped annotation data and that the chart's underlying dataframe query only
runs once, rather than once per viewer.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code