From eb5738886accb318dab69851a42b3c0b3d89f172 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 11 Jan 2019 09:57:02 +0000 Subject: [PATCH] Merge pull request #1382 from sharelatex/bg-fix-locking-for-copy fix locking for copy file method in ProjectEntityUpdateHandler GitOrigin-RevId: a0c2a69f31bd17d8ae5cbfbc1047db22207f9bbe --- .../coffee/Features/Project/ProjectDuplicator.coffee | 2 +- .../Project/ProjectEntityUpdateHandler.coffee | 8 +++----- .../coffee/Project/ProjectDuplicatorTests.coffee | 12 ++++++------ .../Project/ProjectEntityUpdateHandlerTests.coffee | 4 ++-- 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectDuplicator.coffee b/services/web/app/coffee/Features/Project/ProjectDuplicator.coffee index 3ac138a974..d461ed4699 100644 --- a/services/web/app/coffee/Features/Project/ProjectDuplicator.coffee +++ b/services/web/app/coffee/Features/Project/ProjectDuplicator.coffee @@ -38,7 +38,7 @@ module.exports = ProjectDuplicator = jobs = fileRefs.map (file)-> return (cb)-> return async.setImmediate(cb) if firstError? # skip further copies if an error has occurred - ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject newProject, desFolder._id, originalProject_id, file, owner_id, (err) -> + ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject newProject._id, newProject, desFolder._id, originalProject_id, file, owner_id, (err) -> firstError ||= err if err? # set the error flag if this copy failed return cb() # If one of these jobs fails then we wait until all running jobs have diff --git a/services/web/app/coffee/Features/Project/ProjectEntityUpdateHandler.coffee b/services/web/app/coffee/Features/Project/ProjectEntityUpdateHandler.coffee index 66275a01b1..def34bcf45 100644 --- a/services/web/app/coffee/Features/Project/ProjectEntityUpdateHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectEntityUpdateHandler.coffee @@ -47,8 +47,7 @@ wrapWithLock = (methodWithoutLock) -> module.exports = ProjectEntityUpdateHandler = self = copyFileFromExistingProjectWithProject: wrapWithLock beforeLock: (next) -> - (project, folder_id, originalProject_id, origonalFileRef, userId, callback = (error, fileRef, folder_id) ->)-> - project_id = project._id + (project_id, project, folder_id, originalProject_id, origonalFileRef, userId, callback = (error, fileRef, folder_id) ->)-> logger.log { project_id, folder_id, originalProject_id, origonalFileRef }, "copying file in s3 with project" ProjectEntityMongoUpdateHandler._confirmFolder project, folder_id, (folder_id) -> if !origonalFileRef? @@ -63,9 +62,8 @@ module.exports = ProjectEntityUpdateHandler = self = if err? logger.err { err, project_id, folder_id, originalProject_id, origonalFileRef }, "error coping file in s3" return callback(err) - next(project, folder_id, originalProject_id, origonalFileRef, userId, fileRef, fileStoreUrl, callback) - withLock: (project, folder_id, originalProject_id, origonalFileRef, userId, fileRef, fileStoreUrl, callback = (error, fileRef, folder_id) ->)-> - project_id = project._id + next(project_id, project, folder_id, originalProject_id, origonalFileRef, userId, fileRef, fileStoreUrl, callback) + withLock: (project_id, project, folder_id, originalProject_id, origonalFileRef, userId, fileRef, fileStoreUrl, callback = (error, fileRef, folder_id) ->)-> projectHistoryId = project.overleaf?.history?.id ProjectEntityMongoUpdateHandler._putElement project, folder_id, fileRef, "file", (err, result, newProject) -> if err? diff --git a/services/web/test/unit/coffee/Project/ProjectDuplicatorTests.coffee b/services/web/test/unit/coffee/Project/ProjectDuplicatorTests.coffee index db001602ca..5657dab37b 100644 --- a/services/web/test/unit/coffee/Project/ProjectDuplicatorTests.coffee +++ b/services/web/test/unit/coffee/Project/ProjectDuplicatorTests.coffee @@ -69,9 +69,9 @@ describe 'ProjectDuplicator', -> 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) + @ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject.withArgs(sinon.match.any, sinon.match.any, sinon.match.any, sinon.match.any, "BROKEN-FILE", sinon.match.any, sinon.match.any).callsArgWith(6, new Error("failed")) + @ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject.withArgs(sinon.match.any, sinon.match.any, sinon.match.any, sinon.match.any, sinon.match.object, sinon.match.any).callsArg(6) + @ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject.withArgs(sinon.match.any, sinon.match.any, sinon.match.any, sinon.match.any, null, sinon.match.any).callsArg(6) @DocumentUpdaterHandler = flushProjectToMongo: sinon.stub().callsArg(1) @@ -166,13 +166,13 @@ describe 'ProjectDuplicator', -> 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) + .calledWith(@stubbedNewProject._id, @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) + .calledWith(@stubbedNewProject._id, @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) + .calledWith(@stubbedNewProject._id, @stubbedNewProject, @newFolder._id, @project._id, @level2folder.fileRefs[0], @owner._id) .should.equal true done() diff --git a/services/web/test/unit/coffee/Project/ProjectEntityUpdateHandlerTests.coffee b/services/web/test/unit/coffee/Project/ProjectEntityUpdateHandlerTests.coffee index 9fe175c1ca..2d73c7e6a0 100644 --- a/services/web/test/unit/coffee/Project/ProjectEntityUpdateHandlerTests.coffee +++ b/services/web/test/unit/coffee/Project/ProjectEntityUpdateHandlerTests.coffee @@ -86,7 +86,7 @@ describe 'ProjectEntityUpdateHandler', -> @ProjectEntityMongoUpdateHandler._confirmFolder = sinon.stub().yields(folder_id) @ProjectEntityMongoUpdateHandler._putElement = sinon.stub().yields(null, {path:{fileSystem: @fileSystemPath}}) - @ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject @project, folder_id, @oldProject_id, @oldFileRef, userId, @callback + @ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject @project._id, @project, folder_id, @oldProject_id, @oldFileRef, userId, @callback it 'should copy the file in FileStoreHandler', -> @FileStoreHandler.copyFile @@ -135,7 +135,7 @@ describe 'ProjectEntityUpdateHandler', -> @ProjectEntityMongoUpdateHandler._confirmFolder = sinon.stub().yields(folder_id) @ProjectEntityMongoUpdateHandler._putElement = sinon.stub().yields(null, {path:{fileSystem: @fileSystemPath}}) - @ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject @project, folder_id, @oldProject_id, @oldFileRef, userId, @callback + @ProjectEntityUpdateHandler.copyFileFromExistingProjectWithProject @project._id, @project, folder_id, @oldProject_id, @oldFileRef, userId, @callback it 'should copy the file in FileStoreHandler', -> @FileStoreHandler.copyFile