diff --git a/superset/commands/dashboard/importers/v1/__init__.py b/superset/commands/dashboard/importers/v1/__init__.py index fdd331c320e..c3063ed5437 100644 --- a/superset/commands/dashboard/importers/v1/__init__.py +++ b/superset/commands/dashboard/importers/v1/__init__.py @@ -22,7 +22,7 @@ from typing import Any 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 @@ class ImportDashboardsCommand(ImportModelsCommand): ) # 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 @@ class ImportDashboardsCommand(ImportModelsCommand): 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 @@ class ImportDashboardsCommand(ImportModelsCommand): ) # 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 00000000000..8b56d86e81b --- /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