Merge pull request #989 from sharelatex/bg-clean-up-broken-project-on-error

clean up broken project on error in ProjectDuplicator

services/web/app/coffee/Features/Project/ProjectDuplicator.coffee

@@ -2,6 +2,7 @@ projectCreationHandler = require('./ProjectCreationHandler')
 ProjectEntityUpdateHandler = require('./ProjectEntityUpdateHandler')
 projectLocator = require('./ProjectLocator')
 projectOptionsHandler = require('./ProjectOptionsHandler')
+projectDeleter = require('./ProjectDeleter')
 DocumentUpdaterHandler = require("../DocumentUpdater/DocumentUpdaterHandler")
 DocstoreManager = require "../Docstore/DocstoreManager"
 ProjectGetter = require("./ProjectGetter")
@@ -69,28 +70,45 @@ module.exports = ProjectDuplicator =
 				DocumentUpdaterHandler.flushProjectToMongo originalProject_id, cb
 			originalProject: (cb)->
 				ProjectGetter.getProject originalProject_id, {compiler:true, rootFolder:true, rootDoc_id:true}, cb
-			newProject: (cb)->
-				projectCreationHandler.createBlankProject owner._id, newProjectName, cb
 			originalRootDoc: (cb)->
 				projectLocator.findRootDoc {project_id:originalProject_id}, cb
 			docContentsArray: (cb)-> 
 				DocstoreManager.getAllDocs originalProject_id, cb
 
+		# Get the contents of the original project first
 		async.series jobs, (err, results)->
 			if err?
-				logger.err err:err, originalProject_id:originalProject_id, "error duplicating project"
+				logger.err err:err, originalProject_id:originalProject_id, "error duplicating project reading original project"
 				return callback(err)
-			{originalProject, newProject, originalRootDoc, docContentsArray} = results
+			{originalProject, originalRootDoc, docContentsArray} = results
 
 			originalRootDoc = originalRootDoc?[0]
 
 			docContents = {}
 			for docContent in docContentsArray
 				docContents[docContent._id] = docContent
-			
-			projectOptionsHandler.setCompiler newProject._id, originalProject.compiler, ->
 
-			ProjectDuplicator._copyFolderRecursivly owner._id, newProject._id, originalProject_id, originalRootDoc, originalProject.rootFolder[0], newProject.rootFolder[0], docContents, ->
+			# Now create the new project, cleaning it up on failure if necessary
+			projectCreationHandler.createBlankProject owner._id, newProjectName, (err, newProject) ->
 				if err?
-					logger.err err:err, originalProject_id:originalProject_id,  newProjectName:newProjectName, "error cloning project"
-				callback(err, newProject)
+					logger.err err:err, originalProject_id:originalProject_id, "error duplicating project when creating new project"
+					return callback(err)
+
+				copyJobs =
+					setCompiler: (cb) ->
+						projectOptionsHandler.setCompiler newProject._id, originalProject.compiler, cb
+					copyFiles: (cb) ->
+						ProjectDuplicator._copyFolderRecursivly owner._id, newProject._id, originalProject_id, originalRootDoc, originalProject.rootFolder[0], newProject.rootFolder[0], docContents, cb
+
+				# Copy the contents of the original project into the new project
+				async.series copyJobs, (err) ->
+					if err?
+						logger.err err:err, originalProject_id:originalProject_id, newProjectName:newProjectName, newProject_id: newProject._id, "error cloning project, will delete broken clone"
+						# Clean up broken clone on error.
+						# Make sure we delete the new failed project, not the original one!
+						projectDeleter.deleteProject newProject._id, (delete_err) ->
+							if delete_err?
+								logger.error newProject_id: newProject._id, delete_err:delete_err, "error deleting broken clone of project"
+							callback(err)
+					else
+						callback(null, newProject)

services/web/test/unit/coffee/Project/ProjectDuplicatorTests.coffee

@@ -17,7 +17,7 @@ describe 'ProjectDuplicator', ->
 			_id:"level1folderId"
 			docs:[@doc1 = {_id: "doc1_id", name:"level1folderDocName"}]
 			folders:[@level2folder]
-			fileRefs:[{name:"file1", _id:"file1"}, null]
+			fileRefs:[{name:"file1", _id:"file1"}, null] # the null is intentional to test null docs/files
 		@rootFolder = 
 			name:"rootFolder"
 			_id:"rootFolderId"
@@ -62,13 +62,17 @@ describe 'ProjectDuplicator', ->
 			findRootDoc : sinon.stub().callsArgWith(1, null, @foundRootDoc, {})
 
 		@projectOptionsHandler =
-			setCompiler : sinon.stub()								
+			setCompiler : sinon.stub().callsArg(2)
 		@ProjectEntityUpdateHandler =
 			addDoc: sinon.stub().callsArgWith(5, null, {name:"somDoc"})
-			copyFileFromExistingProjectWithProject: sinon.stub().callsArgWith(5)
+			copyFileFromExistingProjectWithProject: sinon.stub()
 			setRootDoc: sinon.stub()
 			addFolder: sinon.stub().callsArgWith(3, null, @newFolder)
 
