fix(assets api): import replaces dashboard (#22208)

Co-authored-by: Stan Houcke <stan.houcke@skyscanner.net>

superset/commands/importers/v1/assets.py

@@ -14,12 +14,12 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-from typing import Any, Dict, List, Optional, Tuple
+from typing import Any, Dict, List, Optional
 
 from marshmallow import Schema
 from marshmallow.exceptions import ValidationError
 from sqlalchemy.orm import Session
-from sqlalchemy.sql import select
+from sqlalchemy.sql import delete, insert
 
 from superset import db
 from superset.charts.commands.importers.v1.utils import import_chart
@@ -70,7 +70,6 @@ def __init__(self, contents: Dict[str, str], *args: Any, **kwargs: Any):
         self.passwords: Dict[str, str] = kwargs.get("passwords") or {}
         self._configs: Dict[str, Any] = {}
 
-    # pylint: disable=too-many-locals
     @staticmethod
     def _import(session: Session, configs: Dict[str, Any]) -> None:
         # import databases first
@@ -106,30 +105,30 @@ def _import(session: Session, configs: Dict[str, Any]) -> None:
                 chart = import_chart(session, config, overwrite=True)
                 chart_ids[str(chart.uuid)] = chart.id
 
-        # store the existing relationship between dashboards and charts
-        existing_relationships = session.execute(
-            select([dashboard_slices.c.dashboard_id, dashboard_slices.c.slice_id])
-        ).fetchall()
-
         # import dashboards
-        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(session, config, overwrite=True)
+
+                # 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
-        ]
-        session.execute(dashboard_slices.insert(), values)
+                    dashboard_chart_id = {
+                        "dashboard_id": dashboard.id,
+                        "slice_id": chart_id,
+                    }
+                    dashboard_chart_ids.append(dashboard_chart_id)
+
+                session.execute(
+                    delete(dashboard_slices).where(
+                        dashboard_slices.c.dashboard_id == dashboard.id
+                    )
+                )
+                session.execute(insert(dashboard_slices).values(dashboard_chart_ids))
 
     def run(self) -> None:
         self.validate()

tests/unit_tests/commands/importers/__init__.py

@@ -0,0 +1,16 @@
+# 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/unit_tests/commands/importers/v1/__init__.py

@@ -0,0 +1,16 @@
+# 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/unit_tests/commands/importers/v1/assets_test.py

@@ -0,0 +1,131 @@
+# 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 copy
+
+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_import_new_assets(session: Session) -> None:
+    """
+    Test that all new assets are imported correctly.
+    """
+    from superset.commands.importers.v1.assets import ImportAssetsCommand
+    from superset.models.dashboard import dashboard_slices
+    from superset.models.slice import Slice
+
+    engine = 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)
+
+    ImportAssetsCommand._import(session, configs)
+    dashboard_ids = session.scalars(
+        select(dashboard_slices.c.dashboard_id).distinct()
+    ).all()
+    chart_ids = 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
+
+
+def test_import_adds_dashboard_charts(session: Session) -> None:
+    """
+    Test that existing dashboards are updated with new charts.
+    """
+    from superset.commands.importers.v1.assets import ImportAssetsCommand
+    from superset.models.dashboard import dashboard_slices
+    from superset.models.slice import Slice
+
+    engine = session.get_bind()
+    Slice.metadata.create_all(engine)  # pylint: disable=no-member
+    base_configs = {
+        **copy.deepcopy(databases_config),
+        **copy.deepcopy(datasets_config),
+        **copy.deepcopy(charts_config_2),
+        **copy.deepcopy(dashboards_config_2),
+    }
+    new_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)
+
+    ImportAssetsCommand._import(session, base_configs)
+    ImportAssetsCommand._import(session, new_configs)
+    dashboard_ids = session.scalars(
+        select(dashboard_slices.c.dashboard_id).distinct()
+    ).all()
+    chart_ids = 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
+
+
+def test_import_removes_dashboard_charts(session: Session) -> None:
+    """
+    Test that existing dashboards are updated without old charts.
+    """
+    from superset.commands.importers.v1.assets import ImportAssetsCommand
+    from superset.models.dashboard import dashboard_slices
+    from superset.models.slice import Slice
+
+    engine = session.get_bind()
+    Slice.metadata.create_all(engine)  # pylint: disable=no-member
+    base_configs = {
+        **copy.deepcopy(databases_config),
+        **copy.deepcopy(datasets_config),
+        **copy.deepcopy(charts_config_1),
+        **copy.deepcopy(dashboards_config_1),
+    }
+    new_configs = {
+        **copy.deepcopy(databases_config),
+        **copy.deepcopy(datasets_config),
+        **copy.deepcopy(charts_config_2),
+        **copy.deepcopy(dashboards_config_2),
+    }
+    expected_number_of_dashboards = len(dashboards_config_2)
+    expected_number_of_charts = len(charts_config_2)
+
+    ImportAssetsCommand._import(session, base_configs)
+    ImportAssetsCommand._import(session, new_configs)
+    dashboard_ids = session.scalars(
+        select(dashboard_slices.c.dashboard_id).distinct()
+    ).all()
+    chart_ids = 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

tests/unit_tests/fixtures/__init__.py

@@ -0,0 +1,16 @@
+# 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.