aminghadersohi commented on code in PR #36739:
URL: https://github.com/apache/superset/pull/36739#discussion_r2631353412
##########
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:
given the rate at which some clients can call the mcp with the same "list
datasets" call i would say we need caching at this layer to save the database
from melting. what do you think?
--
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]
