Repository navigation
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 56.19% 83.76% +27.56%
===========================================
Files 3053 3053
Lines 200807 200992 +185
Branches 46867 46911 +44
===========================================
+ Hits 112842 168356 +55514
+ Misses 86880 29398 -57482
- Partials 1085 3238 +2153
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?
There was a problem hiding this comment.
I don't think the read-back runs the annotation query in the request. With force_cached=True an annotation cache miss for B's scope raises CacheLoadError, _run_async catches it and submits a fresh task under B, which warms B's scoped entry. The cost is one extra task round trip for B after joining A's task, not an HTTP-request timeout. The only live-in-request case is the unresolved-scope fallback, which is deliberate. Happy to fold the annotation key into the task key if you think the extra round trip matters, but I'd rather not in this PR.
There was a problem hiding this comment.
The read-back after a joined task succeeds does not go through the force_cached probe: requestChartDataResolved reissues it with enableAsyncMode: false, so _run_async is never entered and viewer B's missing annotation entry is computed inline with force_cached=False. A slow annotation query can therefore still exceed the request timeout after the task reported success. Should the annotation scope be part of the task identity, or be warmed by the task, so each viewer's entry exists before completion?
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
d5fe17a to
6a06c5e
Compare
github-actions
Bot
removed
the
requires:rebase
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
…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>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Addresses three review points on the annotation cache scoping: - Record chart-backed layer scopes per layer, not per referenced chart ID, so two layers referencing the same chart with different overrides no longer overwrite each other's scope in the key. - Treat an unresolvable scope (missing chart, no saved query context and no resolvable datasource, or any derivation error) as "do not cache": the payload is fetched live and never written under a shared placeholder identity. A chart with a cleared datasource_id but a saved query context is scoped through that context's own authorization, matching the live fetch. - Retain the overridden logical time_range on the referenced query objects so a relative override keeps a stable cache key instead of contributing freshly resolved timestamps on every request. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…s to scoped keys Merge master, drop imports orphaned by the annotation cache split, and update the semantic-view annotation cache tests to the scoped annotation key shape. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e after rebase Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
- Re-raise SemanticResultCompletenessError from the annotation fetch so an incomplete semantic annotation result is not reported as success. - Count annotation fetch time in data_acquisition_ns. - Skip deriving a data key for a requester denied the source chart. - Resolve annotation time_range overrides with the saved query's time_shift, matching the query object factory. - Tighten tests: ordering of overrides vs key derivation, per-user context equality, force flag forwarding, caller-level failure paths, marker persistence, cache read errors, semantic version rotation. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…base Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ures Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ched read-back failures as 400 Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…n denial test Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
6a73c52 to
d9a080c
Compare
…urce chart's cache opt-out Review follow-ups on the annotation cache split: - Derive a chart-backed annotation layer's `data_key` from the query object its result type actually executes (`prepare_query_object`, shared with `get_query_results_with_timing`), so a samples/drill-detail source chart whose metrics are cleared at execution keys on the same query it runs. - Return no scope (never cache) when the referenced chart or its datasource has caching disabled, so rows the source never shares through its own cache are not shared through the host chart's annotation entry. - Tests: the `_run_async` 400 branch for a failed cached read-back, `notify_cursor` routing by database through the real seam, the annotation `CACHE_DISABLED_TIMEOUT` guard, the validation-type (and cause) assertion on an annotation failure, and the summed dataframe+annotation acquisition timing with a deterministic clock. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| """ | ||
| Returns a QueryObject cache key for objects in self.queries | ||
|
|
||
| This key covers the dataframe alone. It intentionally does not bind |
There was a problem hiding this comment.
With the annotation scope removed from this key, the shared async task key (_query_task_cache_key in async_queries.py) no longer separates users whose annotation access differs, but the worker still fetches annotations as whoever submitted first. If a user who can read the host dataset but not the annotation source starts the task, an authorized user who joins it gets that task's annotation access failure on a chart they're allowed to see. Should the task identity also include the annotation key, or the annotation fetch move out of the shared task?
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| ``force_cached`` too: computing here beats failing the read-back of | ||
| an otherwise successful async task. | ||
| """ | ||
| if cache_key is None: |
There was a problem hiding this comment.
When an annotation layer's scope is None (including the new cache-disabled case where the referenced chart has cache_timeout=-1), this fetches the annotation live even under force_cached, and never caches it. In async mode that means a warm-dataframe load runs the source chart's query inside the HTTP request in _run_async's probe, and on a cold load the worker's annotation run is discarded and the enableAsyncMode: false read-back runs it again in the request, with no task timeout or cancellation. A slow source chart with caching disabled can therefore hit the web request timeout even though the task succeeded. Would it make sense to return a cache miss from the async probe for uncacheable layers, or otherwise skip the in-request fetch when running async?
…ontext Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| # the per-user Jinja material the key has to carry. | ||
| data_key: Any = ( | ||
| [ | ||
| annotation_query_context.query_cache_key( |
There was a problem hiding this comment.
Whitespace-separated identity macros still escape this key: QueryContext.query_cache_key only renders the dataset's Jinja when SqlaTable.has_extra_cache_key_calls matches ExtraCache.regex, and that regex requires current_user_id( with no space. With an RLS clause like owner_id = {{ current_user_id () }} on the annotation source's dataset, two users with the same grants derive the same data_key (the raw RLS clause is identical and no user material is added), so the second viewer is served the first viewer's cached annotation rows. Before this PR the annotation context bound the requester's user ID, which hid that gap. Should an annotation entry only be shared when the key demonstrably carries the user-dependent material, rather than relying on that syntactic gate?
| patch("superset.tasks.async_queries.current_app"), | ||
| ): | ||
| with _capture_query_cancellation(qc): | ||
| notify_cursor(MagicMock(), other_database) |
There was a problem hiding this comment.
This still bypasses the production caller: notify_cursor is invoked directly with the database, so reverting Database._execute_sql_with_mutation_and_logging in models/core.py back to notify_cursor(cursor) keeps every test green. In that case a warm host dataframe with a cold annotation layer on another PostgreSQL server records the annotation cursor against the host database, and abort or timeout can cancel the wrong server's session. Could a test run the real execute path under capture_cancel_id (or assert the notify_cursor call args from it) and check the host database registers the handle while another database does not?
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