sadpandajoe commented on code in PR #38387:
URL: https://github.com/apache/superset/pull/38387#discussion_r2887806019


##########
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();

Review Comment:
   Good catch — removed the `permissionSearchCache.clear()` on empty search in 
3a661fc. The cache now persists across empty/non-empty transitions and relies 
solely on `MAX_CACHE_ENTRIES` for eviction.



##########
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) {

Review Comment:
   Done in 3a661fc — cache key is now normalized with `.trim().toLowerCase()` 
so "Dataset" and "dataset" share the same cache entry. Added a test that locks 
this behavior.



-- 
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]

Reply via email to