Merge pull request #626 from sharelatex/bg-use-atomic-project-structure-version

use atomic project structure version

GitOrigin-RevId: ac61039a63af3e94fe842031e90e0aac802ff2b8

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

@@ -138,9 +138,21 @@ module.exports = DocumentUpdaterHandler =
 
 			docUpdates = DocumentUpdaterHandler._getUpdates('doc', changes.oldDocs, changes.newDocs)
 			fileUpdates = DocumentUpdaterHandler._getUpdates('file', changes.oldFiles, changes.newFiles)
+			projectVersion = changes?.newProject?.version
 
 			return callback() if (docUpdates.length + fileUpdates.length) < 1
 
+			# FIXME: remove this check and the request to get the project structure version above
+			# when we are confident in the use of $inc to increment the project structure version
+			# in all cases.
+			if projectVersion? && currentProject.version == projectVersion
+				logger.log {project_id, projectVersion}, "got project version in changes"
+			else if projectVersion? && currentProject.version != projectVersion
+				logger.error {project_id, changes, projectVersion, currentProject: currentProject.version}, "project version from db was different from changes (broken lock?)"
+			else
+				projectVersion = currentProject.version
+				logger.warn {project_id, changes, projectVersion}, "did not receive project version in changes"
+
 			logger.log {project_id}, "updating project structure in doc updater"
 			DocumentUpdaterHandler._makeRequest {
 				path: "/project/#{project_id}"
@@ -148,7 +160,7 @@ module.exports = DocumentUpdaterHandler =
 					docUpdates,
 					fileUpdates,
 					userId,
-					version: currentProject.version
+					version: projectVersion
 					projectHistoryId
 				}
 				method: "POST"

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

@@ -67,9 +67,15 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
 					update =
 						"$inc": inc
 						"$set": set
-					Project.update conditions, update, {}, (err) ->
+					# Note: Mongoose uses new:true to return the modified document
+					# https://mongoosejs.com/docs/api.html#model_Model.findOneAndUpdate
+					# but Mongo uses returnNewDocument:true instead
+					# https://docs.mongodb.com/manual/reference/method/db.collection.findOneAndUpdate/
+					# We are using Mongoose here, but if we ever switch to a direct mongo call
+					# the next line will need to be updated.
+					Project.findOneAndUpdate conditions, update, {new:true}, (err, newProject) ->
 						return callback(err) if err?
-						callback null, fileRef, project, path
+						callback null, fileRef, project, path, newProject
 
 	mkdirp: wrapWithLock (project_id, path, options, callback) ->
 		# defaults to case insensitive paths, use options {exactCaseMatch:true}
@@ -128,7 +134,7 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
 									return callback(err) if err?
 									startPath = entityPath.fileSystem
 									endPath = result.path.fileSystem
-									changes = {oldDocs, newDocs, oldFiles, newFiles}
+									changes = {oldDocs, newDocs, oldFiles, newFiles, newProject}
 									callback null, project, startPath, endPath, entity.rev, changes, callback
 
 	deleteEntity: wrapWithLock (project_id, entity_id, entityType, callback) ->
@@ -136,9 +142,9 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
 			return callback(error) if error?
 			ProjectLocator.findElement {project: project, element_id: entity_id, type: entityType}, (error, entity, path) ->
 				return callback(error) if error?
-				self._removeElementFromMongoArray Project, project_id, path.mongo, (error) ->
+				self._removeElementFromMongoArray Project, project_id, path.mongo, (error, newProject) ->
 					return callback(error) if error?
-					callback null, entity, path, project
+					callback null, entity, path, project, newProject
 
 	renameEntity: wrapWithLock (project_id, entity_id, entityType, newName, callback) ->
 		ProjectGetter.getProjectWithoutLock project_id, {rootFolder:true, name:true, overleaf:true}, (error, project)=>
@@ -162,7 +168,7 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
 							ProjectEntityHandler.getAllEntitiesFromProject newProject, (error, newDocs, newFiles) =>
 								return callback(error) if error?
 								startPath = entPath.fileSystem
-								changes = {oldDocs, newDocs, oldFiles, newFiles}
+								changes = {oldDocs, newDocs, oldFiles, newFiles, newProject}
 								callback null, project, startPath, endPath, entity.rev, changes, callback
 
 	addFolder: wrapWithLock (project_id, parentFolder_id, folderName, callback) ->
