Copilot commented on code in PR #43575:
URL: https://github.com/apache/superset/pull/43575#discussion_r3868686315
##########
superset/models/dashboard.py:
##########
@@ -378,43 +378,90 @@ def position(self) -> dict[str, Any]:
return {}
@property
- def tabs(self) -> dict[str, Any]:
+ def tabs(self) -> dict[str, Any]: # noqa: C901
+ if not isinstance(self.position, dict):
+ logger.warning("Dashboard %s: layout is not a mapping", self.id)
+ return {}
if self.position == {}:
return {}
- def get_node(node_id: str) -> dict[str, Any]:
+ def get_node(node_id: str) -> Optional[dict[str, Any]]:
"""
Helper function for getting a node from the position_data
"""
- return self.position[node_id]
+ return self.position.get(node_id)
+
+ def register_tab(node: dict[str, Any]) -> None:
+ """
+ Helper function for titling a TAB node and adding it to all_tabs
+ """
+ meta = node.get("meta")
+ if not isinstance(meta, dict):
+ meta = {}
+ if "text" not in meta:
+ logger.warning(
+ "Dashboard %s: tab node %s has no title in the layout",
+ self.id,
+ node.get("id"),
+ )
+ node["title"] = meta.get("text", "")
+ node_id = node.get("id")
+ if node_id is None:
+ logger.warning(
+ "Dashboard %s: skipping tab node with no id in the layout",
+ self.id,
+ )
+ return
+ node["value"] = node_id
+ all_tabs[node_id] = node["title"]
def build_tab_tree(
node: dict[str, Any], children: list[dict[str, Any]]
) -> None:
"""
Function for building the tab tree structure and list of all tabs
"""
+ if "type" not in node:
+ logger.warning(
+ "Dashboard %s: skipping untyped layout node %s",
+ self.id,
+ node.get("id"),
+ )
+ return
+ # A node whose type is not one of the four below is walked through
+ # without contributing to the tree, exactly as an untabbed layout
+ # element always has been.
+ node_type = node["type"]
new_children: list[dict[str, Any]] = []
# new children to overwrite parent's children
for child_id in node.get("children", []):
child = get_node(child_id)
Review Comment:
`build_tab_tree` assumes `node["children"]` is iterable. Since
`position_json` is only validated for JSON parseability, a stored node can have
`"children": null` (or another non-list), which will raise `TypeError:
'NoneType' object is not iterable` and reintroduce a 500. Consider
normalizing/validating the `children` field before iterating and logging when
it is malformed.
##########
superset/models/dashboard.py:
##########
@@ -378,43 +378,90 @@ def position(self) -> dict[str, Any]:
return {}
@property
- def tabs(self) -> dict[str, Any]:
+ def tabs(self) -> dict[str, Any]: # noqa: C901
+ if not isinstance(self.position, dict):
+ logger.warning("Dashboard %s: layout is not a mapping", self.id)
+ return {}
if self.position == {}:
Review Comment:
`tabs` repeatedly accesses `self.position`, which re-parses `position_json`
via `json.loads` on every call (including inside `get_node`). For large layouts
this becomes an avoidable hotspot and can amplify warning logs if parsing ever
changes behavior. Cache the parsed value in a local variable and have
`get_node` close over it.
--
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]
