L4XB opened a new pull request, #44347:
URL: https://github.com/apache/superset/pull/44347
### SUMMARY
Exporting a chart whose dataset has been deleted produces a ZIP that
Superset's
own importer rejects. This is the chart and dashboard half of #44309.
`ExportChartsCommand` reads the chart's dataset off the `Slice.table`
relationship:
```python
payload["version"] = EXPORT_VERSION
if model.table:
payload["dataset_uuid"] = str(model.table.uuid)
```
<sub>`superset/commands/chart/export.py:77-78`</sub>
Deleting a dataset soft-deletes it (`SOFT_DELETE` defaults to `True`,
`superset/config.py:715`), and the global visibility listener
`_add_soft_delete_filter` deliberately covers relationship loads
(`superset/models/helpers.py:1501`). `Slice.table`
(`superset/models/slice.py:169`) therefore resolves to `None` while the chart
row itself survives — the "Missing dataset. The dataset linked to this chart
may have been deleted." state.
The same `model.table` guard also gates the nested dataset export
(`superset/commands/chart/export.py:130-132`), so the bundle loses **both**
the
`dataset_uuid` key and the `datasets/*.yaml` file. `ImportV1ChartSchema`
declares `dataset_uuid = fields.UUID(required=True)`
(`superset/charts/schemas.py:1998`), and validation fails with exactly the
error in the report:
```
Error importing chart: charts/<name>.yaml: {'dataset_uuid': ['Missing data
for required field.']}
```
Dashboards fail identically because `ExportDashboardsCommand` exports its
charts through `ExportChartsCommand`
(`superset/commands/dashboard/export.py:435`).
**This is an exporter defect, not an importer one.** `dataset_uuid` has been
`required=True` since #11743, the commit that introduced the chart importer,
and the `if model.table:` guard has been there since #11349 introduced ZIP
export — both in 2020. Neither side changed; soft delete made the gap
reachable from the ordinary Delete button.
#### The fix
Resolve the chart's dataset through the DAO with the soft-delete visibility
filter bypassed, so a trashed dataset still reaches the bundle and the chart
keeps its only link to it:
```python
def find_chart_dataset(model: Slice) -> SqlaTable | None:
if model.table:
return model.table
if model.datasource_type != DatasourceType.TABLE or model.datasource_id
is None:
return None
return DatasetDAO.find_by_id(model.datasource_id,
skip_visibility_filter=True)
```
`skip_visibility_filter` suppresses only the soft-delete predicate; the DAO
base filter still runs, so a dataset the user is not permitted to read stays
unreachable here exactly as it is through the relationship. Restoring the
dataset on import is the existing, already-tested behaviour of
`import_dataset`, which treats a re-imported soft-deleted UUID as an implicit
restore-with-update.
Nested exports need the same bypass one level down, so
`ExportModelsCommand.__init__` gains an `include_deleted` keyword (default
`False`, so every other caller is unchanged) that `validate()` forwards to
`find_by_ids`. This is an internal command constructor argument, not a REST
API change.
#### Why the importer is not also relaxed
Making `dataset_uuid` optional would trade a loud failure for a silent one.
`ImportChartsCommand._import` only imports a chart when its dataset is
present
in the bundle:
```python
if file_name.startswith("charts/") and config["dataset_uuid"] in datasets:
```
<sub>`superset/commands/chart/importers/v1/__init__.py:102`</sub>
A chart with no `dataset_uuid` would fall through that condition and be
skipped without an error, so the user would be told the import succeeded
while
the chart was never created.
This also means **already-exported ZIPs cannot be repaired by any import-side
change**: they are missing the `datasets/*.yaml` file itself, not just the
pointer to it, so the information needed to restore the chart is not in the
archive. Re-exporting after this fix produces a complete bundle. That is
worth
saying plainly rather than implying existing backups are covered.
#### Scope
The reported dataset symptom is a **separate** defect and is not addressed
here. Datasets fail with `GENERIC_COMMAND_ERROR`, which
`ImportModelsCommand.run` raises for any unexpected exception, so it carries
no
diagnostic. A dataset export → delete → re-import round trip succeeds on
`master` in the unit tier (the dataset is restored), so I could not reproduce
it and have not guessed at a cause. #44309 should stay open for it after this
merges.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Backend-only; the observable change is the contents of the exported ZIP.
**Before** — chart whose dataset was deleted, exported by
`ExportChartsCommand`:
```
files: ['charts/my_chart_1.yaml']
charts/my_chart_1.yaml:
slice_name: my_chart
viz_type: table
uuid: 1e53adc8-6457-4d54-9e72-97218fadfac7
version: 1.0.0
# no dataset_uuid
```
**After** — same chart, same command:
```
files: ['charts/my_chart_1.yaml',
'datasets/my_database/my_table_1.yaml',
'databases/my_database.yaml']
charts/my_chart_1.yaml:
...
dataset_uuid: cd51bc68-7041-44d7-974c-42178161d890
```
### TESTING INSTRUCTIONS
Manually, against a running instance:
1. Create a dataset and a chart on it.
2. Delete the **dataset** (the chart survives, and Explore shows "Missing
dataset").
3. Export the chart from the Charts list view.
4. Inspect the ZIP: `charts/*.yaml` carries `dataset_uuid` and a
`datasets/*.yaml` is present.
5. Import the ZIP — it succeeds, and both the dataset and the chart are back.
Automated — three unit tests are added in
`tests/unit_tests/commands/chart/export_test.py`:
```
pytest tests/unit_tests/commands/chart/export_test.py
```
Two of the three are discriminating. On the merge base (`5dd63ef95b`), with
the
new test file copied in and the source untouched:
```
FAILED
tests/unit_tests/commands/chart/export_test.py::test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted
FAILED
tests/unit_tests/commands/chart/export_test.py::test_export_import_round_trip_restores_chart_and_dataset
2 failed, 1 passed in 0.89s
```
with the failure being the reported error verbatim:
```
superset.commands.exceptions.CommandInvalidError: Error importing chart:
charts/my_chart_1.yaml: {'dataset_uuid': ['Missing data for required
field.']}
```
On this branch:
```
tests/unit_tests/commands/chart/export_test.py::test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted
PASSED
tests/unit_tests/commands/chart/export_test.py::test_export_import_round_trip_restores_chart_and_dataset
PASSED
tests/unit_tests/commands/chart/export_test.py::test_export_chart_omits_dataset_uuid_when_dataset_is_gone
PASSED
3 passed in 0.91s
```
The third test is a regression guard rather than a reproduction: it pins
that a
hard-deleted dataset (no row to resolve) still exports the chart file without
`dataset_uuid` instead of raising, which is why it also passes on the base.
Every unit test file naming a symbol this touches was run together, plus the
chart, dashboard and dataset importer suites — **315 passed**:
```
pytest tests/unit_tests/commands/chart/export_test.py \
tests/unit_tests/commands/dashboard/export_test.py \
tests/unit_tests/commands/export_test.py \
tests/unit_tests/datasets/commands/export_test.py \
tests/unit_tests/datasets/commands/importers/v1/import_test.py \
tests/unit_tests/utils/test_file.py \
tests/unit_tests/charts/commands/importers/v1/ \
tests/unit_tests/dashboards/commands/importers/v1/ \
tests/unit_tests/commands/importers/
315 passed in 17.10s
```
The surrounding tree was run on the merge base and on this branch to separate
this change from any pre-existing failure. `tests/unit_tests/commands/` goes
from **1232 passed** on the base to **1235 passed** here — a difference of
exactly the three added tests, with no failures on either side. For
`tests/unit_tests/{charts,datasets,dashboards,daos,models,utils}/` both trees
give an identical **2280 passed, 2 xfailed**.
`pre-commit run --files` over the three changed files passes, including
`mypy (main)`, `ruff`, `ruff-format` and `pylint with custom Superset
plugins`.
This is the unit tier only; the integration tier needs a provisioned metadata
database that I did not have available.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #44309 (charts and dashboards; the dataset
`GENERIC_COMMAND_ERROR` in that issue is a separate defect and is not
fixed here)
- [x] Required feature flags: `SOFT_DELETE` (defaults to `True`) is what
makes
the bug reachable through the Delete button; the export gap itself
predates it and also applies to a hard-deleted dataset
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
File and line references are against `master` @
`5dd63ef95b2c3a2dd913371492c28598ed27930a`.
--
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]
