Merge pull request #9514 from overleaf/em-send-deleted-ids-to-tpds

Send a list of deleted entities to TPDS

GitOrigin-RevId: 41813c31846338edc36c6ec6abc8c5a9e96731f4

services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js

@@ -1130,10 +1130,13 @@ const ProjectEntityUpdateHandler = {
           path.fileSystem,
           userId,
           source,
-          error => {
+          (error, subtreeListing) => {
             if (error != null) {
               return callback(error)
             }
+            const subtreeEntityIds = subtreeListing.map(entry =>
+              entry.entity._id.toString()
+            )
             TpdsUpdateSender.deleteEntity(
               {
                 projectId,
@@ -1141,6 +1144,7 @@ const ProjectEntityUpdateHandler = {
                 projectName: projectBeforeDeletion.name,
                 entityId,
                 entityType,
+                subtreeEntityIds,
               },
               error => {
                 if (error != null) {
@@ -1557,87 +1561,69 @@ const ProjectEntityUpdateHandler = {
     source,
     callback
   ) {
+    const subtreeListing = _listSubtree(entity, entityType, path)
     ProjectEntityUpdateHandler._updateProjectStructureWithDeletedEntity(
       project,
       newProject,
-      entity,
-      entityType,
-      path,
+      subtreeListing,
       userId,
       source,
       error => {
         if (error != null) {
           return callback(error)
         }
-        if (entityType.indexOf('file') !== -1) {
-          ProjectEntityUpdateHandler._cleanUpFile(
-            project,
-            entity,
-            path,
-            userId,
-            callback
-          )
-        } else if (entityType.indexOf('doc') !== -1) {
-          ProjectEntityUpdateHandler._cleanUpDoc(
-            project,
-            entity,
-            path,
-            userId,
-            callback
-          )
-        } else if (entityType.indexOf('folder') !== -1) {
-          ProjectEntityUpdateHandler._cleanUpFolder(
-            project,
-            entity,
-            path,
-            userId,
-            callback
-          )
-        } else {
-          callback()
+        const jobs = []
+
+        for (const entry of subtreeListing) {
+          if (entry.type === 'doc') {
+            jobs.push(cb => {
+              ProjectEntityUpdateHandler._cleanUpDoc(
+                project,
+                entry.entity,
+                entry.path,
+                userId,
+                cb
+              )
+            })
+          } else if (entry.type === 'file') {
+            jobs.push(cb => {
+              ProjectEntityUpdateHandler._cleanUpFile(
+                project,
+                entry.entity,
+                entry.path,
+                userId,
+                cb
+              )
+            })
+          }
         }
+        async.series(jobs, err => {
+          if (err) {
+            return callback(err)
+          }
+          callback(null, subtreeListing)
+        })
       }
     )
   },
 
-  // Note: the _cleanUpEntity code and _updateProjectStructureWithDeletedEntity
-  // methods both need to recursively iterate over the entities in folder.
-  // These are currently using separate implementations of the recursion. In
-  // future, these could be simplified using a common project entity iterator.
   _updateProjectStructureWithDeletedEntity(
     project,
     newProject,
-    entity,
-    entityType,
-    entityPath,
+    subtreeListing,
     userId,
     source,
     callback
   ) {
-    // compute the changes to the project structure
-    let changes
-    if (entityType.indexOf('file') !== -1) {
-      changes = { oldFiles: [{ file: entity, path: entityPath }] }
-    } else if (entityType.indexOf('doc') !== -1) {
-      changes = { oldDocs: [{ doc: entity, path: entityPath }] }
-    } else if (entityType.indexOf('folder') !== -1) {
-      changes = { oldDocs: [], oldFiles: [] }
-      const _recurseFolder = (folder, folderPath) => {
-        for (const doc of iterablePaths(folder, 'docs')) {
-          changes.oldDocs.push({ doc, path: Path.join(folderPath, doc.name) })
-        }
-        for (const file of iterablePaths(folder, 'fileRefs')) {
-          changes.oldFiles.push({
-            file,
-            path: Path.join(folderPath, file.name),
-          })
-        }
-        for (const childFolder of iterablePaths(folder, 'folders')) {
-          _recurseFolder(childFolder, Path.join(folderPath, childFolder.name))
-        }
+    const changes = { oldDocs: [], oldFiles: [] }
+    for (const entry of subtreeListing) {
+      if (entry.type === 'doc') {
+        changes.oldDocs.push({ doc: entry.entity, path: entry.path })
+      } else if (entry.type === 'file') {
+        changes.oldFiles.push({ file: entry.entity, path: entry.path })
       }
-      _recurseFolder(entity, entityPath)
     }
+
     // now send the project structure changes to the docupdater
     changes.newProject = newProject
     const projectId = project._id.toString()
@@ -1692,50 +1678,6 @@ const ProjectEntityUpdateHandler = {
     )
   },
 
-  _cleanUpFolder(project, folder, folderPath, userId, callback) {
-    const jobs = []
-    folder.docs.forEach(doc => {
-      const docPath = Path.join(folderPath, doc.name)
-      jobs.push(callback =>
-        ProjectEntityUpdateHandler._cleanUpDoc(
-          project,
-          doc,
-          docPath,
-          userId,
-          callback
-        )
-      )
-    })
-
-    folder.fileRefs.forEach(file => {
-      const filePath = Path.join(folderPath, file.name)
-      jobs.push(callback =>
-        ProjectEntityUpdateHandler._cleanUpFile(
-          project,
-          file,
-          filePath,
-          userId,
-          callback
-        )
-      )
-    })
-
-    folder.folders.forEach(childFolder => {
-      folderPath = Path.join(folderPath, childFolder.name)
-      jobs.push(callback =>
-        ProjectEntityUpdateHandler._cleanUpFolder(
-          project,
-          childFolder,
-          folderPath,
-          userId,
-          callback
-        )
-      )
-    })
-
-    async.series(jobs, callback)
-  },
-
   convertDocToFile: wrapWithLock({
     beforeLock(next) {
       return function (projectId, docId, userId, source, callback) {
@@ -1881,6 +1823,45 @@ const ProjectEntityUpdateHandler = {
   }),
 }
 
+/**
+ * List all descendants of an entity along with their type and path. Include
+ * the top-level entity as well.
+ */
+function _listSubtree(entity, entityType, entityPath) {
+  if (entityType.indexOf('file') !== -1) {
+    return [{ type: 'file', entity, path: entityPath }]
+  } else if (entityType.indexOf('doc') !== -1) {
+    return [{ type: 'doc', entity, path: entityPath }]
+  } else if (entityType.indexOf('folder') !== -1) {
+    const listing = []
+    const _recurseFolder = (folder, folderPath) => {
+      listing.push({ type: 'folder', entity: folder, path: folderPath })
+      for (const doc of iterablePaths(folder, 'docs')) {
+        listing.push({
+          type: 'doc',
+          entity: doc,
+          path: Path.join(folderPath, doc.name),
+        })
+      }
+      for (const file of iterablePaths(folder, 'fileRefs')) {
+        listing.push({
+          type: 'file',
+          entity: file,
+          path: Path.join(folderPath, file.name),
+        })
+      }
+      for (const childFolder of iterablePaths(folder, 'folders')) {
+        _recurseFolder(childFolder, Path.join(folderPath, childFolder.name))
+      }
+    }
+    _recurseFolder(entity, entityPath)
+    return listing
+  } else {
+    // This shouldn't happen, but if it does, fail silently.
+    return []
+  }
+}
+
 module.exports = ProjectEntityUpdateHandler
 module.exports.promises = promisifyAll(ProjectEntityUpdateHandler, {
   without: ['isPathValidForRootDoc'],

services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js

@@ -108,7 +108,14 @@ function buildTpdsUrl(userId, projectName, filePath) {
 
 async function deleteEntity(params) {
   metrics.inc('tpds.delete-entity')
-  const { projectId, path, projectName, entityId, entityType } = params
+  const {
+    projectId,
+    path,
+    projectName,
+    entityId,
+    entityType,
+    subtreeEntityIds,
+  } = params
 
   const projectUserIds = await getProjectUsersIds(projectId)
 
@@ -123,6 +130,10 @@ async function deleteEntity(params) {
         sl_entity_type: entityType,
       },
       uri: buildTpdsUrl(userId, projectName, path),
+      // We're sending a body with the DELETE request. This is unconventional,
+      // but Express does handle it on the other side. Ideally, this operation
+      // would be moved to a POST endpoint.
+      json: { subtreeEntityIds },
       title: 'deleteEntity',
       sl_all_user_ids: JSON.stringify([userId]),
     }

services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js

@@ -1567,7 +1567,9 @@ describe('ProjectEntityUpdateHandler', function () {
         this.projectBeforeDeletion,
         this.newProject
       )
-      this.ProjectEntityUpdateHandler._cleanUpEntity = sinon.stub().yields()
+      this.ProjectEntityUpdateHandler._cleanUpEntity = sinon
+        .stub()
+        .yields(null, [{ type: 'doc', entity: this.doc, path: this.path }])
 
       this.ProjectEntityUpdateHandler.deleteEntity(
         projectId,
@@ -1600,15 +1602,14 @@ describe('ProjectEntityUpdateHandler', function () {
     })
 
     it('it notifies the tpds', function () {
-      this.TpdsUpdateSender.deleteEntity
-        .calledWith({
-          projectId,
-          path: this.path,
-          projectName: this.projectBeforeDeletion.name,
-          entityId: docId,
-          entityType: 'doc',
-        })
-        .should.equal(true)
+      this.TpdsUpdateSender.deleteEntity.should.have.been.calledWith({
+        projectId,
+        path: this.path,
+        projectName: this.projectBeforeDeletion.name,
+        entityId: docId,
+        entityType: 'doc',
+        subtreeEntityIds: [this.doc._id],
+      })
     })
 
     it('retuns the entity_id', function () {
@@ -2358,7 +2359,13 @@ describe('ProjectEntityUpdateHandler', function () {
           this.path,
           userId,
           this.source,
-          done
+          (err, subtreeListing) => {
+            if (err) {
+              return done(err)
+            }
+            this.subtreeListing = subtreeListing
+            done()
+          }
         )
       })
 
@@ -2378,20 +2385,25 @@ describe('ProjectEntityUpdateHandler', function () {
         this.DocumentUpdaterHandler.deleteDoc.called.should.equal(false)
       })
 
-      it('should should send the update to the doc updater', function () {
+      it('should send the update to the doc updater', function () {
         const oldFiles = [{ file: this.entity, path: this.path }]
-        this.DocumentUpdaterHandler.updateProjectStructure
-          .calledWith(
-            projectId,
-            projectHistoryId,
-            userId,
-            {
-              oldFiles,
-              newProject: this.newProject,
-            },
-            this.source
-          )
-          .should.equal(true)
+        this.DocumentUpdaterHandler.updateProjectStructure.should.have.been.calledWith(
+          projectId,
+          projectHistoryId,
+          userId,
+          {
+            oldFiles,
+            oldDocs: [],
+            newProject: this.newProject,
+          },
+          this.source
+        )
+      })
+
+      it('should return a subtree listing containing only the file', function () {
+        expect(this.subtreeListing).to.deep.equal([
+          { type: 'file', entity: this.entity, path: this.path },
+        ])
       })
     })
 
@@ -2409,7 +2421,13 @@ describe('ProjectEntityUpdateHandler', function () {
           this.path,
           userId,
           this.source,
-          done
+          (err, subtreeListing) => {
+            if (err) {
+              return done(err)
+            }
+            this.subtreeListing = subtreeListing
+            done()
+          }
         )
       })
 
@@ -2419,20 +2437,25 @@ describe('ProjectEntityUpdateHandler', function () {
           .should.equal(true)
       })
 
-      it('should should send the update to the doc updater', function () {
+      it('should send the update to the doc updater', function () {
         const oldDocs = [{ doc: this.entity, path: this.path }]
-        this.DocumentUpdaterHandler.updateProjectStructure
-          .calledWith(
-            projectId,
-            projectHistoryId,
-            userId,
-            {
-              oldDocs,
-              newProject: this.newProject,
-            },
-            this.source
-          )
-          .should.equal(true)
+        this.DocumentUpdaterHandler.updateProjectStructure.should.have.been.calledWith(
+          projectId,
+          projectHistoryId,
+          userId,
+          {
+            oldDocs,
+            oldFiles: [],
+            newProject: this.newProject,
+          },
+          this.source
+        )
+      })
+
+      it('should return a subtree listing containing only the doc', function () {
+        expect(this.subtreeListing).to.deep.equal([
+          { type: 'doc', entity: this.entity, path: this.path },
+        ])
       })
     })
 
@@ -2465,7 +2488,13 @@ describe('ProjectEntityUpdateHandler', function () {
           path,
           userId,
           this.source,
-          done
+          (err, subtreeListing) => {
+            if (err) {
+              return done(err)
+            }
+            this.subtreeListing = subtreeListing
+            done()
+          }
         )
       })
 
@@ -2520,6 +2549,29 @@ describe('ProjectEntityUpdateHandler', function () {
           )
           .should.equal(true)
       })
+
+      it('should return a subtree listing containing all sub-entities', function () {
+        expect(this.subtreeListing).to.have.deep.members([
+          { type: 'folder', entity: this.folder, path: '/folder' },
+          {
+            type: 'folder',
+            entity: this.folder.folders[0],
+            path: '/folder/subfolder',
+          },
+          {
+            type: 'file',
+            entity: this.file1,
+            path: '/folder/subfolder/file-name-1',
+          },
+          {
+            type: 'doc',
+            entity: this.doc1,
+            path: '/folder/subfolder/doc-name-1',
+          },
+          { type: 'file', entity: this.file2, path: '/folder/file-name-2' },
+          { type: 'doc', entity: this.doc2, path: '/folder/doc-name-2' },
+        ])
+      })
     })
   })
 

services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js

@@ -2,6 +2,7 @@ const { ObjectId } = require('mongodb')
 const SandboxedModule = require('sandboxed-module')
 const path = require('path')
 const sinon = require('sinon')
+const { expect } = require('chai')
 
 const modulePath = path.join(
   __dirname,
@@ -200,11 +201,13 @@ describe('TpdsUpdateSender', function () {
 
     it('deleting entity', async function () {
       const path = '/path/here/t.tex'
+      const subtreeEntityIds = ['id1', 'id2']
 
       await this.TpdsUpdateSender.promises.deleteEntity({
         projectId,
         path,
         projectName,
+        subtreeEntityIds,
       })
 
       const {
@@ -221,6 +224,7 @@ describe('TpdsUpdateSender', function () {
       )}${encodeURIComponent(path)}`
       job0.headers.sl_all_user_ids.should.eql(JSON.stringify([userId]))
       job0.uri.should.equal(expectedUrl)
+      expect(job0.json).to.deep.equal({ subtreeEntityIds })
 
       const { group: group1, job: job1 } = this.request.secondCall.args[0].json
       group1.should.equal(collaberatorRef)