michael-s-molina commented on code in PR #36739:
URL: https://github.com/apache/superset/pull/36739#discussion_r2631464051
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -52,36 +59,75 @@ async def execute_sql(request: ExecuteSqlRequest, ctx:
Context) -> ExecuteSqlRes
# Log SQL query details (truncated for security)
sql_preview = request.sql[:100] + "..." if len(request.sql) > 100 else
request.sql
await ctx.debug(
- "SQL query details: sql_preview=%r, sql_length=%s, has_parameters=%s"
+ "SQL query details: sql_preview=%r, sql_length=%s,
has_template_params=%s"
% (
sql_preview,
len(request.sql),
- bool(request.parameters),
+ bool(request.template_params),
)
)
logger.info("Executing SQL query on database ID: %s", request.database_id)
try:
- # Use the ExecuteSqlCore to handle all the logic
- sql_tool = ExecuteSqlCore(use_command_mode=False, logger=logger)
- result = sql_tool.run_tool(request)
+ # Import inside function to avoid initialization issues
+ from superset import db, security_manager
+ from superset.models.core import Database
+
+ # 1. Get database and check access
+ database =
db.session.query(Database).filter_by(id=request.database_id).first()
+ if not database:
+ raise SupersetErrorException(
+ SupersetError(
+ message=f"Database with ID {request.database_id} not
found",
+ error_type=SupersetErrorType.DATABASE_NOT_FOUND_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ if not security_manager.can_access_database(database):
+ raise SupersetSecurityException(
+ SupersetError(
+ message=f"Access denied to database
{database.database_name}",
+
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ # 2. Build QueryOptions
+ options = QueryOptions(
+ catalog=request.catalog,
+ schema=request.schema_name,
+ limit=request.limit,
+ timeout_seconds=request.timeout,
+ template_params=request.template_params,
+ dry_run=request.dry_run,
+ cache=CacheOptions(force_refresh=True), # No caching for MCP
Review Comment:
We could:
1. Enable caching by default in the tool
2. Add a `force_refresh` field to `ExecuteSqlRequest` with clear
instructions to LLMs:
```
force_refresh: bool = Field(
default=False,
description=(
"Bypass cache and re-execute query. "
"IMPORTANT: Only set to true when the user EXPLICITLY requests "
"fresh/updated data (e.g., 'refresh', 'get latest', 're-run'). "
"Default to false to reduce database load."
),
)
```
--
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]
