diff --git a/superset/mcp_service/sql_lab/schemas.py b/superset/mcp_service/sql_lab/schemas.py index f7c25266fdb..729413a9d53 100644 --- a/superset/mcp_service/sql_lab/schemas.py +++ b/superset/mcp_service/sql_lab/schemas.py @@ -191,6 +191,14 @@ class ExecuteSqlResponse(BaseModel): "Check each entry in the statements array for per-statement data." ), ) + template_warning: str | None = Field( + None, + description=( + "Warning when template_params was supplied but Jinja2 rendering " + "is disabled on this Superset instance. The query was executed " + "with literal '{{ var }}' placeholders unrendered." + ), + ) class SaveSqlQueryRequest(BaseModel): diff --git a/superset/mcp_service/sql_lab/tool/execute_sql.py b/superset/mcp_service/sql_lab/tool/execute_sql.py index 45043a068f5..c1000320a4d 100644 --- a/superset/mcp_service/sql_lab/tool/execute_sql.py +++ b/superset/mcp_service/sql_lab/tool/execute_sql.py @@ -86,7 +86,7 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes try: # Import inside function to avoid initialization issues - from superset import db, security_manager + from superset import db, is_feature_enabled, security_manager from superset.models.core import Database # 1. Get database and check access @@ -134,6 +134,22 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes with event_logger.log_context(action="mcp.execute_sql.response_conversion"): response = _convert_to_response(result) + # Surface a warning when template_params is supplied but Jinja + # rendering is disabled — otherwise the params are silently dropped. + if request.template_params and not is_feature_enabled( + "ENABLE_TEMPLATE_PROCESSING" + ): + response.template_warning = ( + "template_params was supplied but Jinja2 rendering is " + "disabled on this Superset instance " + "(ENABLE_TEMPLATE_PROCESSING feature flag is off). " + "Template variables in the SQL were NOT substituted; " + "the query was executed with literal '{{ var }}' placeholders." + ) + await ctx.warning( + "template_params supplied but ENABLE_TEMPLATE_PROCESSING is off" + ) + # Log successful execution if response.success: await ctx.info( diff --git a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py index e1d3dee8e39..de67736ac55 100644 --- a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py +++ b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py @@ -229,6 +229,105 @@ class TestExecuteSql: assert "{{ table }}" in data["statements"][0]["original_sql"] assert "orders" in data["statements"][0]["executed_sql"] + @patch("superset.is_feature_enabled") + @patch("superset.security_manager") + @patch("superset.db") + @pytest.mark.asyncio + async def test_template_warning_when_flag_disabled( + self, mock_db, mock_security_manager, mock_is_feature_enabled, mcp_server + ): + """Warn when template_params is set but ENABLE_TEMPLATE_PROCESSING is off.""" + mock_database = _mock_database() + mock_database.execute.return_value = _create_select_result( + rows=[], + columns=["id"], + original_sql="SELECT * FROM users WHERE id = '{{ user_id }}'", + executed_sql="SELECT * FROM users WHERE id = '{{ user_id }}'", + ) + mock_db.session.query.return_value.filter_by.return_value.first.return_value = ( + mock_database + ) + mock_security_manager.can_access_database.return_value = True + mock_is_feature_enabled.return_value = False + + request = { + "database_id": 1, + "sql": "SELECT * FROM users WHERE id = '{{ user_id }}'", + "template_params": {"user_id": "42"}, + } + + async with Client(mcp_server) as client: + result = await client.call_tool("execute_sql", {"request": request}) + data = result.structured_content + + assert data["success"] is True + assert data["template_warning"] is not None + assert "ENABLE_TEMPLATE_PROCESSING" in data["template_warning"] + mock_is_feature_enabled.assert_called_with("ENABLE_TEMPLATE_PROCESSING") + + @patch("superset.is_feature_enabled") + @patch("superset.security_manager") + @patch("superset.db") + @pytest.mark.asyncio + async def test_no_template_warning_when_flag_enabled( + self, mock_db, mock_security_manager, mock_is_feature_enabled, mcp_server + ): + """No warning when ENABLE_TEMPLATE_PROCESSING is on.""" + mock_database = _mock_database() + mock_database.execute.return_value = _create_select_result( + rows=[{"id": 42}], + columns=["id"], + original_sql="SELECT * FROM users WHERE id = '42'", + executed_sql="SELECT * FROM users WHERE id = '42'", + ) + mock_db.session.query.return_value.filter_by.return_value.first.return_value = ( + mock_database + ) + mock_security_manager.can_access_database.return_value = True + mock_is_feature_enabled.return_value = True + + request = { + "database_id": 1, + "sql": "SELECT * FROM users WHERE id = '{{ user_id }}'", + "template_params": {"user_id": "42"}, + } + + async with Client(mcp_server) as client: + result = await client.call_tool("execute_sql", {"request": request}) + data = result.structured_content + + assert data["success"] is True + assert data["template_warning"] is None + + @patch("superset.is_feature_enabled") + @patch("superset.security_manager") + @patch("superset.db") + @pytest.mark.asyncio + async def test_no_template_warning_without_template_params( + self, mock_db, mock_security_manager, mock_is_feature_enabled, mcp_server + ): + """No warning when template_params is not supplied, even with flag off.""" + mock_database = _mock_database() + mock_database.execute.return_value = _create_select_result( + rows=[{"id": 1}], + columns=["id"], + ) + mock_db.session.query.return_value.filter_by.return_value.first.return_value = ( + mock_database + ) + mock_security_manager.can_access_database.return_value = True + mock_is_feature_enabled.return_value = False + + request = {"database_id": 1, "sql": "SELECT id FROM users"} + + async with Client(mcp_server) as client: + result = await client.call_tool("execute_sql", {"request": request}) + data = result.structured_content + + assert data["success"] is True + assert data["template_warning"] is None + mock_is_feature_enabled.assert_not_called() + @patch("superset.security_manager") @patch("superset.db") @pytest.mark.asyncio