[GitHub] [superset] villebro commented on a diff in pull request #20114: feat(chart): Enable caching per user when user impersonation is enabled

[GitBox <gi...@apache.org>]

villebro commented on code in PR #20114:
URL: https://github.com/apache/superset/pull/20114#discussion_r877789734


##########
superset/connectors/sqla/models.py:
##########
@@ -2026,6 +2027,15 @@ class and any keys added via `ExtraCache`.
         if self.has_extra_cache_key_calls(query_obj):
             sqla_query = self.get_sqla_query(**query_obj)
             extra_cache_keys += sqla_query.extra_cache_keys
+
+        sqlalchemy_url = make_url_safe(self.database.sqlalchemy_uri_decrypted)
+
+        if effective_user := self.database.get_effective_user(sqlalchemy_url):
+            logger.debug("User impersonation is enabled for database '%s', adding "
+                         "current username to '%s' datasource's extra cache keys",
+                         self.database_name, self.datasource_name)
+            extra_cache_keys.append(effective_user)

Review Comment:
   Instead of appending the effective user to `extra_cache_keys`, I'd almost prefer to have a dedicated key in the `cache_key_dict` for this that gets populated if `self.database.impersonate_user` is `True`. Also, to make it possible to customize this behavior per database type, id add functionality to `BaseEngineSpec` to make it possible to override the behavior. So something similar to this:
   
   1. add a `get_impersonation_key(user: Optional[User]): Any` method to `BaseEngineSpec` that defaults to returning the username, but can be overridden where applicable. We may also want to add other objects to the signature, e.g. `Database`, but we could do that later.
   2. In `QueryObject.cache_key()`, we would check if `self.datasource.database.impersonate_user` is truthy, and if so, we call `self.datasource.database.db_engine_spec.get_impersonation_key(user)` and add the value to `cache_dict["impersonation_key"]`
   
   Thoughts?



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org