aminghadersohi commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4189387783
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -80,6 +83,13 @@
)
+# Display strings the frontend uses when labelling a selected value; mirrored
+# here so a stored default's label reads the same as an applied one.
+_NULL_LABEL = "<NULL>"
Review Comment:
Valid maintainability finding; fixed in
930d5e2d4f9d8093ed89da34e985ebc5a7441ec4. The NULL label uses
superset.constants.NULL_STRING instead of a duplicated literal.
test_select_null_label_uses_shared_constant verifies that the stored label
follows the shared symbol; it failed at 03fe69cb and passes after the fix.
##########
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:
Valid; fixed in 930d5e2d4f9d8093ed89da34e985ebc5a7441ec4. _select_data_mask
reuses _target_key for object and legacy string columns, guards non-dict
targets, and rejects non-string column names with _FilterValidationError
instead of crashing or storing an invalid predicate. Regression tests cover
applied selections, non-empty default updates on legacy targets, and malformed
target shapes; the affected cases fail at 03fe69cb and pass after the fix.
##########
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:
The empty-list finding is invalid: [] is not None evaluates to True. Both
FilterSelectSpec._validate_default_value_compat and
NativeFilterUpdateSpec._validate_default_value_compat therefore reject
default_value=[] together with default_to_first_item=True before the tool body
runs. Commit 930d5e2d4f9d8093ed89da34e985ebc5a7441ec4 extends both MCP-boundary
tests to cover empty and non-empty lists; the empty-list cases pass at 03fe69cb
as well as after this commit. The identical short checks are model-specific
schema validators; the shared-helper suggestion is a refactoring preference,
not an exclusivity defect, so the production schema remains unchanged. All 996
dashboard and tool-inventory tests pass, and pre-commit passes for all
branch-changed files.
--
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]
