codeant-ai-for-open-source[bot] commented on code in PR #39857:
URL: https://github.com/apache/superset/pull/39857#discussion_r3649829917
##########
superset/annotation_layers/schemas.py:
##########
@@ -64,3 +64,24 @@ class AnnotationLayerPutSchema(Schema):
descr = fields.String(
metadata={"description": annotation_layer_descr}, required=False
)
+
+
+class ImportV1AnnotationSchema(Schema):
+ """Schema for validating individual annotations within an exported
layer."""
+
+ start_dttm = fields.DateTime(allow_none=True)
+ end_dttm = fields.DateTime(allow_none=True)
+ short_descr = fields.String(allow_none=True, validate=Length(0, 500))
+ long_descr = fields.String(allow_none=True)
+ json_metadata = fields.Dict(allow_none=True)
+ uuid = fields.UUID(required=True)
+
+
+class ImportV1AnnotationLayerSchema(Schema):
+ """Schema for importing V1 annotation layers from YAML export files."""
+
+ name = fields.String(required=True, validate=Length(1, 250))
+ descr = fields.String(allow_none=True)
+ uuid = fields.UUID(required=True)
+ version = fields.String(required=True)
+ annotation = fields.List(fields.Nested(ImportV1AnnotationSchema),
load_default=[])
Review Comment:
**Suggestion:** The mutable list used as `load_default` is shared across
schema loads. If the importer or downstream processing mutates the default
annotation collection, later imports that omit `annotation` can unexpectedly
receive annotations from an earlier import. Use a callable default such as
`list` so each load receives a fresh list. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Annotation data can leak between imports sharing one schema instance.
- ⚠️ Imported layers without annotations may receive stale annotations.
- ⚠️ Chart-to-annotation relationships can be restored incorrectly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Construct `ImportV1AnnotationLayerSchema` from
`superset/annotation_layers/schemas.py:80-87`.
2. Call `schema.load()` with valid `name`, `uuid`, and `version` fields but
omit
`annotation`; Marshmallow uses the non-callable default list defined at
`superset/annotation_layers/schemas.py:87`.
3. Mutate the returned `annotation` list, for example by appending an
annotation while
downstream import processing handles the loaded layer.
4. Call `schema.load()` again with another valid layer that also omits
`annotation`; the
second result can contain the annotation appended during the first load
because both loads
reuse the same list object. Using `load_default=list` gives each load an
independent
collection.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=48a4498faf0f4f919901024396e0c536&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=48a4498faf0f4f919901024396e0c536&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/annotation_layers/schemas.py
**Line:** 87:87
**Comment:**
*Possible Bug: The mutable list used as `load_default` is shared across
schema loads. If the importer or downstream processing mutates the default
annotation collection, later imports that omit `annotation` can unexpectedly
receive annotations from an earlier import. Use a callable default such as
`list` so each load receives a fresh list.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39857&comment_hash=5aeb6526f2e0a1bc2b1756b314cd4b2921f310b17a962c8dc209238dbfa5b552&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39857&comment_hash=5aeb6526f2e0a1bc2b1756b314cd4b2921f310b17a962c8dc209238dbfa5b552&reaction=dislike'>👎</a>
##########
superset/commands/chart/export.py:
##########
@@ -93,16 +142,61 @@ def enable_tag_export(cls) -> None:
@staticmethod
def _export(
- model: Slice, export_related: bool = True
+ model: Slice,
+ export_related: bool = True,
+ _seen: set[int] | None = None,
) -> Iterator[tuple[str, Callable[[], str]]]:
+ # Guard against circular annotation references (A→B→A).
+ # _seen is passed down the call stack so no class-level state is
needed.
+ if _seen is None:
+ _seen = set()
+ if model.id in _seen:
+ return
+ _seen.add(model.id)
yield (
ExportChartsCommand._file_name(model),
lambda: ExportChartsCommand._file_content(model),
)
+ # Parse params once for deck_multi and annotation layer handling
+ try:
+ model_params = json.loads(model.params or "{}")
+ except json.JSONDecodeError:
+ model_params = {}
+
+ if (
+ model.viz_type == "deck_multi"
+ and export_related
+ and model_params.get("deck_slices")
+ ):
+ slice_ids = model_params.get("deck_slices")
+ yield from ExportChartsCommand(slice_ids).run()
+
if model.table and export_related:
yield from ExportDatasetsCommand([model.table.id]).run()
+ # Export charts referenced as annotation sources (table/line
sourceType)
+ if export_related:
+ annotation_layers = model_params.get("annotation_layers", [])
+ chart_annotation_ids = [
+ layer["value"]
+ for layer in annotation_layers
+ if layer.get("sourceType") in ("table", "line")
+ and isinstance(layer.get("value"), int)
+ ]
+ if chart_annotation_ids:
+ yield from ExportChartsCommand(chart_annotation_ids).run()
Review Comment:
**Suggestion:** The circular-reference guard is ineffective because
recursive exports call `.run()` without passing `_seen`. If chart A references
chart B and chart B references chart A, each nested command creates a new seen
set and recursively exports forever until recursion depth is exceeded.
Propagate the shared visited set through related chart exports or maintain
deduplication at the command level. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard export can fail for cyclic chart annotations.
- ❌ Referenced charts cannot be exported reliably.
- ⚠️ Export requests may consume excessive recursive processing.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create chart A with an annotation layer whose `sourceType` is `table` or
`line` and
whose `value` references chart B; create chart B with the reciprocal
reference to chart A,
matching the annotation-layer structure consumed by
`superset/commands/chart/export.py:179-186`.
2. Start a dashboard export through `superset/dashboards/api.py:1449`, which
invokes
`ExportChartsCommand`.
3. `ExportChartsCommand._export()` at
`superset/commands/chart/export.py:143-188` adds
chart A to its local `_seen` set, then calls
`ExportChartsCommand(chart_annotation_ids).run()` at line 188 for chart B.
4. The nested command creates a new `_seen` set because `.run()` does not
receive the
parent set; exporting chart B then invokes another command for chart A, and
the cycle
continues until recursion depth or export processing fails.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fd4141d2207f4e1abb2ada3758974bf5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fd4141d2207f4e1abb2ada3758974bf5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/chart/export.py
**Line:** 175:188
**Comment:**
*Logic Error: The circular-reference guard is ineffective because
recursive exports call `.run()` without passing `_seen`. If chart A references
chart B and chart B references chart A, each nested command creates a new seen
set and recursively exports forever until recursion depth is exceeded.
Propagate the shared visited set through related chart exports or maintain
deduplication at the command level.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39857&comment_hash=ead53b54daa8ffa32357c2ec4cacdaad5b702d91c9ab41f0af83157092d5ed62&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39857&comment_hash=ead53b54daa8ffa32357c2ec4cacdaad5b702d91c9ab41f0af83157092d5ed62&reaction=dislike'>👎</a>
--
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]
