From a405e323e683525b3d48a29378293f1ad621c0f2 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 11 Dec 2025 15:29:24 -0800 Subject: [PATCH 1/2] fix(dashboard): import with overwrite flag replaces charts instead of merging Fixes #22127 When importing dashboards via /api/v1/dashboard/import/ with overwrite=True, the dashboard's chart associations were being merged with existing charts instead of being replaced. This made it impossible to use the import API for proper source control workflows where the imported dashboard should exactly match the import file. The fix mirrors the behavior of the assets import API (PR #22208) by: 1. Deleting all existing dashboard-slice relationships when overwrite=True before inserting the new ones from the import 2. Only querying existing relationships when overwrite=False (optimization) 3. Changing break to continue when encountering unknown chart UUIDs so all valid charts are processed instead of stopping at the first unknown one Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../dashboard/importers/v1/__init__.py | 42 +++- .../importers/v1/import_command_test.py | 214 ++++++++++++++++++ 2 files changed, 245 insertions(+), 11 deletions(-) create mode 100644 tests/unit_tests/dashboards/commands/importers/v1/import_command_test.py diff --git a/superset/commands/dashboard/importers/v1/__init__.py b/superset/commands/dashboard/importers/v1/__init__.py index fdd331c320ed..c3063ed5437f 100644 --- a/superset/commands/dashboard/importers/v1/__init__.py +++ b/superset/commands/dashboard/importers/v1/__init__.py @@ -22,7 +22,7 @@ from marshmallow import Schema from sqlalchemy.orm import Session # noqa: F401 -from sqlalchemy.sql import select +from sqlalchemy.sql import delete, select from superset import db from superset.charts.schemas import ImportV1ChartSchema @@ -157,9 +157,14 @@ def _import( ) # 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() + # (only used when overwrite=False to avoid inserting duplicates) + existing_relationships: set[tuple[int, int]] = set() + if not overwrite: + existing_relationships = set( + db.session.execute( + select(dashboard_slices.c.dashboard_id, dashboard_slices.c.slice_id) + ).fetchall() + ) # import dashboards dashboards: list[Dashboard] = [] @@ -177,11 +182,25 @@ def _import( del config["theme_uuid"] dashboard = import_dashboard(config, overwrite=overwrite) dashboards.append(dashboard) + + # When overwriting, first delete all existing chart relationships + # so the dashboard is replaced rather than merged + if overwrite: + db.session.execute( + delete(dashboard_slices).where( + dashboard_slices.c.dashboard_id == dashboard.id + ) + ) + + # Collect chart IDs to associate with this dashboard for uuid in find_chart_uuids(config["position"]): if uuid not in chart_ids: - break + continue chart_id = chart_ids[uuid] - if (dashboard.id, chart_id) not in existing_relationships: + if ( + overwrite + or (dashboard.id, chart_id) not in existing_relationships + ): dashboard_chart_ids.append((dashboard.id, chart_id)) # Handle tags using import_tag function @@ -197,11 +216,12 @@ def _import( ) # 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) + if dashboard_chart_ids: + 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) # Migrate any filter-box charts to native dashboard filters. for dashboard in dashboards: diff --git a/tests/unit_tests/dashboards/commands/importers/v1/import_command_test.py b/tests/unit_tests/dashboards/commands/importers/v1/import_command_test.py new file mode 100644 index 000000000000..8b56d86e81be --- /dev/null +++ b/tests/unit_tests/dashboards/commands/importers/v1/import_command_test.py @@ -0,0 +1,214 @@ +# 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. +""" +Tests for ImportDashboardsCommand overwrite functionality. + +These tests verify that when importing dashboards with overwrite=True, +the dashboard's chart associations are replaced rather than merged. +This addresses GitHub issue #22127. +""" + +import copy + +from pytest_mock import MockerFixture +from sqlalchemy.orm.session import Session +from sqlalchemy.sql import select + +from tests.unit_tests.fixtures.assets_configs import ( + charts_config_1, + charts_config_2, + dashboards_config_1, + dashboards_config_2, + databases_config, + datasets_config, +) + + +def test_dashboard_import_with_overwrite_replaces_charts( + mocker: MockerFixture, session: Session +) -> None: + """ + Test that dashboard import with overwrite=True replaces charts instead of merging. + + This test verifies the fix for GitHub issue #22127: + 1. Import a dashboard with 2 charts + 2. Re-import the same dashboard (same UUID) with only 1 chart + 3. The dashboard should have only 1 chart (replaced), not 3 (merged) + """ + from superset import db, security_manager + from superset.commands.dashboard.importers.v1 import ImportDashboardsCommand + from superset.models.dashboard import dashboard_slices + from superset.models.slice import Slice + + mocker.patch.object(security_manager, "can_access", return_value=True) + + engine = db.session.get_bind() + Slice.metadata.create_all(engine) # pylint: disable=no-member + + # First import: dashboard with 2 charts (charts_config_1 has 2 charts) + initial_configs = { + **copy.deepcopy(databases_config), + **copy.deepcopy(datasets_config), + **copy.deepcopy(charts_config_1), + **copy.deepcopy(dashboards_config_1), + } + ImportDashboardsCommand._import(initial_configs, overwrite=True) + + # Verify initial state: 2 charts associated with the dashboard + initial_chart_ids = db.session.scalars(select(dashboard_slices.c.slice_id)).all() + assert len(initial_chart_ids) == 2 + + # Second import: same dashboard with only 1 chart (charts_config_2 has 1 chart) + updated_configs = { + **copy.deepcopy(databases_config), + **copy.deepcopy(datasets_config), + **copy.deepcopy(charts_config_2), + **copy.deepcopy(dashboards_config_2), + } + ImportDashboardsCommand._import(updated_configs, overwrite=True) + + # Verify final state: only 1 chart (replaced, not merged) + final_chart_ids = db.session.scalars(select(dashboard_slices.c.slice_id)).all() + assert len(final_chart_ids) == 1 + + +def test_dashboard_import_without_overwrite_merges_charts( + mocker: MockerFixture, session: Session +) -> None: + """ + Test that dashboard import without overwrite merges charts (original behavior). + + When overwrite=False, new chart associations should be added to existing ones. + """ + from superset import db, security_manager + from superset.commands.dashboard.importers.v1 import ImportDashboardsCommand + from superset.models.dashboard import dashboard_slices + from superset.models.slice import Slice + + mocker.patch.object(security_manager, "can_access", return_value=True) + + engine = db.session.get_bind() + Slice.metadata.create_all(engine) # pylint: disable=no-member + + # First import: dashboard with 1 chart + initial_configs = { + **copy.deepcopy(databases_config), + **copy.deepcopy(datasets_config), + **copy.deepcopy(charts_config_2), + **copy.deepcopy(dashboards_config_2), + } + ImportDashboardsCommand._import(initial_configs, overwrite=False) + + # Verify initial state: 1 chart + initial_chart_ids = db.session.scalars(select(dashboard_slices.c.slice_id)).all() + assert len(initial_chart_ids) == 1 + + # Second import: same dashboard with 2 charts, but overwrite=False + updated_configs = { + **copy.deepcopy(databases_config), + **copy.deepcopy(datasets_config), + **copy.deepcopy(charts_config_1), + **copy.deepcopy(dashboards_config_1), + } + ImportDashboardsCommand._import(updated_configs, overwrite=False) + + # When overwrite=False, new charts are added but existing ones remain + # The dashboard_config_1 has 2 charts, one of which is the same as in config_2 + # So we should have 2 total (not 3, since the common chart is not duplicated) + final_chart_ids = db.session.scalars(select(dashboard_slices.c.slice_id)).all() + assert len(final_chart_ids) == 2 + + +def test_dashboard_import_with_overwrite_adds_new_charts( + mocker: MockerFixture, session: Session +) -> None: + """ + Test that dashboard import with overwrite=True correctly adds new charts. + + This verifies that overwrite mode not only removes old charts but also + properly adds the new charts from the import. + """ + from superset import db, security_manager + from superset.commands.dashboard.importers.v1 import ImportDashboardsCommand + from superset.models.dashboard import dashboard_slices + from superset.models.slice import Slice + + mocker.patch.object(security_manager, "can_access", return_value=True) + + engine = db.session.get_bind() + Slice.metadata.create_all(engine) # pylint: disable=no-member + + # First import: dashboard with 1 chart + initial_configs = { + **copy.deepcopy(databases_config), + **copy.deepcopy(datasets_config), + **copy.deepcopy(charts_config_2), + **copy.deepcopy(dashboards_config_2), + } + ImportDashboardsCommand._import(initial_configs, overwrite=True) + + # Verify initial state: 1 chart + initial_chart_ids = db.session.scalars(select(dashboard_slices.c.slice_id)).all() + assert len(initial_chart_ids) == 1 + + # Second import: same dashboard with 2 charts + updated_configs = { + **copy.deepcopy(databases_config), + **copy.deepcopy(datasets_config), + **copy.deepcopy(charts_config_1), + **copy.deepcopy(dashboards_config_1), + } + ImportDashboardsCommand._import(updated_configs, overwrite=True) + + # Verify final state: 2 charts (replaced with new set) + final_chart_ids = db.session.scalars(select(dashboard_slices.c.slice_id)).all() + assert len(final_chart_ids) == 2 + + +def test_dashboard_import_new_dashboard( + mocker: MockerFixture, session: Session +) -> None: + """ + Test that importing a new dashboard works correctly. + """ + from superset import db, security_manager + from superset.commands.dashboard.importers.v1 import ImportDashboardsCommand + from superset.models.dashboard import dashboard_slices + from superset.models.slice import Slice + + mocker.patch.object(security_manager, "can_access", return_value=True) + + engine = db.session.get_bind() + Slice.metadata.create_all(engine) # pylint: disable=no-member + + configs = { + **copy.deepcopy(databases_config), + **copy.deepcopy(datasets_config), + **copy.deepcopy(charts_config_1), + **copy.deepcopy(dashboards_config_1), + } + expected_number_of_dashboards = len(dashboards_config_1) + expected_number_of_charts = len(charts_config_1) + + ImportDashboardsCommand._import(configs, overwrite=True) + dashboard_ids = db.session.scalars( + select(dashboard_slices.c.dashboard_id).distinct() + ).all() + chart_ids = db.session.scalars(select(dashboard_slices.c.slice_id)).all() + + assert len(chart_ids) == expected_number_of_charts + assert len(dashboard_ids) == expected_number_of_dashboards From d6baff4394f485836d7df2ef4f4fb444ad59d3b6 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 11 Dec 2025 15:37:30 -0800 Subject: [PATCH 2/2] chore: trigger CI re-run