aminghadersohi commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4188418036
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -64,6 +72,102 @@ 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
+ if 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:
+ extra_form_data = (
+ {
+ "adhoc_filters": [
+ {
+ "expressionType": "SQL",
+ "clause": "WHERE",
+ "sqlExpression": EMPTY_FILTER_SQL_EXPRESSION,
+ }
+ ]
+ }
+ if control_values.get("enableEmptyFilter")
+ else {}
+ )
+ filter_state = {"value": None}
Review Comment:
Validated and fixed in ad35e49363b4826eb7a97b1b83c7f694ffc2d712. Required
empty select defaults store value=[] alongside the 1 = 0 predicate, so
charts/data/dashboard_filter_context.py::_extract_filter_extra_form_data
recognizes the static default instead of dropping it. The frontend
SelectFilterPlugin.updateDataMask and getSelectExtraFormData also produce a
match-nothing predicate for this empty required selection. Regression:
test_required_empty_default_applies_in_server_dashboard_context; it fails
before the fix and passes afterward.
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -64,6 +72,102 @@ 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
+ if 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:
+ extra_form_data = (
+ {
+ "adhoc_filters": [
+ {
+ "expressionType": "SQL",
+ "clause": "WHERE",
+ "sqlExpression": EMPTY_FILTER_SQL_EXPRESSION,
+ }
+ ]
+ }
+ if control_values.get("enableEmptyFilter")
+ else {}
+ )
+ filter_state = {"value": None}
+
+ return {"extraFormData": extra_form_data, "filterState": filter_state}
+
+
+def _default_data_mask(
+ conf: dict[str, Any], values: list[FilterSelectValue]
+) -> dict[str, Any]:
+ """Build the stored default data mask for a filter_select filter.
+
+ Clearing a default (empty ``values``) writes no predicate, so it must stay
+ possible on filters created in the UI with inverse selection or a
+ non-exact operator; those filters get the plain empty mask. Non-empty
+ defaults go through ``_select_data_mask`` and its guards.
+ """
+ control_values = conf.get("controlValues") or {}
+ if not values and (
+ control_values.get("inverseSelection")
+ or control_values.get("operatorType", "exact") != "exact"
+ ):
+ return _empty_data_mask()
Review Comment:
Validated and fixed in ad35e49363b4826eb7a97b1b83c7f694ffc2d712.
_default_data_mask routes empty selections through _empty_select_data_mask
before the non-empty operator guards. This preserves 1 = 0 for required
non-exact select filters, matching SelectFilterPlugin.updateDataMask and
filters/utils.ts::getSelectExtraFormData (emptyFilter is handled before
matching operators). Inverse-selection empty defaults retain the frontend's
no-predicate behavior. Regression: test_clear_required_non_exact_default,
including the inverse-selection control case.
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -211,6 +318,57 @@ def _merge_target(spec: NativeFilterUpdateSpec, merged:
dict[str, Any]) -> None:
merged["targets"] = [target]
+def _target_key(target: dict[str, Any]) -> tuple[Any, Any]:
+ """Identify a filter target by its dataset and column name."""
+ return target.get("datasetId"), (target.get("column") or {}).get("name")
Review Comment:
Validated and fixed in ad35e49363b4826eb7a97b1b83c7f694ffc2d712. _target_key
accepts either a legacy string column or a column-name object, preventing
AttributeError when _merge_filter_update compares the previous target after a
column-only update. _merge_target also uses that normalization when inheriting
a column during a dataset-only update. Regression:
test_update_legacy_string_target covers both updates and checks stale-default
clearing without mutating the original configuration; both cases fail before
the fix and pass afterward.
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -211,6 +318,57 @@ def _merge_target(spec: NativeFilterUpdateSpec, merged:
dict[str, Any]) -> None:
merged["targets"] = [target]
+def _target_key(target: dict[str, Any]) -> tuple[Any, Any]:
+ """Identify a filter target by its dataset and column name."""
+ return target.get("datasetId"), (target.get("column") or {}).get("name")
+
+
+def _stored_default_is_stale(
+ spec: NativeFilterUpdateSpec, existing: dict[str, Any], target_changed:
bool
+) -> bool:
+ """Whether an update without ``default_value`` invalidates the stored one.
+
+ A stored default goes stale when the filter is retargeted to another
+ column or dataset, when it becomes single-select while the default holds
+ several values, or when ``default_to_first_item`` is switched on (the
+ explicit default would otherwise win and the first item never applies).
+ """
+ if existing.get("filterType") != "filter_select":
+ return False
+ stored = ((existing.get("defaultDataMask") or {}).get("filterState") or
{}).get(
+ "value"
+ )
+ if stored is None:
+ return False
Review Comment:
Validated and fixed in ad35e49363b4826eb7a97b1b83c7f694ffc2d712.
_merge_select_default rebuilds empty default masks when enable_empty_filter is
supplied without default_value. Disabling it removes the stale 1 = 0 predicate,
matching SelectFilterPlugin.updateDataMask's enableEmptyFilter condition, while
non-empty defaults are retained. Regression:
test_disable_required_empty_default covers both legacy value=None and explicit
value=[] masks; both fail before the fix and pass afterward.
--
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]
