aminghadersohi commented on code in PR #38562:
URL: https://github.com/apache/superset/pull/38562#discussion_r2932177356
##########
superset/mcp_service/system/schemas.py:
##########
@@ -125,33 +111,19 @@ class FeatureAvailability(BaseModel):
class InstanceInfo(BaseModel):
- instance_summary: InstanceSummary = Field(
- ..., description="Instance summary information"
- )
- recent_activity: RecentActivity = Field(
- ..., description="Recent activity information"
- )
- dashboard_breakdown: DashboardBreakdown = Field(
- ..., description="Dashboard breakdown information"
- )
- database_breakdown: DatabaseBreakdown = Field(
- ..., description="Database breakdown by type"
- )
- popular_content: PopularContent = Field(
- ..., description="Popular content information"
- )
+ instance_summary: InstanceSummary
+ recent_activity: RecentActivity
+ dashboard_breakdown: DashboardBreakdown
+ database_breakdown: DatabaseBreakdown
+ popular_content: PopularContent
current_user: UserInfo | None = Field(
None,
- description="The authenticated user making the request. "
- "Use current_user.id with created_by_fk filter to find your own
assets.",
- )
- feature_availability: FeatureAvailability = Field(
- ...,
description=(
- "Dynamic feature availability for the current user and deployment"
+ "Use current_user.id with created_by_fk filter to find your own
assets."
),
)
- timestamp: datetime = Field(..., description="Response timestamp")
+ feature_availability: FeatureAvailability
Review Comment:
The observation about the mismatch between `instance_metadata.py` and
`InstanceInfo` is real, but this is a **pre-existing issue not introduced by
this PR**.
The `feature_availability` field was already required before this PR — the
old code used `Field(...)` which means required in Pydantic:
```python
# BEFORE this PR:
feature_availability: FeatureAvailability = Field(
...,
description="Dynamic feature availability for the current user and
deployment",
)
# AFTER this PR (same semantics, just trimmed description):
feature_availability: FeatureAvailability
```
Both versions require `feature_availability` — `Field(...)` and bare type
annotation without a default are equivalent in Pydantic v2. This PR only
removed the description wrapper; it did not change the requiredness.
The `instance_metadata.py` resource has been missing `feature_availability`
from its `metric_calculators` since `feature_availability` was first added to
the schema (commit `1cd35bb102`). When
`InstanceInfoCore._generate_instance_info()` builds the response data without
`feature_availability`, Pydantic validation fails, and the outer `except` block
in `instance_metadata.py` (line 143) returns the fallback error payload. This
is an existing bug in the metadata resource, not a regression from this PR.
That said, the bot's suggestion to add a default
(`default_factory=FeatureAvailability`) is a reasonable improvement — it would
make the metadata resource work correctly without needing to register the
calculator. But that should be a separate fix, not a blocker for this 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: [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]
