From 2feef7f780d7ebdad97ee5056726cb0ef8a0ecbe Mon Sep 17 00:00:00 2001
From: Paul Fitzpatrick <paulfitz@alum.mit.edu>
Date: Fri, 4 Jun 2021 12:26:59 -0400
Subject: [PATCH] (core) avoid typeorm's .save() method for relation with
 multi-column primary key

Summary:
A recently added stress test ("deletes documents reasonably quickly" in removedAt.ts) is sporadically failing under postgres.  It looks like typeorm's .save() method is in some way unreliable when setting a table with multi-column primary keys, via a ManyToMany relation. This diff replaces the .save() with explicit inserts/deletes.

I modified _repairWorkspaceGuests recently, so thought that change might have been the problem. However under the stress test, failures occur as often in _repairOrgGuests (not changed recently) as in _repairWorkspaceGuests (changed recently).

For reference, see schema diagram at https://grist.quip.com/wWpRAMe058Nl/Home-DB (the table being updated is `group_users`).

Possibly related issue: https://github.com/typeorm/typeorm/issues/4122

Test Plan:
After this change, stress test runs well on postgres locally (no failure 70 iterations); before it would fail on postgres within 3 iterations typically.

Separately: I gave a test that failed a little more time to return, and confirmed it was no slower on average, so I think it was unrelated.

Reviewers: jarek

Reviewed By: jarek

Differential Revision: https://phab.getgrist.com/D2848
---
 app/gen-server/lib/HomeDBManager.ts | 40 ++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts
index 5ebeff63..3d02bd2b 100644
--- a/app/gen-server/lib/HomeDBManager.ts
+++ b/app/gen-server/lib/HomeDBManager.ts
@@ -2423,8 +2423,9 @@ export class HomeDBManager extends EventEmitter {
       // Get guest group for workspace.
       const wsQuery = this._workspace(scope, wsId, {manager})
       .leftJoinAndSelect('workspaces.aclRules', 'acl_rules')
-      .leftJoinAndSelect('acl_rules.group', 'groups');
-       const workspace: Workspace = (await wsQuery.getOne())!;
+      .leftJoinAndSelect('acl_rules.group', 'groups')
+      .leftJoinAndSelect('groups.memberUsers', 'users');
+      const workspace: Workspace = (await wsQuery.getOne())!;
       const wsGuestGroup = workspace.aclRules.map(aclRule => aclRule.group)
         .find(_grp => _grp.name === roles.GUEST);
       if (!wsGuestGroup) {
@@ -2440,8 +2441,8 @@ export class HomeDBManager extends EventEmitter {
         .leftJoinAndSelect('doc_groups.memberUsers', 'doc_users')
         .andWhere('doc_users.id is not null');
       const wsWithDocs = await wsWithDocsQuery.getOne();
-      wsGuestGroup.memberUsers = this._filterEveryone(getResourceUsers(wsWithDocs?.docs || []));
-      await manager.save(wsGuestGroup);
+      await this._setGroupUsers(manager, wsGuestGroup.id, wsGuestGroup.memberUsers,
+                                this._filterEveryone(getResourceUsers(wsWithDocs?.docs || [])));
     });
   }
 
@@ -2471,11 +2472,38 @@ export class HomeDBManager extends EventEmitter {
         throw new Error(`_repairOrgGuests error: found ${orgGroups.length} ${roles.GUEST} ACL group(s)`);
       }
       const orgGuestGroup = orgGroups[0]!;
-      orgGuestGroup.memberUsers = this._filterEveryone(getResourceUsers(org.workspaces));
-      await manager.save(orgGuestGroup);
+      await this._setGroupUsers(manager, orgGuestGroup.id, orgGuestGroup.memberUsers,
+                                this._filterEveryone(getResourceUsers(org.workspaces)));
     });
   }
 
+  /**
+   * Update the set of users in a group.  TypeORM's .save() method appears to be
+   * unreliable for a ManyToMany relation with a table with a multi-column primary
+   * key, so we make the update using explicit deletes and inserts.
+   */
+  private async _setGroupUsers(manager: EntityManager, groupId: number, usersBefore: User[],
+                               usersAfter: User[]) {
+    const userIdsBefore = new Set(usersBefore.map(u => u.id));
+    const userIdsAfter = new Set(usersAfter.map(u => u.id));
+    const toDelete = [...userIdsBefore].filter(id => !userIdsAfter.has(id));
+    const toAdd = [...userIdsAfter].filter(id => !userIdsBefore.has(id));
+    if (toDelete.length > 0) {
+      await manager.createQueryBuilder()
+        .delete()
+        .from('group_users')
+        .whereInIds(toDelete.map(id => ({user_id: id, group_id: groupId})))
+        .execute();
+    }
+    if (toAdd.length > 0) {
+      await manager.createQueryBuilder()
+        .insert()
+        .into('group_users')
+        .values(toAdd.map(id => ({user_id: id, group_id: groupId})))
+        .execute();
+    }
+  }
+
   /**
    * Don't add everyone@ as a guest, unless also sharing with anon@.
    * This means that material shared with everyone@ doesn't become