surawut-jirasaktavee opened a new pull request, #45900:
URL: https://github.com/apache/superset/pull/45900
### SUMMARY
With `CACHE_QUERY_BY_USER` on, chart-data results for semantic views were
still cached under one key for all users. `QueryObject.cache_key` adds the
per-user key through
`add_impersonation_cache_key_if_needed(self.datasource.database, ...)`. A
semantic view has no `database`, so the `AttributeError` is swallowed and no
key is added. `SemanticView.get_extra_cache_keys` added nothing per user
either. A provider whose results depend on the user (for example, the upstream
engine applies per-user row filtering) could therefore return one user's cached
result to another. The result cache is read before the semantic containment
cache, so the containment cache's `EXECUTION_CONTEXT` scope does not help.
This change makes `SemanticView.get_extra_cache_keys` append
`("semantic-user", principal_id)` when `CACHE_QUERY_BY_USER` is on.
`principal_id` is the server-authenticated user ID from
`current_host_execution_context()`, the same per-user material datasets get
from their username. It only appears inside the hashed cache key.
Design decisions:
- **Opt-in, no invalidation.** With the flag off, the key material is
unchanged. The recorded golden chart key in `execution_context_test.py` still
matches.
- **Principal ID, not `identity_digest`.**
[`semantic-layer-cache-clear.md`](https://github.com/apache/superset/blob/master/docs/developer_docs/semantic-layer-cache-clear.md)
says `SECRET_KEY` must not affect semantic cache identity, and
`test_metadata_aliases_share_result_cache` checks that a worker-cached result
is still found after `SECRET_KEY` changes. The digest is an HMAC keyed with
`SECRET_KEY`. With it, that test fails with the flag on (HTTP 202 instead of
200), and during a rolling `SECRET_KEY` rotation web and worker processes would
compute different keys, so async chart queries would keep missing worker
results. The principal ID is stable across rotations and processes.
- **Guests are not partitioned.** A guest principal does not tell guest
tokens apart (see the semantic-layer "Execution context" docs). Semantic views
refuse guest row rules, and connection mode runs every guest under the same
upstream account, so guests keep sharing results. Their key still differs from
every logged-in user's key and from anonymous users' key.
- **Only `CACHE_QUERY_BY_USER`, not `CACHE_IMPERSONATION`.** For datasets,
`CACHE_IMPERSONATION` applies only when the database has `impersonate_user`
enabled. Semantic views have no equivalent setting, so treating that flag as
unconditional would change what it means. `CACHE_QUERY_BY_USER` is the
operator's general per-user caching switch.
- **Placement.** The fix goes in `SemanticView.get_extra_cache_keys`, not in
a semantic-view branch in `QueryObject.cache_key`. That keeps the
semantic-layer identity logic in the semantic-layer model, next to the metadata
generation and producer-version keys already there.
- **No SDK contract change.** Providers, `SemanticCredentials` and execution
methods are untouched.
**Relation to #45895.** #45895 targets the same issue with `identity_digest`
and catches any exception from the context lookup. This PR differs in two ways:
the key does not depend on `SECRET_KEY` (see above), and a failed lookup raises
instead of silently dropping the user partition, so it fails closed rather than
sharing results across users.
**Alternative considered: `identity_digest`.** The SDK's opaque partition
token (HMAC over installation namespace, principal, effective role IDs and
guest flag) would also separate users, and would additionally split a user's
entries when their roles change. Its cost is the `SECRET_KEY` dependency above:
every rotation resets all per-user semantic result caches, web and worker keys
disagree during a rolling rotation, and the existing alias test would need
changing. Happy to switch if maintainers prefer it.
**Out of scope: column-values cache.** `_column_values_cache_key` in
`superset/datasource/api.py` (filter dropdown values, also used by MCP filter
validation) does not honor `CACHE_QUERY_BY_USER`, `CACHE_IMPERSONATION` or
`per_user_caching` for datasets or semantic views. That gap predates this PR
and is tracked in #45898; I'll fix it in a separate PR covering both.
Docs: `docs/developer_docs/semantic-layer-cache-clear.md` gains a paragraph
describing the per-user result key.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend only).
Before: users A and B running the same semantic-view chart query get the
same `cache_key`, and B gets A's rows with `is_cached: true`.
After (flag on): A and B get different `cache_key`s, and each gets their own
result.
### TESTING INSTRUCTIONS
Unit tests:
```bash
pytest tests/unit_tests/semantic_layers/execution_context_test.py
tests/unit_tests/semantic_layers/models_test.py
tests/unit_tests/queries/query_object_test.py
```
With the flag forced on for the whole run
(`SUPERSET_FEATURE_CACHE_QUERY_BY_USER=true`), the only new failures are tests
asserting exact semantic cache keys or key lists, which now include the user
entry; the `SECRET_KEY` alias tests pass.
New tests in `tests/unit_tests/semantic_layers/execution_context_test.py`:
- `test_chart_keys_honor_cache_query_by_user`: with the flag on, two users
get different chart-data keys and the same user gets a stable key. With the
flag off, both users get the recorded pre-change key.
- `test_user_chart_keys_survive_secret_rotation_and_background_tasks`: with
the flag on, a user's key is the same in a web request and a background task,
and after `SECRET_KEY` changes.
- `test_chart_keys_do_not_partition_guests`: the flag does not change a
guest's key, and a logged-in user's and an anonymous user's keys differ from it.
Manual check:
1. Enable `SEMANTIC_LAYERS` and `CACHE_QUERY_BY_USER`, and configure a cache
(for example, Redis).
2. Add a semantic layer whose results depend on the querying user.
3. Run the same chart query as two different users. The `cache_key` values
in the responses differ, and the second user's result is not served from the
first user's cache.
4. Turn `CACHE_QUERY_BY_USER` off: both users get the same `cache_key` as
before the change.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #45877
- [x] Required feature flags: `CACHE_QUERY_BY_USER` (behavior only changes
when it is on)
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
