bito-code-review[bot] commented on code in PR #44893:
URL: https://github.com/apache/superset/pull/44893#discussion_r4169858989
##########
tests/unit_tests/mcp_service/dashboard/tool/test_apply_dashboard_filters.py:
##########
@@ -259,6 +299,528 @@ async def
test_required_time_range_cannot_be_cleared(mcp_server: object) -> None
create.assert_not_called()
+# ---------------------------------------------------------------------------
+# filter_range
+# ---------------------------------------------------------------------------
+
+
[email protected]
+async def test_apply_range_distinct_bounds(mcp_server: object) -> None:
+ """Distinct lower/upper bounds produce a >=/<= predicate pair."""
+ captured: dict[str, Any] = {}
+
+ with (
+ patch(DAO_GET, return_value=_mock_dashboard([RANGE_FILTER])),
+ patch(CREATE_PERMALINK, side_effect=_mock_permalink_command(captured)),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Cost", "range": [10, 100]}],
+ },
+ )
+
+ assert data["error"] is None
+ assert data["applied_filters"][0]["range"] == [10, 100]
+ entry = captured["state"]["dataMask"]["NATIVE_FILTER-cost"]
+ assert entry["extraFormData"] == {
+ "filters": [
+ {"col": "cost", "op": ">=", "val": 10},
+ {"col": "cost", "op": "<=", "val": 100},
+ ]
+ }
+ assert entry["filterState"] == {"value": [10, 100]}
+
+
[email protected]
+async def test_apply_range_equal_bounds_produces_equality_predicate(
+ mcp_server: object,
+) -> None:
+ """Equal lower/upper bounds collapse to a single == predicate."""
+ captured: dict[str, Any] = {}
+
+ with (
+ patch(DAO_GET, return_value=_mock_dashboard([RANGE_FILTER])),
+ patch(CREATE_PERMALINK, side_effect=_mock_permalink_command(captured)),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Cost", "range": [50, 50]}],
+ },
+ )
+
+ assert data["error"] is None
+ entry = captured["state"]["dataMask"]["NATIVE_FILTER-cost"]
+ assert entry["extraFormData"] == {
+ "filters": [{"col": "cost", "op": "==", "val": 50}]
+ }
+ assert entry["filterState"] == {"value": [50, 50]}
+
+
[email protected]
+async def test_apply_range_one_sided_bounds(mcp_server: object) -> None:
+ """A null bound on one side produces only the other side's predicate."""
+ captured: dict[str, Any] = {}
+
+ with (
+ patch(DAO_GET, return_value=_mock_dashboard([RANGE_FILTER])),
+ patch(CREATE_PERMALINK, side_effect=_mock_permalink_command(captured)),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Cost", "range": [10,
None]}],
+ },
+ )
+
+ assert data["error"] is None
+ entry = captured["state"]["dataMask"]["NATIVE_FILTER-cost"]
+ assert entry["extraFormData"] == {
+ "filters": [{"col": "cost", "op": ">=", "val": 10}]
+ }
+ assert entry["filterState"] == {"value": [10, None]}
+
+
[email protected]
+async def test_apply_range_lower_unbounded(mcp_server: object) -> None:
+ """A null lower bound produces only the upper side's predicate."""
+ captured: dict[str, Any] = {}
+
+ with (
+ patch(DAO_GET, return_value=_mock_dashboard([RANGE_FILTER])),
+ patch(CREATE_PERMALINK, side_effect=_mock_permalink_command(captured)),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Cost", "range": [None,
100]}],
+ },
+ )
+
+ assert data["error"] is None
+ entry = captured["state"]["dataMask"]["NATIVE_FILTER-cost"]
+ assert entry["extraFormData"] == {
+ "filters": [{"col": "cost", "op": "<=", "val": 100}]
+ }
+ assert entry["filterState"] == {"value": [None, 100]}
+
+
[email protected]
+async def test_null_range_clears_an_optional_range(mcp_server: object) -> None:
+ """[null, null] clears an optional range filter."""
+ captured: dict[str, Any] = {}
+
+ with (
+ patch(DAO_GET, return_value=_mock_dashboard([RANGE_FILTER])),
+ patch(CREATE_PERMALINK, side_effect=_mock_permalink_command(captured)),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Cost", "range": [None,
None]}],
+ },
+ )
+
+ assert data["error"] is None
+ entry = captured["state"]["dataMask"]["NATIVE_FILTER-cost"]
+ assert entry["extraFormData"] == {}
+ assert entry["filterState"] == {"value": [None, None]}
+
+
[email protected]
+async def test_required_range_cannot_be_cleared(mcp_server: object) -> None:
+ """A range filter marked enableEmptyFilter rejects a [null, null] clear."""
+ with (
+ patch(DAO_GET, return_value=_mock_dashboard([REQUIRED_RANGE_FILTER])),
+ patch(CREATE_PERMALINK) as create,
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [
+ {"filter_name_or_id": "Required Cost", "range": [None,
None]}
+ ],
+ },
+ )
+
+ assert "requires a value and cannot be cleared" in data["error"]
+ create.assert_not_called()
+
+
[email protected]
+async def test_range_lower_greater_than_upper_is_rejected(mcp_server: object)
-> None:
+ """A lower bound greater than the upper bound fails schema validation."""
+ from fastmcp.exceptions import ToolError
+
+ with patch(DAO_GET, return_value=_mock_dashboard([RANGE_FILTER])):
+ with pytest.raises(ToolError, match="cannot be greater than"):
+ await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Cost", "range": [100,
10]}],
+ },
+ )
+
+
[email protected]
+async def test_range_on_a_select_filter_is_rejected(mcp_server: object) ->
None:
+ """Range on a select filter is rejected."""
+ with patch(DAO_GET, return_value=_mock_dashboard([SELECT_FILTER])):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Region", "range": [1, 2]}],
+ },
+ )
+
+ assert "is a filter_select filter" in data["error"]
+ assert "provide 'values'" in data["error"]
+
+
[email protected]
+async def test_values_on_a_range_filter_is_rejected(mcp_server: object) ->
None:
+ """Values on a range filter is rejected."""
+ with patch(DAO_GET, return_value=_mock_dashboard([RANGE_FILTER])):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Cost", "values": [1]}],
+ },
+ )
+
+ assert "is a filter_range filter" in data["error"]
+ assert "provide 'range'" in data["error"]
+
+
[email protected]
+async def test_range_filter_without_target_column_is_rejected(
+ mcp_server: object,
+) -> None:
+ """A range filter with no target column cannot have a value applied."""
+ filter_without_target = {**RANGE_FILTER, "targets": [{}]}
+
+ with patch(DAO_GET, return_value=_mock_dashboard([filter_without_target])):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "filters": [{"filter_name_or_id": "Cost", "range": [10, 100]}],
+ },
+ )
+
+ assert "has no target column" in data["error"]
+
+
+# ---------------------------------------------------------------------------
+# filter_timegrain
+# ---------------------------------------------------------------------------
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is correct in its assessment that the `TimeGrain`
enum and engine specifications do not support 'PT2H' or 'P2D'. Applying this
suggestion is recommended to ensure the code only uses engine-supported grains,
preventing potential runtime errors when charts are rendered.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2488,33 +2525,74 @@ class ApplyFilterValueSpec(BaseModel):
"'No filter' to clear the filter."
),
)
+ range: List[int | float | None] | None = Field(
+ None,
+ min_length=2,
+ max_length=2,
+ description=(
+ "[lower, upper] bounds to apply, for a filter_range filter. "
+ "Either bound may be null to leave that side unbounded. Pass "
+ "[null, null] to clear the filter."
+ ),
+ )
+ time_grain: List[Annotated[str, Field(min_length=1)]] | None = Field(
+ None,
+ max_length=1,
+ description=(
+ "Time grain to apply, for a filter_timegrain filter, as a list "
+ "with at most one datasource-supported duration, e.g. ['P1D']. "
+ "Custom engine and semantic-layer durations are accepted. "
+ "Pass an empty list to "
+ "clear the filter."
+ ),
+ )
@field_validator("time_range")
@classmethod
def _validate_time_range(cls, v: str | None) -> str | None:
"""Validate the time range with the shared dashboard parser."""
return validate_time_range(v)
+ @field_validator("range")
+ @classmethod
+ def _validate_range_order(
+ cls, v: List[int | float | None] | None
+ ) -> List[int | float | None] | None:
+ """Reject non-finite bounds and a lower bound greater than the upper
bound."""
+ if v is not None and any(
+ bound is not None and not math.isfinite(bound) for bound in v
+ ):
+ raise ValueError("range bounds must be finite numbers or null.")
Review Comment:
<!-- Bito Reply -->
The suggestion to guard `math.isfinite(bound)` with `isinstance(bound,
float)` is appropriate and improves the robustness of the validator. Applying
this check prevents `OverflowError` when processing extremely large integers,
ensuring that such values are correctly rejected by the Pydantic validation
logic rather than causing an unhandled server error.
**superset/mcp_service/dashboard/schemas.py**
```
@field_validator("range")
@classmethod
def _validate_range_order(
cls, v: List[int | float | None] | None
) -> List[int | float | None] | None:
"""Reject non-finite bounds and a lower bound greater than the upper
bound."""
if v is not None and any(
bound is not None and isinstance(bound, float) and not
math.isfinite(bound) for bound in v
):
raise ValueError("range bounds must be finite numbers or null.")
```
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -224,6 +227,94 @@ def _time_data_mask(conf: dict[str, Any], time_range: str)
-> dict[str, Any]:
}
+def _range_data_mask(
+ conf: dict[str, Any], bounds: list[int | float | None]
+) -> dict[str, Any]:
+ """Build the data mask a filter_range filter produces for ``bounds``.
+
+ Mirrors the frontend's ``getRangeExtraFormData``: distinct non-null
+ bounds become a pair of ``>=``/``<=`` predicates, equal non-null bounds
+ collapse to a single ``==`` predicate, and two null bounds produce no
+ predicate at all (the "required filter, nothing chosen" state when the
+ filter is marked ``enableEmptyFilter``, raised rather than guessed).
+ """
+ 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 _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' has no target "
+ "column, so a value cannot be applied to it."
+ )
+
+ lower, upper = bounds
+ if lower is None and upper is None:
+ if (conf.get("controlValues") or {}).get("enableEmptyFilter"):
+ raise _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' requires a "
+ "value and cannot be cleared."
+ )
+ return {"extraFormData": {}, "filterState": {"value": [None, None]}}
+
+ filters: list[dict[str, Any]] = []
+ if lower == upper:
+ filters.append({"col": column, "op": "==", "val": upper})
+ else:
+ if lower is not None:
+ filters.append({"col": column, "op": ">=", "val": lower})
+ if upper is not None:
+ filters.append({"col": column, "op": "<=", "val": upper})
+
+ return {
+ "extraFormData": {"filters": filters},
+ "filterState": {"value": [lower, upper]},
+ }
Review Comment:
<!-- Bito Reply -->
The suggestion is correct. The `_range_data_mask` function currently returns
a `filterState` object containing only the `value` array, whereas the
UI-applied mask includes a `label` field. Adding the `label` ensures that
tool-applied range filters are consistent with those generated by the UI, which
is necessary for correct permalink behavior.
**superset/mcp_service/dashboard/tool/apply_dashboard_filters.py**
```
return {
"extraFormData": {"filters": filters},
"filterState": {"value": [lower, upper], "label": getLabel(lower,
upper)},
}
```
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -224,6 +227,94 @@ def _time_data_mask(conf: dict[str, Any], time_range: str)
-> dict[str, Any]:
}
+def _range_data_mask(
+ conf: dict[str, Any], bounds: list[int | float | None]
+) -> dict[str, Any]:
+ """Build the data mask a filter_range filter produces for ``bounds``.
+
+ Mirrors the frontend's ``getRangeExtraFormData``: distinct non-null
+ bounds become a pair of ``>=``/``<=`` predicates, equal non-null bounds
+ collapse to a single ``==`` predicate, and two null bounds produce no
+ predicate at all (the "required filter, nothing chosen" state when the
+ filter is marked ``enableEmptyFilter``, raised rather than guessed).
+ """
+ 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 _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' has no target "
+ "column, so a value cannot be applied to it."
+ )
+
+ lower, upper = bounds
+ if lower is None and upper is None:
+ if (conf.get("controlValues") or {}).get("enableEmptyFilter"):
+ raise _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' requires a "
+ "value and cannot be cleared."
+ )
+ return {"extraFormData": {}, "filterState": {"value": [None, None]}}
+
+ filters: list[dict[str, Any]] = []
+ if lower == upper:
+ filters.append({"col": column, "op": "==", "val": upper})
+ else:
+ if lower is not None:
+ filters.append({"col": column, "op": ">=", "val": lower})
+ if upper is not None:
+ filters.append({"col": column, "op": "<=", "val": upper})
+
+ return {
+ "extraFormData": {"filters": filters},
+ "filterState": {"value": [lower, upper]},
+ }
+
+
+def _timegrain_data_mask(
+ conf: dict[str, Any], time_grain: Sequence[str]
+) -> dict[str, Any]:
+ """Build the data mask a filter_timegrain filter produces for
``time_grain``."""
+ is_set = bool(time_grain)
+ allowed_grains = conf.get("time_grains")
+ if is_set and allowed_grains and time_grain[0] not in allowed_grains:
+ raise _FilterApplyError(
+ f"Time grain '{time_grain[0]}' is not allowed for filter "
+ f"'{conf.get('name') or conf.get('id')}'. "
+ f"Available time grains: {', '.join(allowed_grains)}."
+ )
+ if is_set and not allowed_grains:
+ from superset.daos.dataset import DatasetDAO
+
+ targets = [target for target in (conf.get("targets") or []) if target]
+ dataset_id = targets[0].get("datasetId") if targets else None
+ dataset = DatasetDAO.find_by_id(dataset_id) if dataset_id is not None
else None
+ if dataset is None:
+ raise _FilterApplyError(
+ f"Cannot resolve target dataset (ID {dataset_id}) for filter "
+ f"'{conf.get('name') or conf.get('id')}'; "
+ "supported time grains cannot be determined."
+ )
+ # Reuse the datasource options exposed to Explore and native filters.
+ available_grains = [
+ duration for duration, _ in dataset.time_grain_sqla if duration is
not None
+ ]
+ if time_grain[0] not in available_grains:
+ raise _FilterApplyError(
+ f"Time grain '{time_grain[0]}' is not supported by dataset "
+ f"{dataset_id} for filter '{conf.get('name') or
conf.get('id')}'. "
+ f"Available time grains: {', '.join(available_grains) or
'(none)'}."
Review Comment:
<!-- Bito Reply -->
The review comments identify several issues related to filter validation and
API consistency in the new dashboard filter service. Specifically, the reviewer
flagged:
* **CWE-1187: Timegrain mask missing filterState label:** The
`_timegrain_data_mask` function was missing a `label` in `filterState`, causing
inconsistencies with UI-generated permalinks.
* **Type Error in FilterRangeSpec:** The `FilterRangeSpec` was accepting
non-numeric columns, which could lead to unusable filters.
* **Api Mismatch in TimeGrain:** The `TimeGrain` definition excluded
certain durations, and the `time_grain` application lacked validation against
the filter's allowlist.
The PR author has addressed these issues in subsequent commits (e.g.,
`fd0dce1a` for the timegrain label and `ab5c0c37` for the numeric column
restriction), and the changes have been validated with updated unit tests.
--
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]
