codeant-ai-for-open-source[bot] commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4188416739
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `03fe69c`.
Empty required defaults now include the impossible SQL predicate and store
an explicit empty filter-state value (`[]`) instead of `None`.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `03fe69c`.
Empty select defaults route through `_empty_select_data_mask`, which adds
the impossible predicate when `enableEmptyFilter` is enabled and inverse
selection is off.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `03fe69c`.
`_target_key` now handles `column` as either a dictionary or a legacy
string, avoiding a `.get("name")` call on string targets.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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]