@@ -262,6 +268,8 @@ module.exports = ProjectEntityMongoUpdateHandler = self =
 				# we need to increment the project version number for any structure change
 				update["$inc"]["version"] = 1 # increment project version number
 				logger.log project_id: project._id, element_id: element._id, fileType: type, folder_id: folder_id, mongopath:mongopath, "adding element to project"
+				# We are using Mongoose here, but if we ever switch to a direct mongo call
+				# the next line will need to be updated to {returnNewDocument:true}
 				Project.findOneAndUpdate conditions, update, {"new": true}, (err, newProject)->
 					if err?
 						logger.err err: err, project_id: project._id, 'error saving in putElement project'

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

@@ -162,7 +162,7 @@ module.exports = ProjectEntityUpdateHandler = self =
 					path: docPath
 					docLines: docLines.join('\n')
 				]
-				DocumentUpdaterHandler.updateProjectStructure project_id, projectHistoryId, userId, {newDocs}, (error) ->
+				DocumentUpdaterHandler.updateProjectStructure project_id, projectHistoryId, userId, {newDocs, newProject: project}, (error) ->
 					return callback(error) if error?
 					callback null, doc, folder_id
 
@@ -205,7 +205,7 @@ module.exports = ProjectEntityUpdateHandler = self =
 					path: result?.path?.fileSystem
 					url: fileStoreUrl
 				]
-				DocumentUpdaterHandler.updateProjectStructure project_id, projectHistoryId, userId, {newFiles}, (error) ->
+				DocumentUpdaterHandler.updateProjectStructure project_id, projectHistoryId, userId, {newFiles, newProject: project}, (error) ->
 					return callback(error) if error?
 					callback(null, fileRef, folder_id)
 
@@ -221,7 +221,7 @@ module.exports = ProjectEntityUpdateHandler = self =
 					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) ->
