Merge pull request #5853 from overleaf/em-fix-file-tree

More filename fixes when resyncing

GitOrigin-RevId: 15e2e71fa8d16a1c708449eb607918b36f2fb3ee

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

@@ -92,7 +92,7 @@ const ProjectEntityHandler = {
           }
         }
       }
-      callback(null, docs, files)
+      callback(null, docs, files, folders)
     })
   },
 

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

@@ -1351,14 +1351,16 @@ const ProjectEntityUpdateHandler = {
 
         ProjectEntityHandler.getAllEntitiesFromProject(
           project,
-          (error, docs, files) => {
+          (error, docs, files, folders) => {
             if (error != null) {
               return callback(error)
             }
+            // _checkFileTree() must be passed the folders before docs and
+            // files
             ProjectEntityUpdateHandler._checkFiletree(
               projectId,
               projectHistoryId,
-              [...docs, ...files],
+              [...folders, ...docs, ...files],
               error => {
                 if (error) {
                   return callback(error)
@@ -1391,38 +1393,68 @@ const ProjectEntityUpdateHandler = {
   ),
 
   _checkFiletree(projectId, projectHistoryId, entities, callback) {
+    const adjustPathsAfterFolderRename = (oldPath, newPath) => {
+      oldPath = oldPath + '/'
+      newPath = newPath + '/'
+      for (const entity of entities) {
+        if (entity.path.startsWith(oldPath)) {
+          entity.path = newPath + entity.path.slice(oldPath.length)
+        }
+      }
+    }
+
+    // Data structures for recording pending renames
     const renames = []
     const paths = new Set()
     for (const entity of entities) {
-      const filename = entity.doc ? entity.doc.name : entity.file.name
+      const originalName = entity.folder
-      const cleanFilename = SafePath.clean(filename)
+        ? entity.folder.name
-      if (cleanFilename !== filename) {
+        : entity.doc
-        // File name needs to be cleaned
+        ? entity.doc.name
-        let newPath = Path.join(
+        : entity.file.name
-          entity.path.slice(0, -filename.length),
+
-          cleanFilename
+      let newPath = entity.path
+      let newName = originalName
+
+      // Clean the filename if necessary
+      if (newName === '') {
+        newName = 'untitled'
+      } else {
+        newName = SafePath.clean(newName)
+      }
+      if (newName !== originalName) {
+        newPath = Path.join(
+          newPath.slice(0, newPath.length - originalName.length),
+          newName
         )
+      }
+
+      // Check if we've seen that path already
       if (paths.has(newPath)) {
         newPath = ProjectEntityUpdateHandler.findNextAvailablePath(
           paths,
           newPath
         )
+        newName = newPath.split('/').pop()
       }
-        renames.push({ entity, newName: cleanFilename, newPath })
+
-      } else if (paths.has(entity.path)) {
+      // If we've changed the filename, schedule a rename
-        // Duplicate filename
+      if (newName !== originalName) {
-        const newPath = ProjectEntityUpdateHandler.findNextAvailablePath(
-          paths,
-          entity.path
-        )
-        const newName = newPath.split('/').pop()
         renames.push({ entity, newName, newPath })
-        paths.add(newPath)
+        if (entity.folder) {
-      } else {
+          // Here, we rely on entities being processed in the right order.
-        paths.add(entity.path)
+          // Parent folders need to be processed before their children. This is
+          // the case only because getAllEntitiesFromProject() returns folders
+          // in that order and resyncProjectHistory() calls us with the folders
+          // first.
+          adjustPathsAfterFolderRename(entity.path, newPath)
         }
       }
 
+      // Remember that we've seen this path
+      paths.add(newPath)
+    }
+
     if (renames.length === 0) {
       return callback()
     }
@@ -1440,8 +1472,12 @@ const ProjectEntityUpdateHandler = {
     // rename the duplicate files
     const doRename = (rename, cb) => {
       const entity = rename.entity
-      const entityId = entity.doc ? entity.doc._id : entity.file._id
+      const entityId = entity.folder
-      const entityType = entity.doc ? 'doc' : 'file'
+        ? entity.folder._id
+        : entity.doc
+        ? entity.doc._id
+        : entity.file._id
+      const entityType = entity.folder ? 'folder' : entity.doc ? 'doc' : 'file'
       ProjectEntityMongoUpdateHandler.renameEntity(
         projectId,
         entityId,
@@ -1453,7 +1489,9 @@ const ProjectEntityUpdateHandler = {
           }
           // update the renamed entity for the resync
           entity.path = rename.newPath
-          if (entityType === 'doc') {
+          if (entityType === 'folder') {
+            entity.folder.name = rename.newName
+          } else if (entityType === 'doc') {
             entity.doc.name = rename.newName
           } else {
             entity.file.name = rename.newName

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

@@ -1935,7 +1935,8 @@ describe('ProjectEntityUpdateHandler', function () {
         this.ProjectEntityHandler.getAllEntitiesFromProject.yields(
           null,
           docs,
-          files
+          files,
+          []
         )
         this.ProjectEntityUpdateHandler.resyncProjectHistory(
           projectId,
@@ -2021,7 +2022,8 @@ describe('ProjectEntityUpdateHandler', function () {
         this.ProjectEntityHandler.getAllEntitiesFromProject.yields(
           null,
           this.docs,
-          this.files
+          this.files,
+          []
         )
         this.ProjectEntityUpdateHandler.resyncProjectHistory(projectId, done)
       })
@@ -2104,40 +2106,64 @@ describe('ProjectEntityUpdateHandler', function () {
             doc: { _id: 'doc1', name: '/d/e/f/test.tex' },
             path: 'a/b/c/d/e/f/test.tex',
           },
+          {
+            doc: { _id: 'doc2', name: '' },
+            path: 'a',
+          },
         ]
         this.files = [
           {
             file: { _id: 'file1', name: 'A*.png', hash: 'hash1' },
             path: 'A*.png',
           },
+          {
+            file: { _id: 'file2', name: 'A_.png', hash: 'hash2' },
+            path: 'A_.png',
+          },
         ]
         this.ProjectEntityHandler.getAllEntitiesFromProject.yields(
           null,
           this.docs,
-          this.files
+          this.files,
+          []
         )
         this.ProjectEntityUpdateHandler.resyncProjectHistory(projectId, done)
       })
 
       it('renames the files', function () {
         const renameEntity = this.ProjectEntityMongoUpdateHandler.renameEntity
-        expect(renameEntity).to.have.callCount(2)
+        expect(renameEntity).to.have.callCount(4)
         expect(renameEntity).to.have.been.calledWith(
           projectId,
           'doc1',
           'doc',
           '_d_e_f_test.tex'
         )
+        expect(renameEntity).to.have.been.calledWith(
+          projectId,
+          'doc2',
+          'doc',
+          'untitled'
+        )
         expect(renameEntity).to.have.been.calledWith(
           projectId,
           'file1',
           'file',
           'A_.png'
         )
+        expect(renameEntity).to.have.been.calledWith(
+          projectId,
+          'file2',
+          'file',
+          'A_.png (1)'
+        )
       })
 
       it('tells the doc updater to resync the project', function () {
-        const docs = [{ doc: 'doc1', path: 'a/b/c/_d_e_f_test.tex' }]
+        const docs = [
+          { doc: 'doc1', path: 'a/b/c/_d_e_f_test.tex' },
+          { doc: 'doc2', path: 'a/untitled' },
+        ]
         const urlPrefix = `www.filestore.test/${projectId}`
         const files = [
           {
@@ -2146,12 +2172,126 @@ describe('ProjectEntityUpdateHandler', function () {
             url: `${urlPrefix}/file1`,
             _hash: 'hash1',
           },
+          {
+            file: 'file2',
+            path: 'A_.png (1)',
+            url: `${urlPrefix}/file2`,
+            _hash: 'hash2',
+          },
         ]
         expect(
           this.DocumentUpdaterHandler.resyncProjectHistory
         ).to.have.been.calledWith(projectId, projectHistoryId, docs, files)
       })
     })
+
+    describe('a project with a bad folder name', function () {
+      beforeEach(function (done) {
+        this.ProjectGetter.getProject.yields(null, this.project)
+        const folders = [
+          {
+            folder: { _id: 'folder1', name: 'good' },
+            path: 'good',
+          },
+          {
+            folder: { _id: 'folder2', name: 'bad*' },
+            path: 'bad*',
+          },
+        ]
+        const docs = [
+          {
+            doc: { _id: 'doc1', name: 'doc1.tex' },
+            path: 'good/doc1.tex',
+          },
+          {
+            doc: { _id: 'doc2', name: 'duplicate.tex' },
+            path: 'bad*/doc2.tex',
+          },
+        ]
+        const files = []
+        this.ProjectEntityHandler.getAllEntitiesFromProject.yields(
+          null,
+          docs,
+          files,
+          folders
+        )
+        this.ProjectEntityUpdateHandler.resyncProjectHistory(projectId, done)
+      })
+
+      it('renames the folder', function () {
+        const renameEntity = this.ProjectEntityMongoUpdateHandler.renameEntity
+        expect(renameEntity).to.have.callCount(1)
+        expect(renameEntity).to.have.been.calledWith(
+          projectId,
+          'folder2',
+          'folder',
+          'bad_'
+        )
+      })
+
+      it('tells the doc updater to resync the project', function () {
+        const docs = [
+          { doc: 'doc1', path: 'good/doc1.tex' },
+          { doc: 'doc2', path: 'bad_/doc2.tex' },
+        ]
+        const files = []
+        expect(
+          this.DocumentUpdaterHandler.resyncProjectHistory
+        ).to.have.been.calledWith(projectId, projectHistoryId, docs, files)
+      })
+    })
+
+    describe('a project with duplicate names between a folder and a doc', function () {
+      beforeEach(function (done) {
+        this.ProjectGetter.getProject.yields(null, this.project)
+        const folders = [
+          {
+            folder: { _id: 'folder1', name: 'chapters' },
+            path: 'chapters',
+          },
+        ]
+        const docs = [
+          {
+            doc: { _id: 'doc1', name: 'chapters' },
+            path: 'chapters',
+          },
+          {
+            doc: { _id: 'doc2', name: 'chapter1.tex' },
+            path: 'chapters/chapter1.tex',
+          },
+        ]
+        const files = []
+        this.ProjectEntityHandler.getAllEntitiesFromProject.yields(
+          null,
+          docs,
+          files,
+          folders
+        )
+        this.ProjectEntityUpdateHandler.resyncProjectHistory(projectId, done)
+      })
+
+      it('renames the doc', function () {
+        const renameEntity = this.ProjectEntityMongoUpdateHandler.renameEntity
+        expect(renameEntity).to.have.callCount(1)
+        expect(renameEntity).to.have.been.calledWith(
+          projectId,
+          'doc1',
+          'doc',
+          'chapters (1)'
+        )
+      })
+
+      it('tells the doc updater to resync the project', function () {
+        const docs = [
+          { doc: 'doc1', path: 'chapters (1)' },
+          { doc: 'doc2', path: 'chapters/chapter1.tex' },
+        ]
+        const files = []
+        expect(
+          this.DocumentUpdaterHandler.resyncProjectHistory
+        ).to.have.been.calledWith(projectId, projectHistoryId, docs, files)
+      })
+    })
   })
 
   describe('_cleanUpEntity', function () {