You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by hu...@apache.org on 2022/03/07 21:17:53 UTC
[superset] 03/08: fix(chart): deprecate persisting url_params (#18960)
This is an automated email from the ASF dual-hosted git repository.
hugh pushed a commit to branch 2022.9.1
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 639bcbca88511f2053d1d0921a3f6706edde02e2
Author: Ville Brofeldt <33...@users.noreply.github.com>
AuthorDate: Wed Mar 2 07:44:36 2022 +0200
fix(chart): deprecate persisting url_params (#18960)
* fix(chart): deprecate peristing url_params
* remove duplicated backend logic
* use omitBy
* simplify omit
(cherry picked from commit bd63a1bd98c1faf152205b3b862119a1c59b2f05)
---
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 +-
tests/integration_tests/charts/commands_tests.py | 32 +++++++++++++++++++++-
9 files changed, 99 insertions(+), 20 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index 667a47c..e00532f 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 8d431da..b25efdb 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 2923782..b5c4714 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<SaveModalProps, SaveModalState> {
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<SaveModalProps, SaveModalState> {
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 0000000..f14455b
--- /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 38637c1..9987b5d 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<string> =>
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<string> =>
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 960fd87..35f70a8 100644
--- a/superset/charts/commands/export.py
+++ b/superset/charts/commands/export.py
@@ -34,7 +34,7 @@ logger = logging.getLogger(__name__)
# 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 a51cec5..b4b8365 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -742,7 +742,7 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
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 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
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 c688044..65ed10f 100644
--- a/tests/integration_tests/charts/api_tests.py
+++ b/tests/integration_tests/charts/api_tests.py
@@ -84,7 +84,7 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin):
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 4e8ffa1..bf4fe4f 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 @@ import yaml
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 @@ class TestImportChartsCommand(SupersetTestCase):
}
+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")