+			ProjectEntityMongoUpdateHandler.replaceFileWithNew project_id, file_id, newFileRef, (err, oldFileRef, project, path, newProject) ->
 				return callback(err) if err?
 				oldFiles = [
 					file: oldFileRef
@@ -241,7 +241,7 @@ module.exports = ProjectEntityUpdateHandler = self =
 				# but it is acceptable for now.
 				TpdsUpdateSender.addFile {project_id:project._id, file_id:newFileRef._id, path:path.fileSystem, rev:oldFileRef.rev + 1, project_name:project.name}, (err) ->
 					return callback(err) if err?
-					DocumentUpdaterHandler.updateProjectStructure project_id, projectHistoryId, userId, {oldFiles, newFiles}, callback
+					DocumentUpdaterHandler.updateProjectStructure project_id, projectHistoryId, userId, {oldFiles, newFiles, newProject}, callback
 
 	upsertDoc: wrapWithLock (project_id, folder_id, docName, docLines, source, userId, callback = (err, doc, folder_id, isNewDoc)->)->
 		if not SafePath.isCleanFilename docName
@@ -338,9 +338,9 @@ module.exports = ProjectEntityUpdateHandler = self =
 			logger.err err: "No entityType set", project_id: project_id, entity_id: entity_id
 			return callback("No entityType set")
 		entityType = entityType.toLowerCase()
-		ProjectEntityMongoUpdateHandler.deleteEntity project_id, entity_id, entityType, (error, entity, path, projectBeforeDeletion) ->
+		ProjectEntityMongoUpdateHandler.deleteEntity project_id, entity_id, entityType, (error, entity, path, projectBeforeDeletion, newProject) ->
 			return callback(error) if error?
-			self._cleanUpEntity projectBeforeDeletion, entity, entityType, path.fileSystem, userId, (error) ->
+			self._cleanUpEntity projectBeforeDeletion, newProject, entity, entityType, path.fileSystem, userId, (error) ->
 				return callback(error) if error?
 				TpdsUpdateSender.deleteEntity project_id:project_id, path:path.fileSystem, project_name:projectBeforeDeletion.name, (error) ->
 					return callback(error) if error?
@@ -421,8 +421,8 @@ module.exports = ProjectEntityUpdateHandler = self =
 
 				DocumentUpdaterHandler.resyncProjectHistory project_id, projectHistoryId, docs, files, callback
 
-	_cleanUpEntity: (project, entity, entityType, path, userId, callback = (error) ->) ->
-		self._updateProjectStructureWithDeletedEntity project, entity, entityType, path, userId, (error) ->
+	_cleanUpEntity: (project, newProject, entity, entityType, path, userId, callback = (error) ->) ->
+		self._updateProjectStructureWithDeletedEntity project, newProject, entity, entityType, path, userId, (error) ->
 			return callback(error) if error?
 			if(entityType.indexOf("file") != -1)
 				self._cleanUpFile project, entity, path, userId, callback
@@ -437,7 +437,7 @@ module.exports = ProjectEntityUpdateHandler = self =
 	# methods both need to recursively iterate over the entities in folder.
 	# These are currently using separate implementations of the recursion. In
 	# future, these could be simplified using a common project entity iterator.
-	_updateProjectStructureWithDeletedEntity: (project, entity, entityType, entityPath, userId, callback = (error) ->) ->
+	_updateProjectStructureWithDeletedEntity: (project, newProject, entity, entityType, entityPath, userId, callback = (error) ->) ->
 		# compute the changes to the project structure
 		if(entityType.indexOf("file") != -1)
 			changes = oldFiles: [ {file: entity, path: entityPath} ]
@@ -454,6 +454,7 @@ module.exports = ProjectEntityUpdateHandler = self =
 					_recurseFolder(childFolder, path.join(folderPath, childFolder.name))
 			_recurseFolder entity, entityPath
 		# now send the project structure changes to the docupdater
+		changes.newProject = newProject
 		project_id = project._id.toString()
 		projectHistoryId = project.overleaf?.history?.id
 		DocumentUpdaterHandler.updateProjectStructure project_id, projectHistoryId, userId, changes, callback

services/web/test/unit/coffee/DocumentUpdater/DocumentUpdaterHandlerTests.coffee

@@ -33,7 +33,7 @@ describe 'DocumentUpdaterHandler', ->
 		@handler = SandboxedModule.require modulePath, requires:
 			'request': defaults:=> return @request
 			'settings-sharelatex':@settings
-			'logger-sharelatex':{log:(->), error:(->)}
+			'logger-sharelatex':{log:(->), error:(->), warn:(->)}
 			'../Project/ProjectEntityHandler':@projectEntityHandler
 			"../../models/Project": Project: @Project={}
 			'../../Features/Project/ProjectLocator':{}

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

@@ -99,7 +99,9 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 			@path = mongo: 'file.png'
 			@newFile = _id: 'new-file-id'
 			@newFile.linkedFileData = @linkedFileData = {provider: 'url'}
+			@newProject = "new-project"
 			@ProjectLocator.findElement = sinon.stub().yields(null, @file, @path)
+			@ProjectModel.findOneAndUpdate = sinon.stub().yields(null, @newProject)
 			@ProjectModel.update = sinon.stub().yields()
 
 			@subject.replaceFileWithNew project_id, file_id, @newFile, @callback
@@ -131,19 +133,19 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 				.should.equal true
 
 		it 'increments the project version and sets the rev and created_at', ->
-			@ProjectModel.update
+			@ProjectModel.findOneAndUpdate
 				.calledWith(
 					{ _id: project_id },
 					{
 						'$inc': { 'version': 1, 'file.png.rev': 1 }
 						'$set': { 'file.png._id': @newFile._id, 'file.png.created': new Date(), 'file.png.linkedFileData': @linkedFileData }
-					}
-					{}
+					},
+					{new: true}
 				)
 				.should.equal true
 
 		it 'calls the callback', ->
-			@callback.calledWith(null, @file, @project, @path).should.equal true
+			@callback.calledWith(null, @file, @project, @path, @newProject).should.equal true
 
 	describe 'mkdirp', ->
 		beforeEach ->
@@ -243,6 +245,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 
 			@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)
@@ -250,7 +253,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 			@subject._checkValidMove = sinon.stub().yields()
 
 			@subject._removeElementFromMongoArray = sinon.stub().yields(null, @project)
-			@subject._putElement = sinon.stub().yields(null, path: @pathAfterMove)
+			@subject._putElement = sinon.stub().yields(null, path: @pathAfterMove, @newProject)
 
 			@subject.moveEntity project_id, doc_id, folder_id, "docs", @callback
 
@@ -280,7 +283,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 				.should.equal true
 
 		it "calls the callback", ->
-			changes = { @oldDocs, @newDocs, @oldFiles, @newFiles }
+			changes = { @oldDocs, @newDocs, @oldFiles, @newFiles, @newProject }
 			@callback.calledWith(
 				null, @project, @path.fileSystem, @pathAfterMove.fileSystem, @doc.rev, changes
 			).should.equal true
