From 0bddc5552ee86b63ae6bd7713f343ccf540927e2 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 8 May 2014 15:47:50 +0100 Subject: [PATCH] Use docstore for updating documents --- .../Features/Docstore/DocstoreManager.coffee | 6 +- .../DocumentUpdaterHandler.coffee | 16 +++ .../Documents/DocumentController.coffee | 3 +- .../Features/Editor/EditorController.coffee | 3 +- .../Project/ProjectEntityHandler.coffee | 58 +++----- .../Docstore/DocstoreManagerTests.coffee | 8 +- .../DocumentUpdaterHandlerTests.coffee | 34 +++++ .../Documents/DocumentControllerTests.coffee | 8 +- .../Editor/EditorControllerTests.coffee | 11 +- .../Project/ProjectEntityHandlerTests.coffee | 136 ++++++++++++------ 10 files changed, 186 insertions(+), 97 deletions(-) diff --git a/services/web/app/coffee/Features/Docstore/DocstoreManager.coffee b/services/web/app/coffee/Features/Docstore/DocstoreManager.coffee index 88dde1393f..56e74b6f63 100644 --- a/services/web/app/coffee/Features/Docstore/DocstoreManager.coffee +++ b/services/web/app/coffee/Features/Docstore/DocstoreManager.coffee @@ -46,7 +46,7 @@ module.exports = DocstoreManager = logger.error err: error, project_id: project_id, doc_id: doc_id, "error getting doc from docstore" callback(error) - updateDoc: (project_id, doc_id, lines, version, callback = (error, rev) ->) -> + updateDoc: (project_id, doc_id, lines, version, callback = (error, modified, rev) ->) -> logger.log project_id: project_id, doc_id: doc_id, version: version, "updating doc in docstore api" url = "#{settings.apis.docstore.url}/project/#{project_id}/doc/#{doc_id}" request.post { @@ -54,10 +54,10 @@ module.exports = DocstoreManager = json: lines: lines version: version - }, (error, res, body) -> + }, (error, res, result) -> return callback(error) if error? if 200 <= res.statusCode < 300 - callback(null) + callback(null, result.modified, result.rev) else error = new Error("docstore api responded with non-success code: #{res.statusCode}") logger.error err: error, project_id: project_id, doc_id: doc_id, "error updating doc in docstore" diff --git a/services/web/app/coffee/Features/DocumentUpdater/DocumentUpdaterHandler.coffee b/services/web/app/coffee/Features/DocumentUpdater/DocumentUpdaterHandler.coffee index e5c8186811..ba38137fb0 100644 --- a/services/web/app/coffee/Features/DocumentUpdater/DocumentUpdaterHandler.coffee +++ b/services/web/app/coffee/Features/DocumentUpdater/DocumentUpdaterHandler.coffee @@ -58,6 +58,22 @@ module.exports = error = new Error("document updater returned a failure status code: #{res.statusCode}") logger.error err: error, project_id: project_id, sl_req_id: sl_req_id, "document updater returned failure status code: #{res.statusCode}" return callback(error) + + flushDocToMongo: (project_id, doc_id, callback = (error) ->) -> + logger.log project_id:project_id, doc_id: doc_id, "flushing doc from document updater" + timer = new metrics.Timer("flushing.mongo.doc") + url = "#{settings.apis.documentupdater.url}/project/#{project_id}/doc/#{doc_id}/flush" + request.post url, (error, res, body)-> + if error? + logger.error err: error, project_id: project_id, doc_id: doc_id, "error flushing doc from document updater" + return callback(error) + else if res.statusCode >= 200 and res.statusCode < 300 + logger.log project_id: project_id, doc_id: doc_id, "flushed doc from document updater" + return callback(null) + else + error = new Error("document updater returned a failure status code: #{res.statusCode}") + logger.error err: error, project_id: project_id, doc_id: doc_id, "document updater returned failure status code: #{res.statusCode}" + return callback(error) deleteDoc : (project_id, doc_id, sl_req_id, callback = ()->)-> {callback, sl_req_id} = slReqIdHelper.getCallbackAndReqId(callback, sl_req_id) diff --git a/services/web/app/coffee/Features/Documents/DocumentController.coffee b/services/web/app/coffee/Features/Documents/DocumentController.coffee index 6e6f33665a..dc32b05aed 100644 --- a/services/web/app/coffee/Features/Documents/DocumentController.coffee +++ b/services/web/app/coffee/Features/Documents/DocumentController.coffee @@ -22,8 +22,9 @@ module.exports = project_id = req.params.Project_id doc_id = req.params.doc_id lines = req.body.lines + version = req.body.version logger.log doc_id:doc_id, project_id:project_id, "receiving set document request from api (docupdater)" - ProjectEntityHandler.updateDocLines project_id, doc_id, lines, (error) -> + ProjectEntityHandler.updateDocLines project_id, doc_id, lines, version, (error) -> if error? logger.err err:error, doc_id:doc_id, project_id:project_id, "error finding element for getDocument" return next(error) diff --git a/services/web/app/coffee/Features/Editor/EditorController.coffee b/services/web/app/coffee/Features/Editor/EditorController.coffee index 4bda1d4874..0093fc0a52 100644 --- a/services/web/app/coffee/Features/Editor/EditorController.coffee +++ b/services/web/app/coffee/Features/Editor/EditorController.coffee @@ -158,8 +158,7 @@ module.exports = EditorController = DocumentUpdaterHandler.setDocument project_id, doc_id, docLines, (err)=> logger.log project_id:project_id, doc_id:doc_id, "notifying users that the document has been updated" EditorRealTimeController.emitToRoom(project_id, "entireDocUpdate", doc_id) - ProjectEntityHandler.updateDocLines project_id, doc_id, docLines, sl_req_id, (err)-> - callback(err) + DocumentUpdaterHandler.flushDocToMongo project_id, doc_id, callback addDoc: (project_id, folder_id, docName, docLines, sl_req_id, callback = (error, doc)->)-> {callback, sl_req_id} = slReqIdHelper.getCallbackAndReqId(callback, sl_req_id) diff --git a/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee b/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee index bc6b579a43..9762093ecd 100644 --- a/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee @@ -123,7 +123,7 @@ module.exports = ProjectEntityHandler = doc = new Doc name: docName Project.putElement project._id, folder_id, doc, "doc", (err, result)=> return callback(err) if err? - DocstoreManager.updateDoc project._id.toString(), doc._id.toString(), docLines, 0, (err, rev) -> + DocstoreManager.updateDoc project._id.toString(), doc._id.toString(), docLines, 0, (err, modified, rev) -> return callback(err) if err? tpdsUpdateSender.addDoc { project_id: project._id, @@ -234,45 +234,31 @@ module.exports = ProjectEntityHandler = if callback? callback(err, folder, parentFolder_id) - updateDocLines : (project_or_id, doc_id, docLines, sl_req_id, callback = (error) ->)-> - {callback, sl_req_id} = slReqIdHelper.getCallbackAndReqId(callback, sl_req_id) - Project.getProject project_or_id, "", (err, project)-> + updateDocLines : (project_id, doc_id, lines, version, callback = (error) ->)-> + ProjectGetter.getProjectWithoutDocLines project_id, (err, project)-> return callback(err) if err? return callback(new Errors.NotFoundError("project not found")) if !project? - project_id = project._id - if err? - logger.err err:err,project_id:project_id, "error finding project" - callback err - else if !project? - logger.err project_id:project_id, doc_id:doc_id, err: new Error("project #{project_id} could not be found for doc #{doc_id}") - callback "could not find project #{project_id}" - else - projectLocator.findElement {project:project, element_id:doc_id, type:"docs"}, (err, doc, path)-> + logger.log project_id: project_id, doc_id: doc_id, version: version, "updating doc lines" + projectLocator.findElement {project:project, element_id:doc_id, type:"docs"}, (err, doc, path)-> + if err? + logger.error err: err, doc_id: doc_id, project_id: project_id, version: version, lines: lines, "error finding doc while updating doc lines" + return callback err + if !doc? + error = new Errors.NotFoundError("doc not found") + logger.error err: error, doc_id: doc_id, project_id: project_id, version: version, lines: lines, "doc not found while updating doc lines" + return callback(error) + + DocstoreManager.updateDoc project_id, doc_id, lines, version, (err, modified, rev) -> if err? - logger.err "error putting doc #{doc_id} in project #{project_id} #{err}" - callback err - else if docComparitor.areSame docLines, doc.lines - logger.log sl_req_id: sl_req_id, project_id:project_id, doc_id:doc_id, rev:doc.rev, "old doc lines are same as the new doc lines, not updating them" - callback() + logger.error err: err, doc_id: doc_id, project_id:project_id, lines: lines, version: version, "error sending doc to docstore" + return callback(err) + + if modified + # Don't need to block for marking as updated + projectUpdateHandler.markAsUpdated project_id + tpdsUpdateSender.addDoc {project_id:project_id, path:path.fileSystem, docLines:lines, project_name:project.name, rev:rev}, callback else - logger.log sl_req_id: sl_req_id, project_id:project_id, doc_id:doc_id, docLines: docLines, oldDocLines: doc.lines, rev:doc.rev, "updating doc lines" - conditons = _id:project_id - update = {$set:{}, $inc:{}} - changeLines = {} - changeLines["#{path.mongo}.lines"] = docLines - inc = {} - inc["#{path.mongo}.rev"] = 1 - update["$set"] = changeLines - update["$inc"] = inc - Project.update conditons, update, {}, (err, second)-> - if(err) - logger.err(sl_req_id:sl_req_id, doc_id:doc_id, project_id:project_id, err:err, "error saving doc to mongo") - callback(err) - else - logger.log sl_req_id:sl_req_id, doc_id:doc_id, project_id:project_id, newDocLines:docLines, oldDocLines:doc.lines, "doc saved to mongo" - rev = doc.rev+1 - projectUpdateHandler.markAsUpdated project_id - tpdsUpdateSender.addDoc {project_id:project_id, path:path.fileSystem, docLines:docLines, project_name:project.name, rev:rev}, sl_req_id, callback + callback() moveEntity: (project_id, entity_id, folder_id, entityType, sl_req_id, callback = (error) ->)-> {callback, sl_req_id} = slReqIdHelper.getCallbackAndReqId(callback, sl_req_id) diff --git a/services/web/test/UnitTests/coffee/Docstore/DocstoreManagerTests.coffee b/services/web/test/UnitTests/coffee/Docstore/DocstoreManagerTests.coffee index b29d7812e0..572cff3b29 100644 --- a/services/web/test/UnitTests/coffee/Docstore/DocstoreManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Docstore/DocstoreManagerTests.coffee @@ -56,10 +56,12 @@ describe "DocstoreManager", -> beforeEach -> @lines = ["mock", "doc", "lines"] @version = 42 + @rev = 5 + @modified = true describe "with a successful response code", -> beforeEach -> - @request.post = sinon.stub().callsArgWith(1, null, statusCode: 204, "") + @request.post = sinon.stub().callsArgWith(1, null, statusCode: 204, { modified: @modified, rev: @rev }) @DocstoreManager.updateDoc @project_id, @doc_id, @lines, @version, @callback it "should update the doc in the docstore api", -> @@ -72,8 +74,8 @@ describe "DocstoreManager", -> }) .should.equal true - it "should call the callback without an error", -> - @callback.calledWith(null).should.equal true + it "should call the callback with the modified status and revision", -> + @callback.calledWith(null, @modified, @rev).should.equal true describe "with a failed response code", -> beforeEach -> diff --git a/services/web/test/UnitTests/coffee/DocumentUpdater/DocumentUpdaterHandlerTests.coffee b/services/web/test/UnitTests/coffee/DocumentUpdater/DocumentUpdaterHandlerTests.coffee index 11ca80a4dc..097269a731 100644 --- a/services/web/test/UnitTests/coffee/DocumentUpdater/DocumentUpdaterHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/DocumentUpdater/DocumentUpdaterHandlerTests.coffee @@ -156,6 +156,40 @@ describe 'Flushing documents :', -> .calledWith(new Error("doc updater returned failure status code: 500")) .should.equal true + describe 'flushDocToMongo', -> + beforeEach -> + @callback = sinon.stub() + + describe "successfully", -> + beforeEach -> + @request.post = sinon.stub().callsArgWith(1, null, {statusCode: 204}, "") + @handler.flushDocToMongo @project_id, @doc_id, @callback + + it 'should flush the document from the document updater', -> + url = "#{@settings.apis.documentupdater.url}/project/#{@project_id}/doc/#{@doc_id}/flush" + @request.post.calledWith(url).should.equal true + + it "should call the callback with no error", -> + @callback.calledWith(null).should.equal true + + describe "when the document updater API returns an error", -> + beforeEach -> + @request.post = sinon.stub().callsArgWith(1, @error = new Error("something went wrong"), null, null) + @handler.flushDocToMongo @project_id, @doc_id, @callback + + it "should return an error to the callback", -> + @callback.calledWith(@error).should.equal true + + describe "when the document updater returns a failure error code", -> + beforeEach -> + @request.post = sinon.stub().callsArgWith(1, null, { statusCode: 500 }, "") + @handler.flushDocToMongo @project_id, @doc_id, @callback + + it "should return the callback with an error", -> + @callback + .calledWith(new Error("doc updater returned failure status code: 500")) + .should.equal true + describe "deleteDoc", -> beforeEach -> @callback = sinon.stub() diff --git a/services/web/test/UnitTests/coffee/Documents/DocumentControllerTests.coffee b/services/web/test/UnitTests/coffee/Documents/DocumentControllerTests.coffee index eac78ce97a..99b883d668 100644 --- a/services/web/test/UnitTests/coffee/Documents/DocumentControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Documents/DocumentControllerTests.coffee @@ -61,14 +61,15 @@ describe "DocumentController", -> describe "when the document exists", -> beforeEach -> - @ProjectEntityHandler.updateDocLines = sinon.stub().callsArg(3) + @ProjectEntityHandler.updateDocLines = sinon.stub().callsArg(4) @req.body = lines: @doc_lines + version: @version @DocumentController.setDocument(@req, @res, @next) it "should update the document in Mongo", -> @ProjectEntityHandler.updateDocLines - .calledWith(@project_id, @doc_id, @doc_lines) + .calledWith(@project_id, @doc_id, @doc_lines, @version) .should.equal true it "should return a successful response", -> @@ -76,9 +77,10 @@ describe "DocumentController", -> describe "when the document doesn't exist", -> beforeEach -> - @ProjectEntityHandler.updateDocLines = sinon.stub().callsArgWith(3, new Errors.NotFoundError("document does not exist")) + @ProjectEntityHandler.updateDocLines = sinon.stub().callsArgWith(4, new Errors.NotFoundError("document does not exist")) @req.body = lines: @doc_lines + version: @version @DocumentController.setDocument(@req, @res, @next) it "should call next with the NotFoundError", -> diff --git a/services/web/test/UnitTests/coffee/Editor/EditorControllerTests.coffee b/services/web/test/UnitTests/coffee/Editor/EditorControllerTests.coffee index 406c68b0ea..755dc2e960 100644 --- a/services/web/test/UnitTests/coffee/Editor/EditorControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Editor/EditorControllerTests.coffee @@ -372,7 +372,7 @@ describe "EditorController", -> describe 'set document', -> beforeEach -> @docLines = ["foo", "bar"] - @ProjectEntityHandler.updateDocLines = sinon.stub().callsArg(4) + @DocumentUpdaterHandler.flushDocToMongo = sinon.stub().callsArg(2) @DocumentUpdaterHandler.setDocument = sinon.stub().callsArg(3) @EditorRealTimeController.emitToRoom = sinon.stub() @@ -394,12 +394,9 @@ describe "EditorController", -> mock.verify() done() - it 'should update the document lines', (done)-> - @ProjectEntityHandler.updateDocLines = -> - mock = sinon.mock(@ProjectEntityHandler).expects("updateDocLines").withArgs(@project_id, @doc_id, @docLines).once().callsArg(4) - - @EditorController.setDoc @project_id, @doc_id, @docLines, (err)-> - mock.verify() + it 'should flush the doc to mongo', (done)-> + @EditorController.setDoc @project_id, @doc_id, @docLines, (err)=> + @DocumentUpdaterHandler.flushDocToMongo.calledWith(@project_id, @doc_id).should.equal true done() diff --git a/services/web/test/UnitTests/coffee/Project/ProjectEntityHandlerTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectEntityHandlerTests.coffee index 4b8fe460a2..708874125a 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectEntityHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectEntityHandlerTests.coffee @@ -7,6 +7,7 @@ modulePath = "../../../../app/js/Features/Project/ProjectEntityHandler" SandboxedModule = require('sandboxed-module') ObjectId = require("mongoose").Types.ObjectId tk = require 'timekeeper' +Errors = require "../../../../app/js/errors" describe 'ProjectEntityHandler', -> project_id = '4eecb1c1bffa66588e0000a1' @@ -65,7 +66,7 @@ describe 'ProjectEntityHandler', -> './ProjectLocator':@projectLocator = {} '../../Features/DocumentUpdater/DocumentUpdaterHandler':@documentUpdaterHandler = {} '../Docstore/DocstoreManager': @DocstoreManager = {} - 'logger-sharelatex':{log:->} + 'logger-sharelatex': @logger = {log:sinon.stub(), error: sinon.stub()} './ProjectUpdateHandler': @projectUpdater "./ProjectGetter": @ProjectGetter = {} @@ -305,7 +306,7 @@ describe 'ProjectEntityHandler', -> @ProjectModel.putElement = sinon.stub().callsArgWith(4, null, {path:{fileSystem:@path}}) @callback = sinon.stub() @tpdsUpdateSender.addDoc = sinon.stub().callsArg(2) - @DocstoreManager.updateDoc = sinon.stub().callsArgWith(4, null, 0) + @DocstoreManager.updateDoc = sinon.stub().callsArgWith(4, null, true, 0) @ProjectEntityHandler.addDoc project_id, folder_id, @name, @lines, @callback @@ -457,54 +458,105 @@ describe 'ProjectEntityHandler', -> done() - describe 'updating document lines', -> - docId = "123456" - docLines = ['1234','abc', '543543'] - mongoPath = "folders[0].folders[5]" - fileSystemPath = "/somehwere/something.tex" + describe 'updateDocLines', -> + beforeEach -> + @lines = ['mock', 'doc', 'lines'] + @path = "/somewhere/something.tex" + @doc = { + _id: doc_id + } + @version = 42 + @ProjectGetter.getProjectWithoutDocLines = sinon.stub().callsArgWith(1, null, @project) + @projectLocator.findElement = sinon.stub().callsArgWith(1, null, @doc, {fileSystem: @path}) + @tpdsUpdateSender.addDoc = sinon.stub().callsArg(1) + @projectUpdater.markAsUpdated = sinon.stub() + @callback = sinon.stub() - it 'should find project via getProject', (done)-> - @ProjectModel.getProject = (passedProject_id, callback)-> - passedProject_id.should.equal project_id - done() + describe "when the doc has been modified", -> + beforeEach -> + @DocstoreManager.updateDoc = sinon.stub().callsArgWith(4, null, true, @rev = 5) + @ProjectEntityHandler.updateDocLines project_id, doc_id, @lines, @version, @callback - @ProjectEntityHandler.updateDocLines project_id, "", [], -> + it "should get the project without doc lines", -> + @ProjectGetter.getProjectWithoutDocLines + .calledWith(project_id) + .should.equal true - it 'should find the doc', (done)-> - - @projectLocator.findElement = (options, callback)-> - options.element_id.should.equal docId - options.type.should.equal 'docs' - done() + it "should find the doc", -> + @projectLocator.findElement + .calledWith({ + project: @project + type: "docs" + element_id: doc_id + }) + .should.equal true - @ProjectEntityHandler.updateDocLines project_id, docId, "", -> + it "should update the doc in the docstore", -> + @DocstoreManager.updateDoc + .calledWith(project_id, doc_id, @lines, @version) + .should.equal true - it 'should build mongo update statment', (done)-> - @projectLocator.findElement = (opts, callback)-> - callback(null, {lines:[], rev:0}, {mongo:mongoPath}) + it "should mark the project as updated", -> + @projectUpdater.markAsUpdated + .calledWith(project_id) + .should.equal true - @ProjectModel.update = (conditions, update, options, callback)-> - conditions._id.should.equal project_id - update.$set["#{mongoPath}.lines"].should.equal docLines - update.$inc["#{mongoPath}.rev"].should.equal 1 - done() + it "should send the doc the to the TPDS", -> + @tpdsUpdateSender.addDoc + .calledWith({ + project_id: project_id + project_name: @project.name + docLines: @lines + rev: @rev + path: @path + }) + .should.equal true - @ProjectEntityHandler.updateDocLines project_id, docId, docLines, -> + it "should call the callback", -> + @callback.called.should.equal true + + describe "when the doc has not been modified", -> + beforeEach -> + @DocstoreManager.updateDoc = sinon.stub().callsArgWith(4, null, false, @rev = 5) + @ProjectEntityHandler.updateDocLines project_id, doc_id, @lines, @version, @callback + + it "should not mark the project as updated", -> + @projectUpdater.markAsUpdated.called.should.equal false + + it "should not send the doc the to the TPDS", -> + @tpdsUpdateSender.addDoc.called.should.equal false + + it "should call the callback", -> + @callback.called.should.equal true + + describe "when the project is not found", -> + beforeEach -> + @ProjectGetter.getProjectWithoutDocLines = sinon.stub().callsArgWith(1, null, null) + @ProjectEntityHandler.updateDocLines project_id, doc_id, @lines, @version, @callback + + it "should return a not found error", -> + @callback.calledWith(new Errors.NotFoundError()).should.equal true + + describe "when the doc is not found", -> + beforeEach -> + @projectLocator.findElement = sinon.stub().callsArgWith(1, null, null, null) + @ProjectEntityHandler.updateDocLines project_id, doc_id, @lines, @version, @callback + + it "should log out the error", -> + @logger.error + .calledWith( + project_id: project_id + doc_id: doc_id + version: @version + lines: @lines + err: new Errors.NotFoundError("doc not found") + "doc not found while updating doc lines" + ) + .should.equal true + + it "should return a not found error", -> + @callback.calledWith(new Errors.NotFoundError()).should.equal true - it 'should call third party data store ', (done)-> - rev = 3 - @projectLocator.findElement = (opts, callback)-> - callback(null, {lines:[],rev:rev}, {fileSystem:fileSystemPath}) - @ProjectModel.update = (conditions, update, options, callback)-> callback() - @tpdsUpdateSender.addDoc = (options, _, callback)=> - options.project_id.should.equal project_id - options.docLines.should.equal docLines - options.path.should.equal fileSystemPath - options.project_name.should.equal @project.name - options.rev.should.equal (rev+1) - callback() - @projectUpdater.markAsUpdated.calledWith(project_id).should.equal true - @ProjectEntityHandler.updateDocLines project_id, docId, docLines, done describe "getting folders, docs and files", -> beforeEach ->