Copilot commented on code in PR #43232:
URL: https://github.com/apache/superset/pull/43232#discussion_r4164740650
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -287,3 +323,271 @@ def migrate_chart(config: dict[str, Any]) -> dict[str,
Any]:
output["query_context"] = json.dumps(query_context)
return output
+
+
+def _load_query_context(raw: Any) -> Any:
+ try:
+ return json.loads(raw) if raw else None
+ except (json.JSONDecodeError, TypeError):
+ return None
+
+
+def get_chart_annotation_dependencies(chart_config: dict[str, Any]) ->
set[str]:
+ """Return UUIDs of charts referenced as annotation sources by a chart
config."""
+ query_context = _load_query_context(chart_config.get("query_context"))
+ return {
+ annotation["value"]
+ for annotation_layers in get_annotation_layer_lists(
+ chart_config.get("params"), query_context
+ )
+ for annotation in annotation_layers
+ if annotation.get("sourceType") in
ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+ and isinstance(annotation.get("value"), str)
+ }
+
+
+def get_dependency_chart_uuids(
+ contents: dict[str, Any],
+ chart_configs: list[dict[str, Any]],
+) -> set[str]:
+ """
+ Return UUIDs of bundled charts that weren't picked for the export, i.e. the
+ ones only there as annotation sources.
+
+ The picked charts come from ``SELECTED_CHARTS_FILE_NAME``. A bundle without
+ it (or with an unreadable one) treats every chart as picked.
+ """
+ raw_selection = contents.get(SELECTED_CHARTS_FILE_NAME)
+ if not raw_selection:
+ return set()
+ try:
+ selection = yaml.safe_load(raw_selection)
+ except yaml.YAMLError:
+ logger.warning("Ignoring unreadable %s", SELECTED_CHARTS_FILE_NAME)
+ return set()
+ selected = (
+ selection.get(SELECTED_CHARTS_KEY) if isinstance(selection, dict) else
None
+ )
+ if not isinstance(selected, list):
+ logger.warning("Ignoring malformed %s", SELECTED_CHARTS_FILE_NAME)
+ return set()
+ return {str(config["uuid"]) for config in chart_configs} - {
+ str(chart_uuid) for chart_uuid in selected
+ }
+
+
+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. Charts in a
+ cycle are appended in their original order; ``import_charts`` resolves
+ their references in a second pass.
+
+ TODO: Add runtime circular annotation detection in
+ QueryContextProcessor.get_viz_annotation_data to prevent infinite
+ recursion when rendering charts with circular line annotations.
Review Comment:
The importer deliberately preserves cyclic chart annotations, but rendering
either chart recursively invokes `ChartDataCommand` for the other through
`get_viz_annotation_data`, so an A→B→A cycle can recurse until the request
fails or exhausts resources. The TODO acknowledges this failure mode; cycle
detection must be implemented in the runtime path before imports are allowed to
retain both edges (or one cyclic edge should be dropped).
##########
superset/migrations/versions/2026-08-17_18-38_884a2115ebd3_add_uuid_to_annotation_layer_and_annotation.py:
##########
@@ -0,0 +1,121 @@
+# 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,
+ paginated_update,
+)
+
+# 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 _backfill_uuids(model: type[ImportMixin], session: sa.orm.Session) -> None:
+ """
+ Give every row without a UUID a new one, leaving existing UUIDs alone.
+
+ On a freshly added column every row is empty, so ``assign_uuids`` fills the
+ whole table (a single UPDATE on Postgres and MySQL). If an earlier run
+ stopped partway, only the rows still missing a UUID are filled, so UUIDs
+ that bundles may already reference are kept.
+ """
+ missing = session.query(model).filter(model.uuid.is_(None))
+ missing_count = missing.count()
+ if not missing_count:
+ return
+ if missing_count == session.query(model).count():
+ assign_uuids(model, session)
+ return
+ for obj in paginated_update(missing):
+ obj.uuid = uuid4()
Review Comment:
`paginated_update` commits after every batch
(`superset/migrations/shared/utils.py:170-180`). On the fallback dialect path,
a later backfill or unique-constraint failure therefore leaves the UUID column
and earlier batches committed, contradicting the PR's claim that this migration
is atomic and SIP-59's rollback requirement. Backfill without committing inside
the Alembic transaction, or extend the helper with a no-commit mode.
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -287,3 +323,271 @@ def migrate_chart(config: dict[str, Any]) -> dict[str,
Any]:
output["query_context"] = json.dumps(query_context)
return output
+
+
+def _load_query_context(raw: Any) -> Any:
+ try:
+ return json.loads(raw) if raw else None
+ except (json.JSONDecodeError, TypeError):
+ return None
+
+
+def get_chart_annotation_dependencies(chart_config: dict[str, Any]) ->
set[str]:
+ """Return UUIDs of charts referenced as annotation sources by a chart
config."""
+ query_context = _load_query_context(chart_config.get("query_context"))
+ return {
+ annotation["value"]
+ for annotation_layers in get_annotation_layer_lists(
+ chart_config.get("params"), query_context
+ )
+ for annotation in annotation_layers
+ if annotation.get("sourceType") in
ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+ and isinstance(annotation.get("value"), str)
+ }
+
+
+def get_dependency_chart_uuids(
+ contents: dict[str, Any],
+ chart_configs: list[dict[str, Any]],
+) -> set[str]:
+ """
+ Return UUIDs of bundled charts that weren't picked for the export, i.e. the
+ ones only there as annotation sources.
+
+ The picked charts come from ``SELECTED_CHARTS_FILE_NAME``. A bundle without
+ it (or with an unreadable one) treats every chart as picked.
+ """
+ raw_selection = contents.get(SELECTED_CHARTS_FILE_NAME)
+ if not raw_selection:
+ return set()
+ try:
+ selection = yaml.safe_load(raw_selection)
+ except yaml.YAMLError:
+ logger.warning("Ignoring unreadable %s", SELECTED_CHARTS_FILE_NAME)
+ return set()
+ selected = (
+ selection.get(SELECTED_CHARTS_KEY) if isinstance(selection, dict) else
None
+ )
+ if not isinstance(selected, list):
+ logger.warning("Ignoring malformed %s", SELECTED_CHARTS_FILE_NAME)
+ return set()
+ return {str(config["uuid"]) for config in chart_configs} - {
+ str(chart_uuid) for chart_uuid in selected
+ }
+
+
+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. Charts in a
+ cycle are appended in their original order; ``import_charts`` resolves
+ their references in a second pass.
+
+ 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
+
+ batch_uuids = {str(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 = get_chart_annotation_dependencies(c).intersection(
+ batch_uuids - resolved
+ )
+ if not unmet:
+ sorted_refs.append(c)
+ resolved.add(str(c["uuid"]))
+ else:
+ next_remaining.append(c)
+ if len(next_remaining) == len(remaining):
+ logger.info(
+ "Circular annotation dependency detected for charts: %s; "
+ "their references are resolved after all charts are imported.",
+ [c["uuid"] for c in next_remaining],
+ )
+ sorted_refs.extend(next_remaining)
+ break
+ remaining = next_remaining
+ return sorted_refs
+
+
+def import_charts(
+ chart_configs: list[dict[str, Any]],
+ overwrite: bool = False,
+ default_viewers: list[Subject] | None = None,
+ annotation_layer_ids: dict[str, int] | None = None,
+ chart_ids: dict[str, int] | None = None,
+ dependency_chart_uuids: set[str] | None = None,
+) -> list[tuple[dict[str, Any], Slice]]:
+ """
+ Import chart configs in annotation-dependency order.
+
+ Charts in ``dependency_chart_uuids`` are imported with ``overwrite=False``:
+ an existing one is reused unchanged, like a dataset dependency.
+
+ Charts that reference another chart of the bundle as an annotation source
+ are imported after it. When the references form a cycle, the charts
imported
+ first keep the UUIDs of the later ones and get them resolved in a second
+ pass, so both sides of the cycle keep their annotations.
+
+ ``chart_ids`` is updated in place with the UUID-to-ID mapping of every
+ imported chart. Returns ``(config, chart)`` pairs in import order.
+ """
+ chart_ids = {} if chart_ids is None else chart_ids
+ sorted_configs = topological_sort_charts(chart_configs)
+ pending_chart_uuids = {str(config["uuid"]) for config in sorted_configs}
+ imported: list[tuple[dict[str, Any], Slice]] = []
+ deferred: list[Slice] = []
+ for config in sorted_configs:
+ has_pending_refs = bool(
+ get_chart_annotation_dependencies(config) & pending_chart_uuids
+ )
+ is_dependency = str(config["uuid"]) in (dependency_chart_uuids or
set())
+ chart = import_chart(
+ config,
+ overwrite=overwrite and not is_dependency,
+ default_viewers=default_viewers,
+ annotation_layer_ids=annotation_layer_ids,
+ chart_ids=chart_ids,
+ pending_chart_uuids=pending_chart_uuids,
+ )
+ chart_ids[str(chart.uuid)] = chart.id
+ pending_chart_uuids.discard(str(config["uuid"]))
+ imported.append((config, chart))
+ if has_pending_refs:
+ deferred.append(chart)
+
+ for chart in deferred:
+ resolve_deferred_chart_annotations(chart, chart_ids)
+
+ return imported
+
+
+def resolve_deferred_chart_annotations(chart: Slice, chart_ids: dict[str,
int]) -> None:
+ """
+ Resolve chart-source annotation references still stored as UUIDs on an
+ imported chart, dropping the ones that are not in ``chart_ids``.
+
+ Charts returned unchanged by ``import_chart`` only hold integer IDs, so
+ this is a no-op for them.
+ """
+ try:
+ params = json.loads(chart.params or "{}")
+ except json.JSONDecodeError:
+ params = None
+ query_context = _load_query_context(chart.query_context)
+
+ if _resolve_deferred_annotation_lists(
+ get_annotation_layer_lists(params, None), chart_ids
+ ):
+ chart.params = json.dumps(params)
+ if _resolve_deferred_annotation_lists(
+ get_annotation_layer_lists(None, query_context), chart_ids
+ ):
+ chart.query_context = json.dumps(query_context)
+
+
+def _resolve_deferred_annotation_lists(
+ annotation_lists: list[list[dict[str, Any]]],
+ chart_ids: dict[str, int],
+) -> bool:
+ """Rewrite chart-source UUIDs in place; return whether anything changed."""
+ changed = False
+ for annotation_layers in annotation_lists:
+ resolved: list[dict[str, Any]] = []
+ for annotation in annotation_layers:
+ value = annotation.get("value")
+ if annotation.get(
+ "sourceType"
+ ) in ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE and isinstance(
+ value, str
+ ):
+ changed = True
+ if value not in chart_ids:
+ continue
+ annotation["value"] = chart_ids[value]
+ resolved.append(annotation)
+ annotation_layers[:] = resolved
+ return changed
+
+
+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:
+ parsed_uuid = UUID(uuid_value)
+ except ValueError:
+ return None
+ obj = db.session.query(model).filter_by(uuid=parsed_uuid).first()
+ 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,
+ pending_chart_uuids: set[str] | None = None,
+) -> None:
+ """
+ Resolve UUID values to integer IDs in-place for an annotation list.
+
+ - FORMULA: kept unchanged (no DB reference)
+ - NATIVE: UUID resolved to AnnotationLayer.id
+ - table/line: UUID resolved to the referenced Chart.id; UUIDs of charts
+ still pending in the same bundle are kept for a later pass
+ - anything else is dropped, including integer IDs from bundles exported
+ before UUIDs were written, since those IDs belong to another instance
+ """
+ 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 not isinstance(value, str):
+ continue
Review Comment:
Legacy integer references are silently discarded here. The PR description
promises that unresolved and legacy references are dropped *with a warning*,
but none of the import resolution branches log the loss, leaving operators
unaware that imported charts changed. Emit a warning for each dropped reference
(ideally with chart/source context), including the unresolved native and chart
UUID branches below.
--
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]