@@ -321,6 +324,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 				rootFolder: [_id:ObjectId()]
 			@doc = _id: doc_id, name: "old.tex", rev: 1
 			@folder = _id: folder_id
+			@newProject = "new-project"
 
 			@ProjectGetter.getProjectWithoutLock = sinon.stub().yields(null, @project)
 
@@ -334,7 +338,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 
 			@ProjectLocator.findElement = sinon.stub().yields(null, @doc, @path, @folder)
 			@subject._checkValidElementName = sinon.stub().yields()
-			@ProjectModel.findOneAndUpdate = sinon.stub().callsArgWith(3, null, @project)
+			@ProjectModel.findOneAndUpdate = sinon.stub().callsArgWith(3, null, @newProject)
 
 			@subject.renameEntity project_id, doc_id, 'doc', @newName, @callback
 
@@ -362,7 +366,7 @@ describe 'ProjectEntityMongoUpdateHandler', ->
 				).should.equal true
 
 		it 'calls the callback', ->
-			changes = { @oldDocs, @newDocs, @oldFiles, @newFiles }
+			changes = { @oldDocs, @newDocs, @oldFiles, @newFiles, @newProject }
 			@callback.calledWith(
 				null, @project, '/old.tex', '/new.tex', @doc.rev, changes
 			).should.equal true

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

@@ -311,7 +311,7 @@ describe 'ProjectEntityUpdateHandler', ->
 					docLines: @docLines.join('\n')
 				]
 				@DocumentUpdaterHandler.updateProjectStructure
-					.calledWith(project_id, projectHistoryId, userId, {newDocs})
+					.calledWith(project_id, projectHistoryId, userId, {newDocs, newProject: @project})
 					.should.equal true
 
 		describe 'adding a doc with an invalid name', ->
@@ -367,7 +367,7 @@ describe 'ProjectEntityUpdateHandler', ->
 					url: @fileUrl
 				]
 				@DocumentUpdaterHandler.updateProjectStructure
-					.calledWith(project_id, projectHistoryId, userId, {newFiles})
+					.calledWith(project_id, projectHistoryId, userId, {newFiles, newProject: @project})
 					.should.equal true
 
 		describe 'adding a file with an invalid name', ->
@@ -393,8 +393,9 @@ describe 'ProjectEntityUpdateHandler', ->
 			@newFile = _id: new_file_id, name: "dummy-upload-filename", rev: 0, linkedFileData: @linkedFileData
 			@oldFile = _id: file_id, rev: 3
 			@path = "/path/to/file"
+			@newProject = "new project"
 			@ProjectEntityMongoUpdateHandler._insertDeletedFileReference = sinon.stub().yields()
-			@ProjectEntityMongoUpdateHandler.replaceFileWithNew = sinon.stub().yields(null, @oldFile, @project, fileSystem: @path)
+			@ProjectEntityMongoUpdateHandler.replaceFileWithNew = sinon.stub().yields(null, @oldFile, @project, fileSystem: @path, @newProject)
 			@ProjectEntityUpdateHandler.replaceFile project_id, file_id, @fileSystemPath, @linkedFileData, userId, @callback
 
 		it 'uploads a new version of the file', ->
@@ -429,7 +430,7 @@ describe 'ProjectEntityUpdateHandler', ->
 				url: @newFileUrl
 			]
 			@DocumentUpdaterHandler.updateProjectStructure
-				.calledWith(project_id, projectHistoryId, userId, {oldFiles, newFiles})
+				.calledWith(project_id, projectHistoryId, userId, {oldFiles, newFiles, newProject: @newProject})
 				.should.equal true
 
 	describe 'upsertDoc', ->
@@ -712,7 +713,8 @@ describe 'ProjectEntityUpdateHandler', ->
 			@path = '/path/to/doc.tex'
 			@doc = _id: doc_id
 			@projectBeforeDeletion = _id: project_id, name: 'project'
-			@ProjectEntityMongoUpdateHandler.deleteEntity = sinon.stub().yields(null, @doc, {fileSystem: @path}, @projectBeforeDeletion)
+			@newProject = "new-project"
+			@ProjectEntityMongoUpdateHandler.deleteEntity = sinon.stub().yields(null, @doc, {fileSystem: @path}, @projectBeforeDeletion, @newProject)
 			@ProjectEntityUpdateHandler._cleanUpEntity = sinon.stub().yields()
 			@TpdsUpdateSender.deleteEntity = sinon.stub().yields()
 
