[no-hup]

SUMMARY

is_editor and is_viewer both start by resolving the current user's subject IDs, and both take a single resource. Anything that checks a list of objects calls them in a loop, so the subject lookup runs once per object.

The lookup isn't cheap — get_user_subject_ids_subquery is a three-way UNION ALL over the user's own subject, their direct and group-derived roles, and their groups. Loading a dashboard runs can_access_chart per chart, which is where I noticed it.

Counting the union queries while checking 40 charts as a non-admin:

	subject-union queries
master	80
this branch	1

Two per chart, because the editor check and the viewer check each do their own lookup.

The fix memoises the result on the request object (flask.request), keyed by user id. It's tied to the request, not g: g is bound to the app context, which a worker can hold open across many requests, so a g-keyed cache would survive past the request it was built for and serve a stale answer to the next one. The request object is torn down when the request ends, so the cache gets exactly the request lifetime.

The cache can miss a subject created earlier in the same request — the create paths in commands/utils.py read back through this function — but that never flips an access decision: a subject created mid-request isn't listed in any resource's editors or viewers yet, so a check against a cache that omits it returns the same answer. The staleness window is bounded by the request.

Two other things I was deliberate about:

• Outside a request context the behaviour is unchanged. It's guarded by has_request_context(), so Celery tasks and CLI commands keep reading through every time.
• It returns a copy. One caller hands the list into the bootstrap payload that COMMON_BOOTSTRAP_OVERRIDES_FUNC is allowed to edit, so handing out the cached list itself would let a customisation hook corrupt it.

Happy to change the cache key or drop it behind a config flag if you'd rather — this seemed like the smallest version that works.

TESTING INSTRUCTIONS

pytest tests/unit_tests/subjects tests/unit_tests/commands tests/unit_tests/dao — passes.

Tests in tests/unit_tests/subjects/test_utils.py cover the cases I cared about: it runs once per user per request, a second user is a separate entry, the cache doesn't leak from one request into the next inside a shared app context, a subject added mid-request stays invisible until the next request (the staleness contract), it still reads through with no request context, and mutating the returned list doesn't poison the cache.

ruff check, ruff format --check and mypy are clean on both files.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #c659c7

Actionable Suggestions - 0

Additional Suggestions - 1

• tests/unit_tests/subjects/test_utils.py - 1

  • Inline imports redundant · Line 618-618
    The module already imports from `superset.subjects.utils` at the top (lines 31-38), so the inline `from superset.subjects.utils import get_user_subject_ids` in each new test (618, 645, 657) is redundant. No circular dependency exists here. Move it to the module-level import for consistency and discoverability.

Review Details

• Files reviewed - 2 · Commit Range: e688808..e688808

  • superset/subjects/utils.py
  • tests/unit_tests/subjects/test_utils.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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	0a1af5e
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ab48f18c68c3300081f43fd
😎 Deploy Preview	https://deploy-preview-44017--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.28%. Comparing base (8141d66) to head (215ec8a).
⚠️ Report is 168 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44017      +/-   ##
==========================================
+ Coverage   81.07%   81.28%   +0.20%     
==========================================
  Files        2955     2969      +14     
  Lines      178331   180129    +1798     
  Branches    41313    41553     +240     
==========================================
+ Hits       144580   146412    +1832     
+ Misses      31041    30995      -46     
- Partials     2710     2722      +12     

