aminghadersohi commented on code in PR #42002:
URL: https://github.com/apache/superset/pull/42002#discussion_r3598046329
##########
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:
Related to the line-289 issue: even once this targets `editors` instead of
`owners`, `get_user_by_id` returns FAB `User` rows, but `editors` expects
`Subject` rows — a different ID space. `generate_dashboard.py` in this same
directory already has the right pattern (`get_user_subject(user.id)` before
assigning to `dashboard.editors`), and REST's `UpdateDashboardCommand` goes
through `compute_subjects` → `compute_subject_list` →
`populate_subject_list`/`get_or_create_user_subject` in `commands/utils.py`.
Reusing that helper here would fix this and the line-289 issue together and
keep owner-resolution semantics in sync with REST.
##########
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:
Building on msyavuz's question above: once this is wired to `editors`
correctly, worth deciding explicitly whether to add the same
`ensure_no_lockout=True` guard REST uses in `compute_subjects`, so a non-admin
can't remove themselves from the editor list via this tool. If the omission is
intentional it'd be good to say so, otherwise it's an easy way to get locked
out of a dashboard you still need to edit.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py:
##########
@@ -137,6 +137,119 @@ async def test_update_layout_theme_and_css(
changed = set(payload.get("changed_fields") or [])
assert {"position_json", "json_metadata", "css"} <= changed
+ @patch("superset.security_manager.get_user_by_id")
+ @patch("superset.daos.dashboard.DashboardDAO.get_by_id_or_slug")
+ @patch("superset.extensions.db.session")
+ @pytest.mark.asyncio
+ async def test_update_owners_replaces_list(
Review Comment:
Good coverage overall on the input-handling side. One gap:
`test_non_editor_gets_permission_denied` (further down in this file) only
exercises `dashboard_title`, not `owners`. The editorship gate runs before any
field-specific logic so it should cover `owners` too, but a direct test for
that — ideally against something more realistic than `Mock()` for the dashboard
— would likely have caught the persistence issue flagged above before this
merged.
##########
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:
Confirming and expanding on msyavuz's comment above: `Dashboard` doesn't
actually have an `owners` relationship — only `editors`/`viewers`
(Subject-based, see `superset/models/dashboard.py`). This line sets a plain,
unmapped instance attribute, so `db.session.commit()` won't persist anything
here against a real database. The unit tests pass only because `dash` is a
`Mock()` that happily accepts any attribute assignment. `changed_fields` will
still report `"owners"` and the tool will return success, so a caller has no
way to tell the write silently no-op'd. This needs to route through `editors`
the way the sibling `generate_dashboard.py` tool and the REST
`UpdateDashboardCommand` do — see the note on line 183 below.
--
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]
