refactor: Migrates the MCP `execute_sql` tool to use the SQL execution API #36739

michael-s-molina · 2025-12-18T14:15:39Z

Summary

Migrates the MCP execute_sql tool to use the unified Database.execute() API introduced in #36529. This consolidates SQL execution code and fixes a critical security gap where Row-Level Security (RLS) was not being applied to MCP queries.

Key Changes

Security Fix: MCP queries now have RLS applied automatically via AST transformation
Simplified Architecture: Removed sql_lab_utils.py and execute_sql_core.py (~300 lines), replaced with direct Database.execute() call
Jinja2 Templates: The old {placeholder} syntax is removed. Use Jinja2 {{ placeholder }} syntax with template_params.
Multi-Statement Support: Response now includes per-statement execution info
New Options: Added catalog and dry_run parameters

Feature Comparison

Feature	Before	After	Notes
Security
Row-Level Security (RLS)	❌ Not applied	✅ Applied via AST	Critical fix
DML Permission Check	✅ Basic	✅ Full	Uses database `allow_dml` setting
Disallowed Functions	✅ Basic	✅ Full per-engine	Engine-specific checks
Database Access Check	✅ Yes	✅ Yes	No change
Query Processing
Jinja2 Templates	❌ Not supported	✅ Full support	New `template_params` field
SQL Mutation Hook	❌ Not applied	✅ Applied	Custom SQL transformations
Multi-Statement SQL	❌ No	✅ Yes	New `statements` field in response
LIMIT Application	✅ Simple regex	✅ Proper parsing	More reliable
Execution
Sync Execution	✅ Yes	✅ Yes	No change
Timeout Handling	⚠️ Basic	✅ Full	Uses database config
Query Logging	❌ No	✅ Via QUERY_LOGGER	If configured
Results
Result Caching	❌ No	❌ No	Explicitly disabled via `force_refresh`
Query Audit Trail	❌ No	✅ Yes	Query model created for audit
Parameters
`database_id`	✅ Yes	✅ Yes	No change
`sql`	✅ Yes	✅ Yes	No change
`schema`	✅ Yes	✅ Yes	No change
`catalog`	❌ No	✅ Yes	New
`limit`	✅ 1-10000	✅ 1-10000	No change
`timeout`	✅ 1-300s	✅ 1-300s	No change
`template_params`	❌ No	✅ Yes	New - Jinja2 parameters
`dry_run`	❌ No	✅ Yes	New - Returns transformed SQL only

TESTING INSTRUCTIONS

All 12 execute_sql unit tests pass
All 252 MCP service tests pass

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

…abase.execute()` API

michael-s-molina · 2025-12-18T14:17:06Z

@aminghadersohi @betodealmeida @villebro @Antonio-RiveroMartnez I'll open separate threads for some questions that I have regarding this consolidation. Please answer in each thread so we don't lose track 😉

codecov · 2025-12-18T14:25:23Z

Codecov Report

❌ Patch coverage is 0% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.26%. Comparing base (f51f7f3) to head (0d68701).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/sql_lab/tool/execute_sql.py	0.00%	36 Missing ⚠️
superset/mcp_service/sql_lab/schemas.py	0.00%	12 Missing ⚠️
superset/mcp_service/utils/schema_utils.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #36739       +/-   ##
===========================================
+ Coverage        0   68.26%   +68.26%     
===========================================
  Files           0      638      +638     
  Lines           0    47520    +47520     
  Branches        0     5180     +5180     
===========================================
+ Hits            0    32438    +32438     
- Misses          0    13804    +13804     
- Partials        0     1278     +1278

Flag	Coverage Δ
hive	`43.14% <0.00%> (?)`
mysql	`66.26% <0.00%> (?)`
postgres	`66.31% <0.00%> (?)`
presto	`46.75% <0.00%> (?)`
python	`68.23% <0.00%> (?)`
sqlite	`66.02% <0.00%> (?)`
unit	`100.00% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

michael-s-molina · 2025-12-18T14:26:19Z

superset/mcp_service/sql_lab/tool/execute_sql.py

