You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2025/11/06 21:58:51 UTC
[PR] feat(mcp): implement selective column serialization for list tools [superset]
aminghadersohi opened a new pull request, #36035:
URL: https://github.com/apache/superset/pull/36035
### SUMMARY
This PR implements selective column serialization for MCP service list tools (charts, dashboards, datasets). Previously, when users specified `select_columns` to limit which fields to return, the API would return all fields with `null` values for unselected columns. This was confusing and wasteful.
**Problem:**
- List tools accepted `select_columns` parameter but ignored it during serialization
- All fields were always returned, with `null` for columns not queried from database
- Impossible to distinguish "field is null" from "field was not selected"
- Wasted bandwidth sending unnecessary nulls
**Solution:**
- Updated serializers for charts, dashboards, and datasets to use Pydantic's `model_dump(exclude=...)`
- Only requested columns (plus `id` which is always included) are present in response
- Unselected fields are completely excluded from JSON response
- Cleaner API responses for LLM agents and API consumers
**Implementation Details:**
- Chart serializer: `list_charts.py` - Added `_serialize_chart()` with column filtering
- Dashboard serializer: `list_dashboards.py` - Added `_serialize_dashboard()` with column filtering
- Dataset serializer: `list_datasets.py` - Added `_serialize_dataset()` with column filtering
- All serializers follow same pattern: build full model, then exclude unselected fields
- Primary key (`id`) is always included regardless of selection
**Example:**
Before (requesting only `id` and `slice_name`):
```json
{
"id": 123,
"slice_name": "Sales Chart",
"viz_type": null,
"datasource_name": null,
"url": null,
"description": null,
"cache_timeout": null,
"changed_by": null,
"changed_on": null,
"uuid": null,
"tags": null,
"owners": null
}
```
After (requesting only `id` and `slice_name`):
```json
{
"id": 123,
"slice_name": "Sales Chart"
}
```
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - Backend API behavior change only, no UI changes
### TESTING INSTRUCTIONS
**Prerequisites:**
- Superset instance with MCP service configured
- Test data (charts, dashboards, datasets)
**Test 1: Chart listing with column selection**
```bash
# Using MCP client or direct API call
# Request only specific columns
curl -X POST http://localhost:5008/list_charts \
-H "Content-Type: application/json" \
-d '{
"page": 1,
"page_size": 5,
"select_columns": ["id", "slice_name", "viz_type"]
}'
# Verify response only contains: id, slice_name, viz_type
# Verify no null fields for unselected columns
```
**Test 2: Dashboard listing with column selection**
```bash
curl -X POST http://localhost:5008/list_dashboards \
-H "Content-Type: application/json" \
-d '{
"page": 1,
"page_size": 5,
"select_columns": ["id", "dashboard_title"]
}'
# Verify response only contains: id, dashboard_title
```
**Test 3: Dataset listing with column selection**
```bash
curl -X POST http://localhost:5008/list_datasets \
-H "Content-Type: application/json" \
-d '{
"page": 1,
"page_size": 5,
"select_columns": ["id", "table_name", "schema"]
}'
# Verify response only contains: id, table_name, schema
```
**Test 4: Default behavior (no select_columns specified)**
```bash
curl -X POST http://localhost:5008/list_charts \
-H "Content-Type: application/json" \
-d '{"page": 1, "page_size": 5}'
# Verify response contains all default columns as before
```
**Expected Results:**
- ✅ Only requested columns appear in response
- ✅ ID always included even if not requested
- ✅ No null fields for unselected columns
- ✅ Response size smaller when fewer columns selected
- ✅ Default behavior unchanged when select_columns not provided
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI: No
- [ ] Includes DB Migration: No
- [x] Introduces new feature or API: Yes (enhances existing list tools)
- [ ] Removes existing feature or API: No
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #36035:
URL: https://github.com/apache/superset/pull/36035#issuecomment-3499579589
## Addressed All Korbit AI Review Feedback
Thanks for the thorough review\! I've addressed all 5 points by creating a robust utility function:
### Changes Made
**Created `filter_model_fields()` utility** in `superset/mcp_service/utils/__init__.py`:
#### 1. ✅ Required Field Validation
- Automatically detects and includes all required fields
- Prevents ValidationError during model reconstruction
- Uses `field_info.is_required()` to identify required fields
#### 2. ✅ Comprehensive Error Handling
- Try-except with specific `ValidationError` handling
- Catches general exceptions as fallback
- Returns full model on any error (graceful degradation)
- Logs warnings/errors for debugging
#### 3. ✅ ID Field Validation
- Validates `always_include` fields exist in model before adding
- Only adds fields that are actually in `model_fields`
- No assumptions about field existence
#### 4. ✅ Separation of Concerns
- Filtering logic extracted to reusable `filter_model_fields()` function
- Serializers now just call: `filter_model_fields(model, cols, ModelClass, always_include={"id"})`
- Clean 5-line implementation per serializer
#### 5. ✅ Type Safety
- Uses generic TypeVar `T` bound to BaseModel
- Fully typed function signature
- Works with any Pydantic model
### Implementation
```python
def filter_model_fields(
model: T,
requested_columns: List[str] | None,
model_class: Type[T],
always_include: Set[str] | None = None,
) -> T:
# Validates required fields
required_fields = {
name for name, field_info in model_class.model_fields.items()
if field_info.is_required()
}
# Validates always_include fields exist
for field in always_include:
if field in all_fields:
requested_fields.add(field)
# Error handling with fallback
try:
dumped = model.model_dump(exclude=exclude_fields)
return model_class(**dumped)
except ValidationError as e:
logger.warning("Validation failed, returning full model")
return model
except Exception as e:
logger.error("Unexpected error, returning full model")
return model
```
### Simplified Serializers
All three serializers now use the utility:
```python
def _serialize_chart(obj: "Slice | None", cols: Any) -> ChartInfo | None:
from superset.mcp_service.utils import filter_model_fields
full_chart = serialize_chart_object(cast(ChartLike | None, obj))
if not full_chart:
return None
requested_cols = cols if isinstance(cols, list) else None
return filter_model_fields(
full_chart, requested_cols, ChartInfo, always_include={"id"}
)
```
All pre-commit checks pass. Ready for re-review\!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #36035:
URL: https://github.com/apache/superset/pull/36035#issuecomment-3499567590
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/36035?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 48.48%. Comparing base ([`76d897e`](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f844247`](https://app.codecov.io/gh/apache/superset/commit/f844247bb6e1b6be20b4b0409041a16aafbe55c4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2852 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #36035 +/- ##
===========================================
- Coverage 60.48% 48.48% -12.01%
===========================================
Files 1931 622 -1309
Lines 76236 45717 -30519
Branches 8568 4975 -3593
===========================================
- Hits 46114 22165 -23949
+ Misses 28017 22927 -5090
+ Partials 2105 625 -1480
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/36035/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/36035/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `44.20% <ø> (-4.95%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/36035/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/36035/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.76% <ø> (-6.04%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/36035/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.47% <ø> (-15.03%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/36035/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.36%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/36035?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #36035:
URL: https://github.com/apache/superset/pull/36035#discussion_r2505975763
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -79,8 +80,8 @@ class ChartLike(Protocol):
class ChartInfo(BaseModel):
"""Full chart model with all possible attributes."""
- id: int = Field(..., description="Chart ID")
- slice_name: str = Field(..., description="Chart name")
+ id: int | None = Field(None, description="Chart ID")
+ slice_name: str | None = Field(None, description="Chart name")
Review Comment:
this change is intentional. nobody is using this service right now.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -252,8 +259,8 @@ class GetDashboardInfoRequest(MetadataCacheControl):
class DashboardInfo(BaseModel):
- id: int = Field(..., description="Dashboard ID")
- dashboard_title: str = Field(..., description="Dashboard title")
+ id: int | None = Field(None, description="Dashboard ID")
+ dashboard_title: str | None = Field(None, description="Dashboard title")
Review Comment:
no. this change is intentional
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #36035:
URL: https://github.com/apache/superset/pull/36035#discussion_r2501008590
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -73,10 +74,29 @@ async def list_dashboards(
from superset.daos.dashboard import DashboardDAO
+ def _serialize_dashboard(obj: Any, cols: Any) -> DashboardInfo | None:
+ """Serialize dashboard object with column selection."""
+ full_dashboard = serialize_dashboard_object(obj)
+ if not full_dashboard:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(DashboardInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
+ exclude_fields = all_fields - requested_fields
+
+ # Create new model instance with excluded fields removed
+ dumped = full_dashboard.model_dump(exclude=exclude_fields)
+ return DashboardInfo(**dumped)
Review Comment:
### Unsafe model reconstruction from dumped data <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code creates a new DashboardInfo instance from dumped data, but this may fail if required fields are excluded or if the dumped data contains fields that DashboardInfo doesn't accept.
###### Why this matters
This could cause runtime errors when creating the DashboardInfo instance, especially if required fields are excluded from the model or if there are validation issues with the reconstructed model.
###### Suggested change ∙ *Feature Preview*
Add error handling and validation when reconstructing the model:
```python
try:
dumped = full_dashboard.model_dump(exclude=exclude_fields)
return DashboardInfo(**dumped)
except Exception as e:
logger.warning(f"Failed to create filtered DashboardInfo: {e}")
return full_dashboard
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6958c7fc-2804-4f2c-bf26-1345af8bf17d -->
[](6958c7fc-2804-4f2c-bf26-1345af8bf17d)
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -73,10 +74,29 @@ async def list_dashboards(
from superset.daos.dashboard import DashboardDAO
+ def _serialize_dashboard(obj: Any, cols: Any) -> DashboardInfo | None:
+ """Serialize dashboard object with column selection."""
+ full_dashboard = serialize_dashboard_object(obj)
+ if not full_dashboard:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(DashboardInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
Review Comment:
### Hardcoded field assumption without validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes 'id' is always a valid field in DashboardInfo, but doesn't verify this assumption, which could lead to issues if the model structure changes.
###### Why this matters
If the DashboardInfo model doesn't have an 'id' field or if it's renamed, this logic will still try to include it, potentially causing unexpected behavior or errors.
###### Suggested change ∙ *Feature Preview*
Verify that 'id' exists in the model fields before including it:
```python
all_fields = set(DashboardInfo.model_fields.keys())
requested_fields = set(cols)
if "id" in all_fields:
requested_fields.add("id")
exclude_fields = all_fields - requested_fields
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cdcb77f6-1702-4e92-8134-82036db4ab8a -->
[](cdcb77f6-1702-4e92-8134-82036db4ab8a)
##########
superset/mcp_service/dataset/tool/list_datasets.py:
##########
@@ -102,11 +103,30 @@ async def list_datasets(request: ListDatasetsRequest, ctx: Context) -> DatasetLi
try:
from superset.daos.dataset import DatasetDAO
+ def _serialize_dataset(obj: Any, cols: Any) -> DatasetInfo | None:
+ """Serialize dataset object with column selection."""
+ full_dataset = serialize_dataset_object(obj)
+ if not full_dataset:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(DatasetInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
+ exclude_fields = all_fields - requested_fields
+
+ # Create new model instance with excluded fields removed
+ dumped = full_dataset.model_dump(exclude=exclude_fields)
+ return DatasetInfo(**dumped)
Review Comment:
### Potential validation error when excluding required fields <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code creates a new DatasetInfo instance from a dumped dictionary, but this may fail if the excluded fields are required fields in the DatasetInfo model.
###### Why this matters
If any of the excluded fields are required (non-optional) in the DatasetInfo Pydantic model, attempting to create a new instance without them will raise a validation error, causing the function to crash.
###### Suggested change ∙ *Feature Preview*
Add validation to ensure required fields are never excluded, or handle the potential ValidationError:
```python
from pydantic import ValidationError
def _serialize_dataset(obj: Any, cols: Any) -> DatasetInfo | None:
"""Serialize dataset object with column selection."""
full_dataset = serialize_dataset_object(obj)
if not full_dataset:
return None
# If columns were specified, exclude fields not in the selection
# Always include 'id' as it's the primary identifier
if cols and isinstance(cols, list):
all_fields = set(DatasetInfo.model_fields.keys())
requested_fields = set(cols) | {"id"} # Always include id
# Get required fields to ensure they're never excluded
required_fields = {
name for name, field in DatasetInfo.model_fields.items()
if field.is_required()
}
requested_fields |= required_fields # Always include required fields
exclude_fields = all_fields - requested_fields
try:
# Create new model instance with excluded fields removed
dumped = full_dataset.model_dump(exclude=exclude_fields)
return DatasetInfo(**dumped)
except ValidationError:
# Fallback to full dataset if validation fails
return full_dataset
return full_dataset
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a83ef902-b9be-4b34-a131-e2e304a38d49 -->
[](a83ef902-b9be-4b34-a131-e2e304a38d49)
##########
superset/mcp_service/chart/tool/list_charts.py:
##########
@@ -95,8 +95,23 @@ async def list_charts(request: ListChartsRequest, ctx: Context) -> ChartList:
from superset.daos.chart import ChartDAO
def _serialize_chart(obj: "Slice | None", cols: Any) -> ChartInfo | None:
- """Serialize chart object with proper type casting."""
- return serialize_chart_object(cast(ChartLike | None, obj))
+ """Serialize chart object with proper type casting and column selection."""
+ full_chart = serialize_chart_object(cast(ChartLike | None, obj))
+ if not full_chart:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(ChartInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
+ exclude_fields = all_fields - requested_fields
+
+ # Create new model instance with excluded fields removed
+ dumped = full_chart.model_dump(exclude=exclude_fields)
+ return ChartInfo(**dumped)
Review Comment:
### Function has multiple responsibilities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The _serialize_chart function violates the Single Responsibility Principle by handling both serialization and field filtering logic.
###### Why this matters
Mixing these responsibilities makes the code harder to maintain and test. Changes to either serialization or filtering logic could affect the other, increasing the risk of bugs.
###### Suggested change ∙ *Feature Preview*
Split the function into two separate functions:
```python
def _serialize_chart(obj: "Slice | None") -> ChartInfo | None:
return serialize_chart_object(cast(ChartLike | None, obj))
def _filter_chart_fields(chart: ChartInfo, cols: list) -> ChartInfo:
all_fields = set(ChartInfo.model_fields.keys())
requested_fields = set(cols) | {"id"}
exclude_fields = all_fields - requested_fields
return ChartInfo(**chart.model_dump(exclude=exclude_fields))
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4b605e0f-e877-405b-9da3-d5acc64b61ad -->
[](4b605e0f-e877-405b-9da3-d5acc64b61ad)
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -73,10 +74,29 @@ async def list_dashboards(
from superset.daos.dashboard import DashboardDAO
+ def _serialize_dashboard(obj: Any, cols: Any) -> DashboardInfo | None:
Review Comment:
### Overly permissive type hints <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function uses 'Any' type hints for both parameters, which reduces type safety and makes the code less maintainable.
###### Why this matters
Using 'Any' bypasses Python's type checking system, making it harder to catch type-related bugs early and reducing code clarity for other developers.
###### Suggested change ∙ *Feature Preview*
```python
def _serialize_dashboard(
obj: DashboardDAO.Model,
cols: list[str] | None
) -> DashboardInfo | None:
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d3a9f123-de67-42c0-bae2-dc6d2a9c99a7 -->
[](d3a9f123-de67-42c0-bae2-dc6d2a9c99a7)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36035:
URL: https://github.com/apache/superset/pull/36035#issuecomment-3499545460
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #36035:
URL: https://github.com/apache/superset/pull/36035
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36035:
URL: https://github.com/apache/superset/pull/36035#discussion_r2501022167
##########
superset/mcp_service/chart/tool/list_charts.py:
##########
@@ -95,8 +95,23 @@ async def list_charts(request: ListChartsRequest, ctx: Context) -> ChartList:
from superset.daos.chart import ChartDAO
def _serialize_chart(obj: "Slice | None", cols: Any) -> ChartInfo | None:
- """Serialize chart object with proper type casting."""
- return serialize_chart_object(cast(ChartLike | None, obj))
+ """Serialize chart object with proper type casting and column selection."""
+ full_chart = serialize_chart_object(cast(ChartLike | None, obj))
+ if not full_chart:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(ChartInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing required fields in column selection</b></div>
<div id="fix">
The column selection logic in `_serialize_chart` only ensures 'id' is included but misses other required fields like 'slice_name'. This causes a ValidationError when creating `ChartInfo` if `select_columns` excludes required fields, breaking chart listing functionality. The `ChartInfo` model requires both 'id' and 'slice_name' fields, so the fix updates `requested_fields` to always include both.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
requested_fields = set(cols) | {"id", "slice_name"} # Always include id and slice_name
````
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36035#issuecomment-3499545460>#88d182</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #36035:
URL: https://github.com/apache/superset/pull/36035#issuecomment-3505610394
## Addressed Copilot Review Feedback
Removed the unused `filter_model_fields()` utility function as noted by @copilot-pull-request-reviewer.
**Changes in commit ba0be2b:**
- Deleted unused `filter_model_fields()` function and its imports
- Kept `_is_uuid()` function which is still used by `mcp_core.py`
- Reduced file from 147 lines to 30 lines
**Context:**
The `filter_model_fields()` utility was initially created to address Korbit AI's review feedback about field filtering. However, the implementation was refactored to use Pydantic's `@model_serializer` decorator approach instead, which is cleaner and more idiomatic.
The current implementation uses:
- `@model_serializer` decorators in schema files
- Serialization context passed to tools
- Direct `dict[str, Any]` returns from list tools
This approach is better because it:
- Uses Pydantic's built-in serialization features
- Avoids dynamic model creation
- Cleaner separation of concerns
All tests still passing! ✅
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #36035:
URL: https://github.com/apache/superset/pull/36035#issuecomment-3499593729
## Addressed Review Feedback
All 5 Korbit AI review comments have been addressed:
### 1-4: Required field validation, error handling, field validation, separation of concerns
Created comprehensive `filter_model_fields()` utility function in `superset/mcp_service/utils/__init__.py` that:
- Automatically includes required fields to prevent ValidationError
- Validates fields exist in the model before including them
- Has comprehensive error handling with graceful fallback to full model
- Separates filtering logic from serialization logic
- Fully documented with clear docstrings
### 5: Overly permissive type hints
Replaced `Any` types with specific type hints across all three list tool serializers:
**Chart serializer** (`list_charts.py`):
```python
def _serialize_chart(obj: "Slice | None", cols: list[str] | None) -> ChartInfo | None
```
**Dashboard serializer** (`list_dashboards.py`):
```python
def _serialize_dashboard(obj: "Dashboard | None", cols: list[str] | None) -> DashboardInfo | None
```
**Dataset serializer** (`list_datasets.py`):
```python
def _serialize_dataset(obj: "SqlaTable | None", cols: list[str] | None) -> DatasetInfo | None
```
Also removed unnecessary `isinstance` checks since the type hints now make the expected types explicit. Used TYPE_CHECKING imports to avoid circular dependencies.
All changes pass mypy type checking and pre-commit hooks.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): implement selective column serialization for list tools [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #36035:
URL: https://github.com/apache/superset/pull/36035#discussion_r2504918851
##########
superset/mcp_service/utils/__init__.py:
##########
@@ -27,3 +36,111 @@ def _is_uuid(value: str) -> bool:
return True
except ValueError:
return False
+
+
+def filter_model_fields(
+ model: T,
+ requested_columns: List[str] | None,
+ model_class: Type[T],
+ always_include: Set[str] | None = None,
+) -> T:
+ """
+ Filter Pydantic model fields to include only requested columns.
+
+ Args:
+ model: The full Pydantic model instance
+ requested_columns: List of column names to include (None = include all)
+ model_class: The Pydantic model class for reconstruction
+ always_include: Set of field names to always include (e.g., {"id"})
+
+ Returns:
+ Filtered model instance with only requested fields
+
+ Note:
+ - If requested_columns is None, returns the full model unchanged
+ - Only requested fields and always_include fields are included
+ - Fields in always_include are added to requested fields
+ - If model reconstruction fails, returns the original full model
+ """
+ # If no column selection, return full model
+ if not requested_columns:
+ return model
+
+ always_include = always_include or set()
+ all_fields = set(model_class.model_fields.keys())
+
+ # Build final set of fields to include
+ requested_fields = set(requested_columns)
+
+ # Add always_include fields if they exist in the model
+ for field in always_include:
+ if field in all_fields:
+ requested_fields.add(field)
+
+ # Calculate fields to exclude
+ exclude_fields = all_fields - requested_fields
+
+ try:
+ # Get data for only requested fields
+ dumped = model.model_dump(exclude=exclude_fields)
+
+ logger.info(
+ "filter_model_fields: model=%s, requested=%s, dumped_keys=%s",
+ model_class.__name__,
+ requested_fields,
+ dumped.keys(),
+ )
+
+ # Dynamically create new model class with only requested fields
+ # This ensures FastMCP only serializes the requested fields
+ field_definitions = {}
+ for field_name in requested_fields:
+ if field_name in model_class.model_fields:
+ field_info = model_class.model_fields[field_name]
+ field_definitions[field_name] = (field_info.annotation, field_info)
+ else:
+ logger.warning(
+ "Requested field %s not found in %s.model_fields",
+ field_name,
+ model_class.__name__,
+ )
+
+ logger.info("Field definitions for dynamic model: %s", field_definitions.keys())
+
+ # Create dynamic model with original class name to preserve type info
+ filtered_model_class = create_model(
+ f"Filtered{model_class.__name__}",
+ __base__=BaseModel,
+ **field_definitions,
+ )
+
+ # Create instance of filtered model
+ # Filter dumped data to only include fields that exist in the dynamic model
+ filtered_data = {k: v for k, v in dumped.items() if k in field_definitions}
+ logger.info("Filtered data keys: %s", filtered_data.keys())
+
+ filtered = filtered_model_class(**filtered_data)
+
+ logger.info(
+ "filter_model_fields: created filtered model with fields=%s",
+ filtered_model_class.model_fields.keys(),
+ )
+
+ return filtered
+ except ValidationError as e:
+ # If validation fails, log warning and return full model
+ logger.warning(
+ "Failed to create filtered %s: %s. Returning full model.",
+ model_class.__name__,
+ str(e),
+ )
+ return model
+ except Exception as e:
+ # Catch any other unexpected errors
+ logger.error(
+ "Unexpected error filtering %s: %s. Returning full model.",
+ model_class.__name__,
+ str(e),
+ exc_info=True,
+ )
+ return model
Review Comment:
The `filter_model_fields` function is defined but appears to be unused in the codebase. The PR implements field filtering through `model_serializer` decorators in the schema classes instead. Consider removing this unused function to reduce dead code and improve maintainability.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -252,8 +259,8 @@ class GetDashboardInfoRequest(MetadataCacheControl):
class DashboardInfo(BaseModel):
- id: int = Field(..., description="Dashboard ID")
- dashboard_title: str = Field(..., description="Dashboard title")
+ id: int | None = Field(None, description="Dashboard ID")
+ dashboard_title: str | None = Field(None, description="Dashboard title")
Review Comment:
The `id` and `dashboard_title` fields have been changed from required fields to optional (`int | None` and `str | None`). This is a breaking API change that could cause issues for API consumers who expect these fields to always be present. Consider whether this change is intentional, and if so, ensure that all dashboard serialization paths properly populate these fields or document this breaking change.
```suggestion
id: int = Field(..., description="Dashboard ID")
dashboard_title: str = Field(..., description="Dashboard title")
```
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -79,8 +80,8 @@ class ChartLike(Protocol):
class ChartInfo(BaseModel):
"""Full chart model with all possible attributes."""
- id: int = Field(..., description="Chart ID")
- slice_name: str = Field(..., description="Chart name")
+ id: int | None = Field(None, description="Chart ID")
+ slice_name: str | None = Field(None, description="Chart name")
Review Comment:
The `id` and `slice_name` fields have been changed from required fields to optional (`int | None` and `str | None`). This is a breaking API change that could cause issues for API consumers who expect these fields to always be present. Consider whether this change is intentional, and if so, ensure that all chart serialization paths properly populate these fields or document this breaking change.
```suggestion
id: int = Field(..., description="Chart ID")
slice_name: str = Field(..., description="Chart name")
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org