gabotorresruiz commented on code in PR #45972:
URL: https://github.com/apache/superset/pull/45972#discussion_r4460722339
##########
superset/datasource/api.py:
##########
@@ -98,6 +100,19 @@ def _column_values_cache_key(
discriminator: tuple[str, str] | None =
datasource.result_cache_discriminator
if discriminator is not None:
material["semantic_result_version"] = discriminator
+ # Semantic views have no database, so the per-user key below never
+ # applies to them; partition by the server principal instead, the
+ # same way the chart result cache does.
+ if feature_flag_manager.is_feature_enabled("CACHE_QUERY_BY_USER"):
+ context = current_host_execution_context()
+ if not context.is_guest:
+ material["semantic_user"] = context.principal_id
+ elif isinstance(datasource, (SqlaTable, Query)) and datasource.database:
+ # An engine that filters rows per user returns different values per
+ # user, so the entry must be partitioned per user when per-user
+ # caching is enabled; without it every user with the same RLS
+ # shares one entry (#45898).
+ add_impersonation_cache_key_if_needed(datasource.database, material)
Review Comment:
Not a blocker, but worth closing here since it is one line.
`get_impersonation_key` does `user.username`
(`superset/db_engine_specs/base.py:3171`), and an anonymous request carries a
bare `AnonymousUserMixin` with no `username`, so with any of the per-user
settings on this call now raises `AttributeError` where master returned the
shared key. The chart-data path only survives because `query_object.py` wraps
the same call in `except AttributeError`. It needs a Public role that holds
`can_get_column_values` (Alpha-like), so it is narrow, but for those
deployments `@safe` turns every Explore filter popover request from an
anonymous user into a 500. I reproduced it with
`override_user(AnonymousUserMixin())` under `CACHE_QUERY_BY_USER` and under the
`per_user_caching` extra.
Since `get_username()` already returns `None` for anonymous and
`get_effective_user` then runs them as the connection account, one shared entry
is the right outcome, so I would fix it at the source instead of adding another
`try/except`:
```python
# superset/db_engine_specs/base.py, get_impersonation_key
return getattr(user, "username", None) if user else None
```
That also hardens the SQL Lab executor, which calls the same helper. A
`test_anonymous_user_shares_column_values_cache_key_under_per_user_flag`
(`override_user(AnonymousUserMixin())`, flag on, key equals the flag-off key)
would lock it in; it raises today and passes with that change.
##########
superset/datasource/api.py:
##########
@@ -98,6 +100,19 @@ def _column_values_cache_key(
discriminator: tuple[str, str] | None =
datasource.result_cache_discriminator
if discriminator is not None:
material["semantic_result_version"] = discriminator
+ # Semantic views have no database, so the per-user key below never
+ # applies to them; partition by the server principal instead, the
+ # same way the chart result cache does.
+ if feature_flag_manager.is_feature_enabled("CACHE_QUERY_BY_USER"):
+ context = current_host_execution_context()
+ if not context.is_guest:
+ material["semantic_user"] = context.principal_id
Review Comment:
On your open question: I would keep guests shared here. Semantic execution
is connection mode only (`resolve_method` always returns `CONNECTION`), guest
row rules on semantic views are refused inside `get_rls_cache_key`, and nothing
from the guest token reaches the provider, so there is no per-guest result to
separate. On the dataset side this PR already keys guests by the token's
`username` claim through `get_impersonation_key`, which is the same identity
`get_effective_user` impersonates with, so that path is consistent too.
Not a blocker: #45900 adds this exact rule as
`SemanticView._user_cache_partition()` for the chart cache. The two keys never
read each other, so they compose without double partitioning, but whichever
lands second could call the other's helper so the guest decision cannot drift
between the two caches.
--
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]
