aminghadersohi commented on code in PR #38562: URL: https://github.com/apache/superset/pull/38562#discussion_r2932174010
########## superset/mcp_service/dashboard/schemas.py: ########## @@ -286,45 +286,31 @@ class GetDashboardInfoRequest(MetadataCacheControl): class DashboardInfo(BaseModel): - id: int | None = Field(None, description="Dashboard ID") - dashboard_title: str | None = Field(None, description="Dashboard title") - slug: str | None = Field(None, description="Dashboard slug") - description: str | None = Field(None, description="Dashboard description") - css: str | None = Field(None, description="Custom CSS for the dashboard") - certified_by: str | None = Field(None, description="Who certified the dashboard") - certification_details: str | None = Field(None, description="Certification details") - json_metadata: str | None = Field( - None, description="Dashboard metadata (JSON string)" - ) - position_json: str | None = Field(None, description="Chart positions (JSON string)") - published: bool | None = Field( - None, description="Whether the dashboard is published" - ) - is_managed_externally: bool | None = Field( - None, description="Whether managed externally" - ) - external_url: str | None = Field(None, description="External URL") - created_on: str | datetime | None = Field(None, description="Creation timestamp") - changed_on: str | datetime | None = Field( - None, description="Last modification timestamp" - ) - created_by: str | None = Field(None, description="Dashboard creator (username)") - changed_by: str | None = Field(None, description="Last modifier (username)") - uuid: str | None = Field(None, description="Dashboard UUID (converted to string)") - url: str | None = Field(None, description="Dashboard URL") - created_on_humanized: str | None = Field( - None, description="Humanized creation time" - ) - changed_on_humanized: str | None = Field( - None, description="Humanized modification time" - ) - chart_count: int = Field(0, description="Number of charts in the dashboard") - owners: List[UserInfo] = Field(default_factory=list, description="Dashboard owners") - tags: List[TagInfo] = Field(default_factory=list, description="Dashboard tags") - roles: List[RoleInfo] = Field(default_factory=list, description="Dashboard roles") - charts: List[ChartInfo] = Field( - default_factory=list, description="Dashboard charts" - ) + id: int | None = None + dashboard_title: str | None = None + slug: str | None = None + description: str | None = None + css: str | None = None + certified_by: str | None = None + certification_details: str | None = None + json_metadata: str | None = None + position_json: str | None = None + published: bool | None = None + is_managed_externally: bool | None = None + external_url: str | None = None + created_on: str | datetime | None = None + changed_on: str | datetime | None = None + created_by: str | None = None + changed_by: str | None = None Review Comment: Not a valid concern. The bot is confused about the field naming here. `DashboardInfo` has **never** had `created_by_name` or `changed_by_name` fields — the fields are `created_by` and `changed_by` (both `str | None`), and they remain in the model after this PR. The only change is removing `Field(description=...)` wrappers for token savings. Looking at `serialize_dashboard_object` (lines 545, 548), it reads the ORM model's `changed_by_name` **attribute** and maps it to the Pydantic model's `changed_by` **field**: ```python changed_by=getattr(dashboard, "changed_by_name", None), # ORM attr -> Pydantic field created_by=getattr(dashboard, "created_by_name", None), # ORM attr -> Pydantic field ``` This is correct — the data flows through fine. The creator/modifier display names are returned in `created_by` and `changed_by` fields, which is the intended mapping. The schema discovery (`schema_discovery.py`) advertises both `changed_by` and `changed_by_name` as available columns. When a client requests `changed_by_name` via `select_columns`, the DAO loads the data, `serialize_dashboard_object` maps it to `changed_by`, and `DashboardInfo._filter_fields_by_context` filters output to requested columns. The field name difference between the column name (`changed_by_name`) and the Pydantic field (`changed_by`) is a pre-existing design choice, not something introduced by 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]
