aminghadersohi commented on code in PR #40935:
URL: https://github.com/apache/superset/pull/40935#discussion_r3446958340
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -30,6 +30,58 @@
logger = logging.getLogger(__name__)
+def _build_update_props(
+ request: CreateVirtualDatasetRequest, dataset: Any
+) -> dict[str, Any]:
+ update_props: dict[str, Any] = {}
+ if request.metrics:
+ # Merge existing metrics with new ones
+ existing_metrics = [
+ {"id": m.id, "metric_name": m.metric_name} for m in dataset.metrics
+ ]
+ update_props["metrics"] = existing_metrics + [
+ m.model_dump(exclude_none=True) for m in request.metrics
+ ]
+ if request.calculated_columns:
+ # Merge existing columns with new ones
+ existing_cols = [
+ {"id": c.id, "column_name": c.column_name} for c in dataset.columns
+ ]
+ update_props["columns"] = existing_cols + [
+ c.model_dump(exclude_none=True) for c in request.calculated_columns
+ ]
+ return update_props
+
+
+def _cleanup_failed_dataset(dataset_id: int) -> None:
+ from superset.commands.dataset.delete import DeleteDatasetCommand
+
+ try:
+ DeleteDatasetCommand([dataset_id]).run()
+ except Exception as cleanup_exc:
+ logger.error(
+ "Failed to clean up dataset %s after update error: %s",
+ dataset_id,
+ cleanup_exc,
+ )
+
+
+def _update_virtual_dataset(dataset_id: int, update_props: dict[str, Any]) ->
Any:
+ from superset.commands.dataset.exceptions import (
+ DatasetInvalidError,
+ DatasetUpdateFailedError,
+ )
+ from superset.commands.dataset.update import UpdateDatasetCommand
+
+ try:
+ return UpdateDatasetCommand(dataset_id, update_props).run()
+ except Exception as exc:
+ _cleanup_failed_dataset(dataset_id)
+ if not isinstance(exc, (DatasetUpdateFailedError,
DatasetInvalidError)):
+ raise DatasetUpdateFailedError() from exc
+ raise
Review Comment:
[MEDIUM] `DatasetInvalidError` from `UpdateDatasetCommand` is re-raised here
(update phase) and caught by the outer `except DatasetInvalidError` handler at
line 170, which logs at `ctx.warning` level and returns `error=str(messages)` —
no indication that a dataset was briefly created and deleted. The
`DatasetUpdateFailedError` handler at line 193 already produces the correct
caller-facing message (`"Failed to update dataset metadata (creation rolled
back): {exc}"`). Route all update-phase failures through that handler by
converting `DatasetInvalidError` here:
```python
except Exception as exc:
_cleanup_failed_dataset(dataset_id)
if not isinstance(exc, DatasetUpdateFailedError):
raise DatasetUpdateFailedError() from exc
raise
```
This also lets you remove `DatasetInvalidError` from the imports in
`_update_virtual_dataset`. The missing companion test: add
`mock_update_instance.run.side_effect = DatasetInvalidError()` as a second case
in `test_create_virtual_dataset_update_failure_rollback` and assert `"creation
rolled back" in data["error"]` — same assertion the `DatasetUpdateFailedError`
case already makes.
--
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]
