fix: Enforce per-user caching on legacy API endpoint by Vitor-Avila · Pull Request #39789 · apache/superset

Vitor-Avila · 2026-04-30T20:09:49Z

SUMMARY

The /superset/explore_json/ endpoint was not respecting per-user caching, which can be enforced via FF and also required for DB connections authenticating with OAuth2.

This PR fixes the issue by moving the logic responsible for adding the cache keys to a helper that's used on both flows.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No UI changes.

TESTING INSTRUCTIONS

Test coverage added. For manual testing:

Enable the CACHE_QUERY_BY_USER FF.
Create a chart, and load it using Account A.
Access Superset using Account B, and validate this account doesn't use Account's A cache.

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
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

bito-code-review · 2026-04-30T20:09:59Z

Code Review Agent Run #abcfce

Actionable Suggestions - 0

Review Details

Files reviewed - 6 · Commit Range: f41bca7..f41bca7
- superset/common/query_object.py
- superset/utils/cache_keys.py
- superset/viz.py
- tests/unit_tests/queries/query_object_test.py
- tests/unit_tests/test_viz_cache_key.py
- tests/unit_tests/utils/test_impersonation_cache_key.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - 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

codeant-ai-for-open-source · 2026-04-30T20:11:56Z

+from superset.utils.core import override_user
+
+
+def _flag(name: str):


Suggestion: Add an explicit return type annotation to _flag so the helper has fully typed input and output. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The helper is only partially typed: name has a type annotation, but the function has no return type annotation.
This matches the suggested custom rule about fully typing new helper functions, so the violation is real.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_impersonation_cache_key.py **Line:** 27:27 **Comment:** *Custom Rule: Add an explicit return type annotation to `_flag` so the helper has fully typed input and output. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-04-30T20:11:56Z

+    def side_effect(feature=None):
+        return feature == name


Suggestion: Add explicit type hints for the side_effect parameter and return value so the nested function is fully typed. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The nested side_effect function has neither a parameter type annotation nor a return type annotation.
That is a real typing omission in the new code, so the suggestion is verified.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_impersonation_cache_key.py **Line:** 30:31 **Comment:** *Custom Rule: Add explicit type hints for the `side_effect` parameter and return value so the nested function is fully typed. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-04-30T20:11:56Z

+
+def test_no_per_user_caching_yields_no_key():
+    database = Database(database_name="d", sqlalchemy_uri="sqlite://")
+    with override_user(User(username="u")):
+        assert "impersonation_key" not in _run(database)


Suggestion: Add a short docstring to this test function describing the expected behavior being validated. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This test function has no docstring in the final file state.
If the custom rule requires docstrings for new tests, this is a real violation.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_impersonation_cache_key.py **Line:** 42:46 **Comment:** *Custom Rule: Add a short docstring to this test function describing the expected behavior being validated. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-04-30T20:11:56Z

+
+
+@patch("superset.utils.cache_keys.feature_flag_manager")
+def test_cache_query_by_user_adds_username(feature_flag_mock):


Suggestion: Add explicit type annotations for the patched mock argument and the None return type on this new test function. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The patched test function is missing both a type annotation for feature_flag_mock and an explicit -> None return type.
This is a direct mismatch with the suggested typing rule, so the issue is present.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_impersonation_cache_key.py **Line:** 50:50 **Comment:** *Custom Rule: Add explicit type annotations for the patched mock argument and the `None` return type on this new test function. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-04-30T20:11:57Z

+def test_per_user_caching_in_extra_json_enables_key():
+    database = Database(
+        database_name="d",
+        sqlalchemy_uri="sqlite://",
+        extra='{"per_user_caching": true}',
+    )
+    with override_user(User(username="alice")):
+        assert _run(database)["impersonation_key"] == "alice"


Suggestion: Add a docstring to this test function so the intent of the scenario is documented consistently with the rule. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The test function does not include a docstring.
Assuming the custom rule requires docstrings for new test scenarios, this is a genuine violation.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_impersonation_cache_key.py **Line:** 86:93 **Comment:** *Custom Rule: Add a docstring to this test function so the intent of the scenario is documented consistently with the rule. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-04-30T20:12:05Z

+QUERY_OBJ: dict[str, Any] = {"row_limit": 100, "from_dttm": None, "to_dttm": None}
+
+
+def _viz_for(database: Database) -> viz.BaseViz:


Suggestion: Add a short docstring to this helper function to describe its purpose and expected behavior. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The helper function _viz_for is newly introduced and has no docstring in the existing code.
If the custom rule requires docstrings for new functions, this is a real violation.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/test_viz_cache_key.py **Line:** 35:35 **Comment:** *Custom Rule: Add a short docstring to this helper function to describe its purpose and expected behavior. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-04-30T20:12:05Z

+    return viz.BaseViz(datasource=datasource, form_data={"viz_type": "table"})
+
+
+def test_no_per_user_opt_in_keys_match_across_users():


Suggestion: Add an explicit return type annotation to this test function (for example, -> None) to comply with the typing rule. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The test function is defined without an explicit return type annotation.
If the rule requires newly added functions to declare -> None, the existing code violates that rule.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/test_viz_cache_key.py **Line:** 46:46 **Comment:** *Custom Rule: Add an explicit return type annotation to this test function (for example, `-> None`) to comply with the typing rule. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-04-30T20:12:05Z

