EnxDev commented on code in PR #43232:
URL: https://github.com/apache/superset/pull/43232#discussion_r4164534559
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -287,3 +330,147 @@ def migrate_chart(config: dict[str, Any]) -> dict[str,
Any]:
output["query_context"] = json.dumps(query_context)
return output
+
+
+def topological_sort_charts(
+ chart_configs: list[dict[str, Any]],
+) -> list[dict[str, Any]]:
+ """Sort charts so that annotation dependencies are imported first.
+
+ Handles multi-level dependencies (A→B→C) by iteratively resolving
+ charts whose in-batch dependencies are already satisfied.
+
+ TODO: Add runtime circular annotation detection in
+ QueryContextProcessor.get_viz_annotation_data to prevent infinite
+ recursion when rendering charts with circular line annotations.
+ """
+ if len(chart_configs) <= 1:
+ return chart_configs
+
+ def _annotation_dependencies(chart_config: dict[str, Any]) -> set[str]:
+ refs = {
+ ann["value"]
+ for ann in chart_config.get("params", {}).get("annotation_layers",
[])
+ if ann.get("sourceType") in
ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+ and isinstance(ann.get("value"), str)
+ }
+ if query_context_raw := chart_config.get("query_context"):
+ try:
+ query_context = json.loads(query_context_raw)
+ except (json.JSONDecodeError, TypeError):
+ query_context = {}
+
+ for query in query_context.get("queries", []):
+ refs.update(
+ ann["value"]
+ for ann in query.get("annotation_layers", [])
+ if ann.get("sourceType")
+ in ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+ and isinstance(ann.get("value"), str)
+ )
+ refs.update(
+ ann["value"]
+ for ann in query_context.get("form_data", {}).get(
+ "annotation_layers", []
+ )
+ if ann.get("sourceType") in
ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+ and isinstance(ann.get("value"), str)
+ )
+ return refs
+
+ batch_uuids = {c["uuid"] for c in chart_configs}
+ sorted_refs: list[dict[str, Any]] = []
+ remaining = list(chart_configs)
+ resolved: set[str] = set()
+ while remaining:
+ next_remaining = []
+ for c in remaining:
+ unmet = _annotation_dependencies(c).intersection(batch_uuids -
resolved)
+ if not unmet:
+ sorted_refs.append(c)
+ resolved.add(c["uuid"])
+ else:
+ next_remaining.append(c)
+ if len(next_remaining) == len(remaining):
+ logger.warning(
+ "Circular annotation dependency detected for charts: %s — "
+ "these charts may have unresolved annotation references after
import.",
+ [c["uuid"] for c in next_remaining],
+ )
+ sorted_refs.extend(next_remaining)
+ break
+ remaining = next_remaining
+ return sorted_refs
+
+
+def _resolve_uuid_to_id(
+ uuid_value: str,
+ id_map: dict[str, int] | None,
+ model: type,
+) -> int | None:
+ """Resolve a UUID to a local integer ID using a map or DB fallback."""
+ if id_map and uuid_value in id_map:
+ return id_map[uuid_value]
+ try:
+ obj = db.session.query(model).filter_by(uuid=uuid_value).first()
+ except Exception: # noqa: BLE001 — malformed UUID raises at bind time
+ return None
+ return obj.id if obj else None
+
+
+def _resolve_annotation_list(
+ annotations: list[dict[str, Any]],
+ annotation_layer_ids: dict[str, int] | None,
+ chart_ids: dict[str, int] | None,
+) -> None:
+ """Resolve UUID values to integer IDs in-place for an annotation list."""
+ resolved_annotations: list[dict[str, Any]] = []
+ for annotation in annotations:
+ if annotation.get("annotationType") == AnnotationType.FORMULA:
+ resolved_annotations.append(annotation)
+ continue
+ source_type = annotation.get("sourceType")
+ value = annotation.get("value")
+ if isinstance(value, int):
+ resolved_annotations.append(annotation)
+ continue
+ if not isinstance(value, str):
+ continue
+ if source_type == "NATIVE":
+ layer_id = _resolve_uuid_to_id(value, annotation_layer_ids,
AnnotationLayer)
+ if layer_id is not None:
+ annotation["value"] = layer_id
+ resolved_annotations.append(annotation)
+ elif source_type in ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE:
+ ref_chart_id = _resolve_uuid_to_id(value, chart_ids, Slice)
+ if ref_chart_id is not None:
+ annotation["value"] = ref_chart_id
+ resolved_annotations.append(annotation)
+ annotations[:] = resolved_annotations
+
+
+def _resolve_query_context_annotations(
+ config: dict[str, Any],
+ annotation_layer_ids: dict[str, int] | None,
+ chart_ids: dict[str, int] | None,
+) -> None:
+ """Resolve annotation UUIDs to IDs in query_context (in-place)."""
+ if not config.get("query_context"):
+ return
+ try:
+ query_context = json.loads(config["query_context"])
+ for query in query_context.get("queries", []):
+ _resolve_annotation_list(
+ query.get("annotation_layers", []),
+ annotation_layer_ids,
+ chart_ids,
+ )
+ form_data = query_context.get("form_data", {})
+ _resolve_annotation_list(
+ form_data.get("annotation_layers", []),
+ annotation_layer_ids,
+ chart_ids,
+ )
+ config["query_context"] = json.dumps(query_context)
+ except json.JSONDecodeError:
+ pass
Review Comment:
Yes, malformed query_context is skipped through `_load_query_context` and
the shape checks. Resolving.
##########
superset/commands/annotation_layer/importers/v1/utils.py:
##########
@@ -0,0 +1,71 @@
+# 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.
+
+from typing import Any
+
+from marshmallow import fields
+
+from superset import db, security_manager
+from superset.commands.exceptions import ImportFailedError
+from superset.models.annotations import AnnotationLayer
+from superset.utils import json
+
+DATETIME_FIELD = fields.DateTime(allow_none=True)
+
+
+def import_annotation_layer(
+ config: dict[str, Any],
+ overwrite: bool = False,
+ ignore_permissions: bool = False,
+) -> AnnotationLayer:
+ """Upsert annotation layer config and return persisted layer.
+
+ If an existing layer is found and overwrite is False, return it unchanged.
+ """
+ can_write = ignore_permissions or security_manager.can_access(
+ "can_write", "Annotation"
+ )
+ if not can_write:
+ raise ImportFailedError(
+ "Annotation layer import requires can_write permission on
Annotation"
+ )
Review Comment:
Yes, the check only applies when a layer would be created or overwritten,
and the Gamma tests cover it through the chart and dashboard importers.
Resolving.
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -119,6 +127,23 @@ def _import(
if config.get("theme_uuid"):
theme_uuids.add(config["theme_uuid"])
+ # discover charts used as annotation sources by those charts, which can
+ # be in the bundle without being in any dashboard layout
+ chart_configs_by_uuid = {
+ str(config["uuid"]): config
+ for file_name, config in configs.items()
+ if file_name.startswith("charts/")
+ }
+ pending_chart_uuids = list(chart_uuids)
Review Comment:
Annotations are in. Resolving.
##########
superset/migrations/versions/2026-08-17_18-38_884a2115ebd3_add_uuid_to_annotation_layer_and_annotation.py:
##########
@@ -0,0 +1,100 @@
+# 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.
+"""add_uuid_to_annotation_layer_and_annotation
+
+Revision ID: 884a2115ebd3
+Revises: 95d8a99c822e
+Create Date: 2026-08-11 18:38:15.412396
+
+"""
+
+from uuid import uuid4
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+from superset.migrations.shared.utils import (
+ add_columns,
+ assign_uuids,
+ drop_columns,
+ has_table,
+)
+
+# revision identifiers, used by Alembic.
+revision = "884a2115ebd3"
+down_revision = "95d8a99c822e"
+
+Base = declarative_base()
+
+
+class ImportMixin:
+ id = sa.Column(sa.Integer, primary_key=True)
+ uuid = sa.Column(UUIDType(binary=True), primary_key=False, default=uuid4)
+
+
+class AnnotationLayer(ImportMixin, Base):
+ __tablename__ = "annotation_layer"
+
+
+class Annotation(ImportMixin, Base):
+ __tablename__ = "annotation"
+
+
+MODELS = (AnnotationLayer, Annotation)
+
+
+def _has_unique_constraint(table_name: str, constraint_name: str) -> bool:
+ inspector = sa.inspect(op.get_bind())
+ return any(
+ constraint["name"] == constraint_name
+ for constraint in inspector.get_unique_constraints(table_name)
+ )
+
+
+def upgrade() -> None:
+ session = db.Session(bind=op.get_bind())
+
+ for model in MODELS:
+ table_name = model.__tablename__
+ if not has_table(table_name):
+ continue
+ add_columns(
+ table_name,
+ sa.Column("uuid", UUIDType(binary=True), primary_key=False,
default=uuid4),
+ )
+ if session.query(model).filter(model.uuid.is_(None)).count():
+ assign_uuids(model, session)
Review Comment:
Small correction on the snippet: the migration goes through
`_backfill_uuids`, which uses `assign_uuids` only when every row is empty and
otherwise fills the empty rows through `paginated_update` (1becc18). Existing
UUIDs are kept either way. Resolving.
##########
superset/commands/chart/export.py:
##########
@@ -100,28 +190,53 @@ def enable_tag_export(cls) -> None:
def run(
self, seen: set[str] | None = None
) -> Iterator[tuple[str, Callable[[], str]]]:
+ is_root = seen is None
yield from super().run(seen=seen)
+ if not self.export_related:
+ return
+
+ chart_ids =
ExportChartsCommand.chart_ids_with_annotation_sources(self._models)
Review Comment:
The walk is gated on what consumes it: nested exports that write neither
`selected_charts.yaml` nor `tags.yaml` skip it. Resolving.
--
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]
