Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions superset/mcp_service/dashboard/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
model_validator,
PositiveInt,
)
from sqlalchemy.orm.exc import DetachedInstanceError

if TYPE_CHECKING:
from superset.models.dashboard import Dashboard
Expand Down Expand Up @@ -152,15 +153,58 @@ def serialize_role_object(role: Any) -> RoleInfo | None:
if not role:
return None

try:
raw_permissions = getattr(role, "permissions", None)
except DetachedInstanceError:
raw_permissions = None

permissions: list[str] | None = None
if raw_permissions is not None:
permissions = []
try:
permission_iterator = iter(raw_permissions)
except TypeError:
permission_iterator = iter(())

while True:
try:
permission = next(permission_iterator)
except StopIteration:
break
except (DetachedInstanceError, TypeError):
break

try:
permission_name = _serialize_permission_name(permission)
except (DetachedInstanceError, TypeError):
continue

if permission_name is not None:
permissions.append(permission_name)

return RoleInfo(
id=getattr(role, "id", None),
name=getattr(role, "name", None),
permissions=[perm.name for perm in getattr(role, "permissions", [])]
if hasattr(role, "permissions")
else None,
permissions=permissions,
)


def _serialize_permission_name(permission: Any) -> str | None:
"""Return direct permission names or FAB permission/view pairs.

Returns None when neither representation is available.
"""
if (name := getattr(permission, "name", None)) is not None:
return str(name)

permission_name = getattr(getattr(permission, "permission", None), "name", None)
view_menu_name = getattr(getattr(permission, "view_menu", None), "name", None)
if permission_name and view_menu_name:
return f"{permission_name} on {view_menu_name}"

return None
Comment thread
hoon-e marked this conversation as resolved.


class DashboardFilter(ColumnOperator):
"""
Filter object for dashboard listing.
Expand Down
106 changes: 106 additions & 0 deletions tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,19 @@

import logging
from importlib import import_module
from types import SimpleNamespace
from unittest.mock import Mock, patch

import pytest
from fastmcp import Client
from fastmcp.exceptions import ToolError
from flask import g
from sqlalchemy.orm.exc import DetachedInstanceError

from superset.mcp_service.app import mcp
from superset.mcp_service.dashboard.schemas import (
ListDashboardsRequest,
serialize_role_object,
)
from superset.mcp_service.dashboard.tool.get_dashboard_info import (
_refresh_request_user_for_permalink_access,
Expand Down Expand Up @@ -134,6 +137,109 @@ async def test_list_dashboards_basic(mock_list, mcp_server):
assert "slug" in data["columns_loaded"]


def test_dashboard_role_serializer_serializes_permission_view_names() -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add a short docstring to this new test function describing the serializer behavior it validates. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is a newly added Python test function and it has no docstring. The custom rule requires new functions to be documented inline, so the suggestion identifies a real violation.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 140:140
**Comment:**
	*Custom Rule: Add a short docstring to this new test function describing the serializer behavior it validates.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

permission_view = SimpleNamespace(
permission=SimpleNamespace(name="can_read"),
view_menu=SimpleNamespace(name="Dashboard"),
)
role = SimpleNamespace(id=1, name="Gamma", permissions=[permission_view])

role_info = serialize_role_object(role)

assert role_info is not None
assert role_info.model_dump() == {
"id": 1,
"name": "Gamma",
"permissions": ["can_read on Dashboard"],
}


def test_dashboard_role_serializer_skips_bad_permission_items() -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add a docstring to this new test function so its edge-case intent is explicitly documented. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is a newly added Python test function and there is no docstring immediately under it. That violates the rule requiring new functions to include docstrings.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 157:157
**Comment:**
	*Custom Rule: Add a docstring to this new test function so its edge-case intent is explicitly documented.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

class DetachedPermission:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add a brief class docstring explaining the purpose of this helper class used in the test. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This newly introduced helper class does not have a docstring. The custom rule explicitly requires new classes to be documented inline.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 158:158
**Comment:**
	*Custom Rule: Add a brief class docstring explaining the purpose of this helper class used in the test.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

@property
def name(self) -> str:
raise DetachedInstanceError("detached permission")

permission_view = SimpleNamespace(
permission=SimpleNamespace(name="can_read"),
view_menu=SimpleNamespace(name="Dashboard"),
)
role = SimpleNamespace(
id=1,
name="Gamma",
permissions=[
permission_view,
DetachedPermission(),
SimpleNamespace(name="can_write"),
],
)

role_info = serialize_role_object(role)

assert role_info is not None
assert role_info.permissions == ["can_read on Dashboard", "can_write"]


def test_dashboard_role_serializer_handles_detached_permissions() -> None:
class DetachedRole:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add a class docstring to describe why this detached-role test double exists. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is a newly added helper class inside a test function and it lacks a docstring. That matches the custom rule for new classes needing inline documentation.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 184:184
**Comment:**
	*Custom Rule: Add a class docstring to describe why this detached-role test double exists.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

id = 1
name = "Gamma"

@property
def permissions(self) -> list[object]:
raise DetachedInstanceError("detached permissions")

role_info = serialize_role_object(DetachedRole())

assert role_info is not None
assert role_info.permissions is None


def test_dashboard_role_serializer_handles_non_iterable_permissions() -> None:
role = SimpleNamespace(id=1, name="Gamma", permissions=object())

role_info = serialize_role_object(role)

assert role_info is not None
assert role_info.permissions == []


def test_dashboard_role_serializer_stops_on_detached_permission_iterator() -> None:
class DetachedPermissionIterator:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add a short docstring to this iterator helper class to clarify the failure scenario it simulates. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This new helper class also lacks a docstring. Since the rule requires newly added classes to be documented, the suggestion is valid.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 208:208
**Comment:**
	*Custom Rule: Add a short docstring to this iterator helper class to clarify the failure scenario it simulates.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

def __iter__(self) -> "DetachedPermissionIterator":
return self

def __next__(self) -> object:
raise DetachedInstanceError("detached permission iterator")

role = SimpleNamespace(
id=1,
name="Gamma",
permissions=DetachedPermissionIterator(),
)

role_info = serialize_role_object(role)

assert role_info is not None
assert role_info.permissions == []


def test_dashboard_role_serializer_skips_unnamed_permission_items() -> None:
role = SimpleNamespace(
id=1,
name="Gamma",
permissions=[
SimpleNamespace(permission=SimpleNamespace(name="can_read")),
SimpleNamespace(view_menu=SimpleNamespace(name="Dashboard")),
],
)

role_info = serialize_role_object(role)

assert role_info is not None
assert role_info.permissions == []


@patch("superset.daos.dashboard.DashboardDAO.list")
@pytest.mark.asyncio
async def test_list_dashboards_with_filters(mock_list, mcp_server):
Expand Down
Loading