From 84d4302628d18aa19c13cc5322e68abbc690ea4d Mon Sep 17 00:00:00 2001 From: Cody Leff Date: Tue, 19 Jul 2022 12:53:55 -0400 Subject: [PATCH] fix(explore): Fix chart standalone URL for report/thumbnail generation (#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 <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> --- docs/docs/installation/sql-templating.mdx | 2 +- .../src/utils/explore.js | 2 +- .../src/vendor/superset/exploreUtils.js | 2 +- .../spec/fixtures/mockSliceEntities.js | 22 +++++------ superset-frontend/src/utils/urlUtils.ts | 6 +-- .../CRUD/data/dataset/DatasetList.test.jsx | 2 +- superset/connectors/sqla/views.py | 2 +- superset/models/core.py | 2 +- superset/reports/commands/execute.py | 2 +- superset/templates/email/role_extended.txt | 2 +- superset/templates/email/role_granted.txt | 2 +- superset/views/core.py | 4 +- superset/views/redirects.py | 39 +++++++++++++------ tests/integration_tests/core_tests.py | 17 ++------ .../reports/commands_tests.py | 10 ++--- tests/unit_tests/utils/urls_tests.py | 4 +- 16 files changed, 63 insertions(+), 57 deletions(-) diff --git a/docs/docs/installation/sql-templating.mdx b/docs/docs/installation/sql-templating.mdx index 8a093e643..72c2c0a9a 100644 --- 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 index 5714eb5da..cf1d691ff 100644 --- 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 index cb2561efb..c4c9c3264 100644 --- 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 index 69570c5c8..cb6d84ea3 100644 --- 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 index 0ca104ef0..2aac5f3e2 100644 --- 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 index 948c22fa4..2f23f4557 100644 --- 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 index 18533905e..cee0a07b5 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -535,7 +535,7 @@ class TableModelView( # pylint: disable=too-many-ancestors 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 index 0cfcea166..25beb2bee 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -386,7 +386,7 @@ class Database( 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 index ad1b1edcd..03b9efce4 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -167,7 +167,7 @@ class BaseReportState: 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 index 89ba1b0f7..463fb32c9 100644 --- a/superset/templates/email/role_extended.txt +++ b/superset/templates/email/role_extended.txt @@ -20,7 +20,7 @@ Dear {{ user.username }},
{{ granter.username }} has extended the role {{ role.name }} to include - + {{datasource.full_name}} and granted you access to it.

diff --git a/superset/templates/email/role_granted.txt b/superset/templates/email/role_granted.txt index 8027f41ac..312a04947 100644 --- a/superset/templates/email/role_granted.txt +++ b/superset/templates/email/role_granted.txt @@ -21,7 +21,7 @@ Dear {{ user.username }}, {{ granter.username }} has granted you the role {{ role.name }} that gives access to the - + {{datasource.full_name}}

diff --git a/superset/views/core.py b/superset/views/core.py index c27dcfb4a..cfa9f8eaa 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -427,13 +427,13 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods _, 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 index 831fc978b..93a433940 100644 --- 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("/") 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 index abe61ff59..6bc08cae8 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -123,15 +123,6 @@ class TestCore(SupersetTestCase): @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 '
Explore in Superset' @@ -769,7 +769,7 @@ def test_email_chart_report_schedule_force_screenshot( ) # assert that the link sent is correct assert ( - 'Explore in Superset' @@ -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 ( - 'Explore in Superset' @@ -882,7 +882,7 @@ def test_email_chart_report_schedule_with_csv( ) # assert that the link sent is correct assert ( - 'Explore in Superset' @@ -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"" + f"" 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 index dba38cb81..f62c276f8 100644 --- 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" )