You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "L4XB (via GitHub)" <gi...@apache.org> on 2026/09/16 11:47:23 UTC
[PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4046678492
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
Review Comment:
All three tests set `can_access_all_datasources=True`, so the permission filter is a no-op and a regression in the new visibility-bypass path could export an unauthorized dataset without failing them. Could we add a chart-export case where the caller can access the chart but is neither a dataset editor nor granted dataset access, then soft-delete the dataset and assert that the bundle contains neither its `datasets/` YAML nor `dataset_uuid`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4306934643
##########
superset/commands/export/assets.py:
##########
@@ -77,11 +85,69 @@ def run(self) -> Iterator[tuple[str, Callable[[], str]]]:
dashboard_ids = ids
elif command == ExportChartsCommand:
chart_ids = ids
+ elif command == ExportDatasetsCommand:
+ dataset_ids = ids
+
+ yield from self._export_referenced_trashed_datasets(
+ chart_ids, dashboard_ids, dataset_ids, seen
+ )
yield from ExportTagsCommand(
dashboard_ids=dashboard_ids,
chart_ids=chart_ids,
).run()
+ @classmethod
+ def _export_referenced_trashed_datasets(
+ cls,
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ seen: set[str],
+ ) -> Iterator[tuple[str, Callable[[], str]]]:
+ if trashed_ids := cls._referenced_trashed_dataset_ids(
+ chart_ids, dashboard_ids, dataset_ids
+ ):
+ for file_name, file_content in ExportDatasetsCommand(
+ trashed_ids, export_related=False, include_deleted=True
+ ).run():
+ if file_name not in seen:
+ yield file_name, file_content
+ seen.add(file_name)
+
+ @staticmethod
+ def _referenced_trashed_dataset_ids(
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ ) -> list[int]:
+ """Datasets in the trash that the exported charts and native filters name.
+
+ Datasets are exported from the visibility-filtered ``find_all()``, which
+ leaves the trash out. A chart or a native filter whose dataset is in the
+ trash still writes its ``dataset_uuid`` / ``datasetUuid``, and the import
+ resolves those from the bundle, so those datasets are exported as well.
+ The DAO base filter still applies to every lookup.
+ """
+ referenced = {
+ dataset.id
+ for chart in ChartDAO.find_by_ids(chart_ids)
+ if (dataset := find_chart_dataset(chart)) is not None
Review Comment:
`find_chart_dataset` returns the `Slice.table` relationship first, and that load is not filtered by the DAO base filter, so this set also contains live datasets the exporter has no grant on (while `dataset_ids` from `find_all()` omits them). Those ids then reach `ExportDatasetsCommand(..., include_deleted=True)`, whose `validate()` applies the base filter, finds fewer rows than ids and raises `DatasetNotFoundError`. A non-Alpha role with export permission who can see a chart on a dataset they lack access to would get a failed assets export where before the bundle was produced. Should the chart branch only keep datasets that pass the same filtered lookup, or should ids that fail it be dropped here?
##########
superset/commands/export/assets.py:
##########
@@ -77,11 +85,69 @@ def run(self) -> Iterator[tuple[str, Callable[[], str]]]:
dashboard_ids = ids
elif command == ExportChartsCommand:
chart_ids = ids
+ elif command == ExportDatasetsCommand:
+ dataset_ids = ids
+
+ yield from self._export_referenced_trashed_datasets(
+ chart_ids, dashboard_ids, dataset_ids, seen
+ )
yield from ExportTagsCommand(
dashboard_ids=dashboard_ids,
chart_ids=chart_ids,
).run()
+ @classmethod
+ def _export_referenced_trashed_datasets(
+ cls,
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ seen: set[str],
+ ) -> Iterator[tuple[str, Callable[[], str]]]:
+ if trashed_ids := cls._referenced_trashed_dataset_ids(
+ chart_ids, dashboard_ids, dataset_ids
+ ):
+ for file_name, file_content in ExportDatasetsCommand(
+ trashed_ids, export_related=False, include_deleted=True
+ ).run():
+ if file_name not in seen:
+ yield file_name, file_content
+ seen.add(file_name)
+
+ @staticmethod
+ def _referenced_trashed_dataset_ids(
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ ) -> list[int]:
+ """Datasets in the trash that the exported charts and native filters name.
+
+ Datasets are exported from the visibility-filtered ``find_all()``, which
+ leaves the trash out. A chart or a native filter whose dataset is in the
+ trash still writes its ``dataset_uuid`` / ``datasetUuid``, and the import
+ resolves those from the bundle, so those datasets are exported as well.
+ The DAO base filter still applies to every lookup.
+ """
+ referenced = {
+ dataset.id
+ for chart in ChartDAO.find_by_ids(chart_ids)
+ if (dataset := find_chart_dataset(chart)) is not None
+ }
+ filter_ids: set[int] = set()
+ for dashboard in DashboardDAO.find_by_ids(dashboard_ids):
+ try:
+ metadata = json.loads(dashboard.json_metadata or "{}")
+ except (TypeError, json.JSONDecodeError):
+ continue
+ filter_ids |= dashboard_dataset_ids(metadata)
Review Comment:
This passes raw metadata to `dashboard_dataset_ids`, but `ExportDashboardsCommand` first runs `export_dashboard_references(metadata)` so semantic-view targets are never read as table ids. Here a semantic-view filter target with `datasetId: 7` is looked up as `SqlaTable` 7, so an unrelated trashed dataset gets bundled and can be restored on re-import. Could the targets be filtered to table-type before the lookup, e.g. by sharing the same normalisation, with a test for a colliding id?
##########
superset/commands/export/assets.py:
##########
@@ -77,11 +85,69 @@ def run(self) -> Iterator[tuple[str, Callable[[], str]]]:
dashboard_ids = ids
elif command == ExportChartsCommand:
chart_ids = ids
+ elif command == ExportDatasetsCommand:
+ dataset_ids = ids
+
+ yield from self._export_referenced_trashed_datasets(
+ chart_ids, dashboard_ids, dataset_ids, seen
+ )
yield from ExportTagsCommand(
dashboard_ids=dashboard_ids,
chart_ids=chart_ids,
).run()
+ @classmethod
+ def _export_referenced_trashed_datasets(
+ cls,
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ seen: set[str],
+ ) -> Iterator[tuple[str, Callable[[], str]]]:
+ if trashed_ids := cls._referenced_trashed_dataset_ids(
+ chart_ids, dashboard_ids, dataset_ids
+ ):
+ for file_name, file_content in ExportDatasetsCommand(
+ trashed_ids, export_related=False, include_deleted=True
+ ).run():
+ if file_name not in seen:
+ yield file_name, file_content
+ seen.add(file_name)
+
+ @staticmethod
+ def _referenced_trashed_dataset_ids(
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ ) -> list[int]:
+ """Datasets in the trash that the exported charts and native filters name.
+
+ Datasets are exported from the visibility-filtered ``find_all()``, which
+ leaves the trash out. A chart or a native filter whose dataset is in the
+ trash still writes its ``dataset_uuid`` / ``datasetUuid``, and the import
+ resolves those from the bundle, so those datasets are exported as well.
+ The DAO base filter still applies to every lookup.
+ """
+ referenced = {
+ dataset.id
+ for chart in ChartDAO.find_by_ids(chart_ids)
+ if (dataset := find_chart_dataset(chart)) is not None
+ }
+ filter_ids: set[int] = set()
+ for dashboard in DashboardDAO.find_by_ids(dashboard_ids):
+ try:
+ metadata = json.loads(dashboard.json_metadata or "{}")
Review Comment:
`json.loads` succeeds for a stored `json_metadata` of `null` or `[]`, but `dashboard_dataset_ids(metadata)` then calls `.get` on `None`/a list and raises `AttributeError`, which aborts the whole assets export (one odd dashboard now breaks every export). `ExportDashboardsCommand._file_content` tolerates these via `payload.get("metadata") or {}`. Should this guard against a non-dict result the same way, and could a test cover an empty/odd-metadata dashboard next to a healthy one?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "L4XB (via GitHub)" <gi...@apache.org>.
L4XB commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4096085159
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
Review Comment:
Added in 768bf006f4; details are in my comment on the PR. The case is a chart editor with no role and no grant on the dataset, and the dataset is soft-deleted. `test_export_chart_does_not_leak_deleted_dataset_without_access` is parametrized over `export_related`. It asserts that the bundle holds the chart alone, with no `dataset_uuid` and no `datasets/` or `databases/` file. A second test pins `ExportDatasetsCommand([id], include_deleted=True)` raising `DatasetNotFoundError` for that user.
I checked that both tests guard the bypass:
- Making `find_chart_dataset` also skip the base filter fails both `does_not_leak` cases.
- Making `validate()` skip it fails the nested-export test.
I also merged master just now. The earlier red `unit-tests` run was `check_pot_drift` flagging a database-connection string that master's `messages.pot` has since picked up.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302677149
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+def _export(chart: Slice) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand([chart.id]).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `e81fe80`.
The export test now asserts that the chart payload contains the expected `dataset_uuid` and that the bundled dataset has the matching UUID.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4303149815
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+def _export(chart: Slice) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand([chart.id]).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `3bfea04`.
The test now imports the export after deleting the dataset and asserts that both the dataset and chart are restored, including the chart’s link to the restored dataset.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "L4XB (via GitHub)" <gi...@apache.org>.
L4XB commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302345346
##########
superset/commands/chart/export.py:
##########
@@ -259,9 +282,12 @@ def _export(
lambda: ExportChartsCommand._file_content(model),
)
- if model.table and export_related:
+ if export_related and (dataset := find_chart_dataset(model)):
Review Comment:
Yes, it should. Thanks for tracing it through the importer. Fixed in 06b820ee89. Both dataset lookups in `ExportDashboardsCommand` now use `skip_visibility_filter=True`: the one in `_file_content` that maps native-filter and display-control `datasetId`s to `datasetUuid`, and the one in `_export` that decides which datasets travel with the bundle. Those datasets are exported with `include_deleted=True`, so a filter on a trashed dataset keeps its `datasetUuid`, and the bundle carries the dataset for the importer to rebuild `datasetId` from.
`test_dashboard_filter_keeps_a_soft_deleted_dataset_through_import` covers exactly your scenario: a chart and a native filter on D, D deleted, export, then re-import with overwrite. It checks that the exported target is `{"datasetUuid": <D>, "column": ...}`, that the bundle holds one `datasets/` file, and that the imported filter points at the restored D. Without the change, the exported target is `{"column": ...}` and the test fails there.
Two existing tests mocked these calls without the new arguments, so I updated their expectations: `test_export_yields_dataset_files_for_display_controls` and `test_orphaned_dashboard_target_does_not_abort_mixed_export`. `ExportModelsCommand.validate` now passes `skip_visibility_filter` only when `include_deleted` is set, so every other caller keeps the plain `find_by_ids(ids)` call.
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,272 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from flask_appbuilder.security.sqla.models import User
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.commands.dataset.exceptions import DatasetNotFoundError
+from superset.commands.dataset.export import ExportDatasetsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+from superset.subjects.models import Subject
+from superset.subjects.types import SubjectType
+from superset.utils.core import override_user
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+@pytest.fixture
+def chart_editor_without_dataset_access(
+ chart_on_dataset: Slice,
+) -> Generator[User, None, None]:
+ """Log in a user who edits the chart but holds no grant on its dataset.
+
+ The user has no role, so no ``all_datasource_access``, ``database_access``,
+ ``datasource_access``, ``schema_access`` or ``catalog_access``, and is not
+ an editor of the dataset, so ``DatasourceFilter`` hides the dataset from
+ them in the trash as well. Editing the chart is what lets ``ChartFilter``
+ hand them the chart.
+ """
+ user = User(
+ first_name="chart",
+ last_name="editor",
+ username="chart_editor",
+ email="chart_editor@example.com",
+ active=True,
+ )
+ db.session.add(user)
+ db.session.flush()
+ chart_on_dataset.editors = [
+ Subject(label="chart_editor", type=SubjectType.USER, user_id=user.id)
+ ]
+ db.session.flush()
+
+ with override_user(user):
+ yield user
+
+
+def _export(chart: Slice, export_related: bool = True) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand(
+ [chart.id], export_related=export_related
+ ).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
+ assert restored.deleted_at is None, "the dataset must come back out of the trash"
+ assert str(restored.uuid) == dataset_uuid
+
+ reimported = db.session.query(Slice).filter_by(uuid=chart.uuid).one()
+ assert reimported.datasource_id == restored.id
Review Comment:
Right, the original row made it vacuous. Fixed in af91468fcd. Before importing, the test renames the chart and sets its `datasource_id` to `None`, and it now asserts that both come back: `slice_name == "my_chart"` and `datasource_id == restored.id`. I checked it against the silent-skip path by making `ImportChartsCommand` drop the chart (`config.get("dataset_uuid") in {}`). The test then fails with `'renamed after the export' == 'my_chart'`. The new dashboard test follows the same idea and clears the native filter before importing.
`pytest tests/unit_tests/commands tests/unit_tests/semantic_layers tests/unit_tests/dao`: 2433 passed. `ruff format --check` and `ruff check` are clean.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4025688787
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+def _export(chart: Slice) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand([chart.id]).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
Review Comment:
**Suggestion:** This test blesses a hard-deleted chart bundle without `dataset_uuid`, although the importer requires that field for every chart, so it treats an unusable export as valid. [api mismatch]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8b01190bdbce4ecea9a9ba2b0e7d462c&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=8b01190bdbce4ecea9a9ba2b0e7d462c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/chart/export_test.py
**Line:** 141:146
**Comment:**
*Api Mismatch: This test blesses a hard-deleted chart bundle without `dataset_uuid`, although the importer requires that field for every chart, so it treats an unusable export as valid.
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%2F44347&comment_hash=4f7e1a418efe94e7916d95bfe9ff7d9da0536d80bb491787c0de06b46ac52070&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44347&comment_hash=4f7e1a418efe94e7916d95bfe9ff7d9da0536d80bb491787c0de06b46ac52070&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "L4XB (via GitHub)" <gi...@apache.org>.
L4XB commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302949897
##########
superset/commands/export/models.py:
##########
@@ -104,6 +114,10 @@ def run(
seen.add(file_name)
def validate(self) -> None:
- self._models = self.dao.find_by_ids(self.model_ids)
+ self._models = (
+ self.dao.find_by_ids(self.model_ids, skip_visibility_filter=True)
Review Comment:
Added in e832160d06: `test_dataset_export_leaves_out_a_soft_deleted_dataset_unless_asked`. It soft-deletes a readable dataset and checks that `list(ExportDatasetsCommand([dataset_id]).run())` raises `DatasetNotFoundError`, while `include_deleted=True` exports exactly that dataset. If `validate()` drops the flag check and always passes `skip_visibility_filter=True`, this test is the one that fails.
##########
superset/commands/chart/export.py:
##########
@@ -55,6 +58,26 @@
REMOVE_KEYS = ["datasource_type", "datasource_name", "url_params"]
+def find_chart_dataset(model: Slice) -> SqlaTable | None:
+ """Resolve the dataset a chart is built on, soft-deleted ones included.
+
+ ``Slice.table`` is loaded through the soft-delete visibility filter, so a
+ chart whose dataset sits in the trash resolves to ``None`` even though
+ ``datasource_id`` still points at a row. Exporting such a chart off that
+ ``None`` drops both ``dataset_uuid`` and the ``datasets/`` file, and the
+ resulting bundle is rejected by Superset's own importer with
+ ``{"dataset_uuid": ["Missing data for required field."]}``.
+
+ The lookup keeps the DAO base filter, so a dataset the user may not read
+ stays unreachable here just as it does through the relationship.
+ """
+ if model.table:
+ return model.table
+ if model.datasource_type != DatasourceType.TABLE or model.datasource_id is None:
Review Comment:
Added in e832160d06: `test_semantic_view_chart_does_not_bundle_the_table_with_its_id`. It exports a `semantic_view` chart whose `datasource_id` matches a readable table, with `export_related=True`. It asserts that no `datasets/` or `databases/` file is produced and that the chart YAML has no `dataset_uuid`. With the `datasource_type != DatasourceType.TABLE` guard removed, this test is the one that fails.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-5696920174
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-5794214591
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #44347:
URL: https://github.com/apache/superset/pull/44347
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-5698103406
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44347?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `31.25000%` with `11 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.29%. Comparing base ([`5dd63ef`](https://app.codecov.io/gh/apache/superset/commit/5dd63ef95b2c3a2dd913371492c28598ed27930a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4ae4cc7`](https://app.codecov.io/gh/apache/superset/commit/4ae4cc7993bae07d20bf2f74bef4b3460ff09c96?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44347?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/chart/export.py](https://app.codecov.io/gh/apache/superset/pull/44347?src=pr&el=tree&filepath=superset%2Fcommands%2Fchart%2Fexport.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvY2hhcnQvZXhwb3J0LnB5) | 30.76% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44347?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/export/models.py](https://app.codecov.io/gh/apache/superset/pull/44347?src=pr&el=tree&filepath=superset%2Fcommands%2Fexport%2Fmodels.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZXhwb3J0L21vZGVscy5weQ==) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44347?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44347 +/- ##
===========================================
- Coverage 80.25% 57.29% -22.97%
===========================================
Files 2927 2928 +1
Lines 173273 174126 +853
Branches 40173 40404 +231
===========================================
- Hits 139058 99758 -39300
- Misses 31620 73416 +41796
+ Partials 2595 952 -1643
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44347/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.23% <31.25%> (-0.20%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.23% <31.25%> (-45.45%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44347?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "L4XB (via GitHub)" <gi...@apache.org>.
L4XB commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302675127
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,343 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from flask_appbuilder.security.sqla.models import User
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.commands.dashboard.export import ExportDashboardsCommand
+from superset.commands.dashboard.importers.v1 import ImportDashboardsCommand
+from superset.commands.dataset.exceptions import DatasetNotFoundError
+from superset.commands.dataset.export import ExportDatasetsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.subjects.models import Subject
+from superset.subjects.types import SubjectType
+from superset.utils import json
+from superset.utils.core import override_user
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+@pytest.fixture
+def chart_editor_without_dataset_access(
+ chart_on_dataset: Slice,
+) -> Generator[User, None, None]:
+ """Log in a user who edits the chart but holds no grant on its dataset.
+
+ The user has no role, so no ``all_datasource_access``, ``database_access``,
+ ``datasource_access``, ``schema_access`` or ``catalog_access``, and is not
+ an editor of the dataset, so ``DatasourceFilter`` hides the dataset from
+ them in the trash as well. Editing the chart is what lets ``ChartFilter``
+ hand them the chart.
+ """
+ user = User(
+ first_name="chart",
+ last_name="editor",
+ username="chart_editor",
+ email="chart_editor@example.com",
+ active=True,
+ )
+ db.session.add(user)
+ db.session.flush()
+ chart_on_dataset.editors = [
+ Subject(label="chart_editor", type=SubjectType.USER, user_id=user.id)
+ ]
+ db.session.flush()
+
+ with override_user(user):
+ yield user
+
+
+def _export(chart: Slice, export_related: bool = True) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand(
+ [chart.id], export_related=export_related
+ ).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ # Change what the import has to write back, so the assertions below fail if
+ # the chart is skipped rather than imported.
+ chart.slice_name = "renamed after the export"
+ chart.datasource_id = None
+ db.session.flush()
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
+ assert restored.deleted_at is None, "the dataset must come back out of the trash"
+ assert str(restored.uuid) == dataset_uuid
+
+ reimported = db.session.query(Slice).filter_by(uuid=chart.uuid).one()
+ assert reimported.slice_name == "my_chart"
+ assert reimported.datasource_id == restored.id
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_dashboard_filter_keeps_a_soft_deleted_dataset_through_import(
+ app_context: None,
+ chart_on_dataset: Slice,
+ mocker: MockerFixture,
+) -> None:
+ """A dashboard filter on a dataset in the trash keeps its target (#44309).
+
+ The filter's dataset lookup skipped the trash, so the export dropped the
+ target's ``datasetId`` without writing a ``datasetUuid``, and the imported
+ filter pointed at no dataset.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ target = {"datasetId": dataset.id, "column": {"name": "ds"}}
Review Comment:
Thanks, you were right. I reverted only the `found_dataset_ids` lookup in `_export` against the previous version of the tests, and all 28 still passed.
Changed in e81fe804aa:
- The filter now targets a second dataset, `filter_table`, which is in the trash and used by no chart on the dashboard. The test checks that the bundle has a `datasets/` file with that dataset's uuid, and that the import brings it back out of the trash with the filter pointing at it.
- The two mocked dashboard tests now also check that `find_by_ids` gets `skip_visibility_filter=True`.
With the trash-inclusive lookup reverted in `_export`, the round trip and `test_export_batches_dataset_export_across_targets` fail. With it reverted in `_file_content`, the round trip and `test_file_content_batches_dataset_lookup_across_targets` fail. With both lookups as in the PR, all 28 pass.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "L4XB (via GitHub)" <gi...@apache.org>.
L4XB commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4333879260
##########
superset/commands/export/assets.py:
##########
@@ -77,11 +85,69 @@ def run(self) -> Iterator[tuple[str, Callable[[], str]]]:
dashboard_ids = ids
elif command == ExportChartsCommand:
chart_ids = ids
+ elif command == ExportDatasetsCommand:
+ dataset_ids = ids
+
+ yield from self._export_referenced_trashed_datasets(
+ chart_ids, dashboard_ids, dataset_ids, seen
+ )
yield from ExportTagsCommand(
dashboard_ids=dashboard_ids,
chart_ids=chart_ids,
).run()
+ @classmethod
+ def _export_referenced_trashed_datasets(
+ cls,
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ seen: set[str],
+ ) -> Iterator[tuple[str, Callable[[], str]]]:
+ if trashed_ids := cls._referenced_trashed_dataset_ids(
+ chart_ids, dashboard_ids, dataset_ids
+ ):
+ for file_name, file_content in ExportDatasetsCommand(
+ trashed_ids, export_related=False, include_deleted=True
+ ).run():
+ if file_name not in seen:
+ yield file_name, file_content
+ seen.add(file_name)
+
+ @staticmethod
+ def _referenced_trashed_dataset_ids(
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ ) -> list[int]:
+ """Datasets in the trash that the exported charts and native filters name.
+
+ Datasets are exported from the visibility-filtered ``find_all()``, which
+ leaves the trash out. A chart or a native filter whose dataset is in the
+ trash still writes its ``dataset_uuid`` / ``datasetUuid``, and the import
+ resolves those from the bundle, so those datasets are exported as well.
+ The DAO base filter still applies to every lookup.
+ """
+ referenced = {
+ dataset.id
+ for chart in ChartDAO.find_by_ids(chart_ids)
+ if (dataset := find_chart_dataset(chart)) is not None
Review Comment:
Fixed in 401370416f, with the first option: chart ids and native-filter ids now go through one `DatasetDAO.find_by_ids(..., skip_visibility_filter=True)` lookup, so the base filter decides for both. The chart side takes `datasource_id` for table-type charts instead of `Slice.table`. A live dataset the user cannot read is therefore dropped and never reaches `ExportDatasetsCommand`. `test_assets_export_leaves_out_a_live_dataset_the_user_cannot_read` (with `chart_editor_without_dataset_access`) expects no ids, and it got the live dataset back on the previous head.
##########
superset/commands/export/assets.py:
##########
@@ -77,11 +85,69 @@ def run(self) -> Iterator[tuple[str, Callable[[], str]]]:
dashboard_ids = ids
elif command == ExportChartsCommand:
chart_ids = ids
+ elif command == ExportDatasetsCommand:
+ dataset_ids = ids
+
+ yield from self._export_referenced_trashed_datasets(
+ chart_ids, dashboard_ids, dataset_ids, seen
+ )
yield from ExportTagsCommand(
dashboard_ids=dashboard_ids,
chart_ids=chart_ids,
).run()
+ @classmethod
+ def _export_referenced_trashed_datasets(
+ cls,
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ seen: set[str],
+ ) -> Iterator[tuple[str, Callable[[], str]]]:
+ if trashed_ids := cls._referenced_trashed_dataset_ids(
+ chart_ids, dashboard_ids, dataset_ids
+ ):
+ for file_name, file_content in ExportDatasetsCommand(
+ trashed_ids, export_related=False, include_deleted=True
+ ).run():
+ if file_name not in seen:
+ yield file_name, file_content
+ seen.add(file_name)
+
+ @staticmethod
+ def _referenced_trashed_dataset_ids(
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ ) -> list[int]:
+ """Datasets in the trash that the exported charts and native filters name.
+
+ Datasets are exported from the visibility-filtered ``find_all()``, which
+ leaves the trash out. A chart or a native filter whose dataset is in the
+ trash still writes its ``dataset_uuid`` / ``datasetUuid``, and the import
+ resolves those from the bundle, so those datasets are exported as well.
+ The DAO base filter still applies to every lookup.
+ """
+ referenced = {
+ dataset.id
+ for chart in ChartDAO.find_by_ids(chart_ids)
+ if (dataset := find_chart_dataset(chart)) is not None
+ }
+ filter_ids: set[int] = set()
+ for dashboard in DashboardDAO.find_by_ids(dashboard_ids):
+ try:
+ metadata = json.loads(dashboard.json_metadata or "{}")
Review Comment:
Fixed in 401370416f: a parsed `json_metadata` that is not a dict names no targets and is skipped, as `payload.get("metadata") or {}` effectively does in the dashboard exporter. `test_assets_export_reads_dashboards_whose_metadata_is_not_an_object` puts `null` and `[]` dashboards next to one whose native filter targets a trashed dataset and expects that dataset. On the previous head it raised `AttributeError`.
##########
superset/commands/export/assets.py:
##########
@@ -77,11 +85,69 @@ def run(self) -> Iterator[tuple[str, Callable[[], str]]]:
dashboard_ids = ids
elif command == ExportChartsCommand:
chart_ids = ids
+ elif command == ExportDatasetsCommand:
+ dataset_ids = ids
+
+ yield from self._export_referenced_trashed_datasets(
+ chart_ids, dashboard_ids, dataset_ids, seen
+ )
yield from ExportTagsCommand(
dashboard_ids=dashboard_ids,
chart_ids=chart_ids,
).run()
+ @classmethod
+ def _export_referenced_trashed_datasets(
+ cls,
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ seen: set[str],
+ ) -> Iterator[tuple[str, Callable[[], str]]]:
+ if trashed_ids := cls._referenced_trashed_dataset_ids(
+ chart_ids, dashboard_ids, dataset_ids
+ ):
+ for file_name, file_content in ExportDatasetsCommand(
+ trashed_ids, export_related=False, include_deleted=True
+ ).run():
+ if file_name not in seen:
+ yield file_name, file_content
+ seen.add(file_name)
+
+ @staticmethod
+ def _referenced_trashed_dataset_ids(
+ chart_ids: list[int | str],
+ dashboard_ids: list[int | str],
+ dataset_ids: list[int | str],
+ ) -> list[int]:
+ """Datasets in the trash that the exported charts and native filters name.
+
+ Datasets are exported from the visibility-filtered ``find_all()``, which
+ leaves the trash out. A chart or a native filter whose dataset is in the
+ trash still writes its ``dataset_uuid`` / ``datasetUuid``, and the import
+ resolves those from the bundle, so those datasets are exported as well.
+ The DAO base filter still applies to every lookup.
+ """
+ referenced = {
+ dataset.id
+ for chart in ChartDAO.find_by_ids(chart_ids)
+ if (dataset := find_chart_dataset(chart)) is not None
+ }
+ filter_ids: set[int] = set()
+ for dashboard in DashboardDAO.find_by_ids(dashboard_ids):
+ try:
+ metadata = json.loads(dashboard.json_metadata or "{}")
+ except (TypeError, json.JSONDecodeError):
+ continue
+ filter_ids |= dashboard_dataset_ids(metadata)
Review Comment:
Fixed in 401370416f inside the shared helper: `dashboard_dataset_ids` now only reads targets whose `datasourceType` is absent or `"table"`, the same split `export_dashboard_references` makes. So the assets path and the dashboard exporter normalise the same way. In `ExportDashboardsCommand` the semantic targets have already been rewritten at that point, so nothing changes there. `test_assets_export_does_not_read_a_semantic_view_target_as_a_table` gives a filter target the trashed table's id and is parametrized over `semantic_view`, `table` and no type. Only the semantic case leaves the table out, and that case failed on the previous head.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-5769770593
Good catch on the soft-delete relationship gap, this closes a real import-breaking hole. One thing before this merges though: sadpandajoe's right that all three new tests mock `can_access_all_datasources=True`, so a regression in the new `skip_visibility_filter` bypass wouldn't fail anything.
Could you add a case where the caller can't access the dataset and assert the bundle doesn't leak it?
Also curious about codeant's point on the hard-delete test, since the importer still requires `dataset_uuid`, does that bundle just stay unimportable, or is that an accepted edge case?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302951614
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+def _export(chart: Slice) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand([chart.id]).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `e832160`.
The export test now asserts that the chart payload contains the expected `dataset_uuid` and that the bundled dataset has the matching UUID.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302971075
##########
superset/commands/chart/export.py:
##########
@@ -88,8 +111,8 @@ def _file_content(model: Slice) -> str:
if model.datasource_type == "semantic_view":
if model.semantic_view:
payload["datasource_ref"] = export_view_reference(model.semantic_view)
- elif model.table:
- payload["dataset_uuid"] = str(model.table.uuid)
+ elif dataset := find_chart_dataset(model):
Review Comment:
`ExportAssetsCommand` runs the chart and dashboard exporters with `export_related=False` and builds its dataset list from the visibility-filtered `find_all()`, so a trashed dataset's file is never added to that bundle. With this change the chart YAML now still gets `dataset_uuid` for that dataset (and the dashboard's native filters get a `datasetUuid`). On re-import via `ImportAssetsCommand`, `dataset_info[config["dataset_uuid"]]` (`superset/commands/importers/v1/assets.py:185`) then raises a `KeyError` for the missing dataset, where before the bundle was rejected with a clear schema error. Should the full-assets export also include trashed datasets referenced by the exported charts and filters, or should `dataset_uuid` only be written when the dataset file travels with the bundle?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-5696941643
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 4ae4cc7993bae07d20bf2f74bef4b3460ff09c96 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aaa81cca95aa60008fc6308 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44347--superset-docs-preview.netlify.app](https://deploy-preview-44347--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#HcpBCoAgEEDRu8w6aaIS7QYdY1IjoXIYDIzo7kmr_xfvAZZ0cIYJoIFF6HRb_TWWdhh6tCoUTpJVPP5Iuk6vskSu2gfe0z376jURmc45siORRkSzOt2jgfcD) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44347--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-5696992459
<!-- Bito Reply -->
The flagged issue is not correct. The test `test_export_chart_omits_dataset_uuid_when_dataset_is_gone` correctly handles hard-deleted datasets by verifying that the `dataset_uuid` is omitted from the chart export when the dataset no longer exists in the database. This is the expected behavior for a chart with a dangling `datasource_id` where the dataset cannot be resolved, ensuring the export command does not fail while correctly reflecting the absence of the related dataset.
**tests/unit_tests/commands/chart/export_test.py**
```
contents = _export(chart)
chart_files = [name for name in contents if name.startswith("charts/")]
assert len(chart_files) == 1
assert not [name for name in contents if name.startswith("datasets/")]
assert "dataset_uuid" not in yaml.safe_load(contents[chart_files[0]])
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302347001
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+def _export(chart: Slice) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand([chart.id]).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `af91468`.
The test now imports the export made after deleting the dataset, then verifies the dataset is restored and the chart is reattached to it.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "L4XB (via GitHub)" <gi...@apache.org>.
L4XB commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-5791798154
@rusackas Both done.
**Access.** Added in 768bf00: a chart editor with no role and no grant on the dataset, so `DatasourceFilter` hides it (trash included). The dataset is soft-deleted, and the test asserts the row is still there, so only the missing access keeps it out.
- `test_export_chart_does_not_leak_deleted_dataset_without_access`, parametrized over `export_related`: the bundle holds the chart alone, with no `dataset_uuid` and no `datasets/` or `databases/` file.
- `test_nested_dataset_export_keeps_access_filter_for_deleted_dataset`: `ExportDatasetsCommand([id], include_deleted=True)` still raises `DatasetNotFoundError` for that user. `include_deleted` lifts the soft-delete filter and nothing else.
To check that they guard the bypass, I widened each lookup to skip the permission filter as well:
| mutation | result |
|---|---|
| `find_chart_dataset` also passes `skip_base_filter=True` | both `does_not_leak` cases fail. With `export_related=False` the chart YAML carries the dataset's uuid (`assert 'dataset_uuid' not in {…, 'dataset_uuid': 'f91627cb-…'}`); with it, the nested export raises. The three original tests still pass. |
| `validate()` also passes `skip_base_filter=self.include_deleted` | `keeps_access_filter` fails; the other 5 pass |
The file passes 6/6 as pushed, and `ruff` and pre-commit's `mypy` are clean on it.
**Hard delete.** I measured it. After `DatasetDAO.hard_delete`, the export contains `charts/my_chart_1.yaml`, `metadata.yaml` and `tags.yaml`, and importing that bundle fails with `CommandInvalidError {'charts/my_chart_1.yaml': {'dataset_uuid': ['Missing data for required field.']}}`. This PR doesn't change that case: with no row left to resolve, `find_chart_dataset` returns `None` exactly where `model.table` did before.
I'd treat it as accepted for this PR. The chart no longer has any datasource, so the importer can't recreate it, and refusing is correct. The weak part is the message, which doesn't say the dataset is gone. If you want it handled, a follow-up could either have the export skip such a chart with a warning, or have the importer name the chart and the missing dataset. I'd lean towards the importer, since that keeps the chart's params in the bundle.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4201240701
##########
superset/commands/chart/export.py:
##########
@@ -259,9 +282,12 @@ def _export(
lambda: ExportChartsCommand._file_content(model),
)
- if model.table and export_related:
+ if export_related and (dataset := find_chart_dataset(model)):
Review Comment:
With this change a dashboard whose dataset is in the trash now exports a bundle that imports cleanly, but the dashboard's native-filter targets still lose their dataset reference: `ExportDashboardsCommand` resolves `datasetId` through `DatasetDAO.find_by_ids` without `skip_visibility_filter`, so it pops `datasetId` and never sets `datasetUuid` (`superset/commands/dashboard/export.py:348-361`), and the importer only rebuilds `datasetId` when `datasetUuid` is present (`dashboard/importers/v1/utils.py:145-147`). Exporting a dashboard with a chart and a native filter on dataset D, deleting D, and re-importing with overwrite restores the chart and D but leaves the filter pointing at nothing, where before the bundle was rejected up front. Should the dashboard's native-filter and display-control lookups use the same trash-inclusive resolution so the PR's dashboard claim holds end to end?
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,272 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from flask_appbuilder.security.sqla.models import User
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.commands.dataset.exceptions import DatasetNotFoundError
+from superset.commands.dataset.export import ExportDatasetsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+from superset.subjects.models import Subject
+from superset.subjects.types import SubjectType
+from superset.utils.core import override_user
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+@pytest.fixture
+def chart_editor_without_dataset_access(
+ chart_on_dataset: Slice,
+) -> Generator[User, None, None]:
+ """Log in a user who edits the chart but holds no grant on its dataset.
+
+ The user has no role, so no ``all_datasource_access``, ``database_access``,
+ ``datasource_access``, ``schema_access`` or ``catalog_access``, and is not
+ an editor of the dataset, so ``DatasourceFilter`` hides the dataset from
+ them in the trash as well. Editing the chart is what lets ``ChartFilter``
+ hand them the chart.
+ """
+ user = User(
+ first_name="chart",
+ last_name="editor",
+ username="chart_editor",
+ email="chart_editor@example.com",
+ active=True,
+ )
+ db.session.add(user)
+ db.session.flush()
+ chart_on_dataset.editors = [
+ Subject(label="chart_editor", type=SubjectType.USER, user_id=user.id)
+ ]
+ db.session.flush()
+
+ with override_user(user):
+ yield user
+
+
+def _export(chart: Slice, export_related: bool = True) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand(
+ [chart.id], export_related=export_related
+ ).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
+ assert restored.deleted_at is None, "the dataset must come back out of the trash"
+ assert str(restored.uuid) == dataset_uuid
+
+ reimported = db.session.query(Slice).filter_by(uuid=chart.uuid).one()
+ assert reimported.datasource_id == restored.id
Review Comment:
The final assertion can't tell whether the chart was actually re-imported: the original chart row is never deleted, and dataset restoration keeps its id, so `reimported.datasource_id == restored.id` holds even if `ImportChartsCommand` skips the chart (the silent-skip path from `config["dataset_uuid"] in datasets`). A regression there would still pass this test. Could the test hard-delete the chart (or change something the import must overwrite) before importing, so the assertion proves the chart came back?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "L4XB (via GitHub)" <gi...@apache.org>.
L4XB commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-5993031803
Merged master to resolve the conflict in `superset/commands/chart/export.py` with #44396 and #43232. No behavior change beyond what was approved:
- `_file_content`: the semantic-view branch from #44396 comes first, and a dataset-backed chart now goes through `find_chart_dataset` in its `elif`.
- `_export`: the dataset export keeps `include_deleted=True`, followed by the annotation-layer export from #43232. Annotation-source charts are exported through `_export` too, so they get the same dataset lookup.
Checked on the merged tree:
- `tests/unit_tests/commands/chart/export_test.py`: 6 passed.
- `tests/unit_tests/{commands,charts,datasets,dashboards,annotation_layers}`: 2382 passed. The one failure, `sql_lab/test_estimate.py`, is a missing `psycopg2` in my local env.
- `ruff format --check` and `ruff check` (0.9.7) and `mypy --check-untyped-defs` (1.15.0, the pre-commit hook's stubs) are clean on the three files.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302494462
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,343 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from flask_appbuilder.security.sqla.models import User
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.commands.dashboard.export import ExportDashboardsCommand
+from superset.commands.dashboard.importers.v1 import ImportDashboardsCommand
+from superset.commands.dataset.exceptions import DatasetNotFoundError
+from superset.commands.dataset.export import ExportDatasetsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.subjects.models import Subject
+from superset.subjects.types import SubjectType
+from superset.utils import json
+from superset.utils.core import override_user
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+@pytest.fixture
+def chart_editor_without_dataset_access(
+ chart_on_dataset: Slice,
+) -> Generator[User, None, None]:
+ """Log in a user who edits the chart but holds no grant on its dataset.
+
+ The user has no role, so no ``all_datasource_access``, ``database_access``,
+ ``datasource_access``, ``schema_access`` or ``catalog_access``, and is not
+ an editor of the dataset, so ``DatasourceFilter`` hides the dataset from
+ them in the trash as well. Editing the chart is what lets ``ChartFilter``
+ hand them the chart.
+ """
+ user = User(
+ first_name="chart",
+ last_name="editor",
+ username="chart_editor",
+ email="chart_editor@example.com",
+ active=True,
+ )
+ db.session.add(user)
+ db.session.flush()
+ chart_on_dataset.editors = [
+ Subject(label="chart_editor", type=SubjectType.USER, user_id=user.id)
+ ]
+ db.session.flush()
+
+ with override_user(user):
+ yield user
+
+
+def _export(chart: Slice, export_related: bool = True) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand(
+ [chart.id], export_related=export_related
+ ).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ # Change what the import has to write back, so the assertions below fail if
+ # the chart is skipped rather than imported.
+ chart.slice_name = "renamed after the export"
+ chart.datasource_id = None
+ db.session.flush()
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
+ assert restored.deleted_at is None, "the dataset must come back out of the trash"
+ assert str(restored.uuid) == dataset_uuid
+
+ reimported = db.session.query(Slice).filter_by(uuid=chart.uuid).one()
+ assert reimported.slice_name == "my_chart"
+ assert reimported.datasource_id == restored.id
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_dashboard_filter_keeps_a_soft_deleted_dataset_through_import(
+ app_context: None,
+ chart_on_dataset: Slice,
+ mocker: MockerFixture,
+) -> None:
+ """A dashboard filter on a dataset in the trash keeps its target (#44309).
+
+ The filter's dataset lookup skipped the trash, so the export dropped the
+ target's ``datasetId`` without writing a ``datasetUuid``, and the imported
+ filter pointed at no dataset.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ target = {"datasetId": dataset.id, "column": {"name": "ds"}}
Review Comment:
This filter targets the chart's own dataset, which `ExportChartsCommand` already bundles, so the test stays green if the trash-inclusive lookup in `ExportDashboardsCommand._export` (the `found_dataset_ids` query) is reverted. The two mocked dashboard tests only assert `include_deleted=True` on the dataset export, and their `find_by_ids` stubs ignore `skip_visibility_filter`. A dashboard whose native filter or display control points at a trashed dataset that no chart in the dashboard uses would then export without that dataset. Could the target here use a second dataset that no chart on the dashboard uses, so the `datasets/` assertion only passes through the dashboard-level lookup?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44347:
URL: https://github.com/apache/superset/pull/44347#issuecomment-6232805717
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because exports for charts with hard-deleted datasets omit the dataset reference.
- The hard-delete test checks serialization only; verify whether the importer accepts this bundle or define how exports should handle it.
**Assessed commit:** `af91468fcdce`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4303030158
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+def _export(chart: Slice) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand([chart.id]).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `572dece`.
The test now imports the export after deleting the dataset and asserts that both the dataset and chart are restored, including the chart’s link to the restored dataset.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "L4XB (via GitHub)" <gi...@apache.org>.
L4XB commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4303027302
##########
superset/commands/chart/export.py:
##########
@@ -88,8 +111,8 @@ def _file_content(model: Slice) -> str:
if model.datasource_type == "semantic_view":
if model.semantic_view:
payload["datasource_ref"] = export_view_reference(model.semantic_view)
- elif model.table:
- payload["dataset_uuid"] = str(model.table.uuid)
+ elif dataset := find_chart_dataset(model):
Review Comment:
Good catch, thanks. I went with the first option in 572dece93b. Writing `dataset_uuid` only when the dataset file is in the bundle would bring back the rejected bundle that #44309 is about.
`ExportAssetsCommand` now collects the datasets in the trash that the exported charts (through `find_chart_dataset`) and the dashboards' native filters or display controls (through a shared `dashboard_dataset_ids()` helper) still name. It exports those with `include_deleted=True` after the regular passes. The DAO base filter still applies to every lookup, so this only adds datasets the user could read through the chart. A full export also still leaves out trashed datasets that nothing references.
The new `test_assets_export_carries_the_soft_deleted_datasets_it_references` trashes a chart's dataset and a filter-only dataset. It checks that both are in the `ExportAssetsCommand` bundle, and that `ImportAssetsCommand` imports it and brings both out of the trash. Without the change, the bundle has neither dataset file. The chart, dashboard, dataset, importers, export and importexport suites pass (432 + 208), and ruff and mypy are clean on the changed modules.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4302822121
##########
superset/commands/chart/export.py:
##########
@@ -55,6 +58,26 @@
REMOVE_KEYS = ["datasource_type", "datasource_name", "url_params"]
+def find_chart_dataset(model: Slice) -> SqlaTable | None:
+ """Resolve the dataset a chart is built on, soft-deleted ones included.
+
+ ``Slice.table`` is loaded through the soft-delete visibility filter, so a
+ chart whose dataset sits in the trash resolves to ``None`` even though
+ ``datasource_id`` still points at a row. Exporting such a chart off that
+ ``None`` drops both ``dataset_uuid`` and the ``datasets/`` file, and the
+ resulting bundle is rejected by Superset's own importer with
+ ``{"dataset_uuid": ["Missing data for required field."]}``.
+
+ The lookup keeps the DAO base filter, so a dataset the user may not read
+ stays unreachable here just as it does through the relationship.
+ """
+ if model.table:
+ return model.table
+ if model.datasource_type != DatasourceType.TABLE or model.datasource_id is None:
Review Comment:
The `datasource_type != DatasourceType.TABLE` guard is what keeps a semantic-view chart from resolving an unrelated dataset that happens to share its numeric `datasource_id`, but no test reaches it with `export_related=True`: the semantic-view tests either call `_file_content` or use `export_related=False`. If the guard were removed, exporting such a chart would bundle another table's dataset and database YAML, and nothing here would fail. Could you add a test that exports a semantic-view chart with `export_related=True` next to a readable table with the same ID and asserts no `datasets/` or `databases/` files appear?
##########
superset/commands/export/models.py:
##########
@@ -104,6 +114,10 @@ def run(
seen.add(file_name)
def validate(self) -> None:
- self._models = self.dao.find_by_ids(self.model_ids)
+ self._models = (
+ self.dao.find_by_ids(self.model_ids, skip_visibility_filter=True)
Review Comment:
Nothing pins the default `include_deleted=False` behavior: a regression that dropped the flag check and always passed `skip_visibility_filter=True` would make every other `ExportDatasetsCommand` caller (e.g. the dataset export endpoint) start exporting trashed datasets, and none of the new tests would fail. The access-denied tests don't catch it either, since they exercise the permission filter rather than the soft-delete one. Could you add a case with a readable soft-deleted dataset asserting that `list(ExportDatasetsCommand([dataset_id]).run())` raises `DatasetNotFoundError` while `include_deleted=True` exports it?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(chart): keep dataset_uuid in exports when the dataset is deleted [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44347:
URL: https://github.com/apache/superset/pull/44347#discussion_r4333884807
##########
tests/unit_tests/commands/chart/export_test.py:
##########
@@ -0,0 +1,177 @@
+# 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.
+"""Unit tests for ExportChartsCommand."""
+
+from __future__ import annotations
+
+from collections.abc import Generator
+
+import pytest
+import yaml
+from pytest_mock import MockerFixture
+from sqlalchemy.orm.session import Session
+
+from superset import db, security_manager
+from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.importers.v1 import ImportChartsCommand
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.models.slice import Slice
+
+
+@pytest.fixture
+def chart_on_dataset(session: Session) -> Generator[Slice, None, None]:
+ """A chart backed by a dataset, both alive."""
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="my_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ chart = Slice(
+ slice_name="my_chart",
+ viz_type="table",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="my_table",
+ params="{}",
+ )
+ session.add(chart)
+ session.flush()
+
+ yield chart
+ session.rollback()
+
+
+@pytest.fixture
+def permissive_security(mocker: MockerFixture) -> None:
+ """Grant every read/write check the export and import paths consult."""
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ mocker.patch.object(security_manager, "is_admin", return_value=True)
+
+
+def _export(chart: Slice) -> dict[str, str]:
+ return {
+ file_name: file_content()
+ for file_name, file_content in ExportChartsCommand([chart.id]).run()
+ }
+
+
+def _next_request() -> None:
+ """Drop everything loaded in this session.
+
+ ``Slice.table`` is ``lazy="subquery"``, so a relationship loaded before the
+ delete stays warm in the identity map and would mask the visibility filter.
+ Export runs in its own request in a deployment; expiring models that.
+ """
+ db.session.expire_all()
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_chart_bundle_is_importable_when_dataset_is_soft_deleted(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """A chart whose dataset was deleted still exports a complete bundle.
+
+ Deleting a dataset soft-deletes it, which hides it from ``Slice.table``
+ while the chart itself survives. The export must still name the dataset,
+ otherwise ``charts/*.yaml`` carries no ``dataset_uuid`` and the bundle is
+ rejected on import with ``Missing data for required field``.
+ """
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+ assert chart.table is None, "dataset should be hidden from the relationship"
+
+ contents = _export(chart)
+
+ chart_files = [name for name in contents if name.startswith("charts/")]
+ dataset_files = [name for name in contents if name.startswith("datasets/")]
+ assert len(chart_files) == 1
+ assert len(dataset_files) == 1, "the dataset must travel with the chart"
+
+ payload = yaml.safe_load(contents[chart_files[0]])
+ assert payload["dataset_uuid"] == dataset_uuid
+ assert yaml.safe_load(contents[dataset_files[0]])["uuid"] == dataset_uuid
+
+
+@pytest.mark.usefixtures("permissive_security")
+def test_export_import_round_trip_restores_chart_and_dataset(
+ app_context: None,
+ chart_on_dataset: Slice,
+) -> None:
+ """Export → delete → import restores both assets (issue #44309)."""
+ chart = chart_on_dataset
+ dataset = chart.table
+ dataset_uuid = str(dataset.uuid)
+
+ DatasetDAO.delete([dataset])
+ db.session.flush()
+ _next_request()
+
+ contents = _export(chart)
+
+ ImportChartsCommand(contents, overwrite=True).run()
+ db.session.flush()
+
+ restored = db.session.query(SqlaTable).filter_by(uuid=dataset.uuid).one()
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `4013704`.
The test now imports the export after deleting the dataset and asserts that both the dataset and chart are restored, including the chart’s link to the restored dataset.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org