bito-code-review[bot] commented on code in PR #43232:
URL: https://github.com/apache/superset/pull/43232#discussion_r4164538049
##########
tests/integration_tests/dashboards/commands_tests.py:
##########
@@ -1016,3 +1024,575 @@ def test_fave_unfave_dashboard_command_forbidden(self,
mock_get):
with self.assertRaises(DashboardAccessDeniedError): # noqa:
PT027
DelFavoriteDashboardCommand(example_dashboard.uuid).run()
+
+
+def _cleanup_dashboard_annotation_import(chart_uuids, layer_uuids,
dashboard_uuid):
+ dashboard =
db.session.query(Dashboard).filter_by(uuid=dashboard_uuid).one_or_none()
+ if dashboard:
+ db.session.delete(dashboard)
+ for chart_uuid in chart_uuids:
+ chart =
db.session.query(Slice).filter_by(uuid=chart_uuid).one_or_none()
+ if chart:
+ db.session.delete(chart)
+ for layer_uuid in layer_uuids:
+ layer = (
+
db.session.query(AnnotationLayer).filter_by(uuid=layer_uuid).one_or_none()
+ )
+ if layer:
+ db.session.query(Annotation).filter(
+ Annotation.layer_id == layer.id
+ ).delete()
+ db.session.delete(layer)
+ dataset = (
+
db.session.query(SqlaTable).filter_by(uuid=dataset_config["uuid"]).one_or_none()
+ )
+ if dataset:
+ db.session.delete(dataset)
+ database = (
+
db.session.query(Database).filter_by(uuid=database_config["uuid"]).one_or_none()
+ )
+ if database:
+ db.session.delete(database)
+ db.session.commit()
+
+
+class TestImportDashboardsAnnotationLayers(SupersetTestCase):
+ """Dashboard-level annotation layer dependency coverage for import."""
+
+ @patch("superset.utils.core.g")
+ @patch("superset.security.manager.g")
+ @patch("superset.commands.database.importers.v1.utils.add_permissions")
+ def test_import_dashboard_native_annotation_dependency_chain(
+ self, mock_add_permissions, sm_g, utils_g
+ ):
+ """Import dashboard, chart, native layer, and child annotations."""
+ sm_g.user = utils_g.user = security_manager.find_user("admin")
+ dashboard_uuid = str(uuid4())
+ chart_uuid = str(uuid4())
+ layer_uuid = str(uuid4())
+ chart_cfg = dashboard_chart_config(chart_uuid, "Dashboard Native Main")
+ chart_cfg["params"]["annotation_layers"] = [
+ {
+ "name": "Native Layer",
+ "annotationType": "EVENT",
+ "sourceType": "NATIVE",
+ "value": layer_uuid,
+ "show": True,
+ "style": "solid",
+ }
+ ]
+ dash_cfg = dashboard_config_for_charts(
+ dashboard_uuid,
+ "Dashboard Native Test",
+ [("CHART-1", chart_uuid, chart_cfg["slice_name"])],
+ )
+ contents = dashboard_import_bundle(
+ dash_cfg,
+ {"main_chart.yaml": chart_cfg},
+ {
+ "native_layer.yaml": dashboard_annotation_layer_config(
+ layer_uuid,
+ "Native Layer",
+ [
+ {
+ "uuid": str(uuid4()),
+ "short_descr": "child-a",
+ "long_descr": "child annotation",
+ "json_metadata": {"flag": "a"},
+ },
+ {
+ "uuid": str(uuid4()),
+ "short_descr": "child-b",
+ "long_descr": "child annotation 2",
+ "json_metadata": {"flag": "b"},
+ },
+ ],
+ descr="native layer descr",
+ )
+ },
+ )
+ try:
+ v1.ImportDashboardsCommand(contents, overwrite=True).run()
+
+ dashboard =
db.session.query(Dashboard).filter_by(uuid=dashboard_uuid).one()
+ chart = db.session.query(Slice).filter_by(uuid=chart_uuid).one()
+ layer =
db.session.query(AnnotationLayer).filter_by(uuid=layer_uuid).one()
+ annotations = (
+ db.session.query(Annotation).filter_by(layer_id=layer.id).all()
+ )
+
+ assert chart in dashboard.slices
+ imported_layers = json.loads(chart.params)["annotation_layers"]
+ assert len(imported_layers) == 1
+ assert imported_layers[0]["sourceType"] == "NATIVE"
+ assert imported_layers[0]["value"] == layer.id
+ assert imported_layers[0]["show"] is True
+ assert imported_layers[0]["style"] == "solid"
+ assert layer.name == "Native Layer"
+ assert layer.descr == "native layer descr"
+ assert {annotation.short_descr for annotation in annotations} == {
+ "child-a",
+ "child-b",
+ }
+ assert all(annotation.layer_id == layer.id for annotation in
annotations)
+ finally:
+ _cleanup_dashboard_annotation_import(
+ [chart_uuid], [layer_uuid], dashboard_uuid
+ )
+
+ @patch("superset.utils.core.g")
+ @patch("superset.security.manager.g")
+ @patch("superset.commands.database.importers.v1.utils.add_permissions")
+ def test_import_dashboard_annotation_dependency_graph(
+ self, mock_add_permissions, sm_g, utils_g
+ ):
+ """Import dashboard with two native layers and chart dependency."""
+ sm_g.user = utils_g.user = security_manager.find_user("admin")
+ dashboard_uuid = str(uuid4())
+ main_chart_uuid = str(uuid4())
+ ref_chart_uuid = str(uuid4())
+ layer_a_uuid = str(uuid4())
+ layer_b_uuid = str(uuid4())
+
+ ref_chart_cfg = dashboard_chart_config(ref_chart_uuid, "Dashboard Ref
Chart")
+ main_chart_cfg = dashboard_chart_config(main_chart_uuid, "Dashboard
Main Chart")
+ main_chart_cfg["params"]["annotation_layers"] = [
+ {
+ "name": "Native Layer A",
+ "annotationType": "EVENT",
+ "sourceType": "NATIVE",
+ "value": layer_a_uuid,
+ "show": True,
+ "style": "solid",
+ },
+ {
+ "name": "Native Layer B",
+ "annotationType": "EVENT",
+ "sourceType": "NATIVE",
+ "value": layer_b_uuid,
+ "show": False,
+ "style": "dashed",
+ },
+ {
+ "name": "Table Annotation",
+ "annotationType": "EVENT",
+ "sourceType": "table",
+ "value": ref_chart_uuid,
+ "show": True,
+ "style": "solid",
+ },
+ ]
+ dash_cfg = dashboard_config_for_charts(
+ dashboard_uuid,
+ "Dashboard Dependency Graph Test",
+ [
+ ("CHART-1", main_chart_uuid, main_chart_cfg["slice_name"]),
+ ("CHART-2", ref_chart_uuid, ref_chart_cfg["slice_name"]),
+ ],
+ )
+ contents = dashboard_import_bundle(
+ dash_cfg,
+ {
+ "main_chart.yaml": main_chart_cfg,
+ "ref_chart.yaml": ref_chart_cfg,
+ },
+ {
+ "layer_a.yaml": dashboard_annotation_layer_config(
+ layer_a_uuid,
+ "Layer A",
+ [
+ {
+ "uuid": str(uuid4()),
+ "short_descr": "A1",
+ "long_descr": "A1 long",
+ "json_metadata": {"layer": "A"},
+ },
+ {
+ "uuid": str(uuid4()),
+ "short_descr": "A2",
+ "long_descr": "A2 long",
+ "json_metadata": {"layer": "A", "index": 2},
+ },
+ ],
+ ),
+ "layer_b.yaml": dashboard_annotation_layer_config(
+ layer_b_uuid,
+ "Layer B",
+ [
+ {
+ "uuid": str(uuid4()),
+ "short_descr": "B1",
+ "long_descr": "B1 long",
+ "json_metadata": {"layer": "B"},
+ },
+ {
+ "uuid": str(uuid4()),
+ "short_descr": "B2",
+ "long_descr": "B2 long",
+ "json_metadata": {"layer": "B", "index": 2},
+ },
+ ],
+ ),
+ },
+ )
+ try:
+ v1.ImportDashboardsCommand(contents, overwrite=True).run()
+
+ dashboard =
db.session.query(Dashboard).filter_by(uuid=dashboard_uuid).one()
+ main_chart =
db.session.query(Slice).filter_by(uuid=main_chart_uuid).one()
+ ref_chart =
db.session.query(Slice).filter_by(uuid=ref_chart_uuid).one()
+ layer_a = (
+
db.session.query(AnnotationLayer).filter_by(uuid=layer_a_uuid).one()
+ )
+ layer_b = (
+
db.session.query(AnnotationLayer).filter_by(uuid=layer_b_uuid).one()
+ )
+ assert {str(chart.uuid) for chart in dashboard.slices} == {
+ main_chart_uuid,
+ ref_chart_uuid,
+ }
+
+ imported_layers =
json.loads(main_chart.params)["annotation_layers"]
+ assert [layer_cfg["sourceType"] for layer_cfg in imported_layers]
== [
+ "NATIVE",
+ "NATIVE",
+ "table",
+ ]
+ assert [layer_cfg["value"] for layer_cfg in imported_layers] == [
+ layer_a.id,
+ layer_b.id,
+ ref_chart.id,
+ ]
+
+ a_annotations = (
+
db.session.query(Annotation).filter_by(layer_id=layer_a.id).all()
+ )
+ b_annotations = (
+
db.session.query(Annotation).filter_by(layer_id=layer_b.id).all()
+ )
+ assert {annotation.short_descr for annotation in a_annotations} ==
{
+ "A1",
+ "A2",
+ }
+ assert {annotation.short_descr for annotation in b_annotations} ==
{
+ "B1",
+ "B2",
+ }
+ assert all(
+ annotation.layer_id == layer_a.id for annotation in
a_annotations
+ )
+ assert all(
+ annotation.layer_id == layer_b.id for annotation in
b_annotations
+ )
+ finally:
+ _cleanup_dashboard_annotation_import(
+ [main_chart_uuid, ref_chart_uuid],
+ [layer_a_uuid, layer_b_uuid],
+ dashboard_uuid,
+ )
+
+ @patch("superset.utils.core.g")
+ @patch("superset.security.manager.g")
+ @patch("superset.commands.database.importers.v1.utils.add_permissions")
+ def test_import_dashboard_existing_annotation_layer_reuses_existing_layer(
+ self, mock_add_permissions, sm_g, utils_g
+ ):
+ """Reuse existing native layer during dashboard import."""
+ sm_g.user = utils_g.user = security_manager.find_user("admin")
+ dashboard_uuid = str(uuid4())
+ chart_uuid = str(uuid4())
+ layer_uuid = str(uuid4())
+
+ existing_layer = AnnotationLayer(name="existing-layer", descr="before")
+ existing_layer.uuid = layer_uuid
+ db.session.add(existing_layer)
+ db.session.commit()
+ existing_layer_id = existing_layer.id
+ db.session.add(
+ Annotation(
+ layer=existing_layer, short_descr="existing-child",
json_metadata=None
+ )
+ )
+ db.session.commit()
+
+ chart_cfg = dashboard_chart_config(chart_uuid, "Dashboard Existing
Layer")
+ chart_cfg["params"]["annotation_layers"] = [
+ {
+ "name": "Native",
+ "annotationType": "EVENT",
+ "sourceType": "NATIVE",
+ "value": layer_uuid,
+ "show": True,
+ "style": "solid",
+ }
+ ]
+ dash_cfg = dashboard_config_for_charts(
+ dashboard_uuid,
+ "Dashboard Existing Layer Test",
+ [("CHART-1", chart_uuid, chart_cfg["slice_name"])],
+ )
+ contents = dashboard_import_bundle(
+ dash_cfg,
+ {"chart.yaml": chart_cfg},
+ {
+ "layer.yaml": dashboard_annotation_layer_config(
+ layer_uuid,
+ "incoming-layer-name",
+ [
+ {
+ "uuid": str(uuid4()),
+ "short_descr": "incoming-child",
+ "long_descr": "incoming",
+ "json_metadata": {"incoming": True},
+ }
+ ],
+ descr="incoming-descr",
+ )
+ },
+ )
+ try:
+ v1.ImportDashboardsCommand(contents, overwrite=True).run()
+
+ chart = db.session.query(Slice).filter_by(uuid=chart_uuid).one()
+ layer =
db.session.query(AnnotationLayer).filter_by(uuid=layer_uuid).one()
+ annotations = (
+ db.session.query(Annotation).filter_by(layer_id=layer.id).all()
+ )
+ assert layer.id == existing_layer_id
+ assert layer.name == "existing-layer"
+ assert layer.descr == "before"
+ assert len(annotations) == 1
+ assert annotations[0].short_descr == "existing-child"
+ assert json.loads(chart.params)["annotation_layers"][0]["value"]
== layer.id
+ finally:
+ _cleanup_dashboard_annotation_import(
+ [chart_uuid], [layer_uuid], dashboard_uuid
+ )
+
+ @patch("superset.utils.core.g")
+ @patch("superset.security.manager.g")
+ @patch("superset.commands.database.importers.v1.utils.add_permissions")
+ def test_import_dashboard_annotation_source_chart_on_other_database(
+ self, mock_add_permissions, sm_g, utils_g
+ ) -> None:
+ """Import an annotation source chart that isn't in the layout."""
+ sm_g.user = utils_g.user = security_manager.find_user("admin")
+ dashboard_uuid = str(uuid4())
+ main_chart_uuid = str(uuid4())
+ source_chart_uuid = str(uuid4())
+ source_database_config = {
+ **deepcopy(database_config),
+ "uuid": str(uuid4()),
+ "database_name": f"annotation_source_db_{uuid4().hex[:8]}",
+ "sqlalchemy_uri": "postgresql://user:pass@host2",
+ }
+ source_dataset_config = {
+ **deepcopy(dataset_config),
+ "uuid": str(uuid4()),
+ "table_name": "annotation_source_dataset",
+ "database_uuid": source_database_config["uuid"],
+ }
+ source_chart_cfg = dashboard_chart_config(
+ source_chart_uuid, "Annotation Source Chart"
+ )
+ source_chart_cfg["dataset_uuid"] = source_dataset_config["uuid"]
+ main_chart_cfg = dashboard_chart_config(main_chart_uuid, "Annotated
Chart")
+ main_chart_cfg["params"]["annotation_layers"] = [
+ {
+ "name": "Source",
+ "annotationType": "EVENT",
+ "sourceType": "table",
+ "value": source_chart_uuid,
+ }
+ ]
+ dash_cfg = dashboard_config_for_charts(
+ dashboard_uuid,
+ "Dashboard Cross Database Annotation",
+ [("CHART-1", main_chart_uuid, main_chart_cfg["slice_name"])],
+ )
+ contents = dashboard_import_bundle(
+ dash_cfg,
+ {"main_chart.yaml": main_chart_cfg, "source_chart.yaml":
source_chart_cfg},
+ )
+ contents["databases/source_database.yaml"] = yaml.safe_dump(
+ source_database_config
+ )
+ contents["datasets/source_dataset.yaml"] =
yaml.safe_dump(source_dataset_config)
+ try:
+ v1.ImportDashboardsCommand(contents, overwrite=True).run()
+
+ main_chart =
db.session.query(Slice).filter_by(uuid=main_chart_uuid).one()
+ source_chart = (
+ db.session.query(Slice).filter_by(uuid=source_chart_uuid).one()
+ )
+ assert source_chart.table.database.uuid == UUID(
+ source_database_config["uuid"]
+ )
+ assert [
+ layer["value"]
+ for layer in json.loads(main_chart.params)["annotation_layers"]
+ ] == [source_chart.id]
+ finally:
+ _cleanup_dashboard_annotation_import(
+ [main_chart_uuid, source_chart_uuid], [], dashboard_uuid
+ )
+ source_dataset = (
+ db.session.query(SqlaTable)
+ .filter_by(uuid=source_dataset_config["uuid"])
+ .one_or_none()
+ )
+ if source_dataset:
+ db.session.delete(source_dataset)
+ source_database = (
+ db.session.query(Database)
+ .filter_by(uuid=source_database_config["uuid"])
+ .one_or_none()
+ )
+ if source_database:
+ db.session.delete(source_database)
+ db.session.commit()
+
+ @patch("superset.utils.core.g")
+ @patch("superset.security.manager.g")
+ @patch("superset.commands.database.importers.v1.utils.add_permissions")
+ def test_import_dashboard_gamma_overwrite_keeps_existing_annotation_layer(
+ self, mock_add_permissions, sm_g, utils_g
+ ) -> None:
+ """Gamma's overwrite dashboard import reuses a layer without changing
it."""
+ sm_g.user = utils_g.user = security_manager.find_user("admin")
+ layer = AnnotationLayer(name=f"Gamma Dash Kept {uuid4()}",
descr="original")
+ db.session.add(layer)
+ db.session.flush()
+ db.session.add(Annotation(layer_id=layer.id,
short_descr="original-child"))
+ db.session.commit()
+ layer_uuid = str(layer.uuid)
+ layer_name = layer.name
+ dashboard_uuid = str(uuid4())
+ chart_uuid = str(uuid4())
+ chart_cfg = dashboard_chart_config(chart_uuid, "Gamma Dashboard Chart")
+ chart_cfg["params"]["annotation_layers"] = [
+ {
+ "name": "Native",
+ "annotationType": "EVENT",
+ "sourceType": "NATIVE",
+ "value": layer_uuid,
+ }
+ ]
+ dash_cfg = dashboard_config_for_charts(
+ dashboard_uuid,
+ "Gamma Dashboard",
+ [("CHART-1", chart_uuid, chart_cfg["slice_name"])],
+ )
+ contents = dashboard_import_bundle(
+ dash_cfg,
+ {"main_chart.yaml": chart_cfg},
+ {
+ "layer.yaml": dashboard_annotation_layer_config(
+ layer_uuid, "renamed-by-gamma", [], descr="changed"
+ )
+ },
+ )
+ try:
+ # the database and dataset have to exist: Gamma can't create them
+ ImportDatasetsCommand(
+ {
+ "metadata.yaml": yaml.safe_dump(dataset_metadata_config),
+ "databases/imported_database.yaml":
yaml.safe_dump(database_config),
+ "datasets/imported_dataset.yaml":
yaml.safe_dump(dataset_config),
+ },
+ overwrite=True,
+ ).run()
+
+ sm_g.user = utils_g.user = security_manager.find_user("gamma")
+ v1.ImportDashboardsCommand(
+ contents, overwrite=True, overwrite_all=True
+ ).run()
+
+ db.session.expire_all()
+ reloaded = (
+
db.session.query(AnnotationLayer).filter_by(uuid=layer_uuid).one()
+ )
+ children =
db.session.query(Annotation).filter_by(layer_id=reloaded.id)
+ assert reloaded.name == layer_name
+ assert reloaded.descr == "original"
+ assert [child.short_descr for child in children] ==
["original-child"]
Review Comment:
<!-- Bito Reply -->
The `/resolve` command is not supported on inline comments. Please use it in
a top-level comment instead.
##########
superset/commands/dashboard/export.py:
##########
@@ -441,7 +441,10 @@ def _export(
command.enable_tag_export()
if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"):
yield from ExportTagsCommand(
- dashboard_ids=dashboard_ids, chart_ids=chart_ids
+ dashboard_ids=dashboard_ids,
+
chart_ids=ExportChartsCommand.chart_ids_with_annotation_sources(
+ model.slices
+ ),
Review Comment:
<!-- Bito Reply -->
The suggestion to reuse the value computed in the command's execution is
valid and appropriate. Applying this change avoids redundant queries and JSON
parsing, which improves performance during dashboard exports when the tagging
system is enabled.
**superset/commands/dashboard/export.py**
```
yield from ExportTagsCommand(
dashboard_ids=dashboard_ids,
chart_ids=ExportChartsCommand.chart_ids_with_annotation_sources(
model.slices
),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
