fix(roles): optimize user fetching and resolve N+1 query issue (#37235)

superset-frontend/src/features/roles/RoleListEditModal.test.tsx

@@ -24,6 +24,7 @@ import {
   waitFor,
 } from 'spec/helpers/testing-library';
 import { SupersetClient } from '@superset-ui/core';
+import rison from 'rison';
 import RoleListEditModal from './RoleListEditModal';
 import {
   updateRoleName,
@@ -208,4 +209,40 @@ describe('RoleListEditModal', () => {
     expect(screen.getByTitle('User Name')).toBeInTheDocument();
     expect(screen.getByTitle('Email')).toBeInTheDocument();
   });
+
+  test('fetches users with correct role relationship filter', async () => {
+    const mockGet = SupersetClient.get as jest.Mock;
+    mockGet.mockResolvedValue({
+      json: {
+        count: 0,
+        result: [],
+      },
+    });
+
+    render(<RoleListEditModal {...mockProps} />);
+
+    await waitFor(() => {
+      expect(mockGet).toHaveBeenCalled();
+    });
+
+    // verify the endpoint and query parameters
+    const callArgs = mockGet.mock.calls[0][0];
+    expect(callArgs.endpoint).toContain('/api/v1/security/users/');
+
+    const urlMatch = callArgs.endpoint.match(/\?q=(.+)/);
+    expect(urlMatch).toBeTruthy();
+
+    const decodedQuery = rison.decode(urlMatch[1]);
+    expect(decodedQuery).toEqual({
+      page_size: 100,
+      page: 0,
+      filters: [
+        {
+          col: 'roles',
+          opr: 'rel_m_m',
+          value: mockRole.id,
+        },
+      ],
+    });
+  });
 });

superset-frontend/src/features/roles/RoleListEditModal.tsx