+		@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject.withArgs(sinon.match.any, sinon.match.any, sinon.match.any, "BROKEN-FILE", sinon.match.any, sinon.match.any).callsArgWith(5, new Error("failed"))
+		@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject.withArgs(sinon.match.any, sinon.match.any, sinon.match.any, sinon.match.object, sinon.match.any).callsArg(5)
+		@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject.withArgs(sinon.match.any, sinon.match.any, sinon.match.any, null, sinon.match.any).callsArg(5)
+
 		@DocumentUpdaterHandler =
 			flushProjectToMongo: sinon.stub().callsArg(1)
 
@@ -81,84 +85,116 @@ describe 'ProjectDuplicator', ->
 		@ProjectGetter.getProject.withArgs(@old_project_id, sinon.match.any).callsArgWith(2, null, @project)
 		@ProjectGetter.getProject.withArgs(@new_project_id, sinon.match.any).callsArgWith(2, null, @stubbedNewProject)
 
+		@ProjectDeleter = 
+			deleteProject: sinon.stub().callsArgWith(1,null)
+
 		@duplicator = SandboxedModule.require modulePath, requires:
 			'../../models/Project':{Project:@Project}
 			"../DocumentUpdater/DocumentUpdaterHandler": @DocumentUpdaterHandler
 			'./ProjectCreationHandler': @creationHandler
 			'./ProjectEntityUpdateHandler': @ProjectEntityUpdateHandler
 			'./ProjectLocator': @locator
+			'./ProjectDeleter': @ProjectDeleter
 			'./ProjectOptionsHandler': @projectOptionsHandler
 			"../Docstore/DocstoreManager": @DocstoreManager
 			"./ProjectGetter":@ProjectGetter