@@ -725,7 +727,7 @@ describe 'ProjectEntityUpdateHandler', ->
 
 		it 'cleans up the doc in the docstore', ->
 			@ProjectEntityUpdateHandler._cleanUpEntity
-				.calledWith(@projectBeforeDeletion, @doc, 'doc', @path, userId)
+				.calledWith(@projectBeforeDeletion, @newProject, @doc, 'doc', @path, userId)
 				.should.equal true
 
 		it 'it notifies the tpds', ->
@@ -974,7 +976,8 @@ describe 'ProjectEntityUpdateHandler', ->
 			beforeEach (done) ->
 				@path = "/file/system/path.png"
 				@entity = _id: @entity_id
-				@ProjectEntityUpdateHandler._cleanUpEntity @project, @entity, 'file', @path, userId, done
+				@newProject = "new-project"
+				@ProjectEntityUpdateHandler._cleanUpEntity @project, @newProject, @entity, 'file', @path, userId, done
 
 			it "should insert the file into the deletedFiles array", ->
 				@ProjectEntityMongoUpdateHandler._insertDeletedFileReference
@@ -990,7 +993,7 @@ describe 'ProjectEntityUpdateHandler', ->
 			it "should should send the update to the doc updater", ->
 				oldFiles = [ file: @entity, path: @path ]
 				@DocumentUpdaterHandler.updateProjectStructure
-					.calledWith(project_id, projectHistoryId, userId, {oldFiles})
+					.calledWith(project_id, projectHistoryId, userId, {oldFiles, newProject: @newProject})
 					.should.equal true
 
 		describe "a doc", ->
@@ -998,7 +1001,8 @@ describe 'ProjectEntityUpdateHandler', ->
 				@path = "/file/system/path.tex"
 				@ProjectEntityUpdateHandler._cleanUpDoc = sinon.stub().yields()
 				@entity = {_id: @entity_id}
-				@ProjectEntityUpdateHandler._cleanUpEntity @project, @entity, 'doc', @path, userId, done
+				@newProject = "new-project"
+				@ProjectEntityUpdateHandler._cleanUpEntity @project, @newProject, @entity, 'doc', @path, userId, done
 
 			it "should clean up the doc", ->
 				@ProjectEntityUpdateHandler._cleanUpDoc
@@ -1008,7 +1012,7 @@ describe 'ProjectEntityUpdateHandler', ->
 			it "should should send the update to the doc updater", ->
 				oldDocs = [ doc: @entity, path: @path ]
 				@DocumentUpdaterHandler.updateProjectStructure
-					.calledWith(project_id, projectHistoryId, userId, {oldDocs})
+					.calledWith(project_id, projectHistoryId, userId, {oldDocs, newProject: @newProject})
 					.should.equal true
 
 		describe "a folder", ->
@@ -1026,7 +1030,8 @@ describe 'ProjectEntityUpdateHandler', ->
 				@ProjectEntityUpdateHandler._cleanUpDoc = sinon.stub().yields()
 				@ProjectEntityUpdateHandler._cleanUpFile = sinon.stub().yields()
 				path = "/folder"
-				@ProjectEntityUpdateHandler._cleanUpEntity @project, @folder, "folder", path, userId, done
+				@newProject = "new-project"
+				@ProjectEntityUpdateHandler._cleanUpEntity @project, @newProject, @folder, "folder", path, userId, done
 
 			it "should clean up all sub files", ->
 				@ProjectEntityUpdateHandler._cleanUpFile
@@ -1048,7 +1053,7 @@ describe 'ProjectEntityUpdateHandler', ->
 				oldFiles = [ {file: @file2, path: "/folder/file-name-2"}, {file: @file1, path: "/folder/subfolder/file-name-1"} ]
 				oldDocs = [ {doc: @doc2, path: "/folder/doc-name-2"}, { doc: @doc1, path: "/folder/subfolder/doc-name-1"} ]
 				@DocumentUpdaterHandler.updateProjectStructure
-					.calledWith(project_id, projectHistoryId, userId, {oldFiles, oldDocs})
+					.calledWith(project_id, projectHistoryId, userId, {oldFiles, oldDocs, newProject: @newProject})
 					.should.equal true
 
 	describe "_cleanUpDoc", ->