fix(explore): Fix chart standalone URL for report/thumbnail generation (

apache#20673) * Update explore URLs. * More URL fixes. * Make frontend accept true/false query params case-insensitively. * Fix URL mistake. Co-authored-by: Michael S. Molina <[email protected]> Co-authored-by: Michael S. Molina <[email protected]> (cherry picked from commit 84d4302)
preset-io · Jul 19, 2022 · 297c176 · 297c176
1 parent 1101922
commit 297c176
Show file tree

Hide file tree

Showing 16 changed files with 63 additions and 57 deletions.
diff --git a/docs/docs/installation/sql-templating.mdx b/docs/docs/installation/sql-templating.mdx
@@ -298,7 +298,7 @@ Here's a concrete example:
 
 It's possible to query physical and virtual datasets using the `dataset` macro. This is useful if you've defined computed columns and metrics on your datasets, and want to reuse the definition in adhoc SQL Lab queries.
 
-To use the macro, first you need to find the ID of the dataset. This can be done by going to the view showing all the datasets, hovering over the dataset you're interested in, and looking at its URL. For example, if the URL for a dataset is https://superset.example.org/superset/explore/table/42/ its ID is 42.
+To use the macro, first you need to find the ID of the dataset. This can be done by going to the view showing all the datasets, hovering over the dataset you're interested in, and looking at its URL. For example, if the URL for a dataset is https://superset.example.org/explore/?dataset_type=table&dataset_id=42 its ID is 42.
 
 Once you have the ID you can query it as if it were a table:
 

diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils/explore.js b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils/explore.js
@@ -23,7 +23,7 @@ const MAX_URL_LENGTH = 8000;
 
 export function getURIDirectory(formData, endpointType = 'base') {
   // Building the directory part of the URI
-  let directory = '/superset/explore/';
+  let directory = '/explore/';
   if (['json', 'csv', 'query', 'results', 'samples'].includes(endpointType)) {
     directory = '/superset/explore_json/';
   }

diff --git a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/vendor/superset/exploreUtils.js b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/vendor/superset/exploreUtils.js
@@ -24,7 +24,7 @@ const MAX_URL_LENGTH = 8000;
 
 export function getURIDirectory(formData, endpointType = 'base') {
   // Building the directory part of the URI
-  let directory = '/superset/explore/';
+  let directory = '/explore/';
   if (['json', 'csv', 'query', 'results', 'samples'].includes(endpointType)) {
     directory = '/superset/explore_json/';
   }

diff --git a/superset-frontend/spec/fixtures/mockSliceEntities.js b/superset-frontend/spec/fixtures/mockSliceEntities.js
@@ -26,7 +26,7 @@ export const sliceEntitiesForChart = {
   slices: {
     [sliceId]: {
       slice_id: sliceId,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%2018%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%2018%7D',
       slice_name: 'Genders',
       form_data: {
         slice_id: sliceId,
@@ -62,7 +62,7 @@ export const sliceEntitiesForDashboard = {
   slices: {
     [filterId]: {
       slice_id: filterId,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20127%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20127%7D',
       slice_name: 'Region Filter',
       form_data: {
         instant_filtering: true,
@@ -86,7 +86,7 @@ export const sliceEntitiesForDashboard = {
     },
     128: {
       slice_id: 128,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20128%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20128%7D',
       slice_name: "World's Population",
       form_data: {},
       viz_type: 'big_number',
@@ -98,7 +98,7 @@ export const sliceEntitiesForDashboard = {
     },
     129: {
       slice_id: 129,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20129%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20129%7D',
       slice_name: 'Most Populated Countries',
       form_data: {},
       viz_type: 'table',
@@ -110,7 +110,7 @@ export const sliceEntitiesForDashboard = {
     },
     130: {
       slice_id: 130,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20130%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20130%7D',
       slice_name: 'Growth Rate',
       form_data: {},
       viz_type: 'line',
@@ -122,7 +122,7 @@ export const sliceEntitiesForDashboard = {
     },
     131: {
       slice_id: 131,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20131%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20131%7D',
       slice_name: '% Rural',
       form_data: {},
       viz_type: 'world_map',
@@ -134,7 +134,7 @@ export const sliceEntitiesForDashboard = {
     },
     132: {
       slice_id: 132,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20132%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20132%7D',
       slice_name: 'Life Expectancy VS Rural %',
       form_data: {},
       viz_type: 'bubble',
@@ -146,7 +146,7 @@ export const sliceEntitiesForDashboard = {
     },
     133: {
       slice_id: 133,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20133%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20133%7D',
       slice_name: 'Rural Breakdown',
       form_data: {},
       viz_type: 'sunburst',
@@ -158,7 +158,7 @@ export const sliceEntitiesForDashboard = {
     },
     134: {
       slice_id: 134,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20134%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20134%7D',
       slice_name: "World's Pop Growth",
       form_data: {},
       viz_type: 'area',
@@ -170,7 +170,7 @@ export const sliceEntitiesForDashboard = {
     },
     135: {
       slice_id: 135,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20135%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20135%7D',
       slice_name: 'Box plot',
       form_data: {},
       viz_type: 'box_plot',
@@ -182,7 +182,7 @@ export const sliceEntitiesForDashboard = {
     },
     136: {
       slice_id: 136,
-      slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20136%7D',
+      slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20136%7D',
       slice_name: 'Treemap',
       form_data: {},
       viz_type: 'treemap',

diff --git a/superset-frontend/src/utils/urlUtils.ts b/superset-frontend/src/utils/urlUtils.ts
@@ -52,10 +52,10 @@ export function getUrlParam({ name, type }: UrlParam): unknown {
       if (!urlParam) {
         return null;
       }
-      if (urlParam === 'true') {
+      if (urlParam.toLowerCase() === 'true') {
         return 1;
       }
-      if (urlParam === 'false') {
+      if (urlParam.toLowerCase() === 'false') {
         return 0;
       }
       if (!Number.isNaN(Number(urlParam))) {
@@ -71,7 +71,7 @@ export function getUrlParam({ name, type }: UrlParam): unknown {
       if (!urlParam) {
         return null;
       }
-      return urlParam !== 'false' && urlParam !== '0';
+      return urlParam.toLowerCase() !== 'false' && urlParam !== '0';
     case 'rison':
       if (!urlParam) {
         return null;

diff --git a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.test.jsx b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.test.jsx
@@ -51,7 +51,7 @@ const mockdatasets = [...new Array(3)].map((_, i) => ({
   changed_by: 'user',
   changed_on: new Date().toISOString(),
   database_name: `db ${i}`,
-  explore_url: `/explore/table/${i}`,
+  explore_url: `/explore/?dataset_type=table&dataset_id=${i}`,
   id: i,
   schema: `schema ${i}`,
   table_name: `coolest table ${i}`,

diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py
@@ -535,7 +535,7 @@ def edit(self, pk: str) -> FlaskResponse:
         resp = super().edit(pk)
         if isinstance(resp, str):
             return resp
-        return redirect("/superset/explore/table/{}/".format(pk))
+        return redirect("/explore/?dataset_type=table&dataset_id={}".format(pk))
 
     @expose("/list/")
     @has_access

diff --git a/superset/models/core.py b/superset/models/core.py
@@ -386,7 +386,7 @@ def get_sqla_engine(
             if not source and request and request.referrer:
                 if "/superset/dashboard/" in request.referrer:
                     source = utils.QuerySource.DASHBOARD
-                elif "/superset/explore/" in request.referrer:
+                elif "/explore/" in request.referrer:
                     source = utils.QuerySource.CHART
                 elif "/superset/sqllab/" in request.referrer:
                     source = utils.QuerySource.SQL_LAB

diff --git a/superset/reports/commands/execute.py b/superset/reports/commands/execute.py
@@ -167,7 +167,7 @@ def _get_url(
                     force=force,
                 )
             return get_url_path(
-                "Superset.explore",
+                "ExploreView.root",
                 user_friendly=user_friendly,
                 form_data=json.dumps({"slice_id": self._report_schedule.chart_id}),
                 standalone="true",

diff --git a/superset/templates/email/role_extended.txt b/superset/templates/email/role_extended.txt
@@ -20,7 +20,7 @@ Dear {{ user.username }},
 <br>
 <a href={{ url_for('Superset.profile', username=granter.username, _external=True) }}>
    {{ granter.username }}</a> has extended the role {{ role.name }} to include
-<a href={{ url_for('Superset.explore', datasource_type=datasource.type, datasource_id=datasource.id, _external=True) }}>
+<a href={{ url_for('ExploreView.root', datasource_type=datasource.type, datasource_id=datasource.id, _external=True) }}>
  {{datasource.full_name}}</a> and granted you access to it.
 <br>
 <br>

diff --git a/superset/templates/email/role_granted.txt b/superset/templates/email/role_granted.txt
@@ -21,7 +21,7 @@ Dear {{ user.username }},
 <a href={{ url_for('Superset.profile', username=granter.username, _external=True) }}>
    {{ granter.username }}</a> has granted you the role {{ role.name }}
    that gives access to the
-   <a href={{ url_for('Superset.explore', datasource_type=datasource.type, datasource_id=datasource.id, _external=True) }}>
+   <a href={{ url_for('ExploreView.root', datasource_type=datasource.type, datasource_id=datasource.id, _external=True) }}>
       {{datasource.full_name}}</a>
 <br>
 <br>

diff --git a/superset/views/core.py b/superset/views/core.py
@@ -427,13 +427,13 @@ def slice(self, slice_id: int) -> FlaskResponse:  # pylint: disable=no-self-use
         _, slc = get_form_data(slice_id, use_slice_data=True)
         if not slc:
             abort(404)
-        endpoint = "/superset/explore/?form_data={}".format(
+        endpoint = "/explore/?form_data={}".format(
             parse.quote(json.dumps({"slice_id": slice_id}))
         )
 
         is_standalone_mode = ReservedUrlParameters.is_standalone_mode()
         if is_standalone_mode:
-            endpoint += f"&{ReservedUrlParameters.STANDALONE}={is_standalone_mode}"
+            endpoint += f"&{ReservedUrlParameters.STANDALONE}=true"
         return redirect(endpoint)
 
     def get_query_string_response(self, viz_obj: BaseViz) -> FlaskResponse:

diff --git a/superset/views/redirects.py b/superset/views/redirects.py
@@ -34,25 +34,40 @@ class R(BaseSupersetView):  # pylint: disable=invalid-name
     """used for short urls"""
 
     @staticmethod
-    def _validate_url(url: Optional[str] = None) -> bool:
-        if url and (
-            url.startswith("//superset/dashboard/")
-            or url.startswith("//superset/explore/")
-        ):
-            return True
-        return False
+    def _validate_explore_url(url: str) -> Optional[str]:
+        if url.startswith("//superset/explore/p/"):
+            return url
+
+        if url.startswith("//superset/explore"):
+            return "/" + url[10:]  # Remove /superset from old Explore URLs
+
+        if url.startswith("//explore"):
+            return url
+
+        return None
+
+    @staticmethod
+    def _validate_dashboard_url(url: str) -> Optional[str]:
+        if url.startswith("//superset/dashboard/"):
+            return url
+
+        return None
 
     @event_logger.log_this
     @expose("/<int:url_id>")
     def index(self, url_id: int) -> FlaskResponse:
         url = db.session.query(models.Url).get(url_id)
         if url and url.url:
-            explore_url = "//superset/explore/?"
-            if url.url.startswith(explore_url):
-                explore_url += f"r={url_id}"
+            explore_url = self._validate_explore_url(url.url)
+            if explore_url:
+                if explore_url.startswith("//explore/?"):
+                    explore_url = f"//explore/?r={url_id}"
                 return redirect(explore_url[1:])
-            if self._validate_url(url.url):
-                return redirect(url.url[1:])
+
+            dashboard_url = self._validate_dashboard_url(url.url)
+            if dashboard_url:
+                return redirect(dashboard_url[1:])
+
             return redirect("/")
 
         flash("URL to nowhere...", "danger")

diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py
@@ -123,15 +123,6 @@ def test_dashboard_endpoint(self):
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_slice_endpoint(self):
         self.login(username="admin")
-        slc = self.get_slice("Girls", db.session)
-        resp = self.get_resp("/superset/slice/{}/".format(slc.id))
-        assert "Original value" in resp
-        assert "List Roles" in resp
-
-        # Testing overrides
-        resp = self.get_resp("/superset/slice/{}/?standalone=true".format(slc.id))
-        assert '<div class="navbar' not in resp
-
         resp = self.client.get("/superset/slice/-1/")
         assert resp.status_code == 404
 
@@ -881,7 +872,7 @@ def test_user_activity_access(self, username="gamma"):
 
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_slice_id_is_always_logged_correctly_on_web_request(self):
-        # superset/explore case
+        # explore case
         self.login("admin")
         slc = db.session.query(Slice).filter_by(slice_name="Girls").one()
         qry = db.session.query(models.Log).filter_by(slice_id=slc.id)
@@ -1424,7 +1415,7 @@ def test_feature_flag_serialization(self):
             "/superset/welcome",
             f"/superset/dashboard/{dash_id}/",
             "/superset/profile/admin/",
-            f"/superset/explore/table/{tbl_id}",
+            f"/explore/?dataset_type=table&dataset_id={tbl_id}",
         ]
         for url in urls:
             data = self.get_resp(url)
@@ -1566,7 +1557,7 @@ def test_explore_injected_exceptions(self, mock_db_connection_mutator):
         exception = SupersetException("Error message")
         mock_db_connection_mutator.side_effect = exception
         slice = db.session.query(Slice).first()
-        url = f"/superset/explore/?form_data=%7B%22slice_id%22%3A%20{slice.id}%7D"
+        url = f"/explore/?form_data=%7B%22slice_id%22%3A%20{slice.id}%7D"
 
         self.login()
         data = self.get_resp(url)
@@ -1576,7 +1567,7 @@ def test_explore_injected_exceptions(self, mock_db_connection_mutator):
         exception = SQLAlchemyError("Error message")
         mock_db_connection_mutator.side_effect = exception
         slice = db.session.query(Slice).first()
-        url = f"/superset/explore/?form_data=%7B%22slice_id%22%3A%20{slice.id}%7D"
+        url = f"/explore/?form_data=%7B%22slice_id%22%3A%20{slice.id}%7D"
 
         self.login()
         data = self.get_resp(url)

diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py
@@ -724,7 +724,7 @@ def test_email_chart_report_schedule(
         )
         # assert that the link sent is correct
         assert (
-            '<a href="http://0.0.0.0:8080/superset/explore/?'
+            '<a href="http://0.0.0.0:8080/explore/?'
             "form_data=%7B%22slice_id%22%3A%20"
             f"{create_report_email_chart.chart.id}%7D&"
             'standalone=0&force=false">Explore in Superset</a>'
@@ -769,7 +769,7 @@ def test_email_chart_report_schedule_force_screenshot(
         )
         # assert that the link sent is correct
         assert (
-            '<a href="http://0.0.0.0:8080/superset/explore/?'
+            '<a href="http://0.0.0.0:8080/explore/?'
             "form_data=%7B%22slice_id%22%3A%20"
             f"{create_report_email_chart_force_screenshot.chart.id}%7D&"
             'standalone=0&force=true">Explore in Superset</a>'
@@ -808,7 +808,7 @@ def test_email_chart_alert_schedule(
         notification_targets = get_target_from_report_schedule(create_alert_email_chart)
         # assert that the link sent is correct
         assert (
-            '<a href="http://0.0.0.0:8080/superset/explore/?'
+            '<a href="http://0.0.0.0:8080/explore/?'
             "form_data=%7B%22slice_id%22%3A%20"
             f"{create_alert_email_chart.chart.id}%7D&"
             'standalone=0&force=true">Explore in Superset</a>'
@@ -882,7 +882,7 @@ def test_email_chart_report_schedule_with_csv(
         )
         # assert that the link sent is correct
         assert (
-            '<a href="http://0.0.0.0:8080/superset/explore/?'
+            '<a href="http://0.0.0.0:8080/explore/?'
             "form_data=%7B%22slice_id%22%3A%20"
             f"{create_report_email_chart_with_csv.chart.id}%7D&"
             'standalone=0&force=false">Explore in Superset</a>'
@@ -1260,7 +1260,7 @@ def test_slack_chart_report_schedule_with_text(
 |  1 | c21  | c22  | c23       |"""
         assert table_markdown in post_message_mock.call_args[1]["text"]
         assert (
-            f"<http://0.0.0.0:8080/superset/explore/?form_data=%7B%22slice_id%22%3A%20{create_report_slack_chart_with_text.chart.id}%7D&standalone=0&force=false|Explore in Superset>"
+            f"<http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A%20{create_report_slack_chart_with_text.chart.id}%7D&standalone=0&force=false|Explore in Superset>"
             in post_message_mock.call_args[1]["text"]
         )
 

diff --git a/tests/unit_tests/utils/urls_tests.py b/tests/unit_tests/utils/urls_tests.py
@@ -17,7 +17,7 @@
 # under the License.
 from superset.utils.urls import modify_url_query
 
-EXPLORE_CHART_LINK = "http://localhost:9000/superset/explore/?form_data=%7B%22slice_id%22%3A+76%7D&standalone=true&force=false"
+EXPLORE_CHART_LINK = "http://localhost:9000/explore/?form_data=%7B%22slice_id%22%3A+76%7D&standalone=true&force=false"
 
 EXPLORE_DASHBOARD_LINK = "http://localhost:9000/superset/dashboard/3/?standalone=3"
 
@@ -26,7 +26,7 @@ def test_convert_chart_link() -> None:
     test_url = modify_url_query(EXPLORE_CHART_LINK, standalone="0")
     assert (
         test_url
-        == "http://localhost:9000/superset/explore/?form_data=%7B%22slice_id%22%3A%2076%7D&standalone=0&force=false"
+        == "http://localhost:9000/explore/?form_data=%7B%22slice_id%22%3A%2076%7D&standalone=0&force=false"
     )