Merge pull request #24630 from overleaf/rh-null-editor-fix

Coerce null reviewer_refs to empty array

GitOrigin-RevId: dd1931b306e22fc4b7dbd3709dfac786a2475724

services/web/app/src/Features/Collaborators/CollaboratorsHandler.js

@@ -28,22 +28,34 @@ module.exports = {
     convertTrackChangesToExplicitFormat,
   },
 }
+// Forces null pendingReviewer_refs, readOnly_refs, and reviewer_refs to
+// be empty arrays to avoid errors during $pull ops
+// See https://github.com/overleaf/internal/issues/24610
+async function fixNullCollaboratorRefs(projectId) {
+  // Temporary cleanup for the case where pendingReviewer_refs is null
+  await Project.updateOne(
+    { _id: projectId, pendingReviewer_refs: { $type: 'null' } },
+    { $set: { pendingReviewer_refs: [] } }
+  ).exec()
+
+  // Temporary cleanup for the case where readOnly_refs is null
+  await Project.updateOne(
+    { _id: projectId, readOnly_refs: { $type: 'null' } },
+    { $set: { readOnly_refs: [] } }
+  ).exec()
+
+  // Temporary cleanup for the case where reviewer_refs is null
+  await Project.updateOne(
+    { _id: projectId, reviewer_refs: { $type: 'null' } },
+    { $set: { reviewer_refs: [] } }
+  ).exec()
+}
 
 async function removeUserFromProject(projectId, userId) {
   try {
     const project = await Project.findOne({ _id: projectId }).exec()
 
-    // Temporary workaround for the case where pendingReviewer_refs is null
-    await Project.updateOne(
-      { _id: projectId, pendingReviewer_refs: { $type: 'null' } },
-      { $set: { pendingReviewer_refs: [] } }
-    ).exec()
-
-    // Temporary workaround for the case where readOnly_refs is null
-    await Project.updateOne(
-      { _id: projectId, readOnly_refs: { $type: 'null' } },
-      { $set: { readOnly_refs: [] } }
-    ).exec()
+    await fixNullCollaboratorRefs(projectId)
 
     // Deal with the old type of boolean value for archived
     // In order to clear it
@@ -307,6 +319,9 @@ async function setCollaboratorPrivilegeLevel(
     ],
   }
   let update
+
+  await fixNullCollaboratorRefs(projectId)
+
   switch (privilegeLevel) {
     case PrivilegeLevels.READ_AND_WRITE: {
       update = {

services/web/test/unit/src/Collaborators/CollaboratorsHandlerTests.js

@@ -82,6 +82,46 @@ describe('CollaboratorsHandler', function () {
         './CollaboratorsGetter': this.CollaboratorsGetter,
       },
     })
+
+    // Helper function to set up mock expectations for null reference cleanup
+    this.expectNullReferenceCleanup = projectId => {
+      this.ProjectMock.expects('updateOne')
+        .withArgs(
+          {
+            _id: projectId,
+            pendingReviewer_refs: { $type: 'null' },
+          },
+          {
+            $set: { pendingReviewer_refs: [] },
+          }
+        )
+        .chain('exec')
+        .resolves()
+      this.ProjectMock.expects('updateOne')
+        .withArgs(
+          {
+            _id: projectId,
+            readOnly_refs: { $type: 'null' },
+          },
+          {
+            $set: { readOnly_refs: [] },
+          }
+        )
+        .chain('exec')
+        .resolves()
+      this.ProjectMock.expects('updateOne')
+        .withArgs(
+          {
+            _id: projectId,
+            reviewer_refs: { $type: 'null' },
+          },
+          {
+            $set: { reviewer_refs: [] },
+          }
+        )
+        .chain('exec')
+        .resolves()
+    }
   })
 
   afterEach(function () {
@@ -100,30 +140,7 @@ describe('CollaboratorsHandler', function () {
       })
 
       it('should remove the user from mongo', async function () {
-        this.ProjectMock.expects('updateOne')
-          .withArgs(
-            {
-              _id: this.project._id,
-              pendingReviewer_refs: { $type: 'null' },
-            },
-            {
-              $set: { pendingReviewer_refs: [] },
-            }
-          )
-          .chain('exec')
-          .resolves()
-        this.ProjectMock.expects('updateOne')
-          .withArgs(
-            {
-              _id: this.project._id,
-              readOnly_refs: { $type: 'null' },
-            },
-            {
-              $set: { readOnly_refs: [] },
-            }
-          )
-          .chain('exec')
-          .resolves()
+        this.expectNullReferenceCleanup(this.project._id)
         this.ProjectMock.expects('updateOne')
           .withArgs(
             {
@@ -166,30 +183,7 @@ describe('CollaboratorsHandler', function () {
       })
 
       it('should remove the user from mongo', async function () {
-        this.ProjectMock.expects('updateOne')
-          .withArgs(
-            {
-              _id: this.oldArchivedProject._id,
-              pendingReviewer_refs: { $type: 'null' },
-            },
-            {
-              $set: { pendingReviewer_refs: [] },
-            }
-          )
-          .chain('exec')
-          .resolves()
-        this.ProjectMock.expects('updateOne')
-          .withArgs(
-            {
-              _id: this.oldArchivedProject._id,
-              readOnly_refs: { $type: 'null' },
-            },
-            {
-              $set: { readOnly_refs: [] },
-            }
-          )
-          .chain('exec')
-          .resolves()
+        this.expectNullReferenceCleanup(this.oldArchivedProject._id)
         this.ProjectMock.expects('updateOne')
           .withArgs(
             {
@@ -230,30 +224,7 @@ describe('CollaboratorsHandler', function () {
       })
 
       it('should remove the user from mongo', async function () {
-        this.ProjectMock.expects('updateOne')
-          .withArgs(
-            {
-              _id: this.archivedProject._id,
-              pendingReviewer_refs: { $type: 'null' },
-            },
-            {
-              $set: { pendingReviewer_refs: [] },
-            }
-          )
-          .chain('exec')
-          .resolves()
-        this.ProjectMock.expects('updateOne')
-          .withArgs(
-            {
-              _id: this.archivedProject._id,
-              readOnly_refs: { $type: 'null' },
-            },
-            {
-              $set: { readOnly_refs: [] },
-            }
-          )
-          .chain('exec')
-          .resolves()
+        this.expectNullReferenceCleanup(this.archivedProject._id)
         this.ProjectMock.expects('updateOne')
           .withArgs(
             {
@@ -541,30 +512,7 @@ describe('CollaboratorsHandler', function () {
           .chain('exec')
           .resolves({ _id: projectId })
 
-        this.ProjectMock.expects('updateOne')
-          .withArgs(
-            {
-              _id: projectId,
-              pendingReviewer_refs: { $type: 'null' },
-            },
-            {
-              $set: { pendingReviewer_refs: [] },
-            }
-          )
-          .chain('exec')
-          .resolves()
-        this.ProjectMock.expects('updateOne')
-          .withArgs(
-            {
-              _id: projectId,
-              readOnly_refs: { $type: 'null' },
-            },
-            {
-              $set: { readOnly_refs: [] },
-            }
-          )
-          .chain('exec')
-          .resolves()
+        this.expectNullReferenceCleanup(projectId)
         this.ProjectMock.expects('updateOne')
           .withArgs(
             {
@@ -727,6 +675,8 @@ describe('CollaboratorsHandler', function () {
 
   describe('setCollaboratorPrivilegeLevel', function () {
     it('sets a collaborator to read-only', async function () {
+      this.expectNullReferenceCleanup(this.project._id)
+
       this.ProjectMock.expects('updateOne')
         .withArgs(
           {
@@ -757,6 +707,8 @@ describe('CollaboratorsHandler', function () {
     })
 
     it('sets a collaborator to read-write', async function () {
+      this.expectNullReferenceCleanup(this.project._id)
+
       this.ProjectMock.expects('updateOne')
         .withArgs(
           {
@@ -796,6 +748,8 @@ describe('CollaboratorsHandler', function () {
         })
       })
       it('should correctly update the project', async function () {
+        this.expectNullReferenceCleanup(this.project._id)
+
         this.ProjectMock.expects('updateOne')
           .withArgs(
             {
@@ -839,6 +793,8 @@ describe('CollaboratorsHandler', function () {
         })
       })
       it('should correctly update the project', async function () {
+        this.expectNullReferenceCleanup(this.project._id)
+
         this.ProjectMock.expects('updateOne')
           .withArgs(
             {
@@ -871,6 +827,8 @@ describe('CollaboratorsHandler', function () {
     })
 
     it('sets a collaborator to read-only as a pendingEditor', async function () {
+      this.expectNullReferenceCleanup(this.project._id)
+
       this.ProjectMock.expects('updateOne')
         .withArgs(
           {
@@ -904,6 +862,8 @@ describe('CollaboratorsHandler', function () {
     })
 
     it('sets a collaborator to read-only as a pendingReviewer', async function () {
+      this.expectNullReferenceCleanup(this.project._id)
+
       this.ProjectMock.expects('updateOne')
         .withArgs(
           {
@@ -937,6 +897,8 @@ describe('CollaboratorsHandler', function () {
     })
 
     it('throws a NotFoundError if the project or collaborator does not exist', async function () {
+      this.expectNullReferenceCleanup(this.project._id)
+
       this.ProjectMock.expects('updateOne')
         .chain('exec')
         .resolves({ matchedCount: 0 })