[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10836: feat: Stop pending queries when user close dashboard

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

etr2460 commented on a change in pull request #10836:
URL: https://github.com/apache/incubator-superset/pull/10836#discussion_r488045020



##########
File path: superset/views/core.py
##########
@@ -1421,6 +1421,48 @@ def fave_slices(  # pylint: disable=no-self-use
             payload.append(dash)
         return json_success(json.dumps(payload, default=utils.json_int_dttm_ser))
 
+    @api
+    @has_access_api
+    @event_logger.log_this
+    @expose("/dashboard/<dashboard_id>/stop/", methods=["POST"])
+    def stop_dashboard_queries(  # pylint: disable=no-self-use
+        self, dashboard_id: int
+    ) -> FlaskResponse:
+        if is_feature_enabled("STOP_DASHBOARD_PENDING_QUERIES"):
+            username = g.user.username if g.user else None

Review comment:
       Since this is wrapped by `has_access_api`, does that guarantee that the user object already exists?

##########
File path: superset/config.py
##########
@@ -172,7 +172,11 @@ def _try_json_readsha(  # pylint: disable=unused-argument
 WTF_CSRF_ENABLED = True
 
 # Add endpoints that need to be exempt from CSRF protection
-WTF_CSRF_EXEMPT_LIST = ["superset.views.core.log", "superset.charts.api.data"]
+WTF_CSRF_EXEMPT_LIST = [
+    "superset.views.core.log",
+    "superset.charts.api.data",
+    "superset.views.core.stop_dashboard_queries",

Review comment:
       Is this dangerous? I assume this means that anyone who steals a session cookie could constantly kill all that user's queries?
   
   it's probably fine for internal deployments, but I wonder if there's a way we could do this without removing CSRF protections

##########
File path: superset/db_engine_specs/base.py
##########
@@ -206,6 +206,10 @@ def is_db_column_type_match(
     def get_allow_cost_estimate(cls, version: Optional[str] = None) -> bool:
         return False
 
+    @classmethod

Review comment:
       instead of making this a `get` function, why not call this `allows_stop_pending_queries` and put it up around line 150?
   
   Also, to be more explicit with the API, can you add the `stop_queries` function to this class too and make it pass?
   
   

##########
File path: superset/db_engine_specs/base.py
##########
@@ -206,6 +206,10 @@ def is_db_column_type_match(
     def get_allow_cost_estimate(cls, version: Optional[str] = None) -> bool:
         return False
 
+    @classmethod

Review comment:
       Alternatively, if `stop_queries` was an empty function, you might be able to remove the `allows_stop_pending_queries` attribute altogether. I think the only reason we might need the database level allows attribute would be if you wanted to prevent the frontend from sending the request to stop queries. But that could be messy as dashboards can be based off multiple different databases.
   
   So maybe remove the `allows` check completely and just make `stop_queries` an empty function here




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

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