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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
