Skip to content

fix(mcp): add created_by_fk filter, field validators, and deduplicate defaults for database tools #39113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aminghadersohi merged 2 commits into from
Apr 4, 2026
Merged

fix(mcp): add created_by_fk filter, field validators, and deduplicate defaults for database tools #39113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc8c06f
fix(mcp): add created_by_fk filter, field validators, and deduplicate…
aminghadersohi Apr 4, 2026
be7e1ab
fix(mcp): address review bot findings for database tools
aminghadersohi Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion superset/mcp_service/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,14 @@ def get_default_instructions(branding: str = "Apache Superset") -> str:
3. generate_explore_link(dataset_id, config) -> preview interactively
4. generate_chart(dataset_id, config, save_chart=True) -> save permanently

To find your own charts/dashboards:
To find your own charts/dashboards/databases:
1. get_instance_info -> get current_user.id
2. list_charts(filters=[{{"col": "created_by_fk",
"opr": "eq", "value": current_user.id}}])
3. Or: list_dashboards(filters=[{{"col": "created_by_fk",
"opr": "eq", "value": current_user.id}}])
4. Or: list_databases(filters=[{{"col": "created_by_fk",
"opr": "eq", "value": current_user.id}}])

To explore data with SQL:
1. list_datasets -> find a dataset and note its database_id
Expand Down Expand Up @@ -172,6 +174,8 @@ def get_default_instructions(branding: str = "Apache Superset") -> str:
filters=[{{"col": "created_by_fk", "opr": "eq", "value": <user_id>}}]
- My dashboards:
filters=[{{"col": "created_by_fk", "opr": "eq", "value": <user_id>}}]
- My databases:
filters=[{{"col": "created_by_fk", "opr": "eq", "value": <user_id>}}]

To modify an existing chart (add filters, change metrics, change dimensions, etc.):
1. get_chart_info(chart_id) -> examine current configuration
Expand Down
32 changes: 28 additions & 4 deletions superset/mcp_service/database/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
BaseModel,
ConfigDict,
Field,
field_validator,
model_serializer,
model_validator,
PositiveInt,
Expand All @@ -38,6 +39,10 @@
from superset.mcp_service.common.cache_schemas import MetadataCacheControl
from superset.mcp_service.constants import DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE
from superset.mcp_service.system.schemas import PaginationInfo
from superset.mcp_service.utils.schema_utils import (
parse_json_or_list,
parse_json_or_model_list,
)
from superset.utils import json


Expand All @@ -53,10 +58,14 @@ class DatabaseFilter(ColumnOperator):
"database_name",
"expose_in_sqllab",
"allow_file_upload",
"created_by_fk",
"changed_by_fk",
] = Field(
...,
description="Column to filter on. Use get_schema(model_type='database') for "
"available filter columns.",
"available filter columns. Use created_by_fk with the user "
"ID from get_instance_info's current_user to find "
"databases created by a specific user.",
)
opr: ColumnOperatorEnum = Field(
...,
Expand Down Expand Up @@ -231,6 +240,18 @@ class ListDatabasesRequest(MetadataCacheControl):
),
]

@field_validator("filters", mode="before")
@classmethod
def parse_filters(cls, v: Any) -> List[DatabaseFilter]:
"""Accept both JSON string and list of objects."""
return parse_json_or_model_list(v, DatabaseFilter, "filters")

@field_validator("select_columns", mode="before")
@classmethod
def parse_columns(cls, v: Any) -> List[str]:
"""Accept JSON array, list, or comma-separated string."""
return parse_json_or_list(v, "select_columns")
Comment on lines +243 to +253

Copilot AI Apr 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new parsing behavior for filters and select_columns (JSON-string / CSV inputs) and the newly supported created_by_fk / changed_by_fk filter columns aren’t covered by unit tests. Please add tests (e.g., in tests/unit_tests/mcp_service/database/tool/test_database_tools.py) to verify: (1) JSON-string filters is accepted and converted into DatabaseFilter models, (2) JSON-string and comma-separated select_columns are accepted, and (3) created_by_fk filtering flows through to DatabaseDAO.list (assert the DAO is called with a ColumnOperator where col == 'created_by_fk').

Copilot generated this review using guidance from repository custom instructions.

