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 81.52% 81.56% +0.03%
==========================================
Files 2974 2977 +3
Lines 180288 180713 +425
Branches 41741 41786 +45
==========================================
+ Hits 146985 147397 +412
- Misses 30580 30593 +13
Partials 2723 2723
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
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>
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
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 |
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