@@ -104,7 +104,7 @@ function RoleListEditModal({
   permissions,
   groups,
 }: RoleListEditModalProps) {
-  const { id, name, permission_ids, user_ids, group_ids } = role;
+  const { id, name, permission_ids, group_ids } = role;
   const [activeTabKey, setActiveTabKey] = useState(roleTabs.edit.key);
   const { addDangerToast, addSuccessToast } = useToasts();
   const [roleUsers, setRoleUsers] = useState<UserObject[]>([]);
@@ -112,13 +112,7 @@ function RoleListEditModal({
   const formRef = useRef<FormInstance | null>(null);
 
   useEffect(() => {
-    if (!user_ids.length) {
-      setRoleUsers([]);
-      setLoadingRoleUsers(false);
-      return;
-    }
-
-    const filters = [{ col: 'id', opr: 'in', value: user_ids }];
+    const filters = [{ col: 'roles', opr: 'rel_m_m', value: id }];
 
     fetchPaginatedData({
       endpoint: `/api/v1/security/users/`,
@@ -137,7 +131,7 @@ function RoleListEditModal({
         email: user.email,
       }),
     });
-  }, [user_ids]);
+  }, [id]);
 
   useEffect(() => {
     if (!loadingRoleUsers && formRef.current && roleUsers.length >= 0) {

superset/security/api.py

@@ -26,7 +26,7 @@ from flask_appbuilder.security.sqla.models import RegisterUser, Role
 from flask_wtf.csrf import generate_csrf
 from marshmallow import EXCLUDE, fields, post_load, Schema, ValidationError
 from sqlalchemy import asc, desc
-from sqlalchemy.orm import joinedload
+from sqlalchemy.orm import selectinload
 
 from superset.commands.dashboard.embedded.exceptions import (
     EmbeddedDashboardNotFoundError,
@@ -298,7 +298,9 @@ class RoleRestAPI(BaseSupersetApi):
             page_size = args.get("page_size", 10)
 
             query = db.session.query(Role).options(
-                joinedload(Role.permissions), joinedload(Role.user)
+                selectinload(Role.permissions),
+                selectinload(Role.user),
+                selectinload(Role.groups),
             )
 
             filters = args.get("filters", [])
@@ -318,6 +320,8 @@ class RoleRestAPI(BaseSupersetApi):
             if "name" in filter_dict:
                 query = query.filter(Role.name.ilike(f"%{filter_dict['name']}%"))
 
+            total_count = query.count()
+
             roles = (
                 query.order_by(order_by).offset(page * page_size).limit(page_size).all()
             )
@@ -334,7 +338,7 @@ class RoleRestAPI(BaseSupersetApi):
                     }
                     for role in roles
                 ],
-                count=query.count(),
+                count=total_count,
                 ids=[role.id for role in roles],
             )
         except ForbiddenError as e:

tests/integration_tests/security/api_tests.py

@@ -20,8 +20,9 @@
 import jwt
 import pytest
 
+from flask.ctx import AppContext
 from flask_wtf.csrf import generate_csrf
-from superset import db
+from superset import db, security_manager
 from superset.daos.dashboard import EmbeddedDashboardDAO
 from superset.models.dashboard import Dashboard
 from superset.utils.urls import get_url_host
@@ -35,6 +36,76 @@ from tests.integration_tests.fixtures.birth_names_dashboard import (
 )
 
 
+@pytest.fixture
+def create_test_roles_with_users(app_context: AppContext):
+    """
+    Fixture that creates two test roles with specific users, permissions, and groups.
+    """
+    user1, user2, user3 = [
+        security_manager.add_user(
+            username=f"test_user_{i}",
+            first_name="Test",
+            last_name=f"User{i}",
+            email=f"test_user_{i}@test.com",
+            role=[],
+            password="password",  # noqa: S106
+        )
+        for i in range(3)
+    ]
+
+    test_group = security_manager.add_group(
+        name="test_group_1",
+        label="Test Group 1",
+        description="Test group for role testing",
+        roles=[],
+    )
+
+    pvm1 = security_manager.add_permission_view_menu("can_read", "Dashboard")
+    pvm2 = security_manager.add_permission_view_menu("can_write", "Dashboard")
+    pvm3 = security_manager.add_permission_view_menu("can_read", "Chart")
+
+    test_role_1 = security_manager.add_role("test_role_1", [pvm1, pvm2])
+    test_role_1.user.append(user1)
+    test_role_1.user.append(user2)
+    test_role_1.groups.append(test_group)
+
+    test_role_2 = security_manager.add_role("test_role_2", [pvm3])
+    test_role_2.user.append(user3)
+
+    db.session.commit()
+
+    test_data = {
+        "test_role_1": {
+            "role": test_role_1,
+            "user_ids": sorted([user1.id, user2.id]),
+            "permission_ids": sorted([pvm1.id, pvm2.id]),
+            "group_ids": [test_group.id],
+        },
+        "test_role_2": {
+            "role": test_role_2,
+            "user_ids": [user3.id],
+            "permission_ids": [pvm3.id],
+            "group_ids": [],
+        },
+    }
+
+    yield test_data
+
+    # Cleanup
+    db.session.delete(test_role_1)
+    db.session.delete(test_role_2)
+    db.session.delete(user1)
+    db.session.delete(user2)
+    db.session.delete(user3)
+    db.session.delete(test_group)
+    db.session.commit()
+
+
+@pytest.fixture
+def inject_test_roles_data(request, create_test_roles_with_users):
+    request.instance.test_roles_data = create_test_roles_with_users
+
+
 class TestSecurityCsrfApi(SupersetTestCase):
     resource_name = "security"
 
@@ -293,3 +364,41 @@ class TestSecurityRolesApi(SupersetTestCase):
         self.login(GAMMA_USERNAME)
         response = self.client.get(self.show_uri)
         self.assert403(response)
+
+    @pytest.mark.usefixtures("inject_test_roles_data")
+    def test_get_roles_with_specific_test_data(self):
+        """
+        Security API: Test roles endpoint with specific test data
+        """
+        self.login(ADMIN_USERNAME)
+        response = self.client.get(f"{self.show_uri}?q=(page_size:100)")
+        self.assert200(response)
+
+        data = json.loads(response.data.decode("utf-8"))
+
+        # Create a mapping of role names to API response
+        api_roles_by_name = {role["name"]: role for role in data["result"]}
+
+        # Verify test_role_1
+        assert "test_role_1" in api_roles_by_name, (
+            f"test_role_1 not found in API response. "
+            f"Available roles: {list(api_roles_by_name.keys())}"
+        )
+        role1_api = api_roles_by_name["test_role_1"]
+        role1_expected = self.test_roles_data["test_role_1"]
+
+        assert sorted(role1_api["user_ids"]) == role1_expected["user_ids"]
+        assert sorted(role1_api["permission_ids"]) == role1_expected["permission_ids"]
+        assert sorted(role1_api["group_ids"]) == role1_expected["group_ids"]
+
+        # Verify test_role_2
+        assert "test_role_2" in api_roles_by_name, (
+            f"test_role_2 not found in API response. "
+            f"Available roles: {list(api_roles_by_name.keys())}"
+        )
+        role2_api = api_roles_by_name["test_role_2"]
+        role2_expected = self.test_roles_data["test_role_2"]
+
+        assert sorted(role2_api["user_ids"]) == role2_expected["user_ids"]
+        assert sorted(role2_api["permission_ids"]) == role2_expected["permission_ids"]
+        assert role2_api["group_ids"] == role2_expected["group_ids"]