fitzee commented on code in PR #44028:
URL: https://github.com/apache/superset/pull/44028#discussion_r4001517977
##########
superset/commands/dashboard/update.py:
##########
@@ -77,11 +77,18 @@ def run(self) -> Model:
ObjectType.dashboard, self._model.id, self._model.tags,
tags
)
- # Re-serialize position_json to escape 4-byte Unicode characters
+ # 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). The
+ # ``set_dash_metadata`` path reconciles its own ``positions`` from
+ # ``json_metadata``; this covers a PUT that sends only the raw
+ # ``position_json`` field.
if position_json := self._properties.get("position_json"):
- self._properties["position_json"] = json.dumps(
- json.loads(position_json)
- )
+ positions: object = json.loads(position_json)
+ reconcile_position_json(positions, self._model.id)
Review Comment:
**Redundant reconcile whose result is discarded when a PUT also sends
`json_metadata.positions`.** When both the raw `position_json` field and
`json_metadata` (which carries `positions` — the frontend puts them there,
dashboardState.ts:754-757) are present, this path reconciles the raw field and
`DashboardDAO.update` writes it, but `set_dash_metadata` then re-reconciles its
own `positions` and overwrites `dashboard.position_json` (daos line ~515/388).
So the work here — including the `_existing_chart_ids` DB query — is thrown
away, and the raw `position_json` field is silently ignored in favor of
`json_metadata.positions` for that save. No correctness harm (set_dash_metadata
reconciles too), and the standard frontend save sends only
`json_metadata.positions` so this path doesn't fire there — but for an API
client sending both, the raw-field reconcile is dead work and the
field-vs-metadata precedence is undocumented. Consider skipping the raw
reconcile when `json_metadata`
with positions is present, or noting the precedence.
##########
superset/daos/dashboard.py:
##########
@@ -343,9 +473,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:
**Edge/legacy regression: a non-int `chartId` (float/string) is now silently
unlinked from `dashboard.slices` and left as an orphaned CHART node.**
`_layout_chart_id` requires `isinstance(chart_id, int)` (line 111), so a CHART
node whose `meta.chartId` is `123.0` (float) or `"123"` (string) — from legacy
data, an import, or a JSON round-trip — now returns `None`. The old code used
`value.get('meta',{}).get('chartId')` with no type check, so the id landed in
`slice_ids` and (at least for a float, and for strings on lax backends like
SQLite) still resolved via `Slice.id.in_(...)`, keeping the chart in the
rebuilt `dashboard.slices`.
Now that node is excluded from `slice_ids` → the wholesale `dashboard.slices
= current_slices` rebuild drops its `dashboard_slices` junction row → the chart
is unlinked on the next save. And because `_layout_chart_id` returns `None`,
`_repair_dangling_chart_nodes` also skips it, so the node isn't converted to a
placeholder either — it's left as a permanent orphan CHART node no longer
backed by membership. The frontend never emits non-int chartIds, so this is
legacy/imported-data only, not a mainstream regression — but worth either
coercing numeric chartIds (`int(chart_id)` when it's a float/digit-string) or
logging/repairing such nodes rather than silently dropping them.
--
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]
