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 65.02% 64.95% -0.07%
==========================================
Files 3039 3038 -1
Lines 198382 198216 -166
Branches 46213 46142 -71
==========================================
- Hits 128992 128752 -240
- Misses 66489 66566 +77
+ Partials 2901 2898 -3
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
d5fe17a to
6a06c5e
Compare
github-actions
Bot
removed
the
requires:rebase
| 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
| 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?
CodeAnt PR Risk: Medium Risk
Assessed commit: |
| and cache.status != QueryStatus.FAILED | ||
| ): | ||
| try: | ||
| annotation_data = self._get_annotation_data_cached( |
There was a problem hiding this comment.
With CHART_DATA_INCLUDE_TIMING on, annotation SQL now runs after data_acquisition_ns is captured and before payload_assembly_start_ns starts, so it lands in none of the reported phases. A cold chart with a 10 ms dataframe and a 5 s annotation query reports roughly 10 ms of acquisition, and a dataframe cache hit reports data_acquisition_ms: null even though annotation SQL ran. The configuring-superset docs say data_acquisition_ms "includes database work and annotation dependencies", and version 1 can't change semantics. Should the annotation fetch be timed into data_acquisition_ns (summed with the dataframe time when both run)?
| "superset.common.query_context_processor.security_manager", | ||
| new_callable=MagicMock, | ||
| ): | ||
| processor._annotation_source_scope(layer) |
There was a problem hiding this comment.
query_cache_key is mocked to a constant here and the assertions only look at the final query-object state, so moving _apply_annotation_overrides after key derivation would still pass. That reintroduces the miss where a daily override pulls in current_user_id() Jinja that the saved monthly query lacks, and the second viewer gets the first viewer's rows. Could query_cache_key use a side_effect that asserts the overridden grain, time_range and bounds are already set when it is called?
| # keyed by the dedicated annotation cache key -- not the dataframe's key. | ||
| mock_get_annotation.assert_called_once() | ||
| _, kwargs = mock_get_annotation.call_args | ||
| assert kwargs["cache_key"] == "ann-key" |
There was a problem hiding this comment.
This only asserts cache_key on the _get_annotation_data_cached call. The helper-level force_cached test passes the flag directly and the payload test replaces the helper, so dropping force_cached=force_cached at the call site in get_df_payload_result would still pass. Async cache preflight with force_cached=True and a missing annotation entry would then run the annotation SQL instead of raising CacheLoadError. Should this also assert kwargs["force_cached"] (and force_query), or add a force_cached=True case with the real helper?
github-actions
Bot
added
the
requires:rebase
| get_column_names_from_columns, | ||
| get_column_names_from_metrics, | ||
| get_user_id, | ||
| is_adhoc_column, |
There was a problem hiding this comment.
Dropping get_user_id here leaves the rest of the old annotation-context imports orphaned: DatasourceDAO, DatasourceNotFound, DatasourceTypeNotSupportedError, DatasourceValueIsIncorrect, json and the TYPE_CHECKING Datasource no longer have any reference in this file now that _annotation_source_scope resolves the datasource through the chart's query context. Ruff's F401 will fail lint/pre-commit on the changed file. Can these be removed in the same pass?
| if ( | ||
| query_obj | ||
| and query_obj.annotation_layers | ||
| and cache.status != QueryStatus.FAILED |
There was a problem hiding this comment.
The caller-level wiring around this block has no direct coverage: every new get_df_payload_result test either supplies a successful dataframe cache or replaces _get_annotation_data_cached with a mock. If someone drops the cache.status != QueryStatus.FAILED guard, a failed dataframe would still trigger the annotation fetch, and an annotation exception could replace the original failure response; likewise nothing asserts the annotation nonce marker is written only when set_and_log_cache actually persisted (the existing marker tests call _mark_force_executed directly). Could a test drive get_df_payload_result with annotation layers through the real helper for both a failed dataframe (annotation helper not called, annotation_data == {}, original error kept) and a non-persisting annotation write (no marker, next same-nonce read still forces)?
github-actions
Bot
removed
the
requires:rebase
| # (which would defeat cache and force-nonce reuse), and an | ||
| # override on a saved query without a range of its own is | ||
| # still told apart from a different override. | ||
| query_object.time_range = time_range |
There was a problem hiding this comment.
Assigning query_object.time_range here makes QueryObject.cache_key() drop the resolved bounds, but the bounds above are computed without the saved query's time_shift. For a saved annotation chart with time_shift="1 year ago", a 2025-01-01 : 2025-02-01 override executes January 2025 data, while a normal run of that chart with the same range and shift executes January 2024; both now produce the same query cache key. Whichever runs first populates the entry the other reads, so the annotation can show the wrong year's rows (or the chart can show the annotation's). Should the override keep the explicit bounds in the key whenever the query carries a time_shift, or apply the shift to the override range the way the query factory does?
| layer.cache_version = 1 | ||
| session.info.clear() | ||
| view.forget_metadata() | ||
| assert processor._annotation_cache_context(query) != first |
There was a problem hiding this comment.
This inequality also passes if scope derivation starts failing after the version rotates: _annotation_cache_context returns None on any derivation error, and None != first is true. A regression that silently disables annotation caching for semantic views after a metadata change would therefore still satisfy this test (the call_count == 2 check below doesn't distinguish it either), and the same applies to the version-rotation assertion in test_cache_keys_compose_metadata_and_producer_versions. Could the second context be captured and asserted non-None, with the rotated version field compared and the unchanged one asserted equal?
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>
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>
10e2761 to
7aad011
Compare
| an otherwise successful async task. | ||
| """ | ||
| if cache_key is None: | ||
| return self.get_annotation_data(query_obj) |
There was a problem hiding this comment.
When an annotation layer's scope can't be resolved, this live fetch runs even under force_cached. In the async probe (_run_async only catches the cache-load error), a failed fetch now surfaces as an uncaught ChartDataQueryFailedError instead of falling through to the task path.
Concretely: user A warms the shared dataframe for a chart whose annotation layer references a chart that user B can't see. B loads it in async mode, hits the warm dataframe, the live fetch fails with "chart not found", and B gets a server error where the old code (cache miss, then task) and the sync path return a 400.
Should the force_cached probe return a cache-load miss for an unresolved scope instead of computing, so the request still goes through the normal task path?
| context_a = processor_a._annotation_cache_context(query_obj_a) | ||
| context_b = processor_b._annotation_cache_context(query_obj_b) | ||
|
|
||
| assert context_a == context_b |
There was a problem hiding this comment.
This test never varies the requesting user: both processors run under the same ambient identity, so putting get_user_id() back into the annotation context would still give equal contexts and pass. That is the exact regression this PR is meant to prevent (same-scope viewers sharing one entry).
Could it patch get_user_id to return different values for the two calls and assert the contexts stay equal?
github-actions
Bot
added
the
requires:rebase
Carry the semantic NULL-filter protocol marker into the scoped annotation cache key. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
github-actions
Bot
removed
the
requires:rebase
| except SupersetSecurityException: | ||
| access = False | ||
| data_key: Any = [ | ||
| annotation_query_context.query_cache_key(query_object) |
There was a problem hiding this comment.
With ALLOW_ADHOC_SUBQUERY on, an annotation-source chart whose metric is a SQL expression with a subquery (MAX((SELECT SUM(amount) FROM sales))) gets its tenant RLS on sales injected at execution time, but that RLS is not part of query_cache_key or get_rls_cache_key here. Two users with the same dataset grant and different RLS on sales therefore get identical access and data_key, and when the source chart does not cache itself (cache_timeout=-1) the second user is served the first user's warmed total. The old user_id binding isolated this. Should the scope include the RLS predicates of tables referenced inside adhoc expressions, or skip caching when an adhoc subquery is present?
| ), | ||
| patch("superset.common.query_context_processor.get_user_id", return_value=42), | ||
| ): | ||
| context: dict[str, Any] = processor._annotation_cache_context(query) |
There was a problem hiding this comment.
_annotation_cache_context now returns dict[str, Any] | None, but the result is assigned to a dict[str, Any] local in a function that is fully annotated (-> None), so mypy checks it and will report an incompatible assignment. Could this declare the optional type and assert it is not None before indexing context["source_scope"]?
| """A forced refresh recomputes rather than serving a stale cached value.""" | ||
| with ( | ||
| patch("superset.common.query_context_processor.cache_manager") as cache_manager, | ||
| patch("superset.common.query_context_processor.set_and_log_cache"), |
There was a problem hiding this comment.
set_and_log_cache is patched with a bare MagicMock here and nothing asserts the nonce-marker write that depends on its return value, so replacing persisted with True at the _mark_force_executed call in _get_annotation_data_cached would still pass. A forced refresh whose write fails would then record the marker and a same-nonce read-back would serve the stale annotation entry. Could a test at this boundary drive set_and_log_cache to return both True and False and assert the marker is written under the annotation key only on success?
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| access: Any = True | ||
| except SupersetSecurityException: | ||
| access = False | ||
| data_key: Any = [ |
There was a problem hiding this comment.
When raise_for_access() just denied this requester, access is False, but execution still falls through to annotation_query_context.query_cache_key(...). That call runs get_extra_cache_keys, which can compile the referenced dataset's SQL and render its Jinja (including DB-backed macros like latest_partition) for a user who has no access to it, on every load of the annotated chart — and nothing is ever cached under an access: False scope, so the work repeats. Should the denied branch skip data_key (e.g. return {"access": False}), since a denied scope never needs a data identity?
| ) | ||
|
|
||
|
|
||
| def test_get_annotation_data_cached_force_cached_raises_on_miss( |
There was a problem hiding this comment.
The new except Exception around cache_manager.data_cache.get(cache_key) in _get_annotation_data_cached has no test: the only read-error test (test_resolve_forced_query_forces_when_marker_read_errors) exercises the marker lookup in _resolve_forced_query, not this catch. If that catch were dropped, a cache backend ConnectionError would propagate and fail the whole chart-data request, and every test here would still pass. Could you add a case where a dataframe cache hit is paired with data_cache.get raising, asserting live annotations are returned without force_cached and CacheLoadError is raised (no live fetch) with force_cached=True?
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