codeant-ai-for-open-source[bot] commented on code in PR #44028:
URL: https://github.com/apache/superset/pull/44028#discussion_r4007206778
##########
superset/commands/dashboard/update.py:
##########
@@ -93,15 +87,39 @@ def run(self) -> Model:
# field to its default -- so it is excluded here and applied
# exclusively via ``set_dash_metadata``.
json_metadata = self._properties.get("json_metadata")
+ metadata: dict[str, Any] | None = (
+ json.loads(json_metadata) if json_metadata else None
+ )
+
+ # Re-serialize position_json to escape 4-byte Unicode characters,
+ # and reconcile it against membership: a layout node referencing a
+ # chart that no longer resolves to any Slice row (hard-deleted) is
+ # swapped for a placeholder so ``position_json`` cannot keep
+ # accumulating dangling chart references (sc-115325).
+ #
+ # Precedence: when ``json_metadata`` carries ``positions`` (the
+ # frontend always sends them there), ``set_dash_metadata``
reconciles
+ # those and overwrites ``position_json`` from them, so a raw
+ # ``position_json`` field sent alongside is superseded. Reconciling
+ # it here would be dead work โ including its membership query โ so
+ # this branch runs only for a PUT that sends the raw field without
+ # ``positions`` in ``json_metadata``.
+ metadata_carries_positions = isinstance(metadata, dict) and (
+ "positions" in metadata
+ )
Review Comment:
**Suggestion:** Checking only for the `positions` key skips raw
reconciliation when its value is `null`; `set_dash_metadata` then leaves the
raw dangling layout unchanged. [incorrect condition logic]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ed519338e4b4e01a2e8daeaa497f29f&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=9ed519338e4b4e01a2e8daeaa497f29f&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/commands/dashboard/update.py
**Line:** 107:109
**Comment:**
*Incorrect Condition Logic: Checking only for the `positions` key skips
raw reconciliation when its value is `null`; `set_dash_metadata` then leaves
the raw dangling layout unchanged.
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%2F44028&comment_hash=cfe82cca5ea682b6bb4ad2918f2761177c7374a6ec16b722f1d7feaa4100ada8&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44028&comment_hash=cfe82cca5ea682b6bb4ad2918f2761177c7374a6ec16b722f1d7feaa4100ada8&reaction=dislike'>๐</a>
##########
superset/daos/dashboard.py:
##########
@@ -81,6 +81,151 @@
}
+# User-facing text persisted into a dangling tile's ``position_json`` slot.
+# A plain literal, not ``gettext``: see ``_repair_dangling_chart_nodes``.
+MISSING_CHART_PLACEHOLDER = "This chart no longer exists."
+
+
+def _layout_chart_id(node: Any) -> int | None:
+ """Return the integer ``chartId`` of a ``CHART`` layout node, else
``None``.
+
+ Mirrors the frontend ``layoutChartId`` โ only ``type == "CHART"`` nodes
+ carry a chart reference; everything else (rows, tabs, markdown, headers)
+ returns ``None``.
+
+ Defensive against malformed persisted layout JSON, which is only validated
+ as parseable: a non-dict ``meta`` (e.g. ``"meta": "x"``) or a non-numeric
+ ``chartId`` (e.g. ``"chartId": [1]``) yields ``None`` so the node is
ignored
+ rather than raising on a write path. ``chartId == 0`` is returned as a real
+ reference โ no ``Slice`` has id 0, so it resolves as absent and is
repaired,
+ matching the frontend, which also recognizes 0.
+
+ Numeric *forms* of an id are coerced rather than dropped: legacy, imported,
+ or JSON-round-tripped layouts can carry ``123.0`` (float) or ``"123"``
+ (digit string), which the pre-reconcile code passed straight into
+ ``Slice.id.in_(...)``. Returning ``None`` for those would silently unlink a
+ real chart on the next save (excluded from the membership rebuild) AND
+ skip its repair (no id to resolve) โ a permanent orphan tile. An integral
+ float or a digit string is therefore read as its ``int``; a fractional
+ float, a non-digit string, or a ``bool`` is still ``None``.
+ """
+ if not isinstance(node, dict) or node.get("type") != "CHART":
+ return None
+ meta = node.get("meta")
+ if not isinstance(meta, dict):
+ return None
+ chart_id = meta.get("chartId")
+ # ``bool`` is an ``int`` subclass; exclude it so a stray ``true`` is not
+ # misread as chartId 1.
+ if isinstance(chart_id, bool):
+ return None
+ if isinstance(chart_id, int):
+ return chart_id
+ if isinstance(chart_id, float) and chart_id.is_integer():
+ return int(chart_id)
+ if isinstance(chart_id, str) and chart_id.strip().isdigit():
+ return int(chart_id.strip())
+ return None
+
+
+def _repair_dangling_chart_nodes(
+ positions: dict[str, Any],
+ valid_chart_ids: set[int],
+ dashboard_id: int | None = None,
+) -> int:
+ """Swap every ``CHART`` layout node whose ``chartId`` is absent from
+ *valid_chart_ids* for a markdown placeholder, in place, and return how
+ many were repaired (logging a diagnostic when any were, since this mutates
+ persisted, shared layout data).
+
+ ``position_json`` is a plain column with no coordination against
+ ``dashboard_slices``: a chart hard-deleted while it was a live member
+ leaves a layout node referencing an id that no longer resolves to any
+ ``Slice`` row, and a restore-with-skips reproduces the same divergence
+ (sc-115325). Left alone, such a slot silently accumulates
+ ``meta.uuid = None`` on every save.
+
+ The repair mirrors the frontend ``swapUnreachableChartSlots`` (#41551):
+ keep the node's id, children, and geometry, and replace only ``type`` and
+ ``meta`` so the slot renders the same "chart no longer exists" placeholder
+ the client already shows at render time โ persisting it rather than
+ re-deriving it every load. *valid_chart_ids* must be resolved with the
+ soft-delete visibility filter bypassed, so a soft-deleted (recoverable)
+ member is never treated as dangling.
+
+ The placeholder text is a plain literal, not ``gettext``: ``position_json``
+ is persisted, shared data rendered verbatim to every viewer, so translating
+ it to the saving user's request locale would bake one language into content
+ shown to everyone. Per-viewer localization stays the frontend's job (its
+ ``MissingChart`` render path), which is request-scoped.
+ """
+ repaired = 0
+ for key, node in positions.items():
+ chart_id = _layout_chart_id(node)
+ if chart_id is not None and chart_id not in valid_chart_ids:
+ meta = node.get("meta") or {}
+ positions[key] = {
+ **node,
+ "type": "MARKDOWN",
+ "meta": {
+ "width": meta.get("width"),
+ "height": meta.get("height"),
+ "code": MISSING_CHART_PLACEHOLDER,
+ },
+ }
+ repaired += 1
+ if repaired:
+ logger.info(
+ "Repaired %d dangling chart tile(s) in dashboard %s position_json",
+ repaired,
+ dashboard_id,
+ )
+ return repaired
+
+
+def _existing_chart_ids(chart_ids: set[int]) -> set[int]:
+ """Subset of *chart_ids* that resolve to an actual ``Slice`` row,
+ including soft-deleted ones.
+
+ The soft-delete visibility filter is bypassed on purpose: a soft-deleted
+ chart is still a recoverable dashboard member (its ``dashboard_slices``
+ junction row survives), so its layout slot must be preserved, not
+ repaired away.
+ """
+ ids = {cid for cid in chart_ids if cid}
+ if not ids:
+ return set()
+ with skip_visibility_filter(db.session, Slice):
+ rows = db.session.query(Slice.id).filter(Slice.id.in_(ids)).all()
+ return {row[0] for row in rows}
Review Comment:
**Suggestion:** An attacker with dashboard edit access can submit a huge
parseable layout, producing an oversized `IN` query that may exceed database
parameter limits or consume excessive resources. [performance]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ed7b18c8c55c46c8be4f8d109e988e5c&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=ed7b18c8c55c46c8be4f8d109e988e5c&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/daos/dashboard.py
**Line:** 198:200
**Comment:**
*Performance: An attacker with dashboard edit access can submit a huge
parseable layout, producing an oversized `IN` query that may exceed database
parameter limits or consume excessive resources.
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%2F44028&comment_hash=4e223de7ea5ad0a1d1b17a8607469ccf2f12e6a94c358b07243361961503a523&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44028&comment_hash=4e223de7ea5ad0a1d1b17a8607469ccf2f12e6a94c358b07243361961503a523&reaction=dislike'>๐</a>
##########
superset/daos/dashboard.py:
##########
@@ -343,9 +488,9 @@ def set_dash_metadata(
if (positions := data.get("positions")) is not None:
# find slices in the position data
slice_ids = [
- value.get("meta", {}).get("chartId")
+ chart_id
for value in positions.values()
- if isinstance(value, dict)
+ if (chart_id := _layout_chart_id(value)) is not None
]
Review Comment:
**Suggestion:** Malformed chart nodes produce no ID, so the wholesale
assignment removes existing dashboard memberships even though those nodes are
only meant to be ignored. [api mismatch]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c6acb3b6d684a87adb0e91ac5118db6&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=9c6acb3b6d684a87adb0e91ac5118db6&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/daos/dashboard.py
**Line:** 490:494
**Comment:**
*Api Mismatch: Malformed chart nodes produce no ID, so the wholesale
assignment removes existing dashboard memberships even though those nodes are
only meant to be ignored.
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%2F44028&comment_hash=342675c401d41ffd77303fef3cb9acf833555b110687c4ca816e0d10cbda0a4b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44028&comment_hash=342675c401d41ffd77303fef3cb9acf833555b110687c4ca816e0d10cbda0a4b&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]
