From bd63a1bd98c1faf152205b3b862119a1c59b2f05 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Wed, 2 Mar 2022 07:44:36 +0200 Subject: [PATCH] fix(chart): deprecate persisting url_params (#18960) * fix(chart): deprecate peristing url_params * remove duplicated backend logic * use omitBy * simplify omit --- UPDATING.md | 2 ++ .../src/explore/components/SaveModal.test.jsx | 12 +++---- .../src/explore/components/SaveModal.tsx | 9 ++++-- .../src/explore/exploreUtils/formData.test.ts | 28 ++++++++++++++++ .../src/explore/exploreUtils/formData.ts | 18 +++++++---- superset/charts/commands/export.py | 2 +- superset/views/core.py | 14 ++++++-- tests/integration_tests/charts/api_tests.py | 2 +- .../charts/commands_tests.py | 32 ++++++++++++++++++- 9 files changed, 99 insertions(+), 20 deletions(-) create mode 100644 superset-frontend/src/explore/exploreUtils/formData.test.ts diff --git a/UPDATING.md b/UPDATING.md index 667a47ca991f0..e00532fb4eddc 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -42,6 +42,8 @@ assists people when migrating to a new version. ### Deprecations +- [18960](https://github.com/apache/superset/pull/18960): Persisting URL params in chart metadata is no longer supported. To set a default value for URL params in Jinja code, use the optional second argument: `url_param("my-param", "my-default-value")`. + ### Other - [17589](https://github.com/apache/incubator-superset/pull/17589): It is now possible to limit access to users' recent activity data by setting the `ENABLE_BROAD_ACTIVITY_ACCESS` config flag to false, or customizing the `raise_for_user_activity_access` method in the security manager. diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx index 8d431da973c83..b25efdb773a8b 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.jsx +++ b/superset-frontend/src/explore/components/SaveModal.test.jsx @@ -64,7 +64,7 @@ describe('SaveModal', () => { } return arg; }), - form_data: { datasource: '107__table' }, + form_data: { datasource: '107__table', url_params: { foo: 'bar' } }, }; const mockEvent = { target: { @@ -168,11 +168,11 @@ describe('SaveModal', () => { defaultProps.actions.saveSlice.restore(); }); - it('should save slice', () => { + it('should save slice without url_params in form_data', () => { const wrapper = getWrapper(); wrapper.instance().saveOrOverwrite(true); const { args } = defaultProps.actions.saveSlice.getCall(0); - expect(args[0]).toEqual(defaultProps.form_data); + expect(args[0]).toEqual({ datasource: '107__table' }); }); it('existing dashboard', () => { @@ -219,7 +219,7 @@ describe('SaveModal', () => { defaultProps.actions.saveSlice().then(() => { expect(window.location.assign.callCount).toEqual(1); expect(window.location.assign.getCall(0).args[0]).toEqual( - 'http://localhost/mock_dashboard/', + 'http://localhost/mock_dashboard/?foo=bar', ); done(); }); @@ -235,7 +235,7 @@ describe('SaveModal', () => { defaultProps.actions.saveSlice().then(() => { expect(window.location.assign.callCount).toEqual(1); expect(window.location.assign.getCall(0).args[0]).toEqual( - '/mock_slice/', + '/mock_slice/?foo=bar', ); done(); }); @@ -248,7 +248,7 @@ describe('SaveModal', () => { defaultProps.actions.saveSlice().then(() => { expect(window.location.assign.callCount).toEqual(1); expect(window.location.assign.getCall(0).args[0]).toEqual( - '/mock_slice/', + '/mock_slice/?foo=bar', ); done(); }); diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 292378290e98c..b5c47145e0c97 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -138,9 +138,10 @@ class SaveModal extends React.Component { sliceParams.slice_name = this.state.newSliceName; sliceParams.save_to_dashboard_id = this.state.saveToDashboardId; sliceParams.new_dashboard_name = this.state.newDashboardName; + const { url_params, ...formData } = this.props.form_data || {}; this.props.actions - .saveSlice(this.props.form_data, sliceParams) + .saveSlice(formData, sliceParams) .then((data: JsonObject) => { if (data.dashboard_id === null) { sessionStorage.removeItem(SK_DASHBOARD_ID); @@ -148,7 +149,11 @@ class SaveModal extends React.Component { sessionStorage.setItem(SK_DASHBOARD_ID, data.dashboard_id); } // Go to new slice url or dashboard url - const url = gotodash ? data.dashboard_url : data.slice.slice_url; + let url = gotodash ? data.dashboard_url : data.slice.slice_url; + if (url_params) { + const prefix = url.includes('?') ? '&' : '?'; + url = `${url}${prefix}${new URLSearchParams(url_params).toString()}`; + } window.location.assign(url); }); this.props.onHide(); diff --git a/superset-frontend/src/explore/exploreUtils/formData.test.ts b/superset-frontend/src/explore/exploreUtils/formData.test.ts new file mode 100644 index 0000000000000..f14455b51bd8e --- /dev/null +++ b/superset-frontend/src/explore/exploreUtils/formData.test.ts @@ -0,0 +1,28 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { sanitizeFormData } from './formData'; + +test('sanitizeFormData removes temporary control values', () => { + expect( + sanitizeFormData({ + url_params: { foo: 'bar' }, + metrics: ['foo', 'bar'], + }), + ).toEqual({ metrics: ['foo', 'bar'] }); +}); diff --git a/superset-frontend/src/explore/exploreUtils/formData.ts b/superset-frontend/src/explore/exploreUtils/formData.ts index 38637c1019623..9987b5d8cfa76 100644 --- a/superset-frontend/src/explore/exploreUtils/formData.ts +++ b/superset-frontend/src/explore/exploreUtils/formData.ts @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +import { omit } from 'lodash'; import { SupersetClient, JsonObject } from '@superset-ui/core'; type Payload = { @@ -24,6 +25,11 @@ type Payload = { chart_id?: number; }; +const TEMPORARY_CONTROLS = ['url_params']; + +export const sanitizeFormData = (formData: JsonObject): JsonObject => + omit(formData, TEMPORARY_CONTROLS); + const assembleEndpoint = (key?: string, tabId?: string) => { let endpoint = 'api/v1/explore/form_data'; if (key) { @@ -37,12 +43,12 @@ const assembleEndpoint = (key?: string, tabId?: string) => { const assemblePayload = ( datasetId: number, - form_data: JsonObject, + formData: JsonObject, chartId?: number, ) => { const payload: Payload = { dataset_id: datasetId, - form_data: JSON.stringify(form_data), + form_data: JSON.stringify(sanitizeFormData(formData)), }; if (chartId) { payload.chart_id = chartId; @@ -52,23 +58,23 @@ const assemblePayload = ( export const postFormData = ( datasetId: number, - form_data: JsonObject, + formData: JsonObject, chartId?: number, tabId?: string, ): Promise => SupersetClient.post({ endpoint: assembleEndpoint(undefined, tabId), - jsonPayload: assemblePayload(datasetId, form_data, chartId), + jsonPayload: assemblePayload(datasetId, formData, chartId), }).then(r => r.json.key); export const putFormData = ( datasetId: number, key: string, - form_data: JsonObject, + formData: JsonObject, chartId?: number, tabId?: string, ): Promise => SupersetClient.put({ endpoint: assembleEndpoint(key, tabId), - jsonPayload: assemblePayload(datasetId, form_data, chartId), + jsonPayload: assemblePayload(datasetId, formData, chartId), }).then(r => r.json.message); diff --git a/superset/charts/commands/export.py b/superset/charts/commands/export.py index 960fd87762b6d..35f70a82eab65 100644 --- a/superset/charts/commands/export.py +++ b/superset/charts/commands/export.py @@ -34,7 +34,7 @@ # keys present in the standard export that are not needed -REMOVE_KEYS = ["datasource_type", "datasource_name", "query_context"] +REMOVE_KEYS = ["datasource_type", "datasource_name", "query_context", "url_params"] class ExportChartsCommand(ExportModelsCommand): diff --git a/superset/views/core.py b/superset/views/core.py index a51cec56216fe..b4b8365a58200 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -742,7 +742,7 @@ def explore( form_data_key = request.args.get("form_data_key") if form_data_key: - parameters = CommandParameters(actor=g.user, key=form_data_key,) + parameters = CommandParameters(actor=g.user, key=form_data_key) value = GetFormDataCommand(parameters).run() initial_form_data = json.loads(value) if value else {} @@ -751,10 +751,18 @@ def explore( dataset_id = request.args.get("dataset_id") if slice_id: initial_form_data["slice_id"] = slice_id - flash(_("Form data not found in cache, reverting to chart metadata.")) + if form_data_key: + flash( + _("Form data not found in cache, reverting to chart metadata.") + ) elif dataset_id: initial_form_data["datasource"] = f"{dataset_id}__table" - flash(_("Form data not found in cache, reverting to dataset metadata.")) + if form_data_key: + flash( + _( + "Form data not found in cache, reverting to dataset metadata." + ) + ) form_data, slc = get_form_data( use_slice_data=True, initial_form_data=initial_form_data diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index c688044eb51aa..65ed10fe1376f 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -84,7 +84,7 @@ def create_charts(self): charts = [] admin = self.get_user("admin") for cx in range(CHARTS_FIXTURE_COUNT - 1): - charts.append(self.insert_chart(f"name{cx}", [admin.id], 1,)) + charts.append(self.insert_chart(f"name{cx}", [admin.id], 1)) fav_charts = [] for cx in range(round(CHARTS_FIXTURE_COUNT / 2)): fav_star = FavStar( diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index 4e8ffa1af7706..bf4fe4fe79bd4 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -15,7 +15,6 @@ # specific language governing permissions and limitations # under the License. import json -from datetime import datetime from unittest.mock import patch import pytest @@ -23,6 +22,7 @@ from flask import g from superset import db, security_manager +from superset.charts.commands.create import CreateChartCommand from superset.charts.commands.exceptions import ChartNotFoundError from superset.charts.commands.export import ExportChartsCommand from superset.charts.commands.importers.v1 import ImportChartsCommand @@ -329,6 +329,36 @@ def test_import_v1_chart_validation(self): } +class TestChartsCreateCommand(SupersetTestCase): + @patch("superset.views.base.g") + @patch("superset.security.manager.g") + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_create_v1_response(self, mock_sm_g, mock_g): + """Test that the create chart command creates a chart""" + actor = security_manager.find_user(username="admin") + mock_g.user = mock_sm_g.user = actor + chart_data = { + "slice_name": "new chart", + "description": "new description", + "owners": [actor.id], + "viz_type": "new_viz_type", + "params": json.dumps({"viz_type": "new_viz_type"}), + "cache_timeout": 1000, + "datasource_id": 1, + "datasource_type": "table", + } + command = CreateChartCommand(actor, chart_data) + chart = command.run() + chart = db.session.query(Slice).get(chart.id) + assert chart.viz_type == "new_viz_type" + json_params = json.loads(chart.params) + assert json_params == {"viz_type": "new_viz_type"} + assert chart.slice_name == "new chart" + assert chart.owners == [actor] + db.session.delete(chart) + db.session.commit() + + class TestChartsUpdateCommand(SupersetTestCase): @patch("superset.views.base.g") @patch("superset.security.manager.g")