gabotorresruiz commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4190474026
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -89,6 +98,107 @@ 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_STRING
+ 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 = (
+ _target_key(targets[0])[1] if targets and isinstance(targets[0], dict)
else None
+ )
+ if not isinstance(column, str) or not column:
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' has no target "
+ "column, so a value cannot be applied to it."
+ )
+
+ control_values = conf.get("controlValues") or {}
+ if control_values.get("inverseSelection"):
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' enables inverse "
+ "selection, which this tool does not support."
+ )
+ if (operator := control_values.get("operatorType", "exact")) != "exact":
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' uses matching "
+ f"operator '{operator}', which this tool does not support. "
+ "Only exact-match select filters are supported."
+ )
+ # A single-select filter renders one value; storing several would disagree
+ # with the control the moment a viewer touches it. multiSelect defaults to
+ # true, so only an explicit false restricts the selection.
+ if control_values.get("multiSelect") is False and len(values) > 1:
+ raise _FilterValidationError(
+ f"Filter '{conf.get('name') or conf.get('id')}' is single-select "
+ f"and accepts at most one value, but {len(values)} were given."
+ )
+
+ if values:
+ extra_form_data: dict[str, Any] = {
+ "filters": [{"col": column, "op": "IN", "val": list(values)}]
+ }
+ filter_state: dict[str, Any] = {
+ "value": list(values),
+ "label": ", ".join(_value_label(value) for value in values),
+ }
+ else:
+ return _empty_select_data_mask(conf)
+
+ return {"extraFormData": extra_form_data, "filterState": filter_state}
+
+
+def _empty_select_data_mask(conf: dict[str, Any]) -> dict[str, Any]:
+ """Mirror the frontend's empty selection, including required-filter
semantics."""
+ controls = conf.get("controlValues") or {}
+ if not controls.get("enableEmptyFilter") or
controls.get("inverseSelection"):
+ return _empty_data_mask()
+ return {
+ "extraFormData": {
+ "adhoc_filters": [
+ {
+ "expressionType": "SQL",
+ "clause": "WHERE",
+ "sqlExpression": EMPTY_FILTER_SQL_EXPRESSION,
+ }
+ ]
+ },
+ # An explicit empty selection is a static default to server-side
+ # dashboard context; None would cause its predicate to be skipped.
+ "filterState": {"value": []},
+ }
+
+
+def _default_data_mask(
+ conf: dict[str, Any], values: list[FilterSelectValue]
+) -> dict[str, Any]:
+ """Build a stored select default, allowing empty unsupported UI selections.
+
+ Empty selections do not depend on the matching operator. Required filters
+ still contribute an impossible predicate unless inverse selection is on,
+ matching SelectFilterPlugin.updateDataMask.
+ """
+ if not values:
+ return _empty_select_data_mask(conf)
+ return _select_data_mask(conf, values)
Review Comment:
This block worries me a bit. For a `defaultToFirstItem` filter, every reset
path in `_merge_select_default` ends here with `values=[]`, which stores
`filterState.value` as `null` (or `[]` plus the `1 = 0` predicate when
`enableEmptyFilter` is on). The dashboard treats a defined `value` as an
explicit selection: `SelectFilterPlugin`'s load effect calls
`updateDataMask(filterState.value)` and returns before its `defaultToFirstItem`
branch, so the first item is never applied. The UI itself stores first-item
filters with no `value` key at all, which is why its own ones work.
I verified it on this branch: an update of `{"enable_empty_filter": true}`
on a UI-created first-item filter now writes `{"filterState": {"value": []},
"extraFormData": {"adhoc_filters": [1 = 0]}}` where master left the mask
untouched, and rendering `SelectFilterPlugin` with that mask (or with
`{"value": null}`) never emits the first item, while `filterState: {}` does.
Same outcome for `{"enable_empty_filter": false}` (writes `value: null`) and
for `{"default_to_first_item": true}` on a filter with an explicit default, so
the intent behind `_stored_default_is_stale` ("the first item then applies")
does not hold on the frontend; the dashboard sits on `1 = 0` or on no selection
until the viewer picks something.
Suggested fix, since `_merge_select_default` already rejects non-empty
values when first item is on:
```python
def _default_data_mask(conf, values):
if (conf.get("controlValues") or {}).get("defaultToFirstItem"):
# The UI stores no value for first-item filters; null or [] reads
# as an explicit selection and blocks the first item from applying.
return {"filterState": {}, "extraFormData": {}}
if not values:
return _empty_select_data_mask(conf)
return _select_data_mask(conf, values)
```
plus asserting `"value" not in config["defaultDataMask"]["filterState"]` in
`test_update_default_to_first_item_clears_explicit_default`, and one test for
an `enable_empty_filter` toggle on a first-item filter (both currently pass
with the broken shape, which is why the suite did not catch it).
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -193,6 +303,10 @@ def _build_new_filter_config(
"defaultDataMask": _empty_data_mask(),
}
+ if spec.default_value is not None:
Review Comment:
Not a blocker, and pre-existing on master: the create path has the same
shape issue, `default_to_first_item: true` lands with `_empty_data_mask()`
(`value: null`), so a first-item filter created through this tool never
auto-selects in the UI either. If you take the `_default_data_mask` change
above, routing the first-item case through it here fixes both.
--
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]
