fix(semantic_layers): partition cache keys by user when CACHE_QUERY_BY_USER is enabled (#45877) by FrancescoCastaldi · Pull Request #45895 · apache/superset

FrancescoCastaldi · 2026-10-30T17:18:31Z

SUMMARY

Chart-data results for semantic views (datasource_type: "semantic_view") were cached under the same key across all users, even when CACHE_QUERY_BY_USER was enabled.

Root Cause

In QueryObject.cache_key, per-user cache partitioning attempts to read self.datasource.database:

try:
    add_impersonation_cache_key_if_needed(self.datasource.database, cache_dict)
except AttributeError:
    pass

A SemanticView object does not have a .database attribute, so the AttributeError is silently caught and bypassed, omitting user isolation.

Solution

This PR encapsulates user cache partitioning cleanly and exclusively inside SemanticView.get_extra_cache_keys():

When is_feature_enabled("CACHE_QUERY_BY_USER") is true, queries current_host_execution_context().
Appends ("semantic-user", exec_context.identity_digest) when the caller is not a guest (not exec_context.is_guest) and identity_digest is present.
Preserves shared caching across embedded guest users (is_guest=True) as intended by the guest token security model.
Leaves cache keys unpartitioned when the feature flag is disabled, maintaining full backward compatibility.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A (Backend caching behavior)

TESTING INSTRUCTIONS

Run unit tests:

pytest tests/unit_tests/semantic_layers/models_test.py -k test_semantic_view_cache_query_by_user

ADDITIONAL INFORMATION

Has associated issue: Fixes Semantic views ignore CACHE_QUERY_BY_USER: chart-data results are shared across users #45877
Required feature flags: CACHE_QUERY_BY_USER
Changes UI
Includes DB Migration (follow approval process in SIP-59)
Introduces new feature or API
Removes existing feature or API

codeant-ai-for-open-source · 2026-10-30T17:31:56Z

+            except Exception as ex:
+                logger.debug(
+                    "Could not resolve semantic execution context for cache: %s", ex
+                )


Suggestion: When CACHE_QUERY_BY_USER is enabled, any context-building failure is swallowed and these keys lack a user discriminator, so cache lookup can return another user's semantic results.

Assessment: 🔴 Critical · 🔁 Occurrence: Rarely · 🏷️ Security

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset/semantic_layers/models.py **Line:** 920:923 **Comment:** *Security: When `CACHE_QUERY_BY_USER` is enabled, any context-building failure is swallowed and these keys lack a user discriminator, so cache lookup can return another user's semantic results. 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

👍 | 👎

Resolved in commit 3895a3a.

When CACHE_QUERY_BY_USER is enabled and context resolution encounters an unexpected failure, we now log a warning and append a distinct nonce ('semantic-user-unresolved', uuid.uuid4().hex). This ensures fail-safe user isolation so that failed context resolutions never collapse into a shared unpartitioned cache key or leak semantic results across users. Dedicated test coverage has been added in tests/unit_tests/semantic_layers/models_test.py exercising this fallback branch, ensuring 100% module coverage.

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 3895a3a.

If resolving the execution context raises an exception while user-based caching is enabled, the code now adds a fresh semantic-user-unresolved UUID to the cache keys, preventing reuse of another request’s cached results.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 5b2e270.

When execution-context resolution raises an exception, the cache keys now include a fresh semantic-user-unresolved UUID, preventing reuse of results from another context.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 7272883.

When execution-context resolution raises an exception, the cache keys now include a fresh semantic-user-unresolved UUID, preventing reuse of results from another context.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of d43e6e3.

When execution-context resolution raises an exception, the cache keys now include a fresh semantic-user-unresolved UUID, preventing reuse of results from another context.

_{If that's not right, unresolve this thread and CodeAnt will leave it open.}

codeant-ai-for-open-source · 2026-10-30T17:32:28Z

CodeAnt PR Risk

🟢 Low Risk

The PR appears safe to merge; context-resolution failures use unique cache keys, preventing failed requests from sharing semantic results.

Assessed commit: d43e6e382c6b

codecov · 2026-10-30T17:40:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.76%. Comparing base (7519fa8) to head (d43e6e3).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #45895   +/-   ##
=======================================
  Coverage   84.76%   84.76%           
=======================================
  Files        3072     3072           
  Lines      206119   206129   +10     
  Branches    48539    48541    +2     
=======================================
+ Hits       174718   174732   +14     
+ Misses      28101    28099    -2     
+ Partials     3300     3298    -2

Flag	Coverage Δ
hive	`34.49% <0.00%> (-0.01%)`	⬇️
mysql	`52.59% <0.00%> (-0.01%)`	⬇️
postgres	`52.61% <0.00%> (-0.01%)`	⬇️
presto	`36.21% <0.00%> (-0.01%)`	⬇️
python	`87.77% <100.00%> (+<0.01%)`	⬆️
sqlite	`52.36% <0.00%> (-0.01%)`	⬇️
unit	`82.53% <100.00%> (+<0.01%)`	⬆️

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.

FrancescoCastaldi · 2026-10-30T18:38:38Z

/review

…Y_USER is enabled (apache#45877) Semantic views have no database attribute, which caused QueryObject.cache_key to swallow AttributeError and omit user isolation when CACHE_QUERY_BY_USER was enabled. Adding user isolation directly to SemanticView.get_extra_cache_keys via execution context identity digest ensures per-user cache partitioning for semantic layer chart data while preserving shared caching for embedded guest users. When context resolution encounters an unexpected failure, fall back to a distinct unresolved key per invocation and log a warning to ensure fail-safe isolation without cross-user cache leakage. Signed-off-by: Francesco Castaldi <info@francescocastaldi.it>

FrancescoCastaldi · 2026-10-31T08:54:00Z

PR Remediation & Coverage Hardening Update

Reviewer Thread Addressed (Thread 4426571604):
- Addressed review finding regarding error handling under CACHE_QUERY_BY_USER.
- When CACHE_QUERY_BY_USER is enabled and execution context resolution encounters an unexpected failure, SemanticView.get_extra_cache_keys() now logs a warning and appends a distinct uncacheable nonce ("semantic-user-unresolved", uuid.uuid4().hex).
- This ensures fail-safe user isolation so that context resolution failures never collapse into a shared cache key or leak semantic results across users.
Test Coverage & CI Remediation:
- Added unit test in tests/unit_tests/semantic_layers/models_test.py exercising the exception fallback branch and verifying that successive unresolved context resolutions produce unique cache keys.
- All 167 unit tests pass cleanly, closing the missing coverage on lines 920-924 in superset/semantic_layers/models.py and satisfying the strict 100% module coverage requirement (--cov-fail-under=100).
- Linting and formatting audited and verified with Ruff (<= 88 columns).
Commit & Push:
- Amended commit 3895a3a996 with DCO sign-off pushed to origin/fix/issue-45877-semantic-view-cache-query-by-user.

netlify · 2026-10-31T08:55:27Z

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	`d43e6e3`
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ae8453b83f59d79f6195673
😎 Deploy Preview	https://deploy-preview-45895--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.

EnxDev

Partitioning in get_extra_cache_keys with guests left shared reads right, and flag-off keys don't move.

One note inline about what the key is built from.

EnxDev · 2026-10-31T09:43:25Z

+                if not exec_context.is_guest and exec_context.identity_digest:
+                    keys.append(("semantic-user", exec_context.identity_digest))


identity_digest is an HMAC keyed with SECRET_KEY, so with the flag on a key rotation changes every user's cache key. That breaks what test_metadata_aliases_share_result_cache pins (a worker-cached result stays readable after SECRET_KEY changes), and mid-rotation web and workers keep missing each other's results.

Keying on principal_id avoids it. Heads up that #45900 fixes the same issue that way, so it's probably worth settling on one of the two.

Suggested change

if not exec_context.is_guest and exec_context.identity_digest:

keys.append(("semantic-user", exec_context.identity_digest))

if not exec_context.is_guest and exec_context.principal_id:

keys.append(("semantic-user", exec_context.principal_id))

Resolved in commit 5b2e270.

Updated the user cache partition key to use exec_context.principal_id rather than exec_context.identity_digest. As noted, keying on principal_id decouples the user cache partition from SECRET_KEY, ensuring that cache entries remain valid and shared across web/worker nodes during rolling key rotations and upholding the contract pinned in test_metadata_aliases_share_result_cache.

We also extended unit test coverage in tests/unit_tests/semantic_layers/models_test.py to assert exact principal_id keys (("semantic-user", "1")) and verify that rotating current_app.config["SECRET_KEY"] leaves the user's cache keys unchanged.

FrancescoCastaldi · 2026-10-31T23:34:57Z

Review Feedback Resolution: Stable User Partitioning via `principal_id`

In response to review feedback from @EnxDev (thread #4431282083), commit 5b2e270487 switches user cache key partitioning in SemanticView.get_extra_cache_keys() from exec_context.identity_digest to exec_context.principal_id:

Decoupled from SECRET_KEY: Keying on principal_id (the authenticated user ID string) ensures that rolling SECRET_KEY rotations do not bust every user's cache keys, preventing web and worker nodes from missing each other's cached query results mid-rotation.
Contract & Alias Preservation: Preserves the invariant tested by test_metadata_aliases_share_result_cache where cached results remain readable across configuration secret changes.
Fail-Safe & Guest Semantics Preserved: Unauthenticated/guest users continue to share cache keys without per-user partitioning, and context resolution failures still fail closed with distinct nonces ("semantic-user-unresolved").
Test Coverage: Added test assertions in tests/unit_tests/semantic_layers/models_test.py validating that dynamic rotation of current_app.config["SECRET_KEY"] leaves the user's cache keys identical and stable.

@EnxDev, the update is pushed and ready for your re-review whenever convenient. Thank you!

…e across secret rotation (apache#45877) Signed-off-by: FrancescoCastaldi <info@francescocastaldi.it>

EnxDev

Keying on principal_id takes care of the secret rotation problem, and the new check that rotating SECRET_KEY leaves the keys unchanged pins it. Ran the semantic_layers unit suite on 7272883 and it's green.

Two things outside the code. The description still says the key is built from identity_digest, so worth updating before merge. And #45900 is still open for the same issue, so one of the two should probably be closed once a maintainer picks.

…by-user

pull-request-size Bot added the size/M label Oct 30, 2026

FrancescoCastaldi mentioned this pull request Oct 30, 2026

Semantic views ignore CACHE_QUERY_BY_USER: chart-data results are shared across users #45877

Open

1 of 3 tasks

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

View reviewed changes

surawut-jirasaktavee mentioned this pull request Oct 30, 2026

fix(semantic-layers): honor CACHE_QUERY_BY_USER for semantic views #45900

Open

2 of 9 tasks

FrancescoCastaldi force-pushed the fix/issue-45877-semantic-view-cache-query-by-user branch from b463346 to 3895a3a Compare October 31, 2026 08:53

pull-request-size Bot added size/L and removed size/M labels Oct 31, 2026

EnxDev reviewed Oct 31, 2026

View reviewed changes

fix(semantic_layers): key user cache on principal_id to preserve cach…

7272883

…e across secret rotation (apache#45877) Signed-off-by: FrancescoCastaldi <info@francescocastaldi.it>

FrancescoCastaldi force-pushed the fix/issue-45877-semantic-view-cache-query-by-user branch from 5b2e270 to 7272883 Compare November 1, 2026 00:53

EnxDev reviewed Nov 1, 2026

View reviewed changes

Merge branch 'master' into fix/issue-45877-semantic-view-cache-query-…

d43e6e3

…by-user

		if not exec_context.is_guest and exec_context.identity_digest:
		keys.append(("semantic-user", exec_context.identity_digest))

Conversation

FrancescoCastaldi commented Oct 30, 2026

SUMMARY

Root Cause

Solution

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

FrancescoCastaldi Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodeAnt PR Risk

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

FrancescoCastaldi commented Oct 30, 2026

Uh oh!

FrancescoCastaldi commented Oct 31, 2026

PR Remediation & Coverage Hardening Update

Uh oh!

netlify Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for superset-docs-preview ready!

Uh oh!

EnxDev left a comment

Choose a reason for hiding this comment

Uh oh!

EnxDev Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

FrancescoCastaldi Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

FrancescoCastaldi commented Oct 31, 2026

Review Feedback Resolution: Stable User Partitioning via principal_id

Uh oh!

EnxDev left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codeant-ai-for-open-source Bot commented Oct 30, 2026 •

edited

Loading

codecov Bot commented Oct 30, 2026 •

edited

Loading

netlify Bot commented Oct 31, 2026 •

edited

Loading

Review Feedback Resolution: Stable User Partitioning via `principal_id`