sadpandajoe commented on code in PR #38387:
URL: https://github.com/apache/superset/pull/38387#discussion_r2887771519
##########
superset-frontend/src/features/roles/utils.ts:
##########
@@ -51,3 +54,166 @@ export const updateRoleName = async (roleId: number, name:
string) =>
endpoint: `/api/v1/security/roles/${roleId}`,
jsonPayload: { name },
});
+
+export const formatPermissionLabel = (
+ permissionName: string,
+ viewMenuName: string,
+) => `${permissionName.replace(/_/g, ' ')} ${viewMenuName.replace(/_/g, ' ')}`;
+
+type PermissionResult = {
+ id: number;
+ permission: { name: string };
+ view_menu: { name: string };
+};
+
+const mapPermissionResults = (results: PermissionResult[]) =>
+ results.map(item => ({
+ value: item.id,
+ label: formatPermissionLabel(item.permission.name, item.view_menu.name),
+ }));
+
+const PAGE_SIZE = 1000;
+const CONCURRENCY_LIMIT = 3;
+const MAX_CACHE_ENTRIES = 20;
+const permissionSearchCache = new Map<string, SelectOption[]>();
+
+export const clearPermissionSearchCache = () => {
+ permissionSearchCache.clear();
+};
+
+const fetchPermissionPageRaw = async (queryParams: Record<string, unknown>) =>
{
+ const response = await SupersetClient.get({
+ endpoint:
`/api/v1/security/permissions-resources/?q=${rison.encode(queryParams)}`,
+ });
+ return {
+ data: mapPermissionResults(response.json?.result || []),
+ totalCount: response.json?.count ?? 0,
+ };
+};
+
+const fetchAllPermissionPages = async (
+ filters: Record<string, unknown>[],
+): Promise<SelectOption[]> => {
+ const page0 = await fetchPermissionPageRaw({
+ page: 0,
+ page_size: PAGE_SIZE,
+ filters,
+ });
+ if (page0.data.length === 0 || page0.data.length >= page0.totalCount) {
+ return page0.data;
+ }
+
+ // Use actual returned size — backend may cap below PAGE_SIZE
+ const actualPageSize = page0.data.length;
+ const totalPages = Math.ceil(page0.totalCount / actualPageSize);
+ const allResults = [...page0.data];
+
+ // Fetch remaining pages in batches of CONCURRENCY_LIMIT
+ for (let batch = 1; batch < totalPages; batch += CONCURRENCY_LIMIT) {
+ const batchEnd = Math.min(batch + CONCURRENCY_LIMIT, totalPages);
+ const batchResults = await Promise.all(
+ Array.from({ length: batchEnd - batch }, (_, i) =>
+ fetchPermissionPageRaw({
+ page: batch + i,
+ page_size: PAGE_SIZE,
+ filters,
+ }),
+ ),
+ );
+ for (const r of batchResults) {
+ allResults.push(...r.data);
+ if (r.data.length === 0) return allResults;
+ }
+ if (allResults.length >= page0.totalCount) break;
+ }
+
+ return allResults;
+};
+
+export const fetchPermissionOptions = async (
+ filterValue: string,
+ page: number,
+ pageSize: number,
+ addDangerToast: (msg: string) => void,
+) => {
+ if (!filterValue) {
+ permissionSearchCache.clear();
+ try {
+ return await fetchPermissionPageRaw({ page, page_size: pageSize });
+ } catch {
+ addDangerToast(t('There was an error while fetching permissions'));
+ return { data: [], totalCount: 0 };
+ }
+ }
+
+ try {
+ const cacheKey = filterValue;
+ let cached = permissionSearchCache.get(cacheKey);
+ if (!cached) {
+ const [byViewMenu, byPermission] = await Promise.all([
+ fetchAllPermissionPages([
+ { col: 'view_menu.name', opr: 'ct', value: filterValue },
+ ]),
+ fetchAllPermissionPages([
+ { col: 'permission.name', opr: 'ct', value: filterValue },
+ ]),
+ ]);
+
+ const seen = new Set<number>();
+ cached = [...byViewMenu, ...byPermission].filter(item => {
+ if (seen.has(item.value)) return false;
+ seen.add(item.value);
+ return true;
+ });
+ if (permissionSearchCache.size >= MAX_CACHE_ENTRIES) {
+ const oldestKey = permissionSearchCache.keys().next().value;
+ if (oldestKey !== undefined) {
+ permissionSearchCache.delete(oldestKey);
+ }
+ }
+ permissionSearchCache.set(cacheKey, cached);
+ }
Review Comment:
Intentionally FIFO — the code and comments don't claim LRU behavior. True
LRU would add complexity (re-insert on get to update Map ordering) for minimal
benefit given the 20-entry cap. Happy to revisit if we see cache-miss patterns
in practice.
##########
superset-frontend/src/pages/RolesList/index.tsx:
##########
@@ -209,7 +170,7 @@ function RolesList({ addDangerToast, addSuccessToast, user
}: RolesListProps) {
id: 'group_ids',
Header: t('Groups'),
hidden: true,
- Cell: ({ row: { original } }: any) => original.groups_ids.join(', '),
+ Cell: ({ row: { original } }: any) => original.group_ids.join(', '),
},
Review Comment:
Agree this should be typed properly, but the `any` here is pre-existing (not
introduced by this PR) and the surrounding column definitions all use the same
pattern. Keeping it out of scope to avoid mixing concerns — happy to clean up
the full columns array in a follow-up.
--
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]
