mikebridge commented on code in PR #44028:
URL: https://github.com/apache/superset/pull/44028#discussion_r4008449568
##########
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:
Real — fixed in 2c3a7d9ba4. `set_dash_metadata` skips a null `positions`
entirely (`data.get(\"positions\") is not None`), so a presence-only test
skipped the raw reconcile while nothing superseded it. The skip now mirrors the
DAO's exact test; integration test added (`positions: null` + raw dangling
layout → raw field reconciled).
##########
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:
Not new surface: the pre-existing membership rebuild in `set_dash_metadata`
(the `Slice.id.in_(slice_ids)` a few lines below) already issues the identical
`IN` over the same layout ids, so the reconcile adds no additional bound;
layout size is governed by the request limits that apply to that rebuild today.
Leaving as is.
##########
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:
Real — fixed in 2c3a7d9ba4. Ignoring a malformed CHART node is right for the
repair path (it never rebuilds membership) but not here, where the rebuild is
wholesale and skipping the node silently detached the chart it referenced (the
pre-reconcile code failed that save with a 500 from the bad `IN` value).
`set_dash_metadata` now fails closed with a `DashboardInvalidError` naming the
slot(s) — a 422 through the existing API mapping — before any membership is
touched. Unit + integration tests added (memberships intact after the refusal).
--
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]