Flag	Coverage Δ
hive	36.69% <66.66%> (-0.11%)	⬇️
mysql	55.95% <100.00%> (-0.03%)	⬇️
postgres	55.96% <100.00%> (-0.03%)	⬇️
presto	38.61% <66.66%> (-0.08%)	⬇️
python	85.71% <100.00%> (+0.33%)	⬆️
sqlite	55.69% <100.00%> (-0.02%)	⬇️
unit	78.49% <100.00%> (+0.84%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

Code Review Agent Run #527545

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: e688808..fcad6e5

  • tests/unit_tests/subjects/test_utils.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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[EnxDev]

I checked request boundaries, user changes within one request, non-request callers, and mutation of returned lists. The g key is user-specific, and the defensive copies prevent callers from poisoning the cached value. I didn’t find a correctness or access-control issue in this pass.

[no-hup]

Thanks for the careful pass — checking the request boundary, the g key scoping, and the defensive copies is exactly where this could have gone wrong, so I appreciate you going through those paths.

[EnxDev]

One request-lifetime question below. I checked the changed function with real Flask contexts and a mocked subject query; I did not run the full Superset test suite.

[bito-code-review]

The current implementation uses g to store the cache, which persists for the duration of the application context. As you noted, this means the cache survives across multiple requests within the same app_context(). To tie the cache to the request lifetime specifically, you can store it on the flask.request object instead of g. Since flask.request is only available when has_request_context() is true, this naturally aligns with your existing check.

superset/subjects/utils.py

if not has_request_context():
        return _query_user_subject_ids(user_id)
    # Store on the request object instead of g
    cache: dict[int, list[int]] = getattr(request, "_user_subject_ids", {})
    if not hasattr(request, "_user_subject_ids"):
        request._user_subject_ids = cache
    if user_id not in cache:
        cache[user_id] = _query_user_subject_ids(user_id)
    return list(cache[user_id])

[rusackas]

Nice catch, @EnxDev, and looks like bito's landed on the same fix. g outlives the request inside a shared app context, so the cache doesn't actually have the lifetime the docstring promises, that matters for anything holding an app context across multiple requests (tests with an autouse fixture, some worker setups). Keying it off flask.request instead of g seems like the right move since it's already gated by the same has_request_context() check. Want to see that addressed, plus a test that reuses one app context across two requests, before this merges.

[no-hup]

Done in f1aa7f2. The cache now lives on flask.request (via _get_current_object()) instead of g, so it's gone when the request ends rather than surviving to the next request that shares the app context. Same has_request_context() guard as before — outside a request we still read through.

Added test_get_user_subject_ids_cache_does_not_leak_across_requests: one app context, two test_request_context blocks, the mocked query returns [7, 8] then [9], and it asserts the second request re-queries and sees [9]. It fails against the old g version (returns the stale [7, 8]) and passes with this change. Ran pytest tests/unit_tests/subjects/test_utils.py (31 passed), ruff, and mypy locally; didn't run the full suite.

[bito-code-review]

Code Review Agent Run #f3c530

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: fcad6e5..f1aa7f2

  • superset/subjects/utils.py
  • tests/unit_tests/subjects/test_utils.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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[no-hup]

Heads-up on the red checks (babel-extract and the check_pot_drift unit test) — they aren't from this diff. This PR only touches superset/subjects/utils.py and its test.

The check_pot_drift failure lists five strings missing from messages.pot: the alert/report retry messages, plus Screenshot cache is unavailable and Screenshot request is temporarily unavailable. Those live in superset/reports/notifications/* and superset/dashboards/api.py on master, not in anything here. messages.pot on current master doesn't carry them yet, so any branch cut from master fails this check the same way — rebasing won't clear it, it needs a messages.pot regeneration on master. Leaving this one as-is.

[EnxDev]

Re-read after f1aa7f2. Moving the cache off g and onto the request object is the right fix, and test_get_user_subject_ids_cache_does_not_leak_across_requests does fail against the old version, so the lifetime question from my last pass is closed.

I walked the create-then-check orderings I could find (populate_subject_list, and the get_user_group_subjects backfill on the save-as and importer paths) and none of them flip an access decision, so nothing here blocks. Two notes inline, both about writing the invariant down rather than changing the code.

Also confirmed the red checks aren't from this diff: Screenshot cache is unavailable lives in dashboards/api.py on master and is absent from messages.pot on master, so check_pot_drift fails the same way on any branch cut from it.

One thing not worth an inline note: the description still says the cache lives on g and explains the has_request_context() guard in those terms. Worth refreshing before this merges so the squashed commit message matches the code.

[no-hup]

Thanks, both of these are fair.

• The staleness reasoning. You're right that the "nothing reads a user's subjects after changing them" wording is wrong — commands/utils.py:127 creates the subject and line 133 reads it straight back, and the group backfill does the same. I rewrote the docstring to the reason that actually holds: a subject created mid-request isn't listed in any resource's editors or viewers yet, so a check against a cache that omits it returns the same access decision. The window is bounded by the request either way.
• The test. Added test_get_user_subject_ids_serves_a_stale_set_within_the_request: warm the cache with [7], swap the query to [7, 99] in the same request, assert the call still returns [7]. That pins the staleness window as a contract instead of a docstring claim.
• The description. Refreshed it so it says the cache lives on flask.request, not g, with the corrected staleness note, so the squashed message matches the code.

The red checks are the repo-wide check_pot_drift / babel-extract failure you already confirmed isn't from this diff.

[EnxDev]

Supersedes my earlier pass. Both notes from it are closed.

The docstring now carries the reason that actually holds, and test_get_user_subject_ids_serves_a_stale_set_within_the_request pins it: warm with [7], swap the query to [7, 99] inside the same request, still [7]. Take the caching out and that assert fails, so it guards the contract instead of restating it. The description reads off flask.request now too, so the squashed message will match the code.

Also checked that setting _user_subject_ids on the request is safe: werkzeug's Request defines no __slots__, and _get_current_object() writes to the real object rather than to the proxy, so the attribute lands somewhere teardown can take it.

Re-confirmed the red checks at this sha: the one failing test is check_pot_drift_test, nothing from this diff.

One note inline about the embedded guest path. Not a blocker.

[bito-code-review]

Code Review Agent Run #cf92e9

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: f1aa7f2..32b0f6b

  • superset/subjects/utils.py
  • tests/unit_tests/subjects/test_utils.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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[sha174n]

LGTM — the g-vs-request reasoning is the right call, and worth keeping in the docstring where it is. g is bound to the app context, which a worker can hold open across requests; hanging the cache off request._get_current_object() gives it exactly the lifetime the docstring promises. Copy-on-return and the uncached path outside a request context are both covered by tests.

One suggestion on the safety argument rather than the code. The docstring justifies staleness in the creation direction only ("a subject created mid-request is not yet listed in any resource's editors or viewers"). The direction that isn't argued is removal — a cache retaining a membership that was revoked earlier in the same request.

I went looking for a path that could exercise either, and the only one is the ensure_no_lockout branch in superset/commands/utils.py (~L126-135), which evaluates user_subject_ids & set(subject_ids) while editors are being set. Both directions are safe there, but for different reasons:

• cache missing a just-created subject → intersection empty → the caller's own subject gets appended, over-granting them access to their own resource. Fails safe.
• cache retaining a removed one → intersection non-empty → the caller can lock themselves out. Annoying, not a security issue.

Neither grants access to anyone else, and the window is bounded by the request. Might be worth widening the docstring to say that, since "no decision flips" is the claim doing the work here and it holds for both directions, not just creation.

[no-hup]

good call, thats the direction i left implicit. widened the docstring in 2d17bf4 to argue both ways: a miss on a just-created subject cant list it anywhere yet, and a stale retained membership only reaches ensure_no_lockout in commands/utils.py where the worst case is the caller locking themselves out. neither flips a decision for a third party. thanks for the careful read on both PRs.

[bito-code-review]

Code Review Agent Run #01ab34

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 32b0f6b..2d17bf4

  • superset/subjects/utils.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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[EnxDev]

Supersedes my earlier pass. The only new change is the docstring for the revoke direction, and the code and tests are unchanged since then, so still nothing blocking from me.

One wording note inline on the new sentence, since it's the part carrying the security argument. The guest path note from last time is still fine as a follow-up.

[bito-code-review]

Code Review Agent Run #87c15a

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 2d17bf4..215ec8a

  • superset/subjects/utils.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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by