From b8ab8fbdbd80b2f761f7b3d8038ef9a1e47eda16 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Mon, 22 Nov 2021 09:03:10 -0500 Subject: [PATCH] Merge pull request #5853 from overleaf/em-fix-file-tree More filename fixes when resyncing GitOrigin-RevId: 15e2e71fa8d16a1c708449eb607918b36f2fb3ee --- .../Features/Project/ProjectEntityHandler.js | 2 +- .../Project/ProjectEntityUpdateHandler.js | 98 ++++++++---- .../ProjectEntityUpdateHandlerTests.js | 150 +++++++++++++++++- 3 files changed, 214 insertions(+), 36 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectEntityHandler.js b/services/web/app/src/Features/Project/ProjectEntityHandler.js index 0b8a2e3b1a..11f6364fad 100644 --- a/services/web/app/src/Features/Project/ProjectEntityHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityHandler.js @@ -92,7 +92,7 @@ const ProjectEntityHandler = { } } } - callback(null, docs, files) + callback(null, docs, files, folders) }) }, diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index 791aebff44..55c8672df1 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/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,36 +1393,66 @@ 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 cleanFilename = SafePath.clean(filename) - if (cleanFilename !== filename) { - // File name needs to be cleaned - let newPath = Path.join( - entity.path.slice(0, -filename.length), - cleanFilename - ) - if (paths.has(newPath)) { - newPath = ProjectEntityUpdateHandler.findNextAvailablePath( - paths, - newPath - ) - } - renames.push({ entity, newName: cleanFilename, newPath }) - } else if (paths.has(entity.path)) { - // Duplicate filename - const newPath = ProjectEntityUpdateHandler.findNextAvailablePath( - paths, - entity.path - ) - const newName = newPath.split('/').pop() - renames.push({ entity, newName, newPath }) - paths.add(newPath) + const originalName = entity.folder + ? entity.folder.name + : entity.doc + ? entity.doc.name + : entity.file.name + + let newPath = entity.path + let newName = originalName + + // Clean the filename if necessary + if (newName === '') { + newName = 'untitled' } else { - paths.add(entity.path) + 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() + } + + // If we've changed the filename, schedule a rename + if (newName !== originalName) { + renames.push({ entity, newName, newPath }) + if (entity.folder) { + // Here, we rely on entities being processed in the right order. + // 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) { @@ -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 entityType = entity.doc ? 'doc' : 'file' + const entityId = entity.folder + ? 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 diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js index e826646fdf..581f90c719 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js +++ b/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 () {