gabotorresruiz commented on code in PR #43129:
URL: https://github.com/apache/superset/pull/43129#discussion_r3784857342
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -213,6 +217,21 @@ async def create_virtual_dataset(
url=None,
error=f"Failed to update dataset metadata (creation rolled back):
{exc}",
)
+ except SupersetGenericDBErrorException:
+ logger.warning("Virtual dataset SQL validation failed", exc_info=True)
+ await ctx.warning("Virtual dataset SQL failed validation")
+ return CreateVirtualDatasetResponse(
+ id=None,
+ dataset_name=request.dataset_name,
+ sql=request.sql,
+ database_id=request.database_id,
+ columns=[],
+ url=None,
+ error=(
+ "Dataset SQL could not be executed. Check the query syntax, "
Review Comment:
This block is the blocker: the caller who submitted this SQL already passed
`raise_for_access` for it in `CreateDatasetCommand.validate()`, sees the same
driver messages in SQL Lab, and can get this exact text from `execute_sql` on
this same MCP service (I verified all three empirically, including on master
where the pre-fix ToolError already contained the raw message). Sanitizing here
removes the one field the LLM needs to self-correct, which is the stated goal
of the PR. Suggest:
```python
except SupersetGenericDBErrorException as exc:
logger.warning("Virtual dataset SQL validation failed",
exc_info=True)
await ctx.warning(f"Virtual dataset SQL failed validation: {exc}")
return CreateVirtualDatasetResponse(
...
error=f"Dataset SQL could not be executed: {exc}",
)
```
and in `test_create_virtual_dataset_sql_error_is_actionable`, assert the
driver detail is present rather than absent.
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -67,14 +68,17 @@ def _cleanup_failed_dataset(dataset_id: int) -> None:
def _update_virtual_dataset(dataset_id: int, update_props: dict[str, Any]) ->
Any:
- from superset.commands.dataset.exceptions import DatasetUpdateFailedError
+ 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):
+ if not isinstance(exc, (DatasetInvalidError,
DatasetUpdateFailedError)):
Review Comment:
Not a blocker, but worth knowing since this commit rewrites what the caller
is told on this path: I exercised it live on this branch with a duplicate
metric name. The response is `id=null` with the validation message, but
`_cleanup_failed_dataset` soft-deletes the row, so retrying the same
`dataset_name` with a corrected metric is then rejected with the soft-deleted
twin error. The retry loop this error message invites runs straight into that
wall. Fine as a follow-up rather than in this PR, but the error here should
probably either mention that the name is now occupied or the cleanup should
hard-delete. Small cosmetic note on the same path: the propagated messages
render with the lazy-string repr, e.g. `{'metrics': [l'One or more metrics are
duplicated']}`.
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -213,6 +217,21 @@ async def create_virtual_dataset(
url=None,
error=f"Failed to update dataset metadata (creation rolled back):
{exc}",
)
+ except SupersetGenericDBErrorException:
Review Comment:
Separate from the sanitization question and not a blocker:
`SupersetSecurityException` from the same `fetch_metadata()` call still escapes
as a protocol-level tool error. `get_virtual_table_metadata` raises it for
mutating or multi-statement SQL, and `validate()` never catches that case
because `raise_for_access` returns early for users with database access. I
re-verified on this head: `create_virtual_dataset` with `DELETE FROM
sample_events` comes back as a raw tool error instead of the structured
response. Happy to see it as a follow-up if you want to keep this PR focused.
--
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]