@model_validator(mode="after")
def validate_search_and_filters(self) -> "ListDatabasesRequest":
"""Prevent using both search and filters simultaneously to avoid query
Expand All @@ -253,9 +274,11 @@ class DatabaseError(BaseModel):
@classmethod
def create(cls, error: str, error_type: str) -> "DatabaseError":
"""Create a standardized DatabaseError with timestamp."""
from datetime import datetime
from datetime import datetime, timezone

return cls(error=error, error_type=error_type, timestamp=datetime.now())
return cls(
error=error, error_type=error_type, timestamp=datetime.now(timezone.utc)
)


class GetDatabaseInfoRequest(MetadataCacheControl):
Expand Down Expand Up @@ -285,7 +308,8 @@ def _humanize_timestamp(dt: datetime | None) -> str | None:
"""Convert a datetime to a humanized string like '2 hours ago'."""
if dt is None:
return None
return humanize.naturaltime(datetime.now() - dt)
now = datetime.now(dt.tzinfo) if dt.tzinfo else datetime.now()
return humanize.naturaltime(now - dt)


def serialize_database_object(database: Any) -> DatabaseInfo | None:
Expand Down
12 changes: 2 additions & 10 deletions superset/mcp_service/database/tool/list_databases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,6 @@

logger = logging.getLogger(__name__)

# Minimal defaults for reduced token usage - users can request more via select_columns
DEFAULT_DATABASE_COLUMNS = [
"id",
"database_name",
"backend",
"expose_in_sqllab",
"changed_on_humanized",
]


@tool(
tags=["core"],
Expand Down Expand Up @@ -100,6 +91,7 @@ async def list_databases(request: ListDatabasesRequest, ctx: Context) -> Databas
try:
from superset.daos.database import DatabaseDAO
from superset.mcp_service.common.schema_discovery import (
DATABASE_DEFAULT_COLUMNS,
DATABASE_SORTABLE_COLUMNS,
get_all_column_names,
get_database_columns,
Expand All @@ -120,7 +112,7 @@ def _serialize_database(
output_schema=DatabaseInfo,
item_serializer=_serialize_database,
filter_type=DatabaseFilter,
default_columns=DEFAULT_DATABASE_COLUMNS,
default_columns=DATABASE_DEFAULT_COLUMNS,

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Apr 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: DATABASE_DEFAULT_COLUMNS includes changed_on_humanized but not changed_on; since the serializer computes humanized time from changed_on, default responses will produce changed_on_humanized=None for row-based DAO results. Ensure changed_on is also loaded when using default columns. [logic error]

Severity Level: Major ⚠️ 
- ❌ list_databases default response has changed_on_humanized always null.
- ⚠️ MCP agents cannot display relative last-modified time.
Suggested change
default_columns=DATABASE_DEFAULT_COLUMNS,
default_columns=[*DATABASE_DEFAULT_COLUMNS, "changed_on"],
Steps of Reproduction ✅ 
1. Start Superset with MCP service enabled and call the `list_databases` FastMCP tool
without providing `select_columns` (default usage), which hits `list_databases()` in
`superset/mcp_service/database/tool/list_databases.py:56-63`.

2. Inside `list_databases`, a `ModelListCore` is constructed at
`superset/mcp_service/database/tool/list_databases.py:110-121` with
`default_columns=DATABASE_DEFAULT_COLUMNS` (`list_databases.py:115`), where
`DATABASE_DEFAULT_COLUMNS` is defined as `["id", "database_name", "backend",
"expose_in_sqllab", "changed_on_humanized"]` in
`superset/mcp_service/common/schema_discovery.py:61-67`.

3. Because `ListDatabasesRequest.select_columns` has a default empty list
(`superset/mcp_service/database/schemas.py:125-132`), `ModelListCore.run_tool`
(`superset/mcp_service/mcp_core.py:83-91`) treats it as falsy and sets `columns_to_load =
self.default_columns`, then calls `DatabaseDAO.list(..., columns=columns_to_load)` at
`mcp_core.py:93-102`; `BaseDAO.list` (`superset/daos/base.py:25-49,74-86`) builds a
SQLAlchemy query selecting only real column attributes (`id`, `database_name`, `backend`,
`expose_in_sqllab`) and returns Row objects that do NOT include `changed_on`.

4. Each Row is passed to `serialize_database_object` via `_serialize_database` at
`list_databases.py:103-107`, and `serialize_database_object`
(`superset/mcp_service/database/schemas.py:233-268`) calls `getattr(database,
"changed_on", None)` and `_humanize_timestamp(getattr(database, "changed_on", None))`;
since the Row lacks `changed_on`, `getattr` returns `None`, `_humanize_timestamp` returns
`None`, and both `changed_on` and `changed_on_humanized` end up as `None`. The final
response is produced by `result.model_dump(context={"select_columns": columns_to_filter})`
at `list_databases.py:145-154`, and `DatabaseInfo._filter_fields_by_context`
(`database/schemas.py:61-76`) filters fields to the requested columns, which now include
`"changed_on_humanized"` but not `"changed_on"`, so the client observes
`changed_on_humanized: null` for every database in the default `list_databases` response.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset/mcp_service/database/tool/list_databases.py
**Line:** 115:115
**Comment:**
	*Logic Error: `DATABASE_DEFAULT_COLUMNS` includes `changed_on_humanized` but not `changed_on`; since the serializer computes humanized time from `changed_on`, default responses will produce `changed_on_humanized=None` for row-based DAO results. Ensure `changed_on` is also loaded when using default columns.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

search_columns=["database_name"],
list_field_name="databases",
output_list_schema=DatabaseList,
Expand Down
2 changes: 1 addition & 1 deletion superset/mcp_service/mcp_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ def __init__(
Initialize the schema discovery core.

Args:
model_type: The type of model (chart, dataset, dashboard)
model_type: The type of model (chart, dataset, dashboard, database)
dao_class: The DAO class to query for filter columns
output_schema: Pydantic schema for the response (e.g., ModelSchemaInfo)
select_columns: Column metadata (List[ColumnMetadata] or similar)
Expand Down
20 changes: 10 additions & 10 deletions tests/unit_tests/mcp_service/database/tool/test_database_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,16 @@


def create_mock_database(
database_id=1,
database_name="examples",
backend="postgresql",
expose_in_sqllab=True,
allow_ctas=False,
allow_cvas=False,
allow_dml=False,
allow_file_upload=False,
allow_run_async=False,
):
database_id: int = 1,
database_name: str = "examples",
backend: str = "postgresql",
expose_in_sqllab: bool = True,
allow_ctas: bool = False,
allow_cvas: bool = False,
allow_dml: bool = False,
allow_file_upload: bool = False,
allow_run_async: bool = False,
) -> MagicMock:
"""Factory function to create mock database objects with sensible defaults."""
database = MagicMock()
database.id = database_id
Expand Down
Loading