aminghadersohi commented on code in PR #43476:
URL: https://github.com/apache/superset/pull/43476#discussion_r3846220998
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -237,6 +238,14 @@ def _validate_update_request(
from superset.dashboards.schemas import validate_css
from superset.tags.models import ObjectType
+ if request.position_json is not None:
+ chart_ids = [chart.id for chart in dashboard.slices]
+ if error := validate_dashboard_layout(request.position_json,
chart_ids):
+ return DashboardError(
Review Comment:
Not changing this one, but I traced the consequence rather than waving it
off.
The window is real — `dashboard.slices` is read in
`_validate_update_request` and the commit happens later in the same request —
but both directions of the race degrade into states the frontend already
handles, and neither is the blank-dashboard class this PR exists to prevent:
- **Chart added concurrently** (missing from the committed layout):
hydration has no `CHART` node for it at all, so `chartIdToLayoutId[key]` is
unset and `hydrate.ts` auto-places it into a new row
(`superset-frontend/src/dashboard/actions/hydrate.ts`, the `if
(!chartIdToLayoutId[key] && layout[parentId])` branch). This is the
self-healing "associated but unplaced" state.
- **Chart removed concurrently** (stale node in the committed layout): the
node is reachable, so it renders through the normal missing-chart placeholder
path.
The failure this PR targets is specifically *present but unreachable* — a
`CHART` node that hydration indexes (suppressing the auto-place fallback) while
the renderer never reaches it. The validator guarantees every node in the
committed tree is reachable from `ROOT_ID`, and a concurrently-added chart has
no node at all rather than an unreachable one, so this race cannot produce that
state.
Closing the window properly would need `SELECT ... FOR UPDATE` or a version
column on the dashboard, which is a broader change than this PR and would be
inconsistent with the REST `UpdateDashboardCommand` path, which has the same
read-then-commit shape today. Re-reading `dashboard.slices` just before commit
would narrow the window without closing it while adding a query, so it buys
correctness theater rather than correctness. Happy to file it separately if you
think dashboard-level optimistic locking is worth doing across both paths.
--
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]
