EnxDev commented on PR #43232:
URL: https://github.com/apache/superset/pull/43232#issuecomment-5339731051
## EnxDev's Review Agent β apache/superset#43232 Β· HEAD 1ee21d5
request changes β the feature works on the chart/dashboard import path, but
it adds an unguarded write to an Alpha-only resource, makes export fail hard on
dangling annotation references, and silently binds legacy bundles' integer
annotation IDs to unrelated local rows.
### π΄ Functional
- **`superset/commands/annotation_layer/importers/v1/utils.py:29`** Β· _High_
β `import_annotation_layer` is the only importer with no permission gate;
`import_chart`, `import_dataset` and `import_database` all check
`security_manager.can_access("can_write", <model>)` and raise
`ImportFailedError`. `"Annotation Layers"` and `"Annotation"` are in
`ALPHA_ONLY_VIEW_MENUS`, but Gamma has `can_write on Chart`, so a Gamma user
can `POST /api/v1/chart/import/` a bundle containing `annotation_layers/*.yaml`
and create layers plus annotations. `import_dataset` does not block the path:
for an alive existing dataset it returns early (`if not overwrite or not
can_write: return existing`), and dataset UUIDs are readable from any chart
export Gamma can already run. With `overwrite=True` and a layer UUID (also
readable from an export), `import_from_dict(..., sync=["annotation"])` on line
55 renames the layer and deletes every annotation not present in the bundle.
Add the `can_write on Annotati
on` guard, mirroring `import_dataset`. **regression test:** Gamma imports a
chart bundle carrying `annotation_layers/` β `ImportFailedError` and no
`AnnotationLayer` row is created; Alpha succeeds.
- **`superset/commands/chart/importers/v1/utils.py:62`, `:69`** Β· _High_ β
non-FORMULA annotations whose `value` is an `int` are now kept verbatim. Every
bundle exported before this PR has integer values there (UUID rewriting is new
in this PR), and master dropped them. Import a pre-43232 chart bundle and the
annotation binds to whatever local `annotation_layer` / `slice` happens to hold
that id: the chart renders an unrelated layer's events, or an unrelated chart's
data as a `line`/`table` overlay.
`tests/unit_tests/charts/commands/importers/v1/import_test.py:168` now locks
the behavior in β the fixture layer is `{"sourceType": "NATIVE", "value": 2}`,
commented `"Native layer to be removed on import"`. Drop int values for NATIVE
and chart-reference sources, or resolve them only when the bundle provably came
from this instance. **regression test:** import
`chart_config_with_mixed_annotations` into a DB where `annotation_layer` id 2
exists and is unrelated; assert the annotati
on is dropped, not bound to id 2.
- **`superset/commands/chart/export.py:129`, `:134`, `:204`** Β· _High_ β a
chart whose annotation points at a deleted layer or chart can no longer be
exported at all. Deleting an annotation layer does not clean `slice.params`, so
dangling references are ordinary. `_replace_annotation_layer_uuids` runs inside
`_file_content`, on every export path, including `export_related=False`. Chart
export returns 404 (`superset/charts/api.py:1270`); dashboard export catches
only `DashboardNotFoundError`; `/api/v1/assets/export/`
(`superset/importexport/api.py:80-84`) catches nothing, so one bad reference
anywhere in the instance 500s the whole-instance export. Log and skip the
unresolvable annotation instead of raising. **regression test:** a chart with
`{"sourceType": "NATIVE", "value": <deleted id>}` β
`ExportChartsCommand([id]).run()` completes and the emitted YAML omits that
layer.
- **`superset/commands/importers/v1/assets.py` (not touched)** Β· _Medium_ β
the assets round trip silently loses every native and chart annotation. Export
runs with `export_related=False`, so `_file_content` still rewrites ids to
UUIDs while `_export` never yields `annotation_layers/`. On the way back,
`ImportAssetsCommand.schemas` has no `annotation_layers/` key, so
`load_configs` skips those files outright, and it calls `import_chart` without
`annotation_layer_ids` / `chart_ids` β every UUID fails to resolve and is
dropped. `superset export` β `superset import-directory` therefore drops
annotations that the chart/dashboard path preserves. **regression test:**
assets exportβimport of a chart with a native annotation keeps the reference.
### π‘ Should-fix
-
**`superset/migrations/versions/2026-08-17_18-38_884a2115ebd3_add_uuid_to_annotation_layer_and_annotation.py:67-99`**
β `try: <DDL> except OperationalError: pass` instead of `table_has_column` /
`add_columns` from `superset.migrations.shared.utils`. On Postgres a duplicate
column raises `ProgrammingError`, which is not caught, and any failed DDL
poisons the alembic transaction; swallowing the exception can also leave the
column added while the unique constraint is silently skipped. Guard with
`table_has_column` and let real errors surface. Same pattern in `downgrade()`
at `:105-125`.
- **`β¦884a2115ebd3β¦.py:55`** β `_assign_missing_uuids` loads the whole table
into memory in one query. Use `paginated_update` from
`superset.migrations.shared.utils`.
- **PR description** β SIP-59 migration on two data-bearing tables with
"Runtime estimates and downtime expectations provided" unchecked.
- **`superset/commands/chart/importers/v1/__init__.py:104`** β
`referenced_chart_uuids` is built only from `params.annotation_layers`, while
`topological_sort_charts._annotation_dependencies` and
`_resolve_query_context_annotations` also read `query_context`. A chart
referenced only from `query_context` lands in `dependent_chart_configs`, which
is never sorted, so it can be imported after its dependent and the reference is
dropped. Reuse the `query_context` scan for discovery.
- **Docs** β export bundles now carry `annotation_layers/` and store
annotation `value` as UUID strings. No `UPDATING.md` entry and no `docs/`
change for a format change plus a migration.
- **Coverage** β codecov patch coverage is 86.0%;
`chart/importers/v1/utils.py` 77.1%, `chart/export.py` 78.9%. The gaps are the
new failure branches (`topological_sort_charts` circular-dependency warning,
`_resolve_uuid_to_id` exception path). Both are pure functions β cheap unit
tests, no integration fixtures needed.
### π΅ Nits
- `superset/commands/annotation_layer/importers/dispatcher.py` β no caller.
`annotation_layers/api.py` gains no import/export routes, so it is reachable
only from tests. Wire the endpoints or drop the dispatcher.
- `superset/commands/annotation_layer/importers/v1/utils.py:40` β
`config["id"] = existing.id` is dead; `import_from_dict` strips any key outside
`export_fields | {"uuid"}`.
- `tests/integration_tests/charts/commands_tests.py` β
`test_export_chart_missing_native_annotation_reference_preserves_value` asserts
`pytest.raises(AnnotationLayerNotFoundError)`; the name says the opposite.
- `superset/commands/chart/export.py:116` β `list[dict] # type:
ignore[type-arg]`; write `list[dict[str, Any]]` instead of suppressing.
- `superset/commands/chart/importers/v1/__init__.py:22` β `from
superset_core.common.models import Subject # noqa: F401`; `Subject` is used in
`_import_chart_with_tags`, so drop the noqa.
### π Praise
- `superset/commands/importers/v1/utils.py:146`, `:249` β `config: Any =
None` fixes a real `NameError` when `load_yaml` raises before assignment, and
the `if file_name not in exc.messages` guard stops the `{file: {file: β¦}}`
double-wrap of `load_yaml`'s own `ValidationError`.
### On the existing bot comments
Checked each against the diff at this HEAD rather than taking the replies at
face value:
- **CI** β `test-mysql` is red on
`tests/integration_tests/sql_lab/api_tests.py::TestSqlLabApi::test_execute_custom_templated`
(`Duplicate entry '200' for key 'dbs.PRIMARY'`). Unrelated flake, not this PR.
- "Test expects `ChartImportError` for `not-a-uuid`" (bito, codeant) β fixed
at this HEAD; the test now asserts the drop and the resulting chart contents.
- "`load_configs` does not convert ISO timestamps" (codeant) β handled:
`import_annotation_layer` deserializes `start_dttm`/`end_dttm` before
`import_from_dict`. `load_configs` storing the raw config is by design, and
every other importer compensates the same way.
- "`assign_uuids` assigns the `uuid4` function on SQLite" (codeant) β not
applicable; the migration uses its own `_assign_missing_uuids`, which calls
`uuid4()`.
- "Annotation layers always imported with `overwrite=False` in the dashboard
importer" (codeant, bito) β incorrect. It passes `overwrite_assets`, the same
flag charts, datasets and databases use in that importer.
- "`_resolve_annotation_list` deletes query_context formula annotations"
(codeant) β incorrect. The `annotationType == FORMULA` branch keeps them before
`sourceType` is consulted.
- "Missing chart/layer refs emit a non-portable integer on export" (codeant)
β the export side raises instead, which is its own problem; see the third π΄
above. The integer-passthrough risk is real but on the import side, at
`chart/importers/v1/utils.py:62`.
<!-- enxdev-review-agent:1ee21d5 -->
_Reviewed by EnxDev's Review Agent β @EnxDev Β· HEAD 1ee21d5._
--
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]
