tien238lnd commented on code in PR #43575:
URL: https://github.com/apache/superset/pull/43575#discussion_r3877714395
##########
superset/models/dashboard.py:
##########
@@ -378,43 +378,98 @@ 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")
+ title = meta.get("text") if isinstance(meta, dict) else None
+ if not isinstance(title, str):
+ logger.warning(
+ "Dashboard %s: tab node %s has no title in the layout",
+ self.id,
+ node.get("id"),
+ )
+ title = ""
+ node["title"] = title
+ node_id = node.get("id")
+ if not isinstance(node_id, str):
+ logger.warning(
+ "Dashboard %s: skipping tab node with no usable id in the
layout",
+ self.id,
+ )
+ return
+ node["value"] = node_id
+ all_tabs[node_id] = 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"]
+ child_ids = node.get("children", [])
+ if not isinstance(child_ids, list):
+ logger.warning(
+ "Dashboard %s: layout node %s has malformed children",
+ self.id,
+ node.get("id"),
+ )
+ child_ids = []
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)
- if node["type"] == "TABS":
+ for child_id in child_ids:
+ child = get_node(child_id) if isinstance(child_id, str) else
None
+ if not isinstance(child, dict):
+ logger.warning(
+ "Dashboard %s: skipping layout node %s, missing or
malformed",
+ self.id,
+ child_id,
+ )
+ continue
+ if node_type == "TABS":
# if TABS add create a new list and append children to it
# new_children.append(child)
children.append(child)
Review Comment:
Yes — they should be, and they now are. Fixed in c8d8bbb.
You're right about the ordering: the TABS parent appends the child before
`register_tab` gets a chance to reject it, so a TAB with `id: []` ended up in
`tab_tree` with no `value`, while being correctly excluded from `all_tabs`.
Reproduced against the previous head:
```
all_tabs = {'TAB-2': 'Good'}
tree entry: id=[] value=<missing> title='Bad'
tree entry: id='TAB-2' value='TAB-2' title='Good'
```
Such a tab is unusable either way — it cannot key `all_tabs`, and it cannot
carry a `value` for the picker — so it is now skipped where the parent appends
it, with the same WARNING as the other degraded nodes. `tab_tree` and
`all_tabs` stay consistent, and every tree entry has a `value` again.
`test_tabs_skips_tab_nodes_without_an_id` now also asserts the unusable tabs
are absent from the tree and that each remaining entry carries a `value`. It
fails against the previous commit.
---
_Generated by [Claude Code](https://claude.ai/code)_
--
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]
