Merge branch 'fix-page-service-and-page-grant-service' into fix-page-model-tests

apps/app/src/features/external-user-group/server/service/external-user-group-sync.ts

@@ -1,6 +1,7 @@
 import { IUserHasId } from '~/interfaces/user';
 import ExternalAccount from '~/server/models/external-account';
 import { excludeTestIdsFromTargetIds } from '~/server/util/compare-objectId';
+import { batchProcessPromiseAll } from '~/utils/promise';
 
 import { configManager } from '../../../../server/service/config-manager';
 import { externalAccountService } from '../../../../server/service/external-account';
@@ -10,13 +11,18 @@ import {
 import ExternalUserGroup from '../models/external-user-group';
 import ExternalUserGroupRelation from '../models/external-user-group-relation';
 
+// When d = max depth of group trees
+// Max space complexity of syncExternalUserGroups will be:
+// O(TREES_BATCH_SIZE * d * USERS_BATCH_SIZE)
+const TREES_BATCH_SIZE = 10;
+const USERS_BATCH_SIZE = 30;
+
 abstract class ExternalUserGroupSyncService {
 
   groupProviderType: ExternalGroupProviderType; // name of external service that contains user group info (e.g: ldap, keycloak)
 
   authProviderType: string; // auth provider type (e.g: ldap, oidc)
 
-  // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
   constructor(groupProviderType: ExternalGroupProviderType, authProviderType: string) {
     this.groupProviderType = groupProviderType;
     this.authProviderType = authProviderType;
@@ -35,14 +41,16 @@ abstract class ExternalUserGroupSyncService {
     const syncNode = async(node: ExternalUserGroupTreeNode, parentId?: string) => {
       const externalUserGroup = await this.createUpdateExternalUserGroup(node, parentId);
       existingExternalUserGroupIds.push(externalUserGroup._id);
-      await Promise.all(node.childGroupNodes.map((childNode) => {
-        return syncNode(childNode, externalUserGroup._id);
-      }));
+      // Do not use Promise.all, because the number of promises processed can
+      // exponentially grow when group tree is enormous
+      for await (const childNode of node.childGroupNodes) {
+        await syncNode(childNode, externalUserGroup._id);
+      }
     };
 
-    await Promise.all(trees.map((root) => {
+    await batchProcessPromiseAll(trees, TREES_BATCH_SIZE, (root) => {
       return syncNode(root);
-    }));
+    });
 
     const preserveDeletedLdapGroups: boolean = configManager?.getConfig('crowi', `external-user-group:${this.groupProviderType}:preserveDeletedGroups`);
     if (!preserveDeletedLdapGroups) {
@@ -63,23 +71,21 @@ abstract class ExternalUserGroupSyncService {
     const externalUserGroup = await ExternalUserGroup.findAndUpdateOrCreateGroup(
       node.name, node.id, this.groupProviderType, node.description, parentId,
     );
-    await Promise.all(node.userInfos.map((userInfo) => {
-      return (async() => {
-        const user = await this.getMemberUser(userInfo);
-
-        if (user != null) {
-          const userGroups = await ExternalUserGroup.findGroupsWithAncestorsRecursively(externalUserGroup);
-          const userGroupIds = userGroups.map(g => g._id);
-
-          // remove existing relations from list to create
-          const existingRelations = await ExternalUserGroupRelation.find({ relatedGroup: { $in: userGroupIds }, relatedUser: user._id });
-          const existingGroupIds = existingRelations.map(r => r.relatedGroup.toString());
-          const groupIdsToCreateRelation = excludeTestIdsFromTargetIds(userGroupIds, existingGroupIds);
-
-          await ExternalUserGroupRelation.createRelations(groupIdsToCreateRelation, user);
-        }
-      })();
-    }));
+    await batchProcessPromiseAll(node.userInfos, USERS_BATCH_SIZE, async(userInfo) => {
+      const user = await this.getMemberUser(userInfo);
+
+      if (user != null) {
+        const userGroups = await ExternalUserGroup.findGroupsWithAncestorsRecursively(externalUserGroup);
+        const userGroupIds = userGroups.map(g => g._id);
+
+        // remove existing relations from list to create
+        const existingRelations = await ExternalUserGroupRelation.find({ relatedGroup: { $in: userGroupIds }, relatedUser: user._id });
+        const existingGroupIds = existingRelations.map(r => r.relatedGroup.toString());
+        const groupIdsToCreateRelation = excludeTestIdsFromTargetIds(userGroupIds, existingGroupIds);
+
+        await ExternalUserGroupRelation.createRelations(groupIdsToCreateRelation, user);
+      }
+    });
 
     return externalUserGroup;
   }

apps/app/src/features/external-user-group/server/service/ldap-user-group-sync.ts

@@ -1,6 +1,7 @@
 import { configManager } from '~/server/service/config-manager';
 import LdapService, { SearchResultEntry } from '~/server/service/ldap';
 import PassportService from '~/server/service/passport';
+import { batchProcessPromiseAll } from '~/utils/promise';
 
 import {
   ExternalGroupProviderType, ExternalUserGroupTreeNode, ExternalUserInfo, LdapGroupMembershipAttributeType,
@@ -8,6 +9,12 @@ import {
 
 import ExternalUserGroupSyncService from './external-user-group-sync';
 
+// When d = max depth of group trees
+// Max space complexity of generateExternalUserGroupTrees will be:
+// O(TREES_BATCH_SIZE * d * USERS_BATCH_SIZE)
+const TREES_BATCH_SIZE = 10;
+const USERS_BATCH_SIZE = 30;
+
 class LdapUserGroupSyncService extends ExternalUserGroupSyncService {
 
   passportService: PassportService;
@@ -54,17 +61,23 @@ class LdapUserGroupSyncService extends ExternalUserGroupSyncService {
       converted.push(entry.objectName);
 
       const userIds = getUserIdsFromGroupEntry(entry);
-      const userInfos = (await Promise.all(userIds.map((id) => {
+
+      const userInfos = (await batchProcessPromiseAll(userIds, USERS_BATCH_SIZE, (id) => {
         return this.getUserInfo(id);
-      }))).filter((info): info is NonNullable<ExternalUserInfo> => info != null);
+      })).filter((info): info is NonNullable<ExternalUserInfo> => info != null);
       const name = this.ldapService.getStringValFromSearchResultEntry(entry, groupNameAttribute);
       const description = this.ldapService.getStringValFromSearchResultEntry(entry, groupDescriptionAttribute);
       const childGroupDNs = getChildGroupDnsFromGroupEntry(entry);
 
-      const childGroupNodes: ExternalUserGroupTreeNode[] = (await Promise.all(childGroupDNs.map((dn) => {
+      const childGroupNodesWithNull: (ExternalUserGroupTreeNode | null)[] = [];
+      // Do not use Promise.all, because the number of promises processed can
+      // exponentially grow when group tree is enormous
+      for await (const dn of childGroupDNs) {
         const childEntry = groupEntries.find(ge => ge.objectName === dn);
-        return childEntry != null ? convert(childEntry, converted) : null;
-      }))).filter((node): node is NonNullable<ExternalUserGroupTreeNode> => node != null);
+        childGroupNodesWithNull.push(childEntry != null ? await convert(childEntry, converted) : null);
+      }
+      const childGroupNodes: ExternalUserGroupTreeNode[] = childGroupNodesWithNull
+        .filter((node): node is NonNullable<ExternalUserGroupTreeNode> => node != null);
 
       return name != null ? {
         id: entry.objectName,
@@ -85,7 +98,8 @@ class LdapUserGroupSyncService extends ExternalUserGroupSyncService {
       return !allChildGroupDNs.has(entry.objectName);
     });
 
-    return (await Promise.all(rootEntries.map(entry => convert(entry, [])))).filter((node): node is NonNullable<ExternalUserGroupTreeNode> => node != null);
+    return (await batchProcessPromiseAll(rootEntries, TREES_BATCH_SIZE, entry => convert(entry, [])))
+      .filter((node): node is NonNullable<ExternalUserGroupTreeNode> => node != null);
   }
 
   private async getUserInfo(userId: string): Promise<ExternalUserInfo | null> {

apps/app/src/server/service/page-grant.ts

@@ -1,7 +1,7 @@
 import {
   GrantedGroup,
   pagePathUtils, pathUtils, pageUtils,
-  PageGrant, PageGrantCanBeOnTree, GroupType,
+  PageGrant, PageGrantCanBeOnTree,
 } from '@growi/core';
 import escapeStringRegexp from 'escape-string-regexp';
 import mongoose from 'mongoose';
@@ -297,7 +297,7 @@ class PageGrantService {
 
       const userGroupRelations = await UserGroupRelation.find({ relatedGroup: { $in: grantedUserGroups } }, { _id: 0, relatedUser: 1 });
       const externalUserGroupRelations = await ExternalUserGroupRelation.find({ relatedGroup: { $in: grantedExternalUserGroups } }, { _id: 0, relatedUser: 1 });
-      applicableUserIds = Array.from(new Set([...userGroupRelations, ...externalUserGroupRelations].map(r => r.relatedUser))) as ObjectIdLike[];
+      applicableUserIds = Array.from(new Set([...userGroupRelations, ...externalUserGroupRelations].map(r => r.relatedUser)));
 
       const applicableUserGroups = (await Promise.all(grantedUserGroups.map((groupId) => {
         return UserGroup.findGroupsWithDescendantsById(groupId);

apps/app/src/utils/promise.spec.ts

@@ -0,0 +1,28 @@
+import { batchProcessPromiseAll } from './promise';
+
+describe('batchProcessPromiseAll', () => {
+  it('processes items in batch', async() => {
+    const batch1 = [1, 2, 3, 4, 5];
+    const batch2 = [6, 7, 8, 9, 10];
+    const batch3 = [11, 12];
+
+    const all = [...batch1, ...batch2, ...batch3];
+
+    const actualProcessedBatches: number[][] = [];
+    const result = await batchProcessPromiseAll(all, 5, async(num, i, arr) => {
+      if (arr != null && i === 0) {
+        actualProcessedBatches.push(arr);
+      }
+      return num * 10;
+    });
+
+    const expected = [10, 20, 30, 40, 50, 60, 70, 80, 90, 100, 110, 120];
+
+    expect(result).toStrictEqual(expected);
+    expect(actualProcessedBatches).toStrictEqual([
+      batch1,
+      batch2,
+      batch3,
+    ]);
+  });
+});

apps/app/src/utils/promise.ts

@@ -0,0 +1,25 @@
+/**
+ * Divide arrays into batches, and apply a given function to each batch by using Promise.all
+ * Use when memory consumption can be too large by using simple Promise.all
+ * @param items array to process
+ * @param limit batch size
+ * @param fn function to apply on each item
+ * @returns result of fn applied to each item
+ */
+export const batchProcessPromiseAll = async<I, O>(
+  items: Array<I>,
+  limit: number,
+  fn: (item: I, index?: number, array?: Array<I>) => Promise<O>,
+): Promise<O[]> => {
+  let results: O[] = [];
+  for (let start = 0; start < items.length; start += limit) {
+    const end = Math.min(start + limit, items.length);
+
+    // eslint-disable-next-line no-await-in-loop
+    const slicedResults = await Promise.all(items.slice(start, end).map(fn));
+
+    results = results.concat(slicedResults);
+  }
+
+  return results;
+};