bito-code-review[bot] commented on code in PR #43232: URL: https://github.com/apache/superset/pull/43232#discussion_r4142910640
########## 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: <!-- Bito Reply --> Understood. Since the `Session` import is no longer used in that module, the suggestion to remove it is correct and appropriate to apply. I have noted your feedback regarding this. ########## 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: <!-- Bito Reply --> The suggestion to delegate the logic to `_resolve_annotation_list` is valid and improves the code by centralizing the resolution policy, which prevents the drift identified by the reviewer. Applying this change ensures that both paths handle annotation resolution consistently, including the treatment of integer values with unknown source types. **superset/commands/chart/importers/v1/utils.py** ``` params = chart_config.get("params", {}) annotation_layers = params.get("annotation_layers", []) params["annotation_layers"] = _resolve_annotation_list( annotation_layers, annotation_layer_ids, chart_ids ) ``` ########## 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: <!-- Bito Reply --> The suggestion to extend the guard in `_annotation_dependencies` is appropriate. By wrapping the subsequent traversal in the same `try-except` block, you prevent potential `AttributeError` or `TypeError` exceptions when `query_context` is valid JSON but does not match the expected dictionary structure, ensuring the import process remains robust. **superset/commands/chart/importers/v1/utils.py** ``` if query_context_raw := chart_config.get("query_context"): try: query_context = json.loads(query_context_raw) for query in query_context.get("queries", []): # ... traversal logic ... except (json.JSONDecodeError, TypeError, AttributeError): query_context = {} ``` ########## 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: <!-- Bito Reply --> The update correctly addresses the concern by narrowing the scope of the exception handling. By catching only the `ValueError` during UUID parsing and allowing database-related exceptions to surface, the code now ensures that critical failures are not silently swallowed, improving diagnosability. **superset/commands/chart/importers/v1/utils.py** ``` try: obj = db.session.query(model).filter_by(uuid=uuid_value).first() ``` -- 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]
