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: [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]
