aminghadersohi commented on code in PR #43476:
URL: https://github.com/apache/superset/pull/43476#discussion_r3846218215
##########
superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:
##########
@@ -59,14 +60,12 @@ def _find_chart_keys(layout: Dict[str, Any], chart_id: int)
-> list[str]:
A chart can legitimately appear more than once in a layout (e.g. under
multiple tabs), so all occurrences are returned.
"""
- # Accept both int and string chartId — position_json is
user/frontend-authored
- # and imported or hand-edited layouts may store chartId as a string.
return [
key
for key, node in layout.items()
if isinstance(node, dict)
and node.get("type") == "CHART"
- and (node.get("meta") or {}).get("chartId") in (chart_id,
str(chart_id))
+ and normalize_chart_id((node.get("meta") or {}).get("chartId")) ==
chart_id
Review Comment:
Fixed in 7669bbae. `normalize_chart_id` now rejects non-canonical decimal
strings, so `"001"` no longer matches a chart during layout removal. That keeps
it consistent with `_clean_json_metadata`, which keys off `str(chart_id)` — the
two paths can no longer disagree about whether a reference matches, so a chart
cannot be detached while stale `001` entries survive in `expanded_slices`,
`timed_refresh_immune_slices`, `filter_scopes`, or `default_filters`. Added
`test_rejects_leading_zero_string_chart_id`, which fails without the change.
##########
superset/mcp_service/dashboard/layout_validation.py:
##########
@@ -0,0 +1,261 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Validation for dashboard layouts supplied through MCP tools."""
+
+from __future__ import annotations
+
+from collections.abc import Collection
+from typing import Any
+
+_ROOT_ID = "ROOT_ID"
+_GRID_ID = "GRID_ID"
+_HEADER_ID = "HEADER_ID"
+_VERSION_KEY = "DASHBOARD_VERSION_KEY"
+_CHART_TYPE = "CHART"
+
+# Keep in sync with the frontend's parent/child contract in
+# superset-frontend/src/dashboard/util/isValidChild.ts. The frontend also uses
+# depth limits for drag-and-drop; validation is iterative so deeply nested
input
+# cannot overflow Python's call stack.
+_ALLOWED_CHILD_TYPES: dict[str, frozenset[str]] = {
+ "ROOT": frozenset({"GRID", "TABS"}),
+ "GRID": frozenset(
+ {
+ "CHART",
+ "COLUMN",
+ "DIVIDER",
+ "DYNAMIC",
+ "HEADER",
+ "MARKDOWN",
+ "ROW",
+ "TABS",
+ }
+ ),
+ "ROW": frozenset({"CHART", "COLUMN", "DYNAMIC", "MARKDOWN"}),
+ "TABS": frozenset({"TAB"}),
+ "TAB": frozenset(
+ {
+ "CHART",
+ "COLUMN",
+ "DIVIDER",
+ "DYNAMIC",
+ "HEADER",
+ "MARKDOWN",
+ "ROW",
+ "TABS",
+ }
+ ),
+ "COLUMN": frozenset({"CHART", "DIVIDER", "HEADER", "MARKDOWN", "ROW",
"TABS"}),
+ "CHART": frozenset(),
+ "DIVIDER": frozenset(),
+ "DYNAMIC": frozenset(),
+ "HEADER": frozenset(),
+ "MARKDOWN": frozenset(),
+}
+_CONTAINER_TYPES = frozenset(
+ component_type
+ for component_type, child_types in _ALLOWED_CHILD_TYPES.items()
+ if child_types
+)
+_META_REQUIRED_TYPES = frozenset(_ALLOWED_CHILD_TYPES) - {"ROOT", "GRID"}
+
+
+def normalize_chart_id(value: Any) -> int | None:
+ """Normalize an integer or canonical decimal-string chart ID."""
+ if isinstance(value, bool):
+ return None
+ if isinstance(value, int):
+ return value if value > 0 else None
+ if isinstance(value, str) and value.isascii() and value.isdecimal():
+ normalized = int(value)
+ return normalized if normalized > 0 else None
Review Comment:
Fixed in 7669bbae. The decimal-string length is now bounded before `int()`,
so an oversized value cannot hit CPython's integer string conversion limit
(confirmed: 4300 digits by default) and raise `ValueError` out of the
validator. Malformed IDs return `None` and surface as a structured
`InvalidDashboardLayout` like every other invalid input. Added
`test_rejects_oversized_string_chart_id`, which fails without the change.
--
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]
