msyavuz commented on code in PR #42002:
URL: https://github.com/apache/superset/pull/42002#discussion_r3594814995
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -164,14 +164,81 @@ def _collect_metadata_overrides(request:
UpdateDashboardRequest) -> dict[str, An
return overrides
-def _apply_field_updates(dashboard: Any, request: UpdateDashboardRequest) ->
list[str]:
+def _resolve_owners(owner_ids: list[int]) -> tuple[list[Any], list[int]]:
+ """Resolve owner user IDs to user objects.
+
+ Returns ``(users, missing_ids)``, deduplicating IDs while preserving the
+ caller's order. ``security_manager`` is imported lazily for the same
+ app-context reason as ``_find_and_authorize_dashboard``.
+ """
+ from superset import security_manager
+
+ users: list[Any] = []
+ missing: list[int] = []
+ seen: set[int] = set()
+ for uid in owner_ids:
+ if uid in seen:
+ continue
+ seen.add(uid)
+ user = security_manager.get_user_by_id(uid)
Review Comment:
These are FAB `User` rows, but the editors relationship takes `Subject` rows
— different objects and a different ID space than `list_users` returns;
resolution should go through `populate_subject_list` /
`get_or_create_user_subject`.
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -328,12 +408,18 @@ def update_dashboard(
if validation_error is not None:
return validation_error
+ # Resolve owners once up front so the same list is used for validation and
+ # the write (no second lookup that could drop a concurrently-removed user).
+ resolved_owners, owners_error = _resolve_and_validate_owners(request)
Review Comment:
REST goes through `compute_subjects` → `compute_subject_list(...,
ensure_no_lockout=True)` so a non-admin can't remove themselves — should this
path do the same, or is self-lockout intended?
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -214,6 +281,14 @@ def _apply_field_updates(dashboard: Any, request:
UpdateDashboardRequest) -> lis
update_tags(ObjectType.dashboard, dashboard.id, dashboard.tags,
request.tags)
changed.append("tags")
+ if request.owners is not None:
+ # Full replacement of owners (empty list clears them). The owners were
+ # resolved and validated exactly once by _resolve_and_validate_owners,
+ # so there is no second lookup here that could silently drop a user
+ # removed between validation and this write.
+ dashboard.owners = resolved_owners or []
Review Comment:
`Dashboard` has no `owners` relationship (only `editors`/`viewers` →
`Subject`), so this sets a stray instance attribute and persists nothing while
`changed_fields` still reports `"owners"` — the tests pass only because `dash`
is a `Mock`.
--
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]
