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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
