codeant-ai-for-open-source[bot] commented on code in PR #43476: URL: https://github.com/apache/superset/pull/43476#discussion_r3845966997
########## 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: **Suggestion:** The validator does not enforce the frontend's maximum nesting depths from `parentMaxDepthLookup`. An MCP replacement can therefore persist ROW/COLUMN nesting that the dashboard editor explicitly rejects, producing layouts outside the supported parent/depth contract. Track the effective depth during iterative traversal and reject edges exceeding the corresponding frontend limit. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ MCP can persist layouts outside frontend depth limits. - ⚠️ Dashboard editor operations reject persisted deep nesting. - ⚠️ Users may need manual layout repair before editing. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=319afa51feab49d3b1799fb18cdf77f3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=319afa51feab49d3b1799fb18cdf77f3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/layout_validation.py **Line:** 168:169 **Comment:** *Incomplete Implementation: The validator does not enforce the frontend's maximum nesting depths from `parentMaxDepthLookup`. An MCP replacement can therefore persist ROW/COLUMN nesting that the dashboard editor explicitly rejects, producing layouts outside the supported parent/depth contract. Track the effective depth during iterative traversal and reject edges exceeding the corresponding frontend limit. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43476&comment_hash=2aee941be18783d519b16404cad63e8582cd33c550c5a01f7fb1d05d9cb8f734&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43476&comment_hash=2aee941be18783d519b16404cad63e8582cd33c550c5a01f7fb1d05d9cb8f734&reaction=dislike'>👎</a> ########## 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: **Suggestion:** `int(value)` can raise `ValueError` for an excessively long decimal string when Python's integer string conversion limit is enabled. Because this value comes from the MCP payload and is not caught, a malformed chart ID causes the validator to escape instead of returning `InvalidDashboardLayout`; bound the string length or catch the conversion error and return `None`. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Malformed layout requests escape structured MCP validation. - ⚠️ Clients receive an unexpected tool exception. - ⚠️ Invalid updates may not produce consistent error types. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f3409f546ba942e5bd66db467a790d1d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f3409f546ba942e5bd66db467a790d1d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/layout_validation.py **Line:** 84:86 **Comment:** *Type Error: `int(value)` can raise `ValueError` for an excessively long decimal string when Python's integer string conversion limit is enabled. Because this value comes from the MCP payload and is not caught, a malformed chart ID causes the validator to escape instead of returning `InvalidDashboardLayout`; bound the string length or catch the conversion error and return `None`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43476&comment_hash=76536bba942017b4eaf96829b0b0808ffe7864307251fcc3fa66176ae2205ffe&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43476&comment_hash=76536bba942017b4eaf96829b0b0808ffe7864307251fcc3fa66176ae2205ffe&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/update_dashboard.py: ########## @@ -237,6 +238,14 @@ def _validate_update_request( from superset.dashboards.schemas import validate_css from superset.tags.models import ObjectType + if request.position_json is not None: + chart_ids = [chart.id for chart in dashboard.slices] + if error := validate_dashboard_layout(request.position_json, chart_ids): + return DashboardError( Review Comment: **Suggestion:** The chart set is snapshotted from `dashboard.slices` before the later mutation and commit, with no optimistic-lock or revalidation. If another operation adds a chart after this snapshot but before this commit, this replacement can be accepted without that chart and leave the newly associated chart unreachable; if a chart is removed concurrently, a valid replacement can instead be rejected. Validate and persist against the same locked/versioned dashboard state, or recheck the associations immediately before commit. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Concurrent chart additions can become unreachable. - ⚠️ Concurrent removals can leave stale chart nodes. - ❌ Dashboard layout and chart associations become inconsistent. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3acb407634d948cd9aa7bb76a6cee2f2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3acb407634d948cd9aa7bb76a6cee2f2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/update_dashboard.py **Line:** 241:244 **Comment:** *Race Condition: The chart set is snapshotted from `dashboard.slices` before the later mutation and commit, with no optimistic-lock or revalidation. If another operation adds a chart after this snapshot but before this commit, this replacement can be accepted without that chart and leave the newly associated chart unreachable; if a chart is removed concurrently, a valid replacement can instead be rejected. Validate and persist against the same locked/versioned dashboard state, or recheck the associations immediately before commit. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43476&comment_hash=a067ffd5dcc5751b5cc0c53c7bc7846a6a7f37b10236c8c6cb6587e395d067cd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43476&comment_hash=a067ffd5dcc5751b5cc0c53c7bc7846a6a7f37b10236c8c6cb6587e395d067cd&reaction=dislike'>👎</a> -- 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]
