fix(ag-grid-table): reject null/empty IN_RANGE bounds and tighten tests

Number(null) and Number('') both coerce to 0, so a null/empty filterTo
would silently produce "BETWEEN x AND 0" instead of dropping the clause.
Explicitly reject non-coercible (null/empty) bounds before coercion.

Also strengthen the converter tests to assert the surviving sibling
condition and the full compound WHERE clause, so they fail if filter
composition or the upper bound regresses.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/agGridFilterConverter.ts

@@ -379,7 +379,23 @@ function simpleFilterToWhereClause(
   }
 
   if (type === FILTER_OPERATORS.IN_RANGE && filterTo !== undefined) {
-    return `${columnName} ${SQL_OPERATORS.BETWEEN} ${value} AND ${filterTo}`;
+    // BETWEEN bounds are interpolated directly into the WHERE clause, so only
+    // accept finite numeric bounds (date ranges are handled separately above).
+    // Numeric strings from serialized filter state are coerced; anything that
+    // isn't a finite number is dropped rather than concatenated as raw SQL.
+    // Reject null/empty bounds explicitly: Number(null) and Number('') both
+    // coerce to 0, which would otherwise produce a misleading BETWEEN ... AND 0.
+    const isCoercibleBound = (bound: FilterValue): boolean =>
+      (typeof bound === 'number' || typeof bound === 'string') && bound !== '';
+    if (!isCoercibleBound(value) || !isCoercibleBound(filterTo)) {
+      return '';
+    }
+    const from = Number(value);
+    const to = Number(filterTo);
+    if (!Number.isFinite(from) || !Number.isFinite(to)) {
+      return '';
+    }
+    return `${columnName} ${SQL_OPERATORS.BETWEEN} ${from} AND ${to}`;
   }
 
   const formattedValue = formatValueForOperator(type, value!);

superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/agGridFilterConverter.test.ts

@@ -771,6 +771,60 @@ describe('agGridFilterConverter', () => {
       // Should reject column names longer than 255 characters
       expect(result.simpleFilters).toHaveLength(0);
     });