-			'logger-sharelatex':{log:->}
-
-	it "should look up the original project", (done) ->
-		newProjectName = "someProj"
-		@duplicator.duplicate @owner, @old_project_id, newProjectName, (err, newProject)=>
-			@ProjectGetter.getProject.calledWith(@old_project_id).should.equal true
-			done()
-
-	it "should flush the original project to mongo", (done) ->
-		newProjectName = "someProj"
-		@duplicator.duplicate @owner, @old_project_id, newProjectName, (err, newProject)=>
-			@DocumentUpdaterHandler.flushProjectToMongo.calledWith(@old_project_id).should.equal true
-			done()
-
-	it 'should create a blank project', (done)->
-		newProjectName = "someProj"
-		@duplicator.duplicate @owner, @old_project_id, newProjectName, (err, newProject)=>
-			newProject._id.should.equal @stubbedNewProject._id
-			@creationHandler.createBlankProject.calledWith(@owner._id, newProjectName).should.equal true
-			done()
-
-	it 'should use the same compiler', (done)->
-		@ProjectEntityUpdateHandler.addDoc.callsArgWith(5, null, @rootFolder.docs[0], @owner._id)
-		@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
-			@projectOptionsHandler.setCompiler.calledWith(@stubbedNewProject._id, @project.compiler).should.equal true
-			done()
-	
-	it 'should use the same root doc', (done)->
-		@ProjectEntityUpdateHandler.addDoc.callsArgWith(5, null, @rootFolder.docs[0], @owner._id)
-		@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
-			@ProjectEntityUpdateHandler.setRootDoc.calledWith(@stubbedNewProject._id, @rootFolder.docs[0]._id).should.equal true
-			done()
-
-	it 'should not copy the collaberators or read only refs', (done)->
-		@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
-			newProject.collaberator_refs.length.should.equal 0
-			newProject.readOnly_refs.length.should.equal 0
-			done()	
-
-	it 'should copy all the folders', (done)->
-		@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
-			@ProjectEntityUpdateHandler.addFolder.calledWith(@new_project_id, @stubbedNewProject.rootFolder[0]._id, @level1folder.name).should.equal true
-			@ProjectEntityUpdateHandler.addFolder.calledWith(@new_project_id, @newFolder._id, @level2folder.name).should.equal true
-			@ProjectEntityUpdateHandler.addFolder.callCount.should.equal 2
-			done()
-
-	it 'should copy all the docs', (done)->
-		@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
-			@DocstoreManager.getAllDocs.calledWith(@old_project_id).should.equal true
-			@ProjectEntityUpdateHandler.addDoc
-				.calledWith(@new_project_id, @stubbedNewProject.rootFolder[0]._id, @doc0.name, @doc0_lines, @owner._id)
-				.should.equal true
-			@ProjectEntityUpdateHandler.addDoc
-				.calledWith(@new_project_id, @newFolder._id, @doc1.name, @doc1_lines, @owner._id)
-				.should.equal true
-			@ProjectEntityUpdateHandler.addDoc
-				.calledWith(@new_project_id, @newFolder._id, @doc2.name, @doc2_lines, @owner._id)
-				.should.equal true
-			done()
-
-	it 'should copy all the files', (done)->
-		@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
-			@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject
-				.calledWith(@stubbedNewProject, @stubbedNewProject.rootFolder[0]._id, @project._id, @rootFolder.fileRefs[0], @owner._id)
-				.should.equal true
-			@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject
-				.calledWith(@stubbedNewProject, @newFolder._id, @project._id, @level1folder.fileRefs[0], @owner._id)
-				.should.equal true
-			@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject
-				.calledWith(@stubbedNewProject, @newFolder._id, @project._id, @level2folder.fileRefs[0], @owner._id)
-				.should.equal true
-			done()
+			'logger-sharelatex':{
+				log:->
+				err:->
+			}
+
+	describe "when the copy succeeds", ->
+
+		it "should look up the original project", (done) ->
+			newProjectName = "someProj"
+			@duplicator.duplicate @owner, @old_project_id, newProjectName, (err, newProject)=>
+				@ProjectGetter.getProject.calledWith(@old_project_id).should.equal true
+				done()
+
+		it "should flush the original project to mongo", (done) ->
+			newProjectName = "someProj"
+			@duplicator.duplicate @owner, @old_project_id, newProjectName, (err, newProject)=>
+				@DocumentUpdaterHandler.flushProjectToMongo.calledWith(@old_project_id).should.equal true
+				done()
+
+		it 'should create a blank project', (done)->
+			newProjectName = "someProj"
+			@duplicator.duplicate @owner, @old_project_id, newProjectName, (err, newProject)=>
+				newProject._id.should.equal @stubbedNewProject._id
+				@creationHandler.createBlankProject.calledWith(@owner._id, newProjectName).should.equal true
+				done()
+
+		it 'should use the same compiler', (done)->
+			@ProjectEntityUpdateHandler.addDoc.callsArgWith(5, null, @rootFolder.docs[0], @owner._id)
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				@projectOptionsHandler.setCompiler.calledWith(@stubbedNewProject._id, @project.compiler).should.equal true
+				done()
+		
+		it 'should use the same root doc', (done)->
+			@ProjectEntityUpdateHandler.addDoc.callsArgWith(5, null, @rootFolder.docs[0], @owner._id)
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				@ProjectEntityUpdateHandler.setRootDoc.calledWith(@stubbedNewProject._id, @rootFolder.docs[0]._id).should.equal true
+				done()
+
+		it 'should not copy the collaberators or read only refs', (done)->
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				newProject.collaberator_refs.length.should.equal 0
+				newProject.readOnly_refs.length.should.equal 0
+				done()	
+
+		it 'should copy all the folders', (done)->
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				@ProjectEntityUpdateHandler.addFolder.calledWith(@new_project_id, @stubbedNewProject.rootFolder[0]._id, @level1folder.name).should.equal true
+				@ProjectEntityUpdateHandler.addFolder.calledWith(@new_project_id, @newFolder._id, @level2folder.name).should.equal true
+				@ProjectEntityUpdateHandler.addFolder.callCount.should.equal 2
+				done()
+
+		it 'should copy all the docs', (done)->
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				@DocstoreManager.getAllDocs.calledWith(@old_project_id).should.equal true
+				@ProjectEntityUpdateHandler.addDoc
+					.calledWith(@new_project_id, @stubbedNewProject.rootFolder[0]._id, @doc0.name, @doc0_lines, @owner._id)
+					.should.equal true
+				@ProjectEntityUpdateHandler.addDoc
+					.calledWith(@new_project_id, @newFolder._id, @doc1.name, @doc1_lines, @owner._id)
+					.should.equal true
+				@ProjectEntityUpdateHandler.addDoc
+					.calledWith(@new_project_id, @newFolder._id, @doc2.name, @doc2_lines, @owner._id)
+					.should.equal true
+				done()
+
+		it 'should copy all the files', (done)->
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject
+					.calledWith(@stubbedNewProject, @stubbedNewProject.rootFolder[0]._id, @project._id, @rootFolder.fileRefs[0], @owner._id)
+					.should.equal true
+				@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject
+					.calledWith(@stubbedNewProject, @newFolder._id, @project._id, @level1folder.fileRefs[0], @owner._id)
+					.should.equal true
+				@ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject
+					.calledWith(@stubbedNewProject, @newFolder._id, @project._id, @level2folder.fileRefs[0], @owner._id)
+					.should.equal true
+				done()
+
+	describe 'when there is an error', ->
+		beforeEach ->
+			@rootFolder.fileRefs = [{name:"file0", _id:"file0"}, "BROKEN-FILE"]
+
+		it 'should delete the broken cloned project', (done) ->
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				@ProjectDeleter.deleteProject
+					.calledWith(@stubbedNewProject._id)
+					.should.equal true
+				done()
+
+		it 'should not delete the original project', (done) ->
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				@ProjectDeleter.deleteProject
+					.calledWith(@old_project_id)
+					.should.equal false
+				done()
+
+		it 'should return an error', (done) ->
+			@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
+				err.should.not.equal null
+				done()