Merge pull request #445 from sharelatex/bg-separate-uploads-and-locking

replace files and separate uploads and locking (connects to overleaf/sharelatex#422)

services/web/app/coffee/Features/DocumentUpdater/DocumentUpdaterHandler.coffee

@@ -174,6 +174,23 @@ module.exports = DocumentUpdaterHandler =
 		oldEntitiesHash = _.indexBy oldEntities, (entity) -> entity[entityType]._id.toString()
 		newEntitiesHash = _.indexBy newEntities, (entity) -> entity[entityType]._id.toString()
 
+		# Send deletes before adds (and renames) to keep a 1:1 mapping between
+		# paths and ids
+		#
+		# When a file is replaced, we first delete the old file and then add the
+		# new file. If the 'add' operation is sent to project history before the
+		# 'delete' then we would have two files with the same path at that point
+		# in time.
+		for id, oldEntity of oldEntitiesHash
+			newEntity = newEntitiesHash[id]
+
+			if !newEntity?
+				# entity deleted
+				updates.push
+					id: id
+					pathname: oldEntity.path
+					newPathname: ''
+
 		for id, newEntity of newEntitiesHash
 			oldEntity = oldEntitiesHash[id]
 
@@ -191,16 +208,6 @@ module.exports = DocumentUpdaterHandler =
 					pathname: oldEntity.path
 					newPathname: newEntity.path
 
-		for id, oldEntity of oldEntitiesHash
-			newEntity = newEntitiesHash[id]
-
-			if !newEntity?
-				# entity deleted
-				updates.push
-					id: id
-					pathname: oldEntity.path
-					newPathname: ''
-
 		updates
 
 PENDINGUPDATESKEY = "PendingUpdates"

services/web/app/coffee/Features/Editor/EditorController.coffee

@@ -41,11 +41,13 @@ module.exports = EditorController =
 			callback err, doc
 
 	upsertFile: (project_id, folder_id, fileName, fsPath, linkedFileData, source, user_id, callback = (err, file) ->) ->
-		ProjectEntityUpdateHandler.upsertFile project_id, folder_id, fileName, fsPath, linkedFileData, user_id, (err, file, didAddFile) ->
+		ProjectEntityUpdateHandler.upsertFile project_id, folder_id, fileName, fsPath, linkedFileData, user_id, (err, newFile, didAddFile, existingFile) ->
 			return callback(err) if err?
-			if didAddFile
-				EditorRealTimeController.emitToRoom project_id, 'reciveNewFile', folder_id, file, source, linkedFileData
-			callback null, file
+			if not didAddFile # replacement, so remove the existing file from the client
+				EditorRealTimeController.emitToRoom project_id, 'removeEntity', existingFile._id, source
+			# now add the new file on the client
+			EditorRealTimeController.emitToRoom project_id, 'reciveNewFile', folder_id, newFile, source, linkedFileData
+			callback null, newFile
 
 	upsertDocWithPath: (project_id, elementPath, docLines, source, user_id, callback) ->
 		ProjectEntityUpdateHandler.upsertDocWithPath project_id, elementPath, docLines, source, user_id, (err, doc, didAddNewDoc, newFolders, lastFolder) ->
@@ -57,12 +59,14 @@ module.exports = EditorController =
 				callback()
 
 	upsertFileWithPath: (project_id, elementPath, fsPath, linkedFileData, source, user_id, callback) ->
-		ProjectEntityUpdateHandler.upsertFileWithPath project_id, elementPath, fsPath, linkedFileData, user_id, (err, file, didAddFile, newFolders, lastFolder) ->
+		ProjectEntityUpdateHandler.upsertFileWithPath project_id, elementPath, fsPath, linkedFileData, user_id, (err, newFile, didAddFile, existingFile, newFolders, lastFolder) ->
 			return callback(err) if err?
 			EditorController._notifyProjectUsersOfNewFolders project_id, newFolders, (err) ->
 				return callback(err) if err?
-				if didAddFile
-					EditorRealTimeController.emitToRoom project_id, 'reciveNewFile', lastFolder._id, file, source, linkedFileData
+				if not didAddFile # replacement, so remove the existing file from the client
+					EditorRealTimeController.emitToRoom project_id, 'removeEntity', existingFile._id, source
+				# now add the new file on the client
+				EditorRealTimeController.emitToRoom project_id, 'reciveNewFile', lastFolder._id, newFile, source, linkedFileData
 				callback()
 
 	addFolder : (project_id, folder_id, folderName, source, callback = (error, folder)->)->

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

@@ -48,28 +48,28 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
 			self._confirmFolder project, folder_id, (folder_id)->
 				self._putElement project, folder_id, fileRef, "file", callback
 
-	replaceFile: wrapWithLock (project_id, file_id, linkedFileData, callback) ->
+	replaceFileWithNew: wrapWithLock (project_id, file_id, newFileRef, callback) ->
 		ProjectGetter.getProjectWithoutLock project_id, {rootFolder: true, name:true}, (err, project) ->
 			return callback(err) if err?
 			ProjectLocator.findElement {project:project, element_id: file_id, type: 'file'}, (err, fileRef, path)=>
 				return callback(err) if err?
-				conditions = _id:project._id
-				inc = {}
-				inc["#{path.mongo}.rev"] = 1
-				# currently we do not need to increment the project version number for changes that are replacements
-				# but when we make switch to having immutable files the replace operation will add a new file, and
-				# this will require a version increase.  We will start incrementing the project version now as it does
-				# no harm and will help to test it.
-				inc['version'] = 1
-				set = {}
-				set["#{path.mongo}.created"] = new Date()
-				set["#{path.mongo}.linkedFileData"] = linkedFileData
-				update =
-					"$inc": inc
-					"$set": set
-				Project.update conditions, update, {}, (err) ->
+				ProjectEntityMongoUpdateHandler._insertDeletedFileReference project_id, fileRef, (err) ->
 					return callback(err) if err?
-					callback null, fileRef, project, path
+					conditions = _id:project._id
+					inc = {}
+					# increment the project structure version as we are adding a new file here
+					inc['version'] = 1
+					set = {}
+					set["#{path.mongo}._id"] = newFileRef._id
+					set["#{path.mongo}.created"] = new Date()
+					set["#{path.mongo}.linkedFileData"] = newFileRef.linkedFileData
+					set["#{path.mongo}.rev"] = 1
+					update =
+						"$inc": inc
+						"$set": set
+					Project.update conditions, update, {}, (err) ->
+						return callback(err) if err?
+						callback null, fileRef, project, path
 
 	mkdirp: wrapWithLock (project_id, path, callback) ->
 		folders = path.split('/')
@@ -300,3 +300,29 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
 					if isNestedFolder
 						return callback(new Errors.InvalidNameError("destination folder is a child folder of me"))
 				callback()
+
+	_insertDeletedDocReference: (project_id, doc, callback = (error) ->) ->
+		Project.update {
+			_id: project_id
+		}, {
+			$push: {
+				deletedDocs: {
+					_id:  doc._id
+					name: doc.name
+				}
+			}
+		}, {}, callback
+
+	_insertDeletedFileReference: (project_id, fileRef, callback = (error) ->) ->
+		Project.update {
+			_id: project_id
+		}, {
+			$push: {
+				deletedFiles: {
+					_id:  fileRef._id
+					name: fileRef.name
+					linkedFileData: fileRef.linkedFileData
+					deletedAt: new Date()
+				}
+			}
+		}, {}, callback

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

@@ -24,12 +24,25 @@ wrapWithLock = (methodWithoutLock) ->
 	# This lock is used to make sure that the project structure updates are made
 	# sequentially. In particular the updates must be made in mongo and sent to
 	# the doc-updater in the same order.
-	methodWithLock = (project_id, args..., callback) ->
-		LockManager.runWithLock LOCK_NAMESPACE, project_id,
-			(cb) -> methodWithoutLock project_id, args..., cb
-			callback
-	methodWithLock.withoutLock = methodWithoutLock
-	methodWithLock
+	if typeof methodWithoutLock is 'function'
+		methodWithLock = (project_id, args..., callback) ->
+			LockManager.runWithLock LOCK_NAMESPACE, project_id,
+				(cb) -> methodWithoutLock project_id, args..., cb
+				callback
+		methodWithLock.withoutLock = methodWithoutLock
+		methodWithLock
+	else
+		# handle case with separate setup and locked stages
+		wrapWithSetup = methodWithoutLock.beforeLock  # a function to set things up before the lock
+		mainTask = methodWithoutLock.withLock # function to execute inside the lock
+		methodWithLock = wrapWithSetup (project_id, args..., callback) ->
+			LockManager.runWithLock(LOCK_NAMESPACE, project_id, (cb) ->
+				mainTask(project_id, args..., cb)
+			callback)
+		methodWithLock.withoutLock = wrapWithSetup mainTask
+		methodWithLock.beforeLock = methodWithoutLock.beforeLock
+		methodWithLock.mainTask = methodWithoutLock.withLock
+		methodWithLock
 
 module.exports = ProjectEntityUpdateHandler = self =
 	# this doesn't need any locking because it's only called by ProjectDuplicator
@@ -129,31 +142,72 @@ module.exports = ProjectEntityUpdateHandler = self =
 				return callback(error) if error?
 				callback null, doc, folder_id
 
-	addFile: wrapWithLock (project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback = (error, fileRef, folder_id) ->)->
-		self.addFileWithoutUpdatingHistory.withoutLock project_id, folder_id, fileName, fsPath, linkedFileData, userId, (error, fileRef, folder_id, path, fileStoreUrl) ->
-			return callback(error) if error?
-			newFiles = [
-				file: fileRef
-				path: path
-				url: fileStoreUrl
-			]
-			DocumentUpdaterHandler.updateProjectStructure project_id, userId, {newFiles}, (error) ->
-				return callback(error) if error?
-				callback null, fileRef, folder_id
+	_uploadFile: (project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback = (error, fileRef, fileStoreUrl) ->)->
+		if not SafePath.isCleanFilename fileName
+			return callback new Errors.InvalidNameError("invalid element name")
+		fileRef = new File(
+			name: fileName
+			linkedFileData: linkedFileData
+		)
+		FileStoreHandler.uploadFileFromDisk project_id, fileRef._id, fsPath, (err, fileStoreUrl)->
+			if err?
+				logger.err err:err, project_id: project_id, folder_id: folder_id, file_name: fileName, fileRef:fileRef, "error uploading image to s3"
+				return callback(err)
+			callback(null, fileRef, fileStoreUrl)
 
-	replaceFile: wrapWithLock (project_id, file_id, fsPath, linkedFileData, userId, callback)->
-		FileStoreHandler.uploadFileFromDisk project_id, file_id, fsPath, (err, fileStoreUrl)->
-			return callback(err) if err?
-			ProjectEntityMongoUpdateHandler.replaceFile project_id, file_id, linkedFileData, (err, fileRef, project, path) ->
+	_addFileAndSendToTpds: (project_id, folder_id, fileName, fileRef, callback = (error) ->)->
+		ProjectEntityMongoUpdateHandler.addFile project_id, folder_id, fileRef, (err, result, project) ->
+			if err?
+				logger.err err:err, project_id: project_id, folder_id: folder_id, file_name: fileName, fileRef:fileRef, "error adding file with project"
+				return callback(err)
+			TpdsUpdateSender.addFile {project_id:project_id, file_id:fileRef._id, path:result?.path?.fileSystem, project_name:project.name, rev:fileRef.rev}, (err) ->
+				return callback(err) if err?
+				callback(null, result, project)
+
+	addFile: wrapWithLock
+		beforeLock: (next) ->
+			(project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback) ->
+				ProjectEntityUpdateHandler._uploadFile project_id, folder_id, fileName, fsPath, linkedFileData, userId, (error, fileRef, fileStoreUrl) ->
+					return callback(error) if error?
+					next(project_id, folder_id, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback)
+		withLock: (project_id, folder_id, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback = (error, fileRef, folder_id) ->)->
+			ProjectEntityUpdateHandler._addFileAndSendToTpds project_id, folder_id, fileName, fileRef, (err, result, project) ->
 				return callback(err) if err?
 				newFiles = [
 					file: fileRef
+					path: result?.path?.fileSystem
+					url: fileStoreUrl
+				]
+				DocumentUpdaterHandler.updateProjectStructure project_id, userId, {newFiles}, (error) ->
+					return callback(error) if error?
+					callback(null, fileRef, folder_id)
+
+	replaceFile: wrapWithLock
+		beforeLock: (next) ->
+			(project_id, file_id, fsPath, linkedFileData, userId, callback)->
+				# create a new file
+				fileRef = new File(
+					name: "dummy-upload-filename"
+					linkedFileData: linkedFileData
+				)
+				FileStoreHandler.uploadFileFromDisk project_id, fileRef._id, fsPath, (err, fileStoreUrl)->
+					return callback(err) if err?
+					next project_id, file_id, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback
+		withLock: (project_id, file_id, fsPath, linkedFileData, userId, newFileRef, fileStoreUrl, callback)->
+			ProjectEntityMongoUpdateHandler.replaceFileWithNew project_id, file_id, newFileRef, (err, oldFileRef, project, path) ->
+				return callback(err) if err?
+				oldFiles = [
+					file: oldFileRef
+					path: path.fileSystem
+				]
+				newFiles = [
+					file: newFileRef
 					path: path.fileSystem
 					url: fileStoreUrl
 				]
-				TpdsUpdateSender.addFile {project_id:project._id, file_id:fileRef._id, path:path.fileSystem, rev:fileRef.rev+1, project_name:project.name}, (err) ->
+				TpdsUpdateSender.addFile {project_id:project._id, file_id:newFileRef._id, path:path.fileSystem, rev:newFileRef.rev+1, project_name:project.name}, (err) ->
 					return callback(err) if err?
-					DocumentUpdaterHandler.updateProjectStructure project_id, userId, {newFiles}, callback
+					DocumentUpdaterHandler.updateProjectStructure project_id, userId, {oldFiles, newFiles}, callback
 
 	addDocWithoutUpdatingHistory: wrapWithLock (project_id, folder_id, docName, docLines, userId, callback = (error, doc, folder_id) ->)=>
 		# This method should never be called directly, except when importing a project
@@ -180,29 +234,19 @@ module.exports = ProjectEntityUpdateHandler = self =
 					return callback(err) if err?
 					callback(null, doc, folder_id, result?.path?.fileSystem)
 
-	addFileWithoutUpdatingHistory: wrapWithLock (project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback = (error, fileRef, folder_id, path, fileStoreUrl) ->)->
+	addFileWithoutUpdatingHistory: wrapWithLock
 		# This method should never be called directly, except when importing a project
 		# from Overleaf. It skips sending updates to the project history, which will break
 		# the history unless you are making sure it is updated in some other way.
-
-		if not SafePath.isCleanFilename fileName
-			return callback new Errors.InvalidNameError("invalid element name")
-
-		fileRef = new File(
-			name: fileName
-			linkedFileData: linkedFileData
-		)
-		FileStoreHandler.uploadFileFromDisk project_id, fileRef._id, fsPath, (err, fileStoreUrl)->
-			if err?
-				logger.err err:err, project_id: project_id, folder_id: folder_id, file_name: fileName, fileRef:fileRef, "error uploading image to s3"
-				return callback(err)
-			ProjectEntityMongoUpdateHandler.addFile project_id, folder_id, fileRef, (err, result, project) ->
-				if err?
-					logger.err err:err, project_id: project_id, folder_id: folder_id, file_name: fileName, fileRef:fileRef, "error adding file with project"
-					return callback(err)
-				TpdsUpdateSender.addFile {project_id:project_id, file_id:fileRef._id, path:result?.path?.fileSystem, project_name:project.name, rev:fileRef.rev}, (err) ->
-					return callback(err) if err?
-					callback(null, fileRef, folder_id, result?.path?.fileSystem, fileStoreUrl)
+		beforeLock: (next) ->
+			(project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback) ->
+				ProjectEntityUpdateHandler._uploadFile project_id, folder_id, fileName, fsPath, linkedFileData, userId, (error, fileRef, fileStoreUrl) ->
+					return callback(error) if error?
+					next(project_id, folder_id, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback)
+		withLock: (project_id, folder_id, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback = (error, fileRef, folder_id, path, fileStoreUrl) ->)->
+			ProjectEntityUpdateHandler._addFileAndSendToTpds project_id, folder_id, fileName, fileRef, (err, result, project) ->
+				return callback(err) if err?
+				callback(null, fileRef, folder_id, result?.path?.fileSystem, fileStoreUrl)
 
 	upsertDoc: wrapWithLock (project_id, folder_id, docName, docLines, source, userId, callback = (err, doc, folder_id, isNewDoc)->)->
 		ProjectLocator.findElement project_id: project_id, element_id: folder_id, type: "folder", (error, folder) ->
@@ -224,23 +268,36 @@ module.exports = ProjectEntityUpdateHandler = self =
 					return callback(err) if err?
 					callback null, doc, !existingDoc?
 
-	upsertFile: wrapWithLock (project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback = (err, file, isNewFile)->)->
-		ProjectLocator.findElement project_id: project_id, element_id: folder_id, type: "folder", (error, folder) ->
-			return callback(error) if error?
-			return callback(new Error("Couldn't find folder")) if !folder?
-			existingFile = null
-			for fileRef in folder.fileRefs
-				if fileRef.name == fileName
-					existingFile = fileRef
-					break
-			if existingFile?
-				self.replaceFile.withoutLock project_id, existingFile._id, fsPath, linkedFileData, userId, (err) ->
-					return callback(err) if err?
-					callback null, existingFile, !existingFile?
-			else
-				self.addFile.withoutLock project_id, folder_id, fileName, fsPath, linkedFileData, userId, (err, file) ->
+	upsertFile: wrapWithLock
+		beforeLock: (next) ->
+			(project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback)->
+				# create a new file
+				fileRef = new File(
+					name: fileName
+					linkedFileData: linkedFileData
+				)
+				FileStoreHandler.uploadFileFromDisk project_id, fileRef._id, fsPath, (err, fileStoreUrl)->
 					return callback(err) if err?
-					callback null, file, !existingFile?
+					next(project_id, folder_id, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback)
+		withLock: (project_id, folder_id, fileName, fsPath, linkedFileData, userId, newFileRef, fileStoreUrl, callback = (err, file, isNewFile, existingFile)->)->
+			ProjectLocator.findElement project_id: project_id, element_id: folder_id, type: "folder", (error, folder) ->
+				return callback(error) if error?
+				return callback(new Error("Couldn't find folder")) if !folder?
+				existingFile = null
+				for fileRef in folder.fileRefs
+					if fileRef.name == fileName
+						existingFile = fileRef
+						break
+				if existingFile?
+					# this calls directly into the replaceFile main task (without the beforeLock part)
+					self.replaceFile.mainTask project_id, existingFile._id, fsPath, linkedFileData, userId, newFileRef, fileStoreUrl, (err) ->
+						return callback(err) if err?
+						callback null, newFileRef, !existingFile?, existingFile
+				else
+					# this calls directly into the addFile main task (without the beforeLock part)
+					self.addFile.mainTask project_id, folder_id, fileName, fsPath, linkedFileData, userId, newFileRef, fileStoreUrl, (err) ->
+						return callback(err) if err?
+						callback null, newFileRef, !existingFile?, existingFile
 
 	upsertDocWithPath: wrapWithLock (project_id, elementPath, docLines, source, userId, callback) ->
 		docName = path.basename(elementPath)
@@ -251,14 +308,26 @@ module.exports = ProjectEntityUpdateHandler = self =
 				return callback(err) if err?
 				callback null, doc, isNewDoc, newFolders, folder
 
-	upsertFileWithPath: wrapWithLock (project_id, elementPath, fsPath, linkedFileData, userId, callback) ->
-		fileName = path.basename(elementPath)
-		folderPath = path.dirname(elementPath)
-		self.mkdirp.withoutLock project_id, folderPath, (err, newFolders, folder) ->
-			return callback(err) if err?
-			self.upsertFile.withoutLock project_id, folder._id, fileName, fsPath, linkedFileData, userId, (err, file, isNewFile) ->
+	upsertFileWithPath: wrapWithLock
+		beforeLock: (next) ->
+			(project_id, elementPath, fsPath, linkedFileData, userId, callback)->
+				fileName = path.basename(elementPath)
+				folderPath = path.dirname(elementPath)
+				# create a new file
+				fileRef = new File(
+					name: fileName
+					linkedFileData: linkedFileData
+				)
+				FileStoreHandler.uploadFileFromDisk project_id, fileRef._id, fsPath, (err, fileStoreUrl)->
+					return callback(err) if err?
+					next project_id, folderPath, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback
+		withLock: (project_id, folderPath, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback) ->
+			self.mkdirp.withoutLock project_id, folderPath, (err, newFolders, folder) ->
 				return callback(err) if err?
-				callback null, file, isNewFile, newFolders, folder
+				# this calls directly into the upsertFile main task (without the beforeLock part)
+				self.upsertFile.mainTask project_id, folder._id, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, (err, newFile, isNewFile, existingFile) ->
+					return callback(err) if err?
+					callback null, newFile, isNewFile, existingFile, newFolders, folder
 
 	deleteEntity: wrapWithLock (project_id, entity_id, entityType, userId, callback = (error) ->)->
 		logger.log entity_id:entity_id, entityType:entityType, project_id:project_id, "deleting project entity"
@@ -335,7 +404,7 @@ module.exports = ProjectEntityUpdateHandler = self =
 					path: file.path
 					url: FileStoreHandler._buildUrl(project_id, file.file._id)
 
-				DocumentUpdaterHandler.resyncProjectHistory project_id, docs, files, callback 
+				DocumentUpdaterHandler.resyncProjectHistory project_id, docs, files, callback
 	_cleanUpEntity: (project, entity, entityType, path, userId, callback = (error) ->) ->
 		if(entityType.indexOf("file") != -1)
 			self._cleanUpFile project, entity, path, userId, callback
@@ -357,7 +426,7 @@ module.exports = ProjectEntityUpdateHandler = self =
 
 		unsetRootDocIfRequired (error) ->
 			return callback(error) if error?
-			self._insertDeletedDocReference project._id, doc, (error) ->
+			ProjectEntityMongoUpdateHandler._insertDeletedDocReference project._id, doc, (error) ->
 				return callback(error) if error?
 				DocumentUpdaterHandler.deleteDoc project_id, doc_id, (error) ->
 					return callback(error) if error?
@@ -367,11 +436,12 @@ module.exports = ProjectEntityUpdateHandler = self =
 						DocumentUpdaterHandler.updateProjectStructure project_id, userId, changes, callback
 
 	_cleanUpFile: (project, file, path, userId, callback = (error) ->) ->
-		project_id = project._id.toString()
-		file_id = file._id.toString()
-		FileStoreHandler.deleteFile project_id, file_id, (error) ->
+		ProjectEntityMongoUpdateHandler._insertDeletedFileReference project._id, file, (error) ->
 			return callback(error) if error?
+			project_id = project._id.toString()
 			changes = oldFiles: [ {file, path} ]
+			# we are now keeping a copy of every file versio so we no longer delete
+			# the file from the filestore
 			DocumentUpdaterHandler.updateProjectStructure project_id, userId, changes, callback
 
 	_cleanUpFolder: (project, folder, folderPath, userId, callback = (error) ->) ->
@@ -392,15 +462,3 @@ module.exports = ProjectEntityUpdateHandler = self =
 				jobs.push (callback) -> self._cleanUpFolder project, childFolder, folderPath, userId, callback
 
 		async.series jobs, callback
-
-	_insertDeletedDocReference: (project_id, doc, callback = (error) ->) ->
-		Project.update {
-			_id: project_id
-		}, {
-			$push: {
-				deletedDocs: {
-					_id:  doc._id
-					name: doc.name
-				}
-			}
-		}, {}, callback

services/web/app/coffee/models/Project.coffee

@@ -12,6 +12,10 @@ ObjectId = Schema.ObjectId
 DeletedDocSchema = new Schema
 	name: String
 
+DeletedFileSchema = new Schema
+	name: String
+	deletedAt: {type: Date}
+
 ProjectSchema = new Schema
 	name              :   {type:String, default:'new project'}
 	lastUpdated       :   {type:Date, default: () -> new Date()}
@@ -30,6 +34,7 @@ ProjectSchema = new Schema
 	description : {type:String, default:''}
 	archived          : { type: Boolean }
 	deletedDocs       : [DeletedDocSchema]
+	deletedFiles      : [DeletedFileSchema]
 	imageName         : { type: String }
 	track_changes     : { type: Object }
 	tokens            :

services/web/test/acceptance/coffee/ProjectDuplicateNameTests.coffee

@@ -186,10 +186,13 @@ describe "ProjectDuplicateNames", ->
 									contentType: 'image/jpeg'
 					, (err, res, body) =>
 						@body = body
+						# update the image id because we have replaced the file
+						@imageFile._id = @body.entity_id
 						done()
 
 				it "should succeed (overwriting the file)", ->
 					expect(@body.success).to.equal true
+				# at this point the @imageFile._id has changed
 
 		describe "for an existing folder", ->
 			describe "trying to add a doc with the same name", ->

services/web/test/acceptance/coffee/ProjectStructureTests.coffee

@@ -56,7 +56,6 @@ describe "ProjectStructureChanges", ->
 	describe "duplicating a project", ->
 		before (done) ->
 			MockDocUpdaterApi.clearProjectStructureUpdates()
-			console.log(example_project_id)
 			@owner.request.post {
 				uri: "/Project/#{example_project_id}/clone",
 				json:
@@ -219,11 +218,17 @@ describe "ProjectStructureChanges", ->
 				if res.statusCode < 200 || res.statusCode >= 300
 					throw new Error("failed to upload file #{res.statusCode}")
 
+				example_file_id = JSON.parse(body).entity_id
+
 				{fileUpdates:updates, version} = MockDocUpdaterApi.getProjectStructureUpdates(example_project_id)
-				expect(updates.length).to.equal(1)
+				expect(updates.length).to.equal(2)
 				update = updates[0]
 				expect(update.userId).to.equal(@owner._id)
 				expect(update.pathname).to.equal("/1pixel.png")
+				#expect(update.url).to.be.a('string');
+				update = updates[1]
+				expect(update.userId).to.equal(@owner._id)
+				expect(update.pathname).to.equal("/1pixel.png")
 				expect(update.url).to.be.a('string');
 				expect(version).to.equal(@project_0.version + 1)
 
@@ -591,10 +596,14 @@ describe "ProjectStructureChanges", ->
 					throw new Error("failed to upload file #{res.statusCode}")
 
 				{fileUpdates:updates, version} = MockDocUpdaterApi.getProjectStructureUpdates(@tpds_project_id)
-				expect(updates.length).to.equal(1)
+				expect(updates.length).to.equal(2)
 				update = updates[0]
 				expect(update.userId).to.equal(@owner._id)
 				expect(update.pathname).to.equal("/1pixel.png")
+				#expect(update.url).to.be.a('string');
+				update = updates[1]
+				expect(update.userId).to.equal(@owner._id)
+				expect(update.pathname).to.equal("/1pixel.png")
 				expect(update.url).to.be.a('string');
 				expect(version).to.equal(@project_0.version + 1)
 

services/web/test/unit/coffee/Editor/EditorControllerTests.coffee

@@ -20,6 +20,8 @@ describe "EditorController", ->
 		@fsPath = "/folder/file.png"
 		@linkedFileData = {provider: 'url'}
 
+		@newFile = _id: "new-file-id"
+
 		@folder_id = "123ksajdn"
 		@folder = _id: @folder_id
 		@folderName = "folder"
@@ -107,7 +109,7 @@ describe "EditorController", ->
 
 	describe 'upsertFile', ->
 		beforeEach ->
-			@ProjectEntityUpdateHandler.upsertFile = sinon.stub().yields(null, @file, false)
+			@ProjectEntityUpdateHandler.upsertFile = sinon.stub().yields(null, @newFile, false, @file)
 			@EditorController.upsertFile @project_id, @folder_id, @fileName, @fsPath, @linkedFileData, @source, @user_id, @callback
 
 		it 'upserts the file using the project entity handler', ->
@@ -116,7 +118,7 @@ describe "EditorController", ->
 				.should.equal true
 
 		it 'returns the file', ->
-			@callback.calledWith(null, @file).should.equal true
+			@callback.calledWith(null, @newFile).should.equal true
 
 		describe 'file does not exist', ->
 			beforeEach ->
@@ -171,7 +173,7 @@ describe "EditorController", ->
 		beforeEach ->
 			@filePath = '/folder/file'
 
-			@ProjectEntityUpdateHandler.upsertFileWithPath = sinon.stub().yields(null, @file, false, [], @folder)
+			@ProjectEntityUpdateHandler.upsertFileWithPath = sinon.stub().yields(null, @newFile, false, @file, [], @folder)
 			@EditorController.upsertFileWithPath @project_id, @filePath, @fsPath, @linkedFileData, @source, @user_id, @callback
 
 		it 'upserts the file using the project entity handler', ->
@@ -181,7 +183,7 @@ describe "EditorController", ->
 
 		describe 'file does not exist', ->
 			beforeEach ->
-				@ProjectEntityUpdateHandler.upsertFileWithPath = sinon.stub().yields(null, @file, true, [], @folder)
+				@ProjectEntityUpdateHandler.upsertFileWithPath = sinon.stub().yields(null, @file, true, undefined, [], @folder)
 				@EditorController.upsertFileWithPath @project_id, @filePath, @fsPath, @linkedFileData, @source, @user_id, @callback
 
 			it 'should send the update for the file out to users in the project', ->
@@ -195,7 +197,7 @@ describe "EditorController", ->
 					@folderA = { _id: 2, parentFolder_id: 1}
 					@folderB = { _id: 3, parentFolder_id: 2}
 				]
-				@ProjectEntityUpdateHandler.upsertFileWithPath = sinon.stub().yields(null, @file, true, folders, @folderB)
+				@ProjectEntityUpdateHandler.upsertFileWithPath = sinon.stub().yields(null, @file, true, undefined, folders, @folderB)
 				@EditorController.upsertFileWithPath @project_id, @filePath, @fsPath, @linkedFileData, @source, @user_id, @callback
 
 			it 'should send the update for each folder to users in the project', ->

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

@@ -93,33 +93,50 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 				.calledWith(@project, folder_id, @file, 'file', @callback)
 				.should.equal true
 
-	describe 'replaceFile', ->
+	describe 'replaceFileWithNew', ->
 		beforeEach ->
 			@file = _id: file_id
 			@path = mongo: 'file.png'
-			@linkedFileData = {provider: 'url'}
+			@newFile = _id: 'new-file-id'
+			@newFile.linkedFileData = @linkedFileData = {provider: 'url'}
 			@ProjectLocator.findElement = sinon.stub().yields(null, @file, @path)
 			@ProjectModel.update = sinon.stub().yields()
 
-			@subject.replaceFile project_id, file_id, @linkedFileData, @callback
+			@subject.replaceFileWithNew project_id, file_id, @newFile, @callback
 
 		it 'gets the project', ->
 			@ProjectGetter.getProjectWithoutLock
 				.calledWith(project_id, {rootFolder:true, name: true})
 				.should.equal true
 
-		it 'finds the element', ->
+		it 'finds the existing element', ->
 			@ProjectLocator.findElement
 				.calledWith({ @project, element_id: file_id, type: 'file' })
 				.should.equal true
 
-		it 'increments the rev and sets the created_at', ->
+		it 'inserts a deletedFile reference for the old file', ->
+			@ProjectModel.update
+				.calledWith({ _id: project_id },
+					{
+						$push: {
+							deletedFiles: {
+								_id:  file_id
+								name: @file.name
+								linkedFileData: @file.linkedFileData
+								deletedAt: new Date()
+							}
+						}
+					}
+				)
+				.should.equal true
+
+		it 'increments the project version and sets the rev and created_at', ->
 			@ProjectModel.update
 				.calledWith(
 					{ _id: project_id },
 					{
-						'$inc': { 'file.png.rev': 1, 'version': 1 }
-						'$set': { 'file.png.created': new Date(), 'file.png.linkedFileData': @linkedFileData }
+						'$inc': { 'version': 1 }
+						'$set': { 'file.png._id': @newFile._id, 'file.png.created': new Date(), 'file.png.linkedFileData': @linkedFileData, 'file.png.rev': 1 }
 					}
 					{}
 				)
@@ -596,3 +613,29 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 			folder = name: 'folder_name'
 			@subject._checkValidMove @project, 'folder', folder, fileSystem: '/foo', @destFolder._id, (err) =>
 				expect(err).to.deep.equal new Errors.InvalidNameError("destination folder is a child folder of me")
+
+	describe "_insertDeletedDocReference", ->
+		beforeEach ->
+			@doc =
+				_id: ObjectId()
+				name: "test.tex"
+			@callback = sinon.stub()
+			@ProjectModel.update = sinon.stub().yields()
+			@subject._insertDeletedDocReference project_id, @doc, @callback
+
+		it "should insert the doc into deletedDocs", ->
+			@ProjectModel.update
+				.calledWith({
+					_id: project_id
+				}, {
+					$push: {
+						deletedDocs: {
+							_id: @doc._id
+							name: @doc.name
+						}
+					}
+				})
+				.should.equal true
+
+		it "should call the callback", ->
+			@callback.called.should.equal true

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

@@ -14,6 +14,7 @@ describe 'ProjectEntityUpdateHandler', ->
 	file_id = "4eecaffcbffa66588e000009"
 	folder_id = "4eecaffcbffa66588e000008"
 	rootFolderId = "4eecaffcbffa66588e000007"
+	new_file_id = "4eecaffcbffa66588e000099"
 	userId = 1234
 
 	beforeEach ->
@@ -31,7 +32,11 @@ describe 'ProjectEntityUpdateHandler', ->
 		@FileModel = class File
 			constructor:(options)->
 				{@name} = options
-				@._id = file_id
+				# use a new id for replacement files
+				if @name is 'dummy-upload-filename'
+					@._id = new_file_id
+				else
+					@._id = file_id
 				@rev = 0
 
 		@docName = "doc-name"
@@ -295,14 +300,33 @@ describe 'ProjectEntityUpdateHandler', ->
 		beforeEach ->
 			@path = "/path/to/file"
 
-			@newFile = _id: file_id
-			@ProjectEntityUpdateHandler.addFileWithoutUpdatingHistory =
-				withoutLock: sinon.stub().yields(null, @newFile, folder_id, @path, @fileUrl)
-			@ProjectEntityUpdateHandler.addFile project_id, folder_id, @docName, @fileSystemPath, @linkedFileData, userId, @callback
+			@newFile = {_id: file_id, rev: 0, name: @fileName}
+			@TpdsUpdateSender.addFile = sinon.stub().yields()
+			@ProjectEntityMongoUpdateHandler.addFile = sinon.stub().yields(null, {path: fileSystem: @path}, @project)
+			@ProjectEntityUpdateHandler.addFile project_id, folder_id, @fileName, @fileSystemPath, @linkedFileData, userId, @callback
 
-		it "creates the doc without history", () ->
-			@ProjectEntityUpdateHandler.addFileWithoutUpdatingHistory.withoutLock
-				.calledWith(project_id, folder_id, @docName, @fileSystemPath, @linkedFileData, userId)
+		it "updates the file in the filestore", () ->
+			@FileStoreHandler.uploadFileFromDisk
+				.calledWith(project_id, file_id, @fileSystemPath)
+				.should.equal true
+
+		it "updates the file in mongo", () ->
+			fileMatcher = sinon.match (file) =>
+				file.name == @fileName
+
+			@ProjectEntityMongoUpdateHandler.addFile
+				.calledWithMatch(project_id, folder_id, fileMatcher)
+				.should.equal true
+
+		it "notifies the tpds", () ->
+			@TpdsUpdateSender.addFile
+				.calledWith({
+					project_id: project_id
+					project_name: @project.name
+					file_id: file_id
+					rev: 0
+					path: @path
+				})
 				.should.equal true
 
 		it "sends the change in project structure to the doc updater", () ->
@@ -317,21 +341,26 @@ describe 'ProjectEntityUpdateHandler', ->
 
 	describe 'replaceFile', ->
 		beforeEach ->
-			@newFile = _id: file_id, rev: 0
+			# replacement file now creates a new file object
+			@newFileUrl = "new-file-url"
+			@FileStoreHandler.uploadFileFromDisk = sinon.stub().yields(null, @newFileUrl)
+
+			@newFile = _id: new_file_id, name: "dummy-upload-filename", rev: 0
+			@oldFile = _id: file_id
 			@path = "/path/to/file"
 			@project = _id: project_id, name: 'some project'
-			@ProjectEntityMongoUpdateHandler.replaceFile = sinon.stub().yields(null, @newFile, @project, fileSystem: @path)
-
+			@ProjectEntityMongoUpdateHandler._insertDeletedFileReference = sinon.stub().yields()
+			@ProjectEntityMongoUpdateHandler.replaceFileWithNew = sinon.stub().yields(null, @oldFile, @project, fileSystem: @path)
 			@ProjectEntityUpdateHandler.replaceFile project_id, file_id, @fileSystemPath, @linkedFileData, userId, @callback
 
 		it 'uploads a new version of the file', ->
 			@FileStoreHandler.uploadFileFromDisk
-				.calledWith(project_id, file_id, @fileSystemPath)
+				.calledWith(project_id, new_file_id, @fileSystemPath)
 				.should.equal true
 
 		it 'replaces the file in mongo', ->
-			@ProjectEntityMongoUpdateHandler.replaceFile
-				.calledWith(project_id, file_id, @linkedFileData)
+			@ProjectEntityMongoUpdateHandler.replaceFileWithNew
+				.calledWith(project_id, file_id, @newFile)
 				.should.equal true
 
 		it 'notifies the tpds', ->
@@ -339,20 +368,24 @@ describe 'ProjectEntityUpdateHandler', ->
 				.calledWith({
 					project_id: project_id
 					project_name: @project.name
-					file_id: file_id
+					file_id: new_file_id
 					rev: @newFile.rev + 1
 					path: @path
 				})
 				.should.equal true
 
 		it 'updates the project structure in the doc updater', ->
+			oldFiles = [
+				file: @oldFile
+				path: @path
+			]
 			newFiles = [
 				file: @newFile
 				path: @path
-				url: @fileUrl
+				url: @newFileUrl
 			]
 			@DocumentUpdaterHandler.updateProjectStructure
-				.calledWith(project_id, userId, {newFiles})
+				.calledWith(project_id, userId, {oldFiles, newFiles})
 				.should.equal true
 
 	describe 'addDocWithoutUpdatingHistory', ->
@@ -511,12 +544,12 @@ describe 'ProjectEntityUpdateHandler', ->
 				@existingFile = _id: file_id, name: @fileName
 				@folder = _id: folder_id, fileRefs: [@existingFile]
 				@ProjectLocator.findElement = sinon.stub().yields(null, @folder)
-				@ProjectEntityUpdateHandler.replaceFile = withoutLock: sinon.stub().yields(null, @newFile)
+				@ProjectEntityUpdateHandler.replaceFile = mainTask: sinon.stub().yields(null, @newFile)
 
 				@ProjectEntityUpdateHandler.upsertFile project_id, folder_id, @fileName, @fileSystemPath, @linkedFileData, userId, @callback
 
 			it 'replaces the file', ->
-				@ProjectEntityUpdateHandler.replaceFile.withoutLock
+				@ProjectEntityUpdateHandler.replaceFile.mainTask
 					.calledWith(project_id, file_id, @fileSystemPath, @linkedFileData, userId)
 					.should.equal true
 
@@ -528,7 +561,7 @@ describe 'ProjectEntityUpdateHandler', ->
 				@folder = _id: folder_id, fileRefs: []
 				@newFile = _id: file_id
 				@ProjectLocator.findElement = sinon.stub().yields(null, @folder)
-				@ProjectEntityUpdateHandler.addFile = withoutLock: sinon.stub().yields(null, @newFile)
+				@ProjectEntityUpdateHandler.addFile = mainTask: sinon.stub().yields(null, @newFile)
 
 				@ProjectEntityUpdateHandler.upsertFile project_id, folder_id, @fileName, @fileSystemPath, @linkedFileData, userId, @callback
 
@@ -538,7 +571,7 @@ describe 'ProjectEntityUpdateHandler', ->
 					.should.equal true
 
 			it 'adds the file', ->
-				@ProjectEntityUpdateHandler.addFile.withoutLock
+				@ProjectEntityUpdateHandler.addFile.mainTask
 					.calledWith(project_id, folder_id, @fileName, @fileSystemPath, @linkedFileData, userId)
 					.should.equal true
 
@@ -584,7 +617,7 @@ describe 'ProjectEntityUpdateHandler', ->
 			@ProjectEntityUpdateHandler.mkdirp =
 				withoutLock: sinon.stub().yields(null, @newFolders, @folder)
 			@ProjectEntityUpdateHandler.upsertFile =
-				withoutLock: sinon.stub().yields(null, @file, @isNewFile)
+				mainTask: sinon.stub().yields(null, @file, @isNewFile)
 
 			@ProjectEntityUpdateHandler.upsertFileWithPath project_id, @path, @fileSystemPath, @linkedFileData, userId, @callback
 
@@ -594,13 +627,13 @@ describe 'ProjectEntityUpdateHandler', ->
 				.should.equal true
 
 		it 'upserts the file', ->
-			@ProjectEntityUpdateHandler.upsertFile.withoutLock
+			@ProjectEntityUpdateHandler.upsertFile.mainTask
 				.calledWith(project_id, @folder._id, 'file.png', @fileSystemPath, @linkedFileData, userId)
 				.should.equal true
 
 		it 'calls the callback', ->
 			@callback
-				.calledWith(null, @file, @isNewFile, @newFolders, @folder)
+				.calledWith(null, @file, @isNewFile, undefined, @newFolders, @folder)
 				.should.equal true
 
 	describe 'deleteEntity', ->
@@ -819,6 +852,7 @@ describe 'ProjectEntityUpdateHandler', ->
 			@FileStoreHandler.deleteFile = sinon.stub().yields()
 			@DocumentUpdaterHandler.deleteDoc = sinon.stub().yields()
 			@ProjectEntityUpdateHandler.unsetRootDoc = sinon.stub().yields()
+			@ProjectEntityMongoUpdateHandler._insertDeletedFileReference = sinon.stub().yields()
 
 		describe "a file", ->
 			beforeEach (done) ->
@@ -826,8 +860,13 @@ describe 'ProjectEntityUpdateHandler', ->
 				@entity = _id: @entity_id
 				@ProjectEntityUpdateHandler._cleanUpEntity @project, @entity, 'file', @path, userId, done
 
-			it "should delete the file from FileStoreHandler", ->
-				@FileStoreHandler.deleteFile.calledWith(project_id, @entity_id).should.equal true
+			it "should insert the file into the deletedFiles array", ->
+				@ProjectEntityMongoUpdateHandler._insertDeletedFileReference
+					.calledWith(@project._id, @entity)
+					.should.equal true
+
+			it "should not delete the file from FileStoreHandler", ->
+				@FileStoreHandler.deleteFile.calledWith(project_id, @entity_id).should.equal false
 
 			it "should not attempt to delete from the document updater", ->
 				@DocumentUpdaterHandler.deleteDoc.called.should.equal false
@@ -892,7 +931,7 @@ describe 'ProjectEntityUpdateHandler', ->
 				name: "test.tex"
 			@path = "/path/to/doc"
 			@ProjectEntityUpdateHandler.unsetRootDoc = sinon.stub().yields()
-			@ProjectEntityUpdateHandler._insertDeletedDocReference = sinon.stub().yields()
+			@ProjectEntityMongoUpdateHandler._insertDeletedDocReference = sinon.stub().yields()
 			@DocumentUpdaterHandler.deleteDoc = sinon.stub().yields()
 			@DocstoreManager.deleteDoc = sinon.stub().yields()
 			@callback = sinon.stub()
@@ -912,7 +951,7 @@ describe 'ProjectEntityUpdateHandler', ->
 					.calledWith(project_id, @doc._id.toString())
 
 			it "should insert the doc into the deletedDocs array", ->
-				@ProjectEntityUpdateHandler._insertDeletedDocReference
+				@ProjectEntityMongoUpdateHandler._insertDeletedDocReference
 					.calledWith(@project._id, @doc)
 					.should.equal true
 
@@ -941,28 +980,4 @@ describe 'ProjectEntityUpdateHandler', ->
 			it "should call the callback", ->
 				@callback.called.should.equal true
 
-	describe "_insertDeletedDocReference", ->
-		beforeEach ->
-			@doc =
-				_id: ObjectId()
-				name: "test.tex"
-			@callback = sinon.stub()
-			@ProjectModel.update = sinon.stub().yields()
-			@ProjectEntityUpdateHandler._insertDeletedDocReference project_id, @doc, @callback
-
-		it "should insert the doc into deletedDocs", ->
-			@ProjectModel.update
-				.calledWith({
-					_id: project_id
-				}, {
-					$push: {
-						deletedDocs: {
-							_id: @doc._id
-							name: @doc.name
-						}
-					}
-				})
-				.should.equal true
-
-		it "should call the callback", ->
-			@callback.called.should.equal true
+