+
+    test('should drop inRange bounds that are not numeric', () => {
+      const filterModel: AgGridFilterModel = {
+        age: {
+          filterType: 'number',
+          operator: 'AND',
+          condition1: {
+            filterType: 'number',
+            type: 'inRange',
+            filter: '0 AND 1=1--',
+            filterTo: '100',
+          },
+          condition2: {
+            filterType: 'number',
+            type: 'greaterThan',
+            filter: 5,
+          },
+        } as AgGridCompoundFilter,
+      };
+
+      const result = convertAgGridFiltersToSQL(filterModel);
+
+      // The malicious range condition is dropped, so its payload never reaches
+      // the WHERE clause; the sibling numeric condition survives unchanged.
+      expect(result.complexWhere ?? '').not.toContain('1=1');
+      expect(result.complexWhere ?? '').not.toContain('BETWEEN');
+      expect(result.complexWhere).toBe('age > 5');
+    });
+
+    test('should keep numeric inRange bounds (including numeric strings)', () => {
+      const filterModel: AgGridFilterModel = {
+        age: {
+          filterType: 'number',
+          operator: 'AND',
+          condition1: {
+            filterType: 'number',
+            type: 'inRange',
+            filter: '18',
+            filterTo: 65,
+          },
+          condition2: {
+            filterType: 'number',
+            type: 'lessThan',
+            filter: 100,
+          },
+        } as AgGridCompoundFilter,
+      };
+
+      const result = convertAgGridFiltersToSQL(filterModel);
+
+      // Assert the full compound clause so the upper bound and the sibling
+      // condition are both validated, not just the BETWEEN fragment.
+      expect(result.complexWhere).toBe('(age BETWEEN 18 AND 65 AND age < 100)');
+    });
   });
 
   describe('Edge cases', () => {

superset/mcp_service/dashboard/schemas.py

@@ -480,17 +480,7 @@ class AddChartToDashboardRequest(BaseModel):
     )
     chart_id: int = Field(..., description="ID of the chart to add to the dashboard")
     target_tab: str | None = Field(
-        None,
-        min_length=1,
-        description=(
-            "Tab to place the chart in. Accepts a tab display name "
-            "(e.g. 'Sales') or a tab component ID (e.g. 'TAB-abc123'). "
-            "Display-name matching is case-insensitive and strips all emoji; "
-            "component ID matching is case-sensitive and exact. "
-            "When not found, the error response lists all available tab names. "
-            "When omitted on a tabbed dashboard the chart is placed in the "
-            "first tab."
-        ),
+        None, description="Target tab name (if dashboard has tabs)"
     )
 
 

superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py

@@ -149,46 +149,29 @@ def _first_tab_from_groups(
     return None
 
 
-def _collect_available_tab_names(layout: Dict[str, Any]) -> list[str]:
-    """Collect display names (or IDs) of all TAB components in the layout."""
-    names: list[str] = []
-    for tabs_children in _collect_tabs_groups(layout):
-        for tab_id in tabs_children:
-            tab = layout.get(tab_id)
-            if not tab or tab.get("type") != "TAB":
-                continue
-            text = (tab.get("meta") or {}).get("text", "")
-            names.append(text if text else tab_id)
-    return names
-
-
 def _find_tab_insert_target(
     layout: Dict[str, Any], target_tab: str | None = None
 ) -> str | None:
     """
     Detect if the dashboard uses tabs and return the appropriate tab's ID.
 
-    When *target_tab* is ``None`` the function returns the first TAB child so
-    that new rows are placed inside the tab structure rather than directly
-    under ``GRID_ID``.
-
-    When *target_tab* is provided the function tries to match it against tab
-    ``meta.text`` (display name) or the raw component ID.  If no match is
-    found ``None`` is returned — the caller is responsible for surfacing an
-    error rather than silently placing the chart in the wrong tab.
+    If *target_tab* is provided the function first tries to match it against
+    tab ``meta.text`` (display name) or the raw component ID.  When no match
+    is found (or *target_tab* is ``None``) the first ``TAB`` child is used as
+    a fallback so that new rows are still placed inside the tab structure
+    rather than directly under ``GRID_ID``.
 
     Returns:
-        The ID of the matched (or first) TAB component, or ``None``.
+        The ID of the matched (or first) TAB component, or ``None`` if the
+        dashboard does not use top-level tabs.
     """
     groups = _collect_tabs_groups(layout)
 
-    if target_tab is not None:
+    if target_tab:
         for tabs_children in groups:
             matched = _match_tab_in_children(layout, tabs_children, target_tab)
             if matched:
                 return matched
-        # target_tab specified but not found — signal mismatch to the caller.
-        return None
 
     return _first_tab_from_groups(layout, groups)
 
@@ -333,45 +316,6 @@ def _ensure_layout_structure(
         layout["DASHBOARD_VERSION_KEY"] = "v2"
 
 
-def _resolve_parent_container(
-    layout: Dict[str, Any],
-    dashboard_id: int,
-    target_tab: str | None,
-) -> tuple[str, None] | tuple[None, AddChartToDashboardResponse]:
-    """Return (parent_id, None) on success or (None, error_response) on mismatch.
-
-    When *target_tab* is specified and not found the caller receives a
-    descriptive error listing available tabs rather than a silent fallback.
-    """
-    tab_target = _find_tab_insert_target(layout, target_tab=target_tab)
-
-    if target_tab is not None and tab_target is None:
-        available = _collect_available_tab_names(layout)
-        if available:
-            tab_list = ", ".join(f"'{t}'" for t in available)
-            return None, AddChartToDashboardResponse(
-                dashboard=None,
-                dashboard_url=None,
-                position=None,
-                error=(
-                    f"Tab '{target_tab}' not found in dashboard {dashboard_id}. "
-                    f"Available tabs: {tab_list}."
-                ),
-            )
-        return None, AddChartToDashboardResponse(
-            dashboard=None,
-            dashboard_url=None,
-            position=None,
-            error=(
-                f"Dashboard {dashboard_id} has no tabs. "
-                "Remove the target_tab parameter to add the chart to "
-                "the default grid layout."
-            ),
-        )
-
-    return tab_target if tab_target else "GRID_ID", None
-
-
 def _find_and_authorize_dashboard(
     dashboard_id: int,
 ) -> tuple[Any, AddChartToDashboardResponse | None]:
@@ -425,7 +369,7 @@ def _find_and_authorize_dashboard(
         destructiveHint=False,
     ),
 )
-def add_chart_to_existing_dashboard(  # noqa: C901
+def add_chart_to_existing_dashboard(
     request: AddChartToDashboardRequest, ctx: Context
 ) -> AddChartToDashboardResponse:
     """
@@ -499,16 +443,11 @@ def add_chart_to_existing_dashboard(  # noqa: C901
             # Generate a unique ROW ID for the new row
             row_key = _find_next_row_position(current_layout)
 
-            # Detect tabbed dashboards and resolve target_tab by name or ID.
-            parent_id, tab_error = _resolve_parent_container(
-                current_layout, request.dashboard_id, request.target_tab
+            # Detect tabbed dashboards and resolve target_tab by name or ID
+            tab_target = _find_tab_insert_target(
+                current_layout, target_tab=request.target_tab
             )
-            if tab_error is not None:
-                return tab_error
-            if parent_id is None:
-                raise RuntimeError(
-                    "unreachable: tab_error is None implies parent_id is str"
-                )
+            parent_id = tab_target if tab_target else "GRID_ID"
 
             # Add chart, column, and row to layout
             chart_key, column_key, row_key = _add_chart_to_layout(

tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py

@@ -49,7 +49,7 @@ logger = logging.getLogger(__name__)
 # ---------------------------------------------------------------------------
 
 
-@pytest.fixture()
+@pytest.fixture
 def mcp_server() -> object:
     """Return the FastMCP app instance for use in MCP client tests."""
     return mcp
@@ -138,7 +138,7 @@ def _mock_dashboard(
 
 
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_dashboard_not_found(mock_find_by_id: Mock, mcp_server: object) -> None:
     """Returns a clear error when the target dashboard does not exist."""
     mock_find_by_id.return_value = None
@@ -157,7 +157,7 @@ async def test_dashboard_not_found(mock_find_by_id: Mock, mcp_server: object) ->
 
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_permission_denied(
     mock_find_by_id: Mock, mock_raise_for_ownership: Mock, mcp_server: object
 ) -> None:
@@ -203,7 +203,7 @@ async def test_permission_denied(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_chart_not_found(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -231,7 +231,7 @@ async def test_chart_not_found(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_chart_already_in_dashboard(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -261,7 +261,7 @@ async def test_chart_already_in_dashboard(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_successful_add(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -295,17 +295,3 @@ async def test_successful_add(
     assert "/superset/dashboard/1/" in content["dashboard_url"]
     assert content["position"] is not None
     assert "chart_key" in content["position"]
-
-
-def test_empty_target_tab_rejected_by_schema() -> None:
-    """Empty string target_tab is rejected at schema layer, not as 'Tab not found'."""
-    from pydantic import ValidationError
-
-    from superset.mcp_service.dashboard.schemas import AddChartToDashboardRequest
-
-    with pytest.raises(ValidationError):
-        AddChartToDashboardRequest(dashboard_id=1, chart_id=10, target_tab="")
-
-    # None is valid (tab omitted)
-    req = AddChartToDashboardRequest(dashboard_id=1, chart_id=10, target_tab=None)
-    assert req.target_tab is None

tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py

@@ -32,7 +32,6 @@ from superset.mcp_service.chart.chart_utils import DatasetValidationResult
 from superset.mcp_service.dashboard.constants import generate_id
 from superset.mcp_service.dashboard.tool.add_chart_to_existing_dashboard import (
     _add_chart_to_layout,
-    _collect_available_tab_names,
     _ensure_layout_structure,
     _find_next_row_position,
     _find_tab_insert_target,
@@ -1060,110 +1059,6 @@ class TestAddChartToExistingDashboard:
             assert "TAB-tab2" in chart_parents
             assert "TAB-tab1" not in chart_parents
 
-    @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
-    @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-    @patch("superset.db.session")
-    @pytest.mark.asyncio
-    async def test_add_chart_target_tab_not_found(
-        self, mock_db_session, mock_find_dashboard, mock_update_command, mcp_server
-    ):
-        """target_tab specified but no matching tab → descriptive error listing
-        available tabs, not a silent fallback to the first tab."""
-        mock_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard")
-        mock_dashboard.slices = [_mock_chart(id=10)]
-        mock_dashboard.position_json = json.dumps(
-            {
-                "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type": "ROOT"},
-                "GRID_ID": {
-                    "children": ["TABS-abc123"],
-                    "id": "GRID_ID",
-                    "parents": ["ROOT_ID"],
-                    "type": "GRID",
-                },
-                "TABS-abc123": {
-                    "children": ["TAB-tab1", "TAB-tab2"],
-                    "id": "TABS-abc123",
-                    "parents": ["ROOT_ID", "GRID_ID"],
-                    "type": "TABS",
-                },
-                "TAB-tab1": {
-                    "children": [],
-                    "id": "TAB-tab1",
-                    "meta": {"text": "Overview"},
-                    "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
-                    "type": "TAB",
-                },
-                "TAB-tab2": {
-                    "children": [],
-                    "id": "TAB-tab2",
-                    "meta": {"text": "Details"},
-                    "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
-                    "type": "TAB",
-                },
-                "DASHBOARD_VERSION_KEY": "v2",
-            }
-        )
-        mock_chart = _mock_chart(id=30)
-        mock_db_session.get.return_value = mock_chart
-        mock_find_dashboard.return_value = mock_dashboard
-
-        request = {"dashboard_id": 3, "chart_id": 30, "target_tab": "Nonexistent Tab"}
-
-        async with Client(mcp_server) as client:
-            result = await client.call_tool(
-                "add_chart_to_existing_dashboard", {"request": request}
-            )
-
-            assert result.structured_content["error"] is not None
-            error = result.structured_content["error"]
-            assert "Nonexistent Tab" in error
-            assert "not found" in error
-            # Available tabs should be listed
-            assert "Overview" in error
-            assert "Details" in error
-            # No layout mutation should have been persisted
-            mock_update_command.assert_not_called()
-
-    @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
-    @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-    @patch("superset.db.session")
-    @pytest.mark.asyncio
-    async def test_add_chart_target_tab_on_non_tabbed_dashboard(
-        self, mock_db_session, mock_find_dashboard, mock_update_command, mcp_server
-    ):
-        """target_tab on a dashboard with no tabs → descriptive error."""
-        mock_dashboard = _mock_dashboard(id=5, title="Flat Dashboard")
-        mock_dashboard.slices = []
-        mock_dashboard.position_json = json.dumps(
-            {
-                "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type": "ROOT"},
-                "GRID_ID": {
-                    "children": [],
-                    "id": "GRID_ID",
-                    "parents": ["ROOT_ID"],
-                    "type": "GRID",
-                },
-                "DASHBOARD_VERSION_KEY": "v2",
-            }
-        )
-        mock_chart = _mock_chart(id=99)
-        mock_db_session.get.return_value = mock_chart
-        mock_find_dashboard.return_value = mock_dashboard
-
-        request = {"dashboard_id": 5, "chart_id": 99, "target_tab": "Sales"}
-
-        async with Client(mcp_server) as client:
-            result = await client.call_tool(
-                "add_chart_to_existing_dashboard", {"request": request}
-            )
-
-            assert result.structured_content["error"] is not None
-            error = result.structured_content["error"]
-            assert "no tabs" in error.lower()
-            assert "target_tab" in error
-            # No layout mutation should have been persisted
-            mock_update_command.assert_not_called()
-
     @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
     @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
     @patch("superset.db.session")
@@ -1416,9 +1311,9 @@ class TestLayoutHelpers:
         }
         assert _find_tab_insert_target(layout, target_tab="TAB-second") == "TAB-second"
 
-    def test_find_tab_insert_target_unmatched_returns_none(self):
-        """Test _find_tab_insert_target returns None when target_tab doesn't
-        match any tab name or ID, so the caller can return a descriptive error."""
+    def test_find_tab_insert_target_unmatched_falls_back_to_first(self):
+        """Test _find_tab_insert_target falls back to first tab when target_tab
+        doesn't match any tab name or ID."""
         layout = {
             "GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
             "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"},
@@ -1433,17 +1328,9 @@ class TestLayoutHelpers:
                 "meta": {"text": "Tab 2"},
             },
         }
-        assert _find_tab_insert_target(layout, target_tab="Nonexistent Tab") is None
-
-    def test_find_tab_insert_target_empty_string_returns_none(self):
-        """An empty-string target_tab is treated as specified-but-not-found,
-        not as 'no tab requested', so it returns None rather than first tab."""
-        layout = {
-            "GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
-            "TABS-main": {"children": ["TAB-first"], "type": "TABS"},
-            "TAB-first": {"children": [], "type": "TAB", "meta": {"text": "Tab 1"}},
-        }
-        assert _find_tab_insert_target(layout, target_tab="") is None
+        assert (
+            _find_tab_insert_target(layout, target_tab="Nonexistent Tab") == "TAB-first"
+        )
 
     def test_find_tab_insert_target_tabs_under_root(self):
         """Test _find_tab_insert_target when TABS are under ROOT_ID (real layout)."""
@@ -1471,35 +1358,6 @@ class TestLayoutHelpers:
         """Test _find_tab_insert_target with missing GRID_ID."""
         assert _find_tab_insert_target({"ROOT_ID": {"type": "ROOT"}}) is None
 
-    def test_collect_available_tab_names_returns_display_names(self):
-        """_collect_available_tab_names returns each tab's display text."""
-        layout = {
-            "GRID_ID": {"children": ["TABS-x"], "type": "GRID"},
-            "TABS-x": {"children": ["TAB-a", "TAB-b"], "type": "TABS"},
-            "TAB-a": {"children": [], "type": "TAB", "meta": {"text": "Overview"}},
-            "TAB-b": {"children": [], "type": "TAB", "meta": {"text": "Details"}},
-        }
-        names = _collect_available_tab_names(layout)
-        assert names == ["Overview", "Details"]
-
-    def test_collect_available_tab_names_falls_back_to_id(self):
-        """_collect_available_tab_names uses component ID when text is empty."""
-        layout = {
-            "GRID_ID": {"children": ["TABS-x"], "type": "GRID"},
-            "TABS-x": {"children": ["TAB-a"], "type": "TABS"},
-            "TAB-a": {"children": [], "type": "TAB", "meta": {}},
-        }
-        names = _collect_available_tab_names(layout)
-        assert names == ["TAB-a"]
-
-    def test_collect_available_tab_names_no_tabs(self):
-        """_collect_available_tab_names returns empty list for non-tabbed dashboards."""
-        layout = {
-            "GRID_ID": {"children": ["ROW-1"], "type": "GRID"},
-            "ROW-1": {"children": [], "type": "ROW"},
-        }
-        assert _collect_available_tab_names(layout) == []
-
     def test_add_chart_to_layout_creates_column(self):
         """Test that _add_chart_to_layout creates ROW > COLUMN > CHART."""
         layout = {