FIX slowness when adding user to workspace (#1205)

FIX slowness when adding user to workspace

Summary: This PR addresses an issue where long and never-ending queries were causing failures in adding
users to workspaces, especially in the French administration context. The solution involves splitting complex
queries into simpler ones, removing promisifyAll around `getUsers` queries, and adding an index for faster
queries.

Test Plan: Existing tests

Reviewers: fflorent, paulfitz, berhalak

PR: #1205

app/gen-server/entity/Login.ts

@@ -1,4 +1,4 @@
-import {BaseEntity, Column, Entity, JoinColumn, ManyToOne, PrimaryColumn} from "typeorm";
+import {BaseEntity, Column, Entity, Index, JoinColumn, ManyToOne, PrimaryColumn} from "typeorm";
 
 import {User} from "./User";
 
@@ -9,6 +9,7 @@ export class Login extends BaseEntity {
   public id: number;
 
   // This is the normalized email address we use for equality and indexing.
+  @Index()
   @Column({type: String})
   public email: string;
 

app/gen-server/lib/homedb/HomeDBManager.ts

@@ -2016,30 +2016,28 @@ export class HomeDBManager extends EventEmitter {
     const result = await this._connection.transaction(async manager => {
       const analysis = await this._usersManager.verifyAndLookupDeltaEmails(userId, delta, false, manager);
       let {userIdDelta} = analysis;
-      let wsQuery = this._workspace(scope, wsId, {
+      const options = {
         manager,
         markPermissions: analysis.permissionThreshold,
-      })
-      // Join the workspace's ACL rules and groups/users so we can edit them.
-      .leftJoinAndSelect('workspaces.aclRules', 'acl_rules')
-      .leftJoinAndSelect('acl_rules.group', 'workspace_groups')
-      .leftJoinAndSelect('workspace_groups.memberUsers', 'workspace_users')
-      // Join the workspace's org and org member groups so we know what should be inherited.
-      .leftJoinAndSelect('workspaces.org', 'org')
-      .leftJoinAndSelect('org.aclRules', 'org_acl_rules')
-      .leftJoinAndSelect('org_acl_rules.group', 'org_groups')
-      .leftJoinAndSelect('org_groups.memberUsers', 'org_users');
-      wsQuery = this._addFeatures(wsQuery, 'org');
+      };
+      let wsQuery = this._buildWorkspaceWithACLRules(scope, wsId, options);
       wsQuery = this._withAccess(wsQuery, userId, 'workspaces');
-      const queryResult = await verifyEntity(wsQuery);
-      if (queryResult.status !== 200) {
+      const wsQueryResult = await verifyEntity(wsQuery);
+
+      if (wsQueryResult.status !== 200) {
         // If the query for the workspace failed, return the failure result.
-        return queryResult;
+        return wsQueryResult;
       }
-      this._failIfPowerfulAndChangingSelf(analysis, queryResult);
-      const ws: Workspace = queryResult.data;
+      this._failIfPowerfulAndChangingSelf(analysis, wsQueryResult);
+      const ws: Workspace = wsQueryResult.data;
+
+      const orgId = ws.org.id;
+      let orgQuery = this._buildOrgWithACLRulesQuery(scope, orgId, options);
+      orgQuery = this._addFeatures(orgQuery);
+      const orgQueryResult = await orgQuery.getRawAndEntities();
+      const org: Organization = orgQueryResult.entities[0];
       // Get all the non-guest groups on the org.
-      const orgGroups = getNonGuestGroups(ws.org);
+      const orgGroups = getNonGuestGroups(org);
       // Get all the non-guest groups to be updated by the delta.
       const groups = getNonGuestGroups(ws);
       if ('maxInheritedRole' in delta) {
@@ -2062,7 +2060,7 @@ export class HomeDBManager extends EventEmitter {
         await this._updateUserPermissions(groups, userIdDelta, manager);
         this._checkUserChangeAllowed(userId, groups);
         const nonOrgMembersAfter = this._usersManager.getUserDifference(groups, orgGroups);
-        const features = ws.org.billingAccount.getFeatures();
+        const features = org.billingAccount.getFeatures();
         const limit = features.maxSharesPerWorkspace;
         if (limit !== undefined) {
           this._restrictShares(null, limit, removeRole(nonOrgMembersBefore),
@@ -2072,7 +2070,7 @@ export class HomeDBManager extends EventEmitter {
       await manager.save(groups);
       // If the users in workspace were changed, make a call to repair the guests in the org.
       if (userIdDelta) {
-        await this._repairOrgGuests(scope, ws.org.id, manager);
+        await this._repairOrgGuests(scope, orgId, manager);
         notifications.push(this._inviteNotification(userId, ws, userIdDelta, membersBefore));
       }
       return {
@@ -4652,9 +4650,8 @@ export class HomeDBManager extends EventEmitter {
     return { personal: true, public: !realAccess };
   }
 
-  private _getWorkspaceWithACLRules(scope: Scope, wsId: number, options: Partial<QueryOptions> = {}) {
-    const query = this._workspace(scope, wsId, {
-      markPermissions: Permissions.VIEW,
+  private _buildWorkspaceWithACLRules(scope: Scope, wsId: number, options: Partial<QueryOptions> = {}) {
+    return this._workspace(scope, wsId, {
       ...options
     })
     // Join the workspace's ACL rules (with 1st level groups/users listed).
@@ -4664,6 +4661,13 @@ export class HomeDBManager extends EventEmitter {
     .leftJoinAndSelect('workspace_groups.memberGroups', 'workspace_group_groups')
     .leftJoinAndSelect('workspace_group_users.logins', 'workspace_user_logins')
     .leftJoinAndSelect('workspaces.org', 'org');
+  }
+
+  private _getWorkspaceWithACLRules(scope: Scope, wsId: number, options: Partial<QueryOptions> = {}) {
+    const query = this._buildWorkspaceWithACLRules(scope, wsId, {
+      markPermissions: Permissions.VIEW,
+      ...options
+    });
     return verifyEntity(query);
   }
 

app/gen-server/lib/homedb/UsersManager.ts

@@ -337,15 +337,21 @@ export class UsersManager {
     email: string,
     manager?: EntityManager
   ): Promise<User|undefined> {
-    const normalizedEmail = normalizeEmail(email);
-    return await (manager || this._connection).createQueryBuilder()
-      .select('user')
-      .from(User, 'user')
-      .leftJoinAndSelect('user.logins', 'logins')
-      .where('email = :email', {email: normalizedEmail})
+    return await this._buildExistingUsersByLoginRequest([email], manager)
       .getOne() || undefined;
   }
 
+  /**
+   * Find some users by their emails. Don't create the users if they don't already exist.
+   */
+  public async getExistingUsersByLogin(
+    emails: string[],
+    manager?: EntityManager
+  ): Promise<User[]> {
+    return await this._buildExistingUsersByLoginRequest(emails, manager)
+      .getMany();
+  }
+
   /**
    *
    * Fetches a user record based on an email address. If a user record already
@@ -471,7 +477,18 @@ export class UsersManager {
     });
   }
 
-  /**
+  /*
+   * This function is an alias of getUserByLogin
+   * Its purpose is to be more expressive and avoid confusion when reading code.
+   * FIXME :In the future it may be used to split getUserByLogin in two distinct functions
+   * One for creation
+   * the other for retrieving users in order to make it more maintainable
+   */
+  public async createUser(email: string, options: GetUserOptions = {}): Promise<User> {
+    return await this.getUserByLogin(email, options);
+  }
+
+  /*
    * Deletes a user from the database. For the moment, the only person with the right
    * to delete a user is the user themselves.
    * Users have logins, a personal org, and entries in the group_users table. All are
@@ -612,11 +629,16 @@ export class UsersManager {
       // Lookup emails
       const emailMap = delta.users;
       const emails = Object.keys(emailMap);
-      const emailUsers = await Promise.all(
-        emails.map(async email => await this.getUserByLogin(email, {manager: transaction}))
-      );
-      emails.forEach((email, i) => {
-        const userIdAffected = emailUsers[i]!.id;
+      const existingUsers = await this.getExistingUsersByLogin(emails, transaction);
+      const emailsExistingUsers = existingUsers.map(user => user.loginEmail);
+      const emailsUsersToCreate = emails.filter(email => !emailsExistingUsers.includes(email));
+      const emailUsers = new Map(existingUsers.map(user => [user.loginEmail, user]));
+      for (const email of emailsUsersToCreate) {
+        const user = await this.createUser(email, {manager: transaction});
+        emailUsers.set(user.loginEmail, user);
+      }
+      emails.forEach((email) => {
+        const userIdAffected = emailUsers.get(normalizeEmail(email))!.id;
         // Org-level sharing with everyone would allow serious spamming - forbid it.
         if (emailMap[email] !== null &&                    // allow removing anything
             userId !== this.getSupportUserId() &&          // allow support user latitude
@@ -766,4 +788,16 @@ export class UsersManager {
     }
     delta.users = users;
   }
+
+  private _buildExistingUsersByLoginRequest(
+    emails: string[],
+    manager?: EntityManager
+  ) {
+    const normalizedEmails = emails.map(email=> normalizeEmail(email));
+    return (manager || this._connection).createQueryBuilder()
+      .select('user')
+      .from(User, 'user')
+      .leftJoinAndSelect('user.logins', 'logins')
+      .where('email IN (:...emails)', {emails: normalizedEmails});
+  }
 }

app/gen-server/migration/1729754662550-LoginsEmailIndex.ts

@@ -0,0 +1,16 @@
+import {MigrationInterface, QueryRunner, TableIndex} from "typeorm";
+
+export class LoginsEmailsIndex1729754662550 implements MigrationInterface {
+
+  public async up(queryRunner: QueryRunner): Promise<any> {
+    await queryRunner.createIndex("logins", new TableIndex({
+      name: "logins__email",
+      columnNames: ["email"]
+    }));
+  }
+
+  public async down(queryRunner: QueryRunner): Promise<any> {
+    await queryRunner.dropIndex("logins", "logins__email");
+  }
+}
+

test/gen-server/migrations.ts

@@ -47,6 +47,8 @@ import {UserLastConnection1713186031023
 import {ActivationEnabled1722529827161
         as ActivationEnabled} from 'app/gen-server/migration/1722529827161-Activation-Enabled';
 import {Configs1727747249153 as Configs} from 'app/gen-server/migration/1727747249153-Configs';
+import {LoginsEmailsIndex1729754662550
+        as LoginsEmailsIndex} from 'app/gen-server/migration/1729754662550-LoginsEmailIndex';
 
 const home: HomeDBManager = new HomeDBManager();
 
@@ -56,7 +58,7 @@ const migrations = [Initial, Login, PinDocs, UserPicture, DisplayEmail, DisplayE
                     ExternalBilling, DocOptions, Secret, UserOptions, GracePeriodStart,
                     DocumentUsage, Activations, UserConnectId, UserUUID, UserUniqueRefUUID,
                     Forks, ForkIndexes, ActivationPrefs, AssistantLimit, Shares, BillingFeatures,
-                    UserLastConnection, ActivationEnabled, Configs];
+                    UserLastConnection, ActivationEnabled, Configs, LoginsEmailsIndex];
 
 // Assert that the "members" acl rule and group exist (or not).
 function assertMembersGroup(org: Organization, exists: boolean) {