aminghadersohi commented on code in PR #44893:
URL: https://github.com/apache/superset/pull/44893#discussion_r4169857828
##########
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:
Declined as stale at the reviewed head
17bb990cdedbeebf9d2953d6ad3e63e4d221eadf. `test_apply_timegrain` parametrizes
SQLite-supported P1D, PT1H, and the Sunday-week duration, not PT2H/P2D.
`_timegrain_data_mask` checks the target dataset's `time_grain_sqla` options
when the allowlist is absent/empty.
`test_timegrain_without_allowlist_rejects_unsupported` explicitly rejects PT2H
and P2D before permalink creation; the custom P2D acceptance test supplies a
custom datasource grain. Engine/operator extensions cannot be ruled out from
the built-in enum alone. No additional grain-validation change is needed for
this thread.
Validation: `pytest tests/unit_tests/mcp_service/dashboard` — 770 passed.
Pre-commit on all branch-changed files passed, including mypy, ruff, and pylint.
##########
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:
Valid correctness issue: `math.isfinite(10**1000)` overflows during
validation. Fixed in fd0dce1a60063a8fecacd91ea9ffaa0d334a480f: catch that
overflow and reject the bound with the same Pydantic validation error used for
non-finite values, rather than accepting an integer that cannot be represented
as a frontend number. Regression tests cover positive/negative huge integers in
either bound.
Validation: `pytest tests/unit_tests/mcp_service/dashboard` — 770 passed.
Pre-commit on all branch-changed files passed, including mypy, ruff, and pylint.
##########
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:
Valid mask-shape mismatch. `RangeFilterPlugin.updateDataMaskValue` stores
`getLabel` output, including an empty string when cleared. Fixed in
fd0dce1a60063a8fecacd91ea9ffaa0d334a480f: range masks include equality,
two-sided, and one-sided labels using the shared SMART_NUMBER formatter and the
frontend minus sign. Tests cover every shape, clearing, zero, decimals,
negative values, and SI suffixes.
Validation: `pytest tests/unit_tests/mcp_service/dashboard` — 770 passed.
Pre-commit on all branch-changed files passed, including mypy, ruff, and pylint.
##########
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:
Valid mask-shape mismatch. `TimeGrainFilterPlugin.handleChange` labels the
duration with the datasource option's name. Fixed in
fd0dce1a60063a8fecacd91ea9ffaa0d334a480f: resolve labels from
`dataset.get_time_grains()`, the same data source used by the frontend
timegrains query. This also supplies names for allowlisted selections and
custom engine grains; clearing or a duration absent from the options omits the
label, matching the frontend's undefined label. Tests cover standard/custom
datasource names, an allowlisted standard grain, unknown option labels, and
clearing.
Validation: `pytest tests/unit_tests/mcp_service/dashboard` — 770 passed.
Pre-commit on all branch-changed files passed, including mypy, ruff, and pylint.
--
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]
