fix(chart): deprecate persisting url_params (#18960)

* fix(chart): deprecate peristing url_params

* remove duplicated backend logic

* use omitBy

* simplify omit

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.

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();
           });

superset-frontend/src/explore/components/SaveModal.tsx

@@ -138,17 +138,22 @@ 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);
         } else {
           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();

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'] });
+});

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);

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):

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

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(

tests/integration_tests/charts/commands_tests.py

@@ -15,14 +15,14 @@
 # specific language governing permissions and limitations
 # under the License.
 import json
-from datetime import datetime
 from unittest.mock import patch
 
 import pytest
 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 @@ 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")