bito-code-review[bot] commented on code in PR #44893:
URL: https://github.com/apache/superset/pull/44893#discussion_r4169552574
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Unsupported grain parametrized</b></div>
<div id="fix">
test_apply_timegrain parametrizes 'PT2H' and 'P2D', but no engine spec
registers these grains (`TimeGrain` enum, `builtin_time_grains`,
`TIME_GRAIN_ADDONS` all exclude them), so `BaseEngineSpec.get_timestamp_expr`
raises NotImplementedError when a chart renders with the saved
`time_grain_sqla`. The test enshrines acceptance of grains that break chart
queries; use engine-supported grains (e.g. 'PT1H', 'P1M') or assert rejection
once `_timegrain_data_mask` validates engine support.
</div>
</div>
<small><i>Code Review Run #2fad67</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-248: Uncaught OverflowError In Validator</b></div>
<div id="fix">
`math.isfinite(bound)` raises `OverflowError: int too large to convert to
float` for huge ints (verified on Python 3.11.2 / pydantic 2.13.5), which
escapes pydantic as a non-`ValidationError` and turns an out-of-range `range`
bound into an unhandled 500 instead of a tool validation error. Guard with
`isinstance(bound, float)` before `math.isfinite`.
([CWE-248](https://cwe.mitre.org/data/definitions/248.html))
</div>
</div>
<small><i>Code Review Run #2fad67</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1187: Range mask missing filterState label</b></div>
<div id="fix">
`_range_data_mask` returns `filterState` without `label`, but the UI-applied
shape stores one: `RangeFilterPlugin.updateDataMaskValue` sets `label:
getLabel(...)`, and `_select_data_mask` mirrors labels per the module comment
at the top of the file ('mirrored here so a permalink's label reads the same as
a UI-applied one'). A tool-applied range permalink is therefore not
'indistinguishable from one the UI produced' (comment above `data_mask["id"]`).
([CWE-1187](https://cwe.mitre.org/data/definitions/1187.html))
</div>
</div>
<small><i>Code Review Run #2fad67</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1187: Timegrain mask missing filterState
label</b></div>
<div id="fix">
`_timegrain_data_mask` returns `filterState` without `label`, while
`TimeGrainFilterPlugin.handleChange` stores `label: durationMap[timeGrain]` and
sibling `_select_data_mask` mirrors labels per the module's mirroring comment.
Tool-applied timegrain permalinks therefore differ from UI-applied ones,
contradicting the 'indistinguishable from one the UI produced' invariant above
`data_mask["id"]`.
([CWE-1187](https://cwe.mitre.org/data/definitions/1187.html))
</div>
</div>
<small><i>Code Review Run #2fad67</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
