[GitHub] [superset] villebro commented on a change in pull request #18576: feat: Improve state key generation for dashboards and charts

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

villebro commented on a change in pull request #18576:
URL: https://github.com/apache/superset/pull/18576#discussion_r801054411



##########
File path: superset/dashboards/filter_state/commands/create.py
##########
@@ -23,13 +25,17 @@
 
 
 class CreateFilterStateCommand(CreateKeyValueCommand):
-    def create(self, cmd_params: CommandParameters) -> bool:
+    def create(self, cmd_params: CommandParameters) -> str:
         resource_id = cmd_params.resource_id
         actor = cmd_params.actor
-        key = cache_key(resource_id, cmd_params.key)
+        contextual_key = cache_key(actor.get_user_id(), resource_id)
+        key = cache_manager.filter_state_cache.get(contextual_key)
+        if not key:
+            key = token_urlsafe(48)

Review comment:
       Not introduced here, but noticed this only now.. Since we're using the same `token_urlsafe(48)` both in dashboard and explore form data commands, could we DRY this by creating a util under the `key_value` package that hides both the token generator and the magic number? Something like
   
   ```python
   def get_key() -> str:
       return token_urlsafe(48)
   ```

##########
File path: superset/dashboards/filter_state/commands/create.py
##########
@@ -23,13 +25,17 @@
 
 
 class CreateFilterStateCommand(CreateKeyValueCommand):
-    def create(self, cmd_params: CommandParameters) -> bool:
+    def create(self, cmd_params: CommandParameters) -> str:
         resource_id = cmd_params.resource_id
         actor = cmd_params.actor
-        key = cache_key(resource_id, cmd_params.key)
+        contextual_key = cache_key(actor.get_user_id(), resource_id)
+        key = cache_manager.filter_state_cache.get(contextual_key)
+        if not key:
+            key = token_urlsafe(48)

Review comment:
       This part I find problematic. I get the idea that we want to avoid creating new keys every time we load the same resource with the same key. But recycling the same key every time the same user navigates to the same resource is IMO slightly risky. I'm sure it's a fairly common use case to be exploring the same resource in multiple different tabs. Making them all share the same key-value pair will make it impossible to do this type of exploration without leaking state between the tabs. I tried to record a video to demonstrate what I mean, but here's the repro steps:
   1. Open Superset in two tabs
   2. Navigate the first tab to your favourite dashboard and set a few native filters
   3. Navigate the second tab to the same dashboard
   4. refresh the first tab and notice that the native filter state has been lost
   




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