+            )
+
+        # 2. Build QueryOptions
+        options = QueryOptions(


I'm assuming we want all queries from MCP tools to be audited so I kept the default behavior of adding entries to the query table. Let me know if there's any case where we don't want this.

auditing by default makes sense for MCP queries. good for compliance and debugging AI agent behavior. we can add a config flag later if performance becomes a concern at high volume.

michael-s-molina · 2025-12-18T14:27:00Z

superset/mcp_service/sql_lab/tool/execute_sql.py

+            timeout_seconds=request.timeout,
+            template_params=request.template_params,
+            dry_run=request.dry_run,
+            cache=CacheOptions(force_refresh=True),  # No caching for MCP


Currently, query caching is disabled. Is this what we want for MCP tools or should queries be cached? I know that MCP has a middleware that caches tool calls but this is different.

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?

I agree. We have the following options:

Pass a specific timeout for MCP tools with CacheOptions.timeout

Use the per-database setting with database.cache_timeout

Use the default config CACHE_DEFAULT_TIMEOUT = 300 seconds (5 minutes)

maybe default to a short TTL (30-60s) as a middle ground
can tools op out if they need?
the MCP middleware caching operates at a different layer (request/response), so DB query caching still adds value

We could:

Enable caching by default in the tool

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." ), )

I applied the above changes on 0d68701

michael-s-molina · 2025-12-18T14:27:32Z

superset/mcp_service/sql_lab/schemas.py

    )
-    parameters: dict[str, Any] | None = Field(
-        None, description="Parameters for query substitution"
+    template_params: dict[str, Any] | None = Field(


I replaced the { parameter } syntax with Jinja {{ parameter }} as I'm assuming that would be ok. Let me know if you see a problem with that.

maybe just leave a note about the breaking change in UPDATING.md

@aminghadersohi The MCP is not part of an official release yet and UPDATING.md is reserved for when you are upgrading between versions. By the way, we should remove this documentation from UPDATING.md. This is more appropriate for Release Notes or a page in the Developer Portal. @aminghadersohi Could you open a PR to remove those docs from UPDATING.md?

cc @sadpandajoe as the release manager.

villebro

Looks great. Noticed what's likely minor artifact from earlier iterations, but other than that looks good to go.

villebro · 2025-12-18T19:51:52Z

superset/mcp_service/sql_lab/tool/execute_sql.py

+if TYPE_CHECKING:
+    pass
+


Suggested change

if TYPE_CHECKING:

pass

villebro · 2025-12-18T19:52:00Z

superset/mcp_service/sql_lab/tool/execute_sql.py

+from __future__ import annotations
+
 import logging
+from typing import Any, TYPE_CHECKING


Suggested change

from typing import Any, TYPE_CHECKING

from typing import Any

refactor: Migrates the MCP execute_sql tool to use the unified `Dat…

f9162fd

…abase.execute()` API

michael-s-molina requested review from Antonio-RiveroMartnez, betodealmeida and villebro December 18, 2025 14:15

michael-s-molina self-assigned this Dec 18, 2025

michael-s-molina added this to Superset Extensions Dec 18, 2025

github-project-automation bot moved this to To Do in Superset Extensions Dec 18, 2025

pull-request-size bot added the size/XXL label Dec 18, 2025

michael-s-molina moved this from To Do to In Progress in Superset Extensions Dec 18, 2025

michael-s-molina moved this from In Progress to In Review in Superset Extensions Dec 18, 2025

michael-s-molina commented Dec 18, 2025

View reviewed changes

michael-s-molina moved this from In Review to In Progress in Superset Extensions Dec 18, 2025

michael-s-molina changed the title ~~refactor: Migrates the MCP execute_sql tool to use the unified Database.execute() API~~ refactor: Migrates the MCP execute_sql tool to use the SQL execution API Dec 18, 2025

Enables query caching

0d68701

michael-s-molina marked this pull request as ready for review December 18, 2025 19:46

michael-s-molina moved this from In Progress to In Review in Superset Extensions Dec 18, 2025

dosubot bot added authentication:row-level-security Related to Row Level Security change:backend Requires changing the backend labels Dec 18, 2025

villebro approved these changes Dec 18, 2025

View reviewed changes

refactor: Migrates the MCP execute_sql tool to use the SQL execution API #36739

Are you sure you want to change the base?

refactor: Migrates the MCP execute_sql tool to use the SQL execution API #36739

Conversation

michael-s-molina commented Dec 18, 2025

Summary

Key Changes

Feature Comparison

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

michael-s-molina commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michael-s-molina Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-s-molina Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor: Migrates the MCP `execute_sql` tool to use the SQL execution API #36739

refactor: Migrates the MCP `execute_sql` tool to use the SQL execution API #36739

michael-s-molina commented Dec 18, 2025 •

edited

Loading

codecov bot commented Dec 18, 2025 •

edited

Loading

michael-s-molina Dec 18, 2025 •

edited

Loading

michael-s-molina Dec 18, 2025 •

edited

Loading