This is an automated email from the ASF dual-hosted git repository.
jli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 88ce1425e2c fix(roles): optimize user fetching and resolve N+1 query
issue (#37235)
88ce1425e2c is described below
commit 88ce1425e2c73b6235c294ab8fe4d44598224aea
Author: Jonathan Alberth Quispe Fuentes <[email protected]>
AuthorDate: Thu Feb 12 12:32:19 2026 -0500
fix(roles): optimize user fetching and resolve N+1 query issue (#37235)
---
.../src/features/roles/RoleListEditModal.test.tsx | 37 +++++++
.../src/features/roles/RoleListEditModal.tsx | 12 +--
superset/security/api.py | 10 +-
tests/integration_tests/security/api_tests.py | 111 ++++++++++++++++++++-
4 files changed, 157 insertions(+), 13 deletions(-)
diff --git a/superset-frontend/src/features/roles/RoleListEditModal.test.tsx
b/superset-frontend/src/features/roles/RoleListEditModal.test.tsx
index 2c4d88dc9b3..4a91495b0db 100644
--- a/superset-frontend/src/features/roles/RoleListEditModal.test.tsx
+++ b/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,
+ },
+ ],
+ });
+ });
});
diff --git a/superset-frontend/src/features/roles/RoleListEditModal.tsx
b/superset-frontend/src/features/roles/RoleListEditModal.tsx
index 0cef1ebea92..7159c83f98e 100644
--- a/superset-frontend/src/features/roles/RoleListEditModal.tsx
+++ b/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) {
diff --git a/superset/security/api.py b/superset/security/api.py
index 3f291968af9..c1941dea74e 100644
--- a/superset/security/api.py
+++ b/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:
diff --git a/tests/integration_tests/security/api_tests.py
b/tests/integration_tests/security/api_tests.py
index 5667c64ca11..24e9acc2b69 100644
--- a/tests/integration_tests/security/api_tests.py
+++ b/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 (
)
[email protected]
+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()
+
+
[email protected]
+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"]
