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: [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]
