From c704312c8597b7b56784da59723d1215e1c4ed75 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 18 Mar 2019 09:16:32 +0000 Subject: [PATCH] Merge pull request #1625 from sharelatex/bg-move-entities-safely move project entities safely without losing data on error GitOrigin-RevId: 864fcf14af1045154e9deb7d02a4f2d508e6021e --- .../ProjectEntityMongoUpdateHandler.coffee | 30 ++++++++- ...rojectEntityMongoUpdateHandlerTests.coffee | 62 +++++++++++++++++-- 2 files changed, 86 insertions(+), 6 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectEntityMongoUpdateHandler.coffee b/services/web/app/coffee/Features/Project/ProjectEntityMongoUpdateHandler.coffee index a598715bf1..5eb0ee37fd 100644 --- a/services/web/app/coffee/Features/Project/ProjectEntityMongoUpdateHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectEntityMongoUpdateHandler.coffee @@ -129,15 +129,41 @@ module.exports = ProjectEntityMongoUpdateHandler = self = return callback(error) if error? ProjectEntityHandler.getAllEntitiesFromProject project, (error, oldDocs, oldFiles) -> return callback(error) if error? - self._removeElementFromMongoArray Project, project_id, entityPath.mongo, (err, newProject)-> + # For safety, insert the entity in the destination + # location first, and then remove the original. If + # there is an error the entity may appear twice. This + # will cause some breakage but is better than being + # lost, which is what happens if this is done in the + # opposite order. + self._putElement project, destFolderId, entity, entityType, (err, result)-> return callback(err) if err? - self._putElement newProject, destFolderId, entity, entityType, (err, result, newProject)-> + # Note: putElement always pushes onto the end of an + # array so it will never change an existing mongo + # path. Therefore it is safe to remove an element + # from the project with an existing path after + # calling putElement. But we must be sure that we + # have not moved a folder subfolder of itself (which + # is done by _checkValidMove above) because that + # would lead to it being deleted. + self._removeElementFromMongoArray Project, project_id, entityPath.mongo, (err, newProject)-> return callback(err) if err? ProjectEntityHandler.getAllEntitiesFromProject newProject, (err, newDocs, newFiles) -> return callback(err) if err? startPath = entityPath.fileSystem endPath = result.path.fileSystem changes = {oldDocs, newDocs, oldFiles, newFiles, newProject} + # check that no files have been lost (or duplicated) + if (oldFiles.length != newFiles.length) or (oldDocs.length != newDocs.length) + logger.err { + project_id: project_id + oldDocs: oldDocs.length + newDocs: newDocs.length + oldFiles:oldFiles.length + newFiles: newFiles.length + origProject: project + newProject: newProject + }, "project corrupted moving files - shouldn't happen" + return callback(new Error("unexpected change in project structure")) callback null, project, startPath, endPath, entity.rev, changes, callback deleteEntity: wrapWithLock (project_id, entity_id, entityType, callback) -> diff --git a/services/web/test/unit/coffee/Project/ProjectEntityMongoUpdateHandlerTests.coffee b/services/web/test/unit/coffee/Project/ProjectEntityMongoUpdateHandlerTests.coffee index 7ea2dee454..35c0203298 100644 --- a/services/web/test/unit/coffee/Project/ProjectEntityMongoUpdateHandlerTests.coffee +++ b/services/web/test/unit/coffee/Project/ProjectEntityMongoUpdateHandlerTests.coffee @@ -252,7 +252,7 @@ describe 'ProjectEntityMongoUpdateHandler', -> @subject._checkValidMove = sinon.stub().yields() - @subject._removeElementFromMongoArray = sinon.stub().yields(null, @project) + @subject._removeElementFromMongoArray = sinon.stub().yields(null, @newProject) @subject._putElement = sinon.stub().yields(null, path: @pathAfterMove, @newProject) @subject.moveEntity project_id, doc_id, folder_id, "docs", @callback @@ -272,14 +272,19 @@ describe 'ProjectEntityMongoUpdateHandler', -> .calledWith(@project, 'docs', @doc, @path, folder_id) .should.equal true + it "should put the element in the new folder", -> + @subject._putElement + .calledWith(@project, folder_id, @doc, "docs") + .should.equal true + it 'should remove the element from its current position', -> @subject._removeElementFromMongoArray .calledWith(@ProjectModel, project_id, @path.mongo) .should.equal true - it "should put the element back in the new folder", -> - @subject._putElement - .calledWith(@project, folder_id, @doc, "docs") + it 'should remove the element from its current position after putting the element in the new folder', -> + @subject._removeElementFromMongoArray + .calledAfter(@subject._putElement) .should.equal true it "calls the callback", -> @@ -288,6 +293,55 @@ describe 'ProjectEntityMongoUpdateHandler', -> null, @project, @path.fileSystem, @pathAfterMove.fileSystem, @doc.rev, changes ).should.equal true + describe 'moveEntity must refuse to move the folder to a subfolder of itself', -> + beforeEach -> + @pathAfterMove = { + fileSystem: "/somewhere/else.txt" + } + + @doc = {lines:["1234","312343d"], rev: "1234"} + @path = { mongo:"folders[0]", fileSystem:"/old_folder/somewhere.txt" } + @newProject = "new-project" + @ProjectLocator.findElement = sinon.stub() + .withArgs({@project, element_id: @docId, type: 'docs'}) + .yields(null, @doc, @path) + + # return an error when moving a folder to a subfolder of itself + @subject._checkValidMove = sinon.stub().yields(new Error()) + + @subject._removeElementFromMongoArray = sinon.stub().yields(null, @project) + @subject._putElement = sinon.stub().yields(null, path: @pathAfterMove, @newProject) + + @subject.moveEntity project_id, doc_id, folder_id, "docs", @callback + + it 'should get the project', -> + @ProjectGetter.getProjectWithoutLock + .calledWith(project_id, {rootFolder:true, name:true, overleaf:true}) + .should.equal true + + it 'should find the doc to move', -> + @ProjectLocator.findElement + .calledWith({element_id: doc_id, type: "docs", project: @project }) + .should.equal true + + it 'should check this is an invalid move', -> + @subject._checkValidMove + .calledWith(@project, 'docs', @doc, @path, folder_id) + .should.equal true + + it "should not put the element in the new folder", -> + @subject._putElement.called + .should.equal false + + it 'should not remove the element from its current position', -> + @subject._removeElementFromMongoArray.called + .should.equal false + + it "calls the callback with an error", -> + @callback.calledWith( + new Error() + ).should.equal true + describe 'deleteEntity', -> beforeEach -> @path = mongo: "mongo.path", fileSystem: "/file/system/path"