+    assert key_a != key_b
+
+
+def test_same_user_same_query_idempotent():


Suggestion: Add a function docstring explaining the scenario this test validates. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This newly added test function has no docstring in the existing code.
Assuming the custom rule is to document new functions, the violation is present.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/test_viz_cache_key.py **Line:** 83:83 **Comment:** *Custom Rule: Add a function docstring explaining the scenario this test validates. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codeant-ai-for-open-source · 2026-04-30T20:12:05Z

+
+
+@patch("superset.utils.cache_keys.feature_flag_manager")
+def test_cache_query_by_user_flag_yields_distinct_keys(feature_flag_mock):


Suggestion: Add explicit type hints for the function parameter and return type to satisfy the typing requirement for new functions. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The function has an untyped parameter (feature_flag_mock) and no return annotation.
If the typing rule applies to new functions here, this is a genuine violation.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** tests/unit_tests/test_viz_cache_key.py **Line:** 96:96 **Comment:** *Custom Rule: Add explicit type hints for the function parameter and return type to satisfy the typing requirement for new functions. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

codecov · 2026-04-30T20:14:09Z

Codecov Report

❌ Patch coverage is 70.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.41%. Comparing base (4244ae8) to head (fc62f22).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/utils/cache_keys.py	69.23%	3 Missing and 1 partial ⚠️
superset/viz.py	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #39789      +/-   ##
==========================================
+ Coverage   64.39%   64.41%   +0.02%     
==========================================
  Files        2567     2568       +1     
  Lines      134411   134424      +13     
  Branches    31203    31203              
==========================================
+ Hits        86557    86595      +38     
+ Misses      46354    46332      -22     
+ Partials     1500     1497       -3

Flag	Coverage Δ
hive	`39.71% <50.00%> (+<0.01%)`	⬆️
mysql	`60.01% <70.00%> (?)`
postgres	`60.09% <70.00%> (+<0.01%)`	⬆️
presto	`41.46% <60.00%> (+<0.01%)`	⬆️
python	`61.65% <70.00%> (+0.04%)`	⬆️
sqlite	`59.73% <70.00%> (+<0.01%)`	⬆️
unit	`100.00% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 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.

sadpandajoe

Clean bug fix — correctly extracts the per-user cache key logic into a shared helper and applies it to the legacy viz.BaseViz.cache_key() path. Good test coverage for both the helper and the viz integration. LGTM.

betodealmeida

We should try to get rid of superset/viz.py, having 2 paths like this is not good.

bito-code-review · 2026-05-01T05:29:12Z

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it

pull-request-size Bot added the size/L label Apr 30, 2026

dosubot Bot added api Related to the REST API infra:caching Infra setup and configuration related to caching labels Apr 30, 2026

codeant-ai-for-open-source Bot reviewed Apr 30, 2026

View reviewed changes

Comment thread superset/utils/cache_keys.py

sadpandajoe approved these changes Apr 30, 2026

View reviewed changes

betodealmeida approved these changes Apr 30, 2026

View reviewed changes

fix: Enforce per-user caching on legacy API endpoint

fc62f22

Vitor-Avila force-pushed the fix/legacy-per-user-caching branch from f41bca7 to fc62f22 Compare April 30, 2026 20:47

github-actions Bot removed the api Related to the REST API label Apr 30, 2026

Vitor-Avila merged commit 86eb617 into master Apr 30, 2026
65 checks passed

Vitor-Avila deleted the fix/legacy-per-user-caching branch April 30, 2026 21:04

Vitor-Avila mentioned this pull request May 5, 2026

fix(OAuth2): Support OAuth2 exception with legacy endpoint #39897

Merged

9 tasks

bestlong pushed a commit to bestlong/superset that referenced this pull request May 7, 2026

fix: Enforce per-user caching on legacy API endpoint (apache#39789)

2c16505

		from superset.utils.core import override_user


		def _flag(name: str):



		@patch("superset.utils.cache_keys.feature_flag_manager")
		def test_cache_query_by_user_adds_username(feature_flag_mock):

		QUERY_OBJ: dict[str, Any] = {"row_limit": 100, "from_dttm": None, "to_dttm": None}


		def _viz_for(database: Database) -> viz.BaseViz:

		return viz.BaseViz(datasource=datasource, form_data={"viz_type": "table"})


		def test_no_per_user_opt_in_keys_match_across_users():

		assert key_a != key_b


		def test_same_user_same_query_idempotent():



		@patch("superset.utils.cache_keys.feature_flag_manager")
		def test_cache_query_by_user_flag_yields_distinct_keys(feature_flag_mock):

Conversation

Vitor-Avila commented Apr 30, 2026

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #abcfce

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

sadpandajoe left a comment

Choose a reason for hiding this comment

Uh oh!

betodealmeida left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bito-code-review Bot commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bito-code-review Bot commented Apr 30, 2026 •

edited

Loading

codecov Bot commented Apr 30, 2026 •

edited

Loading