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