aminghadersohi commented on code in PR #43476: URL: https://github.com/apache/superset/pull/43476#discussion_r3846219615
########## 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 + return None + + +def _validate_component_shapes( # noqa: C901 + layout: dict[str, Any], +) -> tuple[dict[str, dict[str, Any]], str | None]: + """Validate and return every component object in a raw layout mapping.""" + if layout.get(_VERSION_KEY) != "v2": + return {}, f"{_VERSION_KEY} must be the string 'v2'." + + components: dict[str, dict[str, Any]] = {} + for component_id, component in layout.items(): + if component_id == _VERSION_KEY: + continue + if not isinstance(component, dict): + return {}, f"Layout value {component_id} must be a component object." + if component.get("id") != component_id: + return {}, f"Layout component {component_id} must have the same id value." + + component_type = component.get("type") + if not isinstance(component_type, str) or component_type not in ( + _ALLOWED_CHILD_TYPES + ): + return {}, f"Layout component {component_id} has unsupported type." + if component_type == "DYNAMIC": + return {}, ( + f"Layout component {component_id} uses DYNAMIC, which cannot be " + "safely validated by the server." + ) + + children = component.get("children") + if component_type in _CONTAINER_TYPES and children is None: + return {}, f"Layout component {component_id} must define children." + if children is not None and ( + not isinstance(children, list) + or not all(isinstance(child_id, str) for child_id in children) + ): + return {}, f"Layout component {component_id}.children must be a list." + if component_type not in _CONTAINER_TYPES and children not in (None, []): + return {}, f"Layout component {component_id} cannot have children." + if component_type == "TABS" and not children: + return {}, f"Tabs component {component_id} must contain at least one tab." + if component_type in _META_REQUIRED_TYPES and not isinstance( + component.get("meta"), dict + ): + return {}, f"Layout component {component_id}.meta must be an object." + + components[component_id] = component + + return components, None + + +def _validate_edges( + components: dict[str, dict[str, Any]], +) -> tuple[dict[str, str], str | None]: + """Validate graph edges and return each component's actual parent.""" + parent_by_child: dict[str, str] = {} + for parent_id, parent in components.items(): + parent_type = parent["type"] + for child_id in parent.get("children") or []: + child = components.get(child_id) + if child is None: + return {}, f"Layout references missing component {child_id}." + if child["type"] not in _ALLOWED_CHILD_TYPES[parent_type]: + return {}, ( + f"Layout component {child_id} cannot be a child of {parent_id}." + ) + if child_id in parent_by_child: + return {}, f"Layout component {child_id} has more than one parent." + parent_by_child[child_id] = parent_id + + if _ROOT_ID in parent_by_child: + return {}, "ROOT_ID must not have a parent." + return parent_by_child, None + + +def _find_cycle(components: dict[str, dict[str, Any]]) -> str | None: + """Return a component ID in a cycle using an iterative depth-first walk.""" + state: dict[str, int] = {} + for start_id in components: + if state.get(start_id) == 2: + continue + stack: list[tuple[str, bool]] = [(start_id, False)] Review Comment: Fixed in 7669bbae. Fair point — the validator was mirroring the type half of `isValidChild.ts` but not the depth half, which is inconsistent since `_ALLOWED_CHILD_TYPES` was itself derived from `parentMaxDepthLookup`'s shape. The allowed-child-type table is now derived directly from the depth table (`_PARENT_MAX_DEPTH`), so the two cannot drift apart. Depth is tracked on the existing iterative reachability walk rather than recursively, and it is derived from the validated `children` edges rather than trusted from the payload — same reasoning as `parents` in 037c7eaf. `TABS` and `TAB` pass their depth through to children, matching the worked examples in the header comment of `isValidChild.ts`. I checked this against the regression class from the earlier `parents` discussion before committing: all 9 shipped example dashboards still validate clean under the depth check, so this does not reintroduce false rejection of real saved layouts. Tests added: `test_rejects_nesting_beyond_frontend_depth_limit` (fails without the change), plus `test_accepts_maximum_supported_nesting_depth` and `test_accepts_tabs_without_consuming_depth` to pin the upper bound and the tab pass-through so a future tightening can't silently over-reject. -- 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]
