bito-code-review[bot] commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4189389545
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -89,6 +99,105 @@ def _empty_data_mask() -> dict[str, Any]:
return {"filterState": {"value": None}, "extraFormData": {}}
+def _value_label(value: FilterSelectValue) -> str:
+ """Format one selected value the way the dashboard UI labels it."""
+ if value is None:
+ return _NULL_LABEL
+ if isinstance(value, bool):
+ return _TRUE_LABEL if value else _FALSE_LABEL
+ return str(value)
+
+
+def _select_data_mask(
+ conf: dict[str, Any], values: list[FilterSelectValue]
+) -> dict[str, Any]:
+ """Build the data mask a filter_select filter produces for ``values``.
+
+ Mirrors the frontend's ``getSelectExtraFormData``: a non-empty selection
+ becomes an ``IN`` predicate on the filter's target column, and an empty
+ selection on a filter marked ``enableEmptyFilter`` becomes an impossible
+ predicate (the "required filter, nothing chosen" state) rather than no
+ filtering at all. Shared by ``apply_dashboard_filters`` (applied values)
+ and this module (default values on create/update) so both paths agree.
+ """
+ targets = [target for target in (conf.get("targets") or []) if target]
+ column = (targets[0].get("column") or {}).get("name") if targets else None
Review Comment:
<!-- Bito Reply -->
The suggestion to use `_target_key` for validating the target shape is
appropriate. It ensures that the code handles both dictionary and string column
shapes consistently, preventing potential attribute errors when processing
stored filter configurations.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2328,6 +2332,23 @@ class FilterSelectSpec(BaseNewFilterSpec):
search_all_options: bool = Field(
False, description="Query the database on search rather than
client-side"
)
+ default_value: List[FilterSelectValue] | None = Field(
+ None,
+ description=(
+ "Default selected value(s), shown when a viewer opens the "
+ "dashboard unchanged. Omit for no default. Mutually exclusive "
+ "with default_to_first_item."
+ ),
+ )
+
+ @model_validator(mode="after")
+ def _validate_default_value_compat(self) -> "FilterSelectSpec":
+ if self.default_to_first_item and self.default_value is not None:
+ raise ValueError(
+ "default_to_first_item and default_value are mutually "
+ "exclusive; set at most one."
+ )
+ return self
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion regarding the empty-list validation is not
required for the production schema. The current implementation correctly
handles `default_value=[]` because `[] is not None` evaluates to `True`,
ensuring that the mutual-exclusion logic in `_validate_default_value_compat`
functions as intended. The existing tests confirm that empty-list cases are
handled correctly, and the suggested refactoring for duplicated validator logic
is a matter of preference rather than a functional defect.
--
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]
