I3eka commented on code in PR #43129:
URL: https://github.com/apache/superset/pull/43129#discussion_r3785136087
##########
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:
Confirmed: SupersetSecurityException is raised before the database-error
handler and still becomes a protocol-level ToolError. That is a valid
structured-error follow-up, but separate from preserving driver diagnostics, so
I left it unchanged here.
##########
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:
Agreed. The authenticated tool already checks Dataset write and database/SQL
access before metadata fetch, while the same authorized principal receives
driver diagnostics in execute_sql and SQL Lab. Restored the raw driver message
in the structured response in 9cfd706472 and made the regression assert Invalid
column name. The focused test passes.
##########
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:
Confirmed: DeleteDatasetCommand soft-deletes the partially created dataset,
so an immediate retry with the same name can collide. This is a valid
follow-up, but changing cleanup semantics is broader than this focused
SQL-error patch, so I left it unchanged here.
--
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]
