EnxDev commented on code in PR #43232:
URL: https://github.com/apache/superset/pull/43232#discussion_r4131081321
##########
superset/commands/chart/export.py:
##########
@@ -84,9 +88,57 @@ def _file_content(model: Slice) -> str:
if extra_fields := get_extra_export_fields(model, "chart"):
payload["extra"] = extra_fields
+ # Replace annotation layer/chart integer IDs with UUIDs for portability
+ if isinstance(payload.get("params"), dict):
+ ExportChartsCommand._replace_annotation_layer_uuids(
+ payload["params"].get("annotation_layers", [])
+ )
+
+ # Also replace annotation IDs with UUIDs in query_context
+ if payload.get("query_context"):
+ try:
+ query_context = json.loads(payload["query_context"])
+ for query in query_context.get("queries", []):
+ ExportChartsCommand._replace_annotation_layer_uuids(
+ query.get("annotation_layers", [])
+ )
+ form_data = query_context.get("form_data", {})
+ ExportChartsCommand._replace_annotation_layer_uuids(
+ form_data.get("annotation_layers", [])
+ )
+ payload["query_context"] = json.dumps(query_context)
+ except json.JSONDecodeError:
+ logger.info(
+ "Unable to decode `query_context` field: %s",
+ payload["query_context"],
+ )
+
file_content = yaml.safe_dump(payload, sort_keys=False,
allow_unicode=True)
return file_content
+ @staticmethod
+ def _replace_annotation_layer_uuids(
+ annotation_layers: list[dict], # type: ignore[type-arg]
+ ) -> None:
+ """Replace integer IDs in annotation_layers with UUIDs for
portability."""
+ for layer in annotation_layers:
+ source_type = layer.get("sourceType")
+ value = layer.get("value")
+ if not isinstance(value, int):
+ continue
+ if source_type == "NATIVE":
+ ann_layer = (
+
db.session.query(AnnotationLayer).filter_by(id=value).first()
+ )
+ if not ann_layer:
+ raise AnnotationLayerNotFoundError()
Review Comment:
Annotation layers can be deleted while charts still point at them (the
delete command only checks for child annotations), so a stale NATIVE reference
raises here and the whole chart export 404s, along with any dashboard export
that contains the chart. On master that export works.
Could we drop the dangling entry with a warning instead of failing the
bundle? Same for the chart branch at 139.
##########
superset/commands/chart/export.py:
##########
@@ -130,3 +194,57 @@ def _export(
if model.table and export_related:
# Pass the shared seen set to the dataset export command
yield from ExportDatasetsCommand([model.table.id]).run(seen=seen)
+
+ if export_related:
+ yield from ExportChartsCommand._export_annotation_layers(
+ model, seen=seen, _chart_seen=_chart_seen
+ )
+
+ @staticmethod
+ def _export_annotation_layers(
+ model: Slice,
+ seen: set[str],
+ _chart_seen: set[int],
+ ) -> Iterator[tuple[str, Callable[[], str]]]:
+ """Export annotation layers/charts referenced by ``model``'s params."""
+ try:
+ model_params = json.loads(model.params or "{}")
+ except json.JSONDecodeError:
+ model_params = {}
+ annotation_layers = model_params.get("annotation_layers", [])
+ if not annotation_layers:
+ return
+
+ # Export charts referenced as annotation sources (table/line
sourceType)
+ chart_annotation_ids = [
+ layer["value"]
+ for layer in annotation_layers
+ if layer.get("sourceType") in
ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+ and isinstance(layer.get("value"), int)
+ ]
+ if chart_annotation_ids:
+ ref_charts = ChartDAO.find_by_ids(chart_annotation_ids)
+ found_ids = {c.id for c in ref_charts}
+ missing_ids = set(chart_annotation_ids) - found_ids
+ if missing_ids:
+ raise ChartNotFoundError()
Review Comment:
`find_by_ids` applies `ChartFilter`, so if the annotation source chart is
one the exporting user can't see (or it's soft-deleted), this raises and they
can't export a dashboard they otherwise have full access to.
I'd skip the unresolvable ones here too. A test with a source chart owned by
another user would cover it.
##########
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"
+ )
+
+ existing =
db.session.query(AnnotationLayer).filter_by(uuid=config["uuid"]).first()
+ if existing:
+ if not overwrite:
+ return existing
+ config["id"] = existing.id
Review Comment:
The gate runs before the existence lookup, so a Gamma user importing a chart
bundle fails outright whenever it carries an `annotation_layers/` file, even
when that layer already exists and nothing would be written. Every chart export
with a NATIVE annotation now includes one, and on master the same import
succeeded (the annotation was just dropped).
Could we mirror `import_chart` / `import_dataset` and only raise when we'd
create a layer? A Gamma test importing a chart bundle whose layer already
exists would pin it.
```suggestion
can_write = ignore_permissions or security_manager.can_access(
"can_write", "Annotation"
)
existing =
db.session.query(AnnotationLayer).filter_by(uuid=config["uuid"]).first()
if existing:
if not overwrite or not can_write:
return existing
config["id"] = existing.id
elif not can_write:
raise ImportFailedError(
"Annotation layer import requires can_write permission on
Annotation"
)
```
##########
superset/migrations/versions/2026-08-17_18-38_884a2115ebd3_add_uuid_to_annotation_layer_and_annotation.py:
##########
@@ -0,0 +1,126 @@
+# 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.exc import OperationalError
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy_utils import UUIDType
+
+from superset import db
+
+# 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"
+
+
+def _assign_missing_uuids(model: type[ImportMixin], session: sa.orm.Session)
-> None:
Review Comment:
`superset.migrations.shared.utils` already covers this with `add_columns`
and `assign_uuids`. The latter does a single dialect-native UPDATE on Postgres
and MySQL, while this loop pulls every `annotation` row through the ORM in one
commit.
The `except OperationalError: pass` guards also miss a duplicate column on
Postgres, which raises `ProgrammingError`. Could we switch to the helpers and
fill in the runtime estimate in the template?
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -168,13 +176,21 @@ def _import(
"datasource_name": dataset.table_name,
}
+ # import annotation layers before charts so UUID→ID maps are ready
+ annotation_layer_ids: dict[str, int] = {}
+ for file_name, config in configs.items():
+ if file_name.startswith("annotation_layers/"):
+ layer = import_annotation_layer(config,
overwrite=overwrite_assets)
Review Comment:
`ImportAssetsCommand` (`superset/commands/importers/v1/assets.py:67`) has no
`annotation_layers/` schema and doesn't pass `chart_ids` to `import_chart`.
Asset exports go through `ExportChartsCommand`, so those bundles will now carry
layer files that the assets import silently ignores, and NATIVE annotations
still get dropped on a fresh instance.
Fine as a follow-up if you'd rather keep this scoped, but worth a line in
the description.
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -28,25 +29,59 @@
from superset.extensions import feature_flag_manager
from superset.migrations.shared.migrate_viz import processors
from superset.migrations.shared.migrate_viz.base import MigrateViz
+from superset.models.annotations import AnnotationLayer
from superset.models.slice import Slice
from superset.subjects.models import Subject
from superset.utils import json
-from superset.utils.core import AnnotationType, get_user
+from superset.utils.core import (
+ ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE,
+ AnnotationType,
+ get_user,
+)
+
+logger = logging.getLogger(__name__)
-def filter_chart_annotations(chart_config: dict[str, Any]) -> None:
+def filter_chart_annotations(
+ chart_config: dict[str, Any],
+ annotation_layer_ids: dict[str, int] | None = None,
+ chart_ids: dict[str, int] | None = None,
+) -> None:
"""
- Mutating the chart's config params to keep only the annotations of
- type FORMULA.
- TODO:
- handle annotation dependencies on either other charts or
- annotation layers objects.
+ Resolve annotation references from exported UUIDs to local integer IDs.
+ - FORMULA: kept unchanged (no DB reference)
+ - NATIVE: UUID resolved to AnnotationLayer.id
+ - table/line: UUID resolved to referenced Chart.id
+ Annotations whose references cannot be resolved are dropped.
"""
params = chart_config.get("params", {})
- als = params.get("annotation_layers", [])
- params["annotation_layers"] = [
- al for al in als if al.get("annotationType") == AnnotationType.FORMULA
- ]
+ annotation_layers = params.get("annotation_layers", [])
+ resolved_annotations: list[dict[str, Any]] = []
+ for annotation in annotation_layers:
+ source_type = annotation.get("sourceType")
+ value = annotation.get("value")
+
+ if annotation.get("annotationType") == AnnotationType.FORMULA:
+ resolved_annotations.append(annotation)
+ elif source_type == "NATIVE" and isinstance(value, int):
Review Comment:
Bundles exported before this change carry raw integer IDs, and this branch
(plus the int branch at 434 for `query_context`) keeps them as is. Imported
into another instance, the annotation binds to whatever local layer or chart
happens to have that ID, so users see unrelated annotations on the chart. On
master these were dropped.
Since new exports always write UUIDs, can we keep dropping integer values on
import? A legacy int bundle test would guard it.
--
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]
