aminghadersohi commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4190548057
##########
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:
Fixed in c4f9575e829fc77b19a3fd227ffab46404ca5f24. First-item defaults now
store `filterState: {}` and `extraFormData: {}` on every reset, rather than a
defined null/empty value or a match-nothing predicate. Enabling first-item
selection also resets legacy `value: null` masks. Regression tests assert the
absence of the `value` key for enabling first-item defaults, both
required-control toggle directions, and target/single-select stale-default
resets. These cases produced 18 failures before the fix (including the
create-path cases); the dashboard, RBAC enforcement, and tool-inventory suite
passes all 1,030 tests after the fix. Pre-commit, including mypy, passes.
##########
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:
Also fixed in c4f9575e829fc77b19a3fd227ffab46404ca5f24. The create path
routes first-item defaults through `_default_data_mask`, leaving `filterState`
and `extraFormData` empty so the dashboard can select the first loaded option.
The create test covers both values of `enable_empty_filter` and explicitly
asserts that no `value` key is stored; both cases failed before the fix and
pass after it. The dashboard, RBAC enforcement, and tool-inventory suite passes
all 1,030 tests; pre-commit passes.
--
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]
