From 280ffbe83edc450b495bef9d879677bfa81be947 Mon Sep 17 00:00:00 2001 From: Linden Haynes Date: Mon, 28 Aug 2023 08:37:04 -0500 Subject: [PATCH 1/2] fix: Dashboard import commands not correctly replacing charts --- .../dashboard/importers/v1/__init__.py | 32 +++---- .../dashboards/commands_tests.py | 63 +++++++++++++ .../fixtures/importexport.py | 90 +++++++++++++++++++ 3 files changed, 169 insertions(+), 16 deletions(-) diff --git a/superset/commands/dashboard/importers/v1/__init__.py b/superset/commands/dashboard/importers/v1/__init__.py index 17fb85fcfe6e4..7cbf1ffb5bf35 100644 --- a/superset/commands/dashboard/importers/v1/__init__.py +++ b/superset/commands/dashboard/importers/v1/__init__.py @@ -19,7 +19,7 @@ from marshmallow import Schema from sqlalchemy.orm import Session # noqa: F401 -from sqlalchemy.sql import select +from sqlalchemy.sql import delete, insert from superset import db from superset.charts.schemas import ImportV1ChartSchema @@ -120,32 +120,32 @@ def _import(configs: dict[str, Any], overwrite: bool = False) -> None: # noqa: charts.append(chart) chart_ids[str(chart.uuid)] = chart.id - # store the existing relationship between dashboards and charts - existing_relationships = db.session.execute( - select([dashboard_slices.c.dashboard_id, dashboard_slices.c.slice_id]) - ).fetchall() - # import dashboards dashboards: list[Dashboard] = [] - dashboard_chart_ids: list[tuple[int, int]] = [] for file_name, config in configs.items(): if file_name.startswith("dashboards/"): config = update_id_refs(config, chart_ids, dataset_info) dashboard = import_dashboard(config, overwrite=overwrite) dashboards.append(dashboard) + + # set ref in the dashboard_slices table + dashboard_chart_ids: list[dict[str, int]] = [] for uuid in find_chart_uuids(config["position"]): if uuid not in chart_ids: break chart_id = chart_ids[uuid] - if (dashboard.id, chart_id) not in existing_relationships: - dashboard_chart_ids.append((dashboard.id, chart_id)) - - # set ref in the dashboard_slices table - values = [ - {"dashboard_id": dashboard_id, "slice_id": chart_id} - for (dashboard_id, chart_id) in dashboard_chart_ids - ] - db.session.execute(dashboard_slices.insert(), values) + dashboard_chart_id = { + "dashboard_id": dashboard.id, + "slice_id": chart_id, + } + dashboard_chart_ids.append(dashboard_chart_id) + + db.session.execute( + delete(dashboard_slices).where( + dashboard_slices.c.dashboard_id == dashboard.id + ) + ) + db.session.execute(insert(dashboard_slices).values(dashboard_chart_ids)) # Migrate any filter-box charts to native dashboard filters. for dashboard in dashboards: diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py index 4dabe1b0136a9..259d6b10c408b 100644 --- a/tests/integration_tests/dashboards/commands_tests.py +++ b/tests/integration_tests/dashboards/commands_tests.py @@ -51,7 +51,9 @@ from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.importexport import ( chart_config, + chart_config_2, dashboard_config, + dashboard_config_2, dashboard_export, dashboard_metadata_config, database_config, @@ -742,6 +744,67 @@ def test_import_v1_dashboard_validation(self): } } + @patch("superset.security.manager.g") + @patch("superset.commands.database.importers.v1.utils.add_permissions") + def test_import_v1_dashboard_overwrite_removes_deleted_charts( + self, mock_add_permissions, mock_g + ): + """Test that existing dashboards are updated without old charts.""" + mock_g.user = security_manager.find_user("admin") + + num_dashboards = db.session.query(Dashboard).count() + + contents = { + "metadata.yaml": yaml.safe_dump(dashboard_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(database_config), + "datasets/imported_dataset.yaml": yaml.safe_dump(dataset_config), + "charts/imported_chart.yaml": yaml.safe_dump(chart_config), + "dashboards/imported_dashboard.yaml": yaml.safe_dump(dashboard_config), + } + command = v1.ImportDashboardsCommand(contents, overwrite=True) + command.run() + + dashboard = ( + db.session.query(Dashboard).filter_by(uuid=dashboard_config["uuid"]).one() + ) + + old_chart = dashboard.slices[0] + assert str(old_chart.uuid) == chart_config["uuid"] + + overwrite_contents = { + "metadata.yaml": yaml.safe_dump(dashboard_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(database_config), + "datasets/imported_dataset.yaml": yaml.safe_dump(dataset_config), + "charts/imported_chart.yaml": yaml.safe_dump(chart_config_2), + "dashboards/imported_dashboard.yaml": yaml.safe_dump(dashboard_config_2), + } + overwrite_command = v1.ImportDashboardsCommand( + overwrite_contents, overwrite=True + ) + overwrite_command.run() + + new_num_dashboards = db.session.query(Dashboard).count() + assert new_num_dashboards == num_dashboards + 1 + + dashboard = ( + db.session.query(Dashboard).filter_by(uuid=dashboard_config["uuid"]).one() + ) + + assert len(dashboard.slices) == 1 + + new_chart = dashboard.slices[0] + assert str(new_chart.uuid) == chart_config_2["uuid"] + + dataset = old_chart.table + database = dataset.database + + db.session.delete(dashboard) + db.session.delete(old_chart) + db.session.delete(new_chart) + db.session.delete(dataset) + db.session.delete(database) + db.session.commit() + class TestCopyDashboardCommand(SupersetTestCase): @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") diff --git a/tests/integration_tests/fixtures/importexport.py b/tests/integration_tests/fixtures/importexport.py index 427d0d24a6977..482a2c80b708d 100644 --- a/tests/integration_tests/fixtures/importexport.py +++ b/tests/integration_tests/fixtures/importexport.py @@ -681,6 +681,96 @@ }, "version": "1.0.0", } + +chart_config_2: dict[str, Any] = { + "slice_name": "Deck Path 2", + "viz_type": "deck_path", + "params": { + "color_picker": {"a": 1, "b": 135, "g": 122, "r": 0}, + "datasource": "12__table", + "js_columns": ["color"], + "js_data_mutator": r"data => data.map(d => ({\n ...d,\n color: colors.hexToRGB(d.extraProps.color)\n}));", + "js_onclick_href": "", + "js_tooltip": "", + "line_column": "path_json", + "line_type": "json", + "line_width": 100, + "mapbox_style": "mapbox://styles/mapbox/light-v9", + "reverse_long_lat": False, + "row_limit": 5000, + "slice_id": 43, + "time_grain_sqla": None, + "time_range": " : ", + "viewport": { + "altitude": 1.5, + "bearing": 0, + "height": 1094, + "latitude": 37.73671752604488, + "longitude": -122.18885402582598, + "maxLatitude": 85.05113, + "maxPitch": 10, + "maxZoom": 40, + "minLatitude": -85.05113, + "minPitch": 0, + "minZoom": 0, + "pitch": 0, + "width": 669, + "zoom": 9.51847667620428, + }, + "viz_type": "deck_path", + }, + "query_context": '{"datasource":{"id":12,"type":"table"},"force":false,"queries":[{"time_range":" : ","filters":[],"extras":{"time_grain_sqla":null,"having":"","where":""},"applied_time_extras":{},"columns":[],"metrics":[],"annotation_layers":[],"row_limit":5000,"timeseries_limit":0,"order_desc":true,"url_params":{},"custom_params":{},"custom_form_data":{}}],"result_format":"json","result_type":"full"}', + "cache_timeout": None, + "uuid": "82bb160d-a859-44ce-86cc-9a92de113514", + "version": "1.0.0", + "dataset_uuid": "10808100-158b-42c4-842e-f32b99d88dfb", +} + +dashboard_config_2: dict[str, Any] = { + "dashboard_title": "Test dash", + "description": None, + "css": "", + "slug": None, + "uuid": "c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51", + "position": { + "CHART-PsLqYTEu9ZS": { + "children": [], + "id": "CHART-PsLqYTEu9ZS", + "meta": { + "chartId": 283, + "height": 50, + "sliceName": "Games", + "uuid": "82bb160d-a859-44ce-86cc-9a92de113514", + "width": 4, + }, + "parents": ["ROOT_ID", "GRID_ID", "ROW-dP_CHaK2q"], + "type": "CHART", + }, + "DASHBOARD_VERSION_KEY": "v2", + "GRID_ID": { + "children": ["ROW-dP_CHaK2q"], + "id": "GRID_ID", + "parents": ["ROOT_ID"], + "type": "GRID", + }, + "HEADER_ID": { + "id": "HEADER_ID", + "meta": {"text": "Test dash"}, + "type": "HEADER", + }, + "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type": "ROOT"}, + "ROW-dP_CHaK2q": { + "children": ["CHART-PsLqYTEu9ZS"], + "id": "ROW-dP_CHaK2q", + "meta": {"0": "ROOT_ID", "background": "BACKGROUND_TRANSPARENT"}, + "parents": ["ROOT_ID", "GRID_ID"], + "type": "ROW", + }, + }, + "metadata": {}, + "version": "1.0.0", +} + saved_queries_config = { "schema": "public", "label": "Test Saved Query", From 054e3bbd8067b198fc921a6492e2504561837baf Mon Sep 17 00:00:00 2001 From: Linden Haynes Date: Sat, 4 Jan 2025 02:37:04 -0600 Subject: [PATCH 2/2] Fixing ruff linter errors --- tests/integration_tests/fixtures/importexport.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/fixtures/importexport.py b/tests/integration_tests/fixtures/importexport.py index 482a2c80b708d..7b8f77b206200 100644 --- a/tests/integration_tests/fixtures/importexport.py +++ b/tests/integration_tests/fixtures/importexport.py @@ -689,7 +689,7 @@ "color_picker": {"a": 1, "b": 135, "g": 122, "r": 0}, "datasource": "12__table", "js_columns": ["color"], - "js_data_mutator": r"data => data.map(d => ({\n ...d,\n color: colors.hexToRGB(d.extraProps.color)\n}));", + "js_data_mutator": r"data => data.map(d => ({\n ...d,\n color: colors.hexToRGB(d.extraProps.color)\n}));", # noqa: E501 "js_onclick_href": "", "js_tooltip": "", "line_column": "path_json", @@ -719,7 +719,7 @@ }, "viz_type": "deck_path", }, - "query_context": '{"datasource":{"id":12,"type":"table"},"force":false,"queries":[{"time_range":" : ","filters":[],"extras":{"time_grain_sqla":null,"having":"","where":""},"applied_time_extras":{},"columns":[],"metrics":[],"annotation_layers":[],"row_limit":5000,"timeseries_limit":0,"order_desc":true,"url_params":{},"custom_params":{},"custom_form_data":{}}],"result_format":"json","result_type":"full"}', + "query_context": '{"datasource":{"id":12,"type":"table"},"force":false,"queries":[{"time_range":" : ","filters":[],"extras":{"time_grain_sqla":null,"having":"","where":""},"applied_time_extras":{},"columns":[],"metrics":[],"annotation_layers":[],"row_limit":5000,"timeseries_limit":0,"order_desc":true,"url_params":{},"custom_params":{},"custom_form_data":{}}],"result_format":"json","result_type":"full"}', # noqa: E501 "cache_timeout": None, "uuid": "82bb160d-a859-44ce-86cc-9a92de113514", "version": "1.0.0",