bito-code-review[bot] commented on code in PR #43232:
URL: https://github.com/apache/superset/pull/43232#discussion_r4068714444
##########
superset/utils/core.py:
##########
@@ -211,6 +211,18 @@ class AnnotationType(StrEnum):
TIME_SERIES = "TIME_SERIES"
+# Annotation source types whose ``value`` field references another Chart
+# (resolved to a local Slice.id on import / serialised back to UUID on export).
+# Add new chart-referencing source types here; all consumers pick them up
+# automatically via this single definition.
+ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE: frozenset[str] = frozenset(
+ {
+ "table",
+ "line",
+ }
+)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>duplicated source-type list</b></div>
<div id="fix">
The new constant's docstring claims "all consumers pick them up
automatically via this single definition", but
`QueryContextProcessor._get_annotation_rls_cache_key`
(query_context_processor.py:461) still hardcodes `("line", "table")`. If a new
chart-referencing source type is added here, that RLS cache-key path silently
diverges. Use the constant there too.
</div>
</div>
<small><i>Code Review Run #fb83a9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/commands/annotation_layer/importers/v1/__init__.py:
##########
@@ -0,0 +1,55 @@
+# 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, Optional
+
+from marshmallow import Schema
+from sqlalchemy.orm import Session # noqa: F401
+
+from superset.annotation_layers.schemas import ImportV1AnnotationLayerSchema
+from superset.commands.annotation_layer.exceptions import
AnnotationLayerImportError
+from superset.commands.annotation_layer.importers.v1.utils import (
+ import_annotation_layer,
+)
+from superset.commands.importers.v1 import ImportModelsCommand
+from superset.daos.annotation_layer import AnnotationLayerDAO
+
+
+class ImportAnnotationLayersCommand(ImportModelsCommand):
+ """Import Annotation Layers"""
+
+ dao = AnnotationLayerDAO
+ model_name = "annotation_layer"
+ prefix = "annotation_layers/"
+ schemas: dict[str, Schema] = {
+ "annotation_layers/": ImportV1AnnotationLayerSchema(),
+ }
+ import_error = AnnotationLayerImportError
+
+ @staticmethod
+ def _import(
+ configs: dict[str, Any],
+ overwrite: bool = False,
+ contents: Optional[dict[str, Any]] = None,
+ ) -> None:
+ """
+ Import annotation layer configs; pass overwrite to each import call;
+ contents is accepted but not used.
+ """
+
+ for file_name, config in configs.items():
+ if file_name.startswith("annotation_layers/"):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Hardcoded prefix literal</b></div>
<div id="fix">
`_import` hardcodes "annotation_layers/" while the class already defines
`prefix` (line 36) and keys `schemas` with it (line 38); base
`ImportModelsCommand._prevent_overwrite_existing_model` filters on
`self.prefix`. If `prefix` ever changes, validation/overwrite checks and the
import filter diverge silently. Prefer `cls.prefix` via `classmethod`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
@classmethod
def _import(
cls,
configs: dict[str, Any],
overwrite: bool = False,
contents: Optional[dict[str, Any]] = None,
) -> None:
"""
Import annotation layer configs; pass overwrite to each import call;
contents is accepted but not used.
"""
for file_name, config in configs.items():
if file_name.startswith(cls.prefix):
````
</div>
</details>
</div>
<small><i>Code Review Run #fb83a9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/commands/annotation_layer/importers/v1/__init__.py:
##########
@@ -0,0 +1,55 @@
+# 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, Optional
+
+from marshmallow import Schema
+from sqlalchemy.orm import Session # noqa: F401
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unused Session import</b></div>
<div id="fix">
This import is never referenced — `Session` appears only on this line in the
module. The `# noqa: F401` suppresses the linter that would otherwise flag it;
sibling importers (`dashboard`, `database`, `dataset`, `query`) carry the same
copy-paste artifact, but new files should not replicate it. Removing the line
drops a dead module-level import.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
````
</div>
</details>
</div>
<small><i>Code Review Run #fb83a9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/commands/importers/v1/utils.py:
##########
@@ -192,6 +192,7 @@ def load_configs(
if not content:
continue
+ config: Any = None
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped Any annotation</b></div>
<div id="fix">
`config` is assigned either `None` or the `dict[str, Any]` returned by
`load_yaml` (line 200), so annotating it `Any` discards static checking for
every later `config.get(...)`/`config[...]` access and the `configs[file_name]
= config` store. AGENTS.md requires full typing on new code; annotate
`dict[str, Any] | None` so mypy checks the schema branch and the
`isinstance(config, dict)` guard.
</div>
</div>
<small><i>Code Review Run #fb83a9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Silent DB error swallow</b></div>
<div id="fix">
`except Exception` converts every failure of the `db.session.query` fallback
— DB outage, `OperationalError`, programming errors — into 'unresolvable',
silently dropping the annotation with no log. The comment scopes the intent to
malformed-UUID bind errors, but the catch is unbounded. Log the failure (or
narrow to the driver-specific bind error) so silent data loss is diagnosable.
</div>
</div>
<small><i>Code Review Run #fb83a9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Unhandled parse errors in resolver</b></div>
<div id="fix">
`_resolve_query_context_annotations` catches only `json.JSONDecodeError`,
but bundle content that is valid JSON of the wrong shape (query_context as a
list, `queries` as a string) raises `TypeError`/`AttributeError` at lines
461-467 and crashes the import. Conversely a real `JSONDecodeError` is
swallowed by `pass` with no log, leaving unresolved UUIDs that fail later in
`get_viz_annotation_data`. Sibling `_annotation_dependencies` (line 360) also
catches `TypeError`.
</div>
</div>
<small><i>Code Review Run #fb83a9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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):
+ resolved_annotations.append(annotation)
+ elif source_type == "NATIVE" and isinstance(value, str):
+ 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 and
isinstance(
+ value, int
+ ):
+ resolved_annotations.append(annotation)
+ elif source_type in ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE and
isinstance(
+ value, str
+ ):
+ 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)
+ params["annotation_layers"] = resolved_annotations
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated resolution logic drift</b></div>
<div id="fix">
`filter_chart_annotations` (lines 58-84) reimplements
`_resolve_annotation_list` (lines 428-449) with divergent semantics: an int
`value` with an unknown `sourceType` is dropped here (falls through all
`elif`s) but kept by `_resolve_annotation_list` at line 434. Delegate the
params path to `_resolve_annotation_list` so the two resolution policies cannot
drift.
</div>
</div>
<small><i>Code Review Run #fb83a9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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", []):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Crash on malformed query_context</b></div>
<div id="fix">
`_annotation_dependencies` guards only `json.loads` (line 360); the
traversal then assumes `query_context` is a dict with list-valued `queries` and
`form_data.annotation_layers`. Valid-JSON bundle content like `"[]"` or
`{"queries": 5}` raises AttributeError/TypeError outside the try, crashing
`topological_sort_charts` during import. Extend the guard to cover the
traversal, mirroring the fix in `_resolve_query_context_annotations`.
</div>
</div>
<small><i>Code Review Run #fb83a9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
