EnxDev commented on code in PR #43232:
URL: https://github.com/apache/superset/pull/43232#discussion_r4165496365
##########
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:
Good point, and it could already happen with a cycle built by hand in the
UI. Fixed at runtime in 7f4d02b: `get_viz_annotation_data` only uses the source
chart's rows, so it now clears that chart's own `annotation_layers` before
running its query. A -> B -> A no longer recurses, and the import keeps both
edges. There's a unit test for it in `test_query_context_processor.py`, and the
TODO is gone.
##########
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:
Right. In 7f4d02b the backfill no longer goes through `paginated_update`. On
Postgres and MySQL a fresh column is still filled by `assign_uuids` with a
single UPDATE. Every other case updates only the empty rows, in batches on the
migration's own connection, with no commits. Checked on SQLite for both a fresh
column and one an earlier run left partly filled.
##########
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:
Added in 7f4d02b. Each dropped annotation logs a warning with the chart
UUID, the annotation name and the reason: an integer reference from an older
export, a layer or source chart that's neither in the bundle nor on the
instance, or an unknown source type. The second pass for circular references
warns too.
--
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]
