[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9650: feat: add a single endpoint that visualizes a new table. Enables external superset integration.

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

john-bodley commented on a change in pull request #9650:
URL: https://github.com/apache/incubator-superset/pull/9650#discussion_r441954225



##########
File path: superset/security/manager.py
##########
@@ -273,15 +273,15 @@ def can_access_datasource(self, datasource: "BaseDatasource") -> bool:
             "datasource_access", datasource.perm or ""
         )
 
-    def get_datasource_access_error_msg(self, datasource: "BaseDatasource") -> str:
+    def get_datasource_access_error_msg(self, datasource_name: Optional[str]) -> str:

Review comment:
       I'm not sure about this. The "datasource" term in `get_datasource_access_error_msg` is in reference to a Superset datasource and thus I'm not sure whether we should be passing in SQL table names which may exist with the virtual Superset datasource. Additionally the returned message _only_ relates to Superset datasources as opposed to SQL tables.
   
   It seems that either this method (and the return statement) should be renamed or a more generic function be added.
   

##########
File path: superset/views/core.py
##########
@@ -565,7 +565,53 @@ def import_dashboards(self) -> FlaskResponse:
 
     @event_logger.log_this
     @has_access
-    @expose("/explore/<datasource_type>/<int:datasource_id>/", methods=["GET", "POST"])
+    @expose(
+        "/explore_new/<database_id>/<datasource_type>/<datasource_name>/",
+        methods=["GET", "POST"],
+    )
+    def explore_new(
+        self,
+        database_id: Optional[int] = None,
+        datasource_type: Optional[str] = None,
+        datasource_name: Optional[str] = None,
+    ) -> FlaskResponse:
+        """Integration endpoint. Allows to visualize tables that were not precreated in superset.
+
+        :param database_id: database id
+        :param datasource_type: table or druid
+        :param datasource_name: full name of the datasource, should include schema name if applicable
+        :return: redirects to the exploration page
+        """
+        if database_id is None:
+            flash(__("Database id parameter needs to be specified"), "danger")
+            return redirect("/")
+        if datasource_type != "table":
+            flash(__("Only table datasource type is supported"), "danger")
+            return redirect("/")
+        if not datasource_name:
+            flash(__("Datasource name was not specified"), "danger")
+            return redirect("/")
+
+        schema_name, table_name = parse_table_full_name(datasource_name)
+        database_obj = db.session.query(models.Database).get(database_id)
+        table = Table(table_name, schema_name)
+
+        if not security_manager.can_access_table(database_obj, table):
+            flash(
+                __(security_manager.get_datasource_access_error_msg(table)), "danger",
+            )
+            return redirect("/")
+
+        # overloading is_sqllab_view to be able to hide the temporary tables from the table list.
+        is_sqllab_view = request.args.get("is_sqllab_view") == "true"
+        table_id = create_if_not_exists_table(
+            database_id, schema_name, table_name, is_sqllab_view=is_sqllab_view
+        )
+        return redirect(f"/superset/explore/{datasource_type}/{table_id}")
+
+    @event_logger.log_this
+    @has_access
+    @expose("/explore/<datasource_type>/<datasource_id>/", methods=["GET", "POST"])

Review comment:
       See note above regarding the URL converter. 

##########
File path: superset/views/core.py
##########
@@ -565,7 +565,53 @@ def import_dashboards(self) -> FlaskResponse:
 
     @event_logger.log_this
     @has_access
-    @expose("/explore/<datasource_type>/<int:datasource_id>/", methods=["GET", "POST"])
+    @expose(
+        "/explore_new/<database_id>/<datasource_type>/<datasource_name>/",
+        methods=["GET", "POST"],
+    )
+    def explore_new(
+        self,
+        database_id: Optional[int] = None,
+        datasource_type: Optional[str] = None,
+        datasource_name: Optional[str] = None,
+    ) -> FlaskResponse:
+        """Integration endpoint. Allows to visualize tables that were not precreated in superset.

Review comment:
       Nit. `superset` -> `Superset`.

##########
File path: superset/views/core.py
##########
@@ -565,7 +565,53 @@ def import_dashboards(self) -> FlaskResponse:
 
     @event_logger.log_this
     @has_access
-    @expose("/explore/<datasource_type>/<int:datasource_id>/", methods=["GET", "POST"])
+    @expose(
+        "/explore_new/<database_id>/<datasource_type>/<datasource_name>/",

Review comment:
       Could you add URL converters for the `database_id` field? Additionally it seems from the URL endpoint `database_id`, `datasource_type`, nor `datasource_name` can be `None` and thus thus `Optional` logic could be removed. 




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