Merge pull request #1625 from sharelatex/bg-move-entities-safely

move project entities safely without losing data on error

GitOrigin-RevId: 864fcf14af1045154e9deb7d02a4f2d508e6021e

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) ->

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"