diff --git a/services/document-updater/app.coffee b/services/document-updater/app.coffee index d036beda55..49cfa1f1b8 100644 --- a/services/document-updater/app.coffee +++ b/services/document-updater/app.coffee @@ -37,14 +37,15 @@ app.param 'doc_id', (req, res, next, doc_id) -> else next new Error("invalid doc id") -app.get '/project/:project_id/doc/:doc_id', HttpController.getDoc -app.post '/project/:project_id/doc/:doc_id', HttpController.setDoc -app.post '/project/:project_id/doc/:doc_id/flush', HttpController.flushDocIfLoaded -app.delete '/project/:project_id/doc/:doc_id', HttpController.flushAndDeleteDoc -app.delete '/project/:project_id', HttpController.deleteProject -app.post '/project/:project_id/flush', HttpController.flushProject -app.post '/project/:project_id/doc/:doc_id/change/:change_id/accept', HttpController.acceptChange -app.del '/project/:project_id/doc/:doc_id/comment/:comment_id', HttpController.deleteComment +app.get '/project/:project_id/doc/:doc_id', HttpController.getDoc +app.post '/project/:project_id/doc/:doc_id', HttpController.setDoc +app.post '/project/:project_id/doc/:doc_id/flush', HttpController.flushDocIfLoaded +app.delete '/project/:project_id/doc/:doc_id', HttpController.flushAndDeleteDoc +app.delete '/project/:project_id', HttpController.deleteProject +app.post '/project/:project_id/flush', HttpController.flushProject +app.post '/project/:project_id/doc/:doc_id/change/:change_id/accept', HttpController.acceptChanges +app.post '/project/:project_id/doc/:doc_id/change/accept', HttpController.acceptChanges +app.del '/project/:project_id/doc/:doc_id/comment/:comment_id', HttpController.deleteComment app.get '/total', (req, res)-> timer = new Metrics.Timer("http.allDocList") diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index fa5284adab..489b79bd56 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -130,9 +130,9 @@ module.exports = DocumentManager = RedisManager.removeDocFromMemory project_id, doc_id, (error) -> return callback(error) if error? callback null - - acceptChange: (project_id, doc_id, change_id, _callback = (error) ->) -> - timer = new Metrics.Timer("docManager.acceptChange") + + acceptChanges: (project_id, doc_id, change_ids = [], _callback = (error) ->) -> + timer = new Metrics.Timer("docManager.acceptChanges") callback = (args...) -> timer.done() _callback(args...) @@ -141,7 +141,7 @@ module.exports = DocumentManager = return callback(error) if error? if !lines? or !version? return callback(new Errors.NotFoundError("document not found: #{doc_id}")) - RangesManager.acceptChange change_id, ranges, (error, new_ranges) -> + RangesManager.acceptChanges change_ids, ranges, (error, new_ranges) -> return callback(error) if error? RedisManager.updateDocument doc_id, lines, version, [], new_ranges, (error) -> return callback(error) if error? @@ -183,9 +183,9 @@ module.exports = DocumentManager = UpdateManager = require "./UpdateManager" UpdateManager.lockUpdatesAndDo DocumentManager.flushAndDeleteDoc, project_id, doc_id, callback - acceptChangeWithLock: (project_id, doc_id, change_id, callback = (error) ->) -> + acceptChangesWithLock: (project_id, doc_id, change_ids, callback = (error) ->) -> UpdateManager = require "./UpdateManager" - UpdateManager.lockUpdatesAndDo DocumentManager.acceptChange, project_id, doc_id, change_id, callback + UpdateManager.lockUpdatesAndDo DocumentManager.acceptChanges, project_id, doc_id, change_ids, callback deleteCommentWithLock: (project_id, doc_id, thread_id, callback = (error) ->) -> UpdateManager = require "./UpdateManager" diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index aae2b51f8e..13e618e734 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/services/document-updater/app/coffee/HttpController.coffee @@ -95,15 +95,18 @@ module.exports = HttpController = return next(error) if error? logger.log project_id: project_id, "deleted project via http" res.send 204 # No Content - - acceptChange: (req, res, next = (error) ->) -> - {project_id, doc_id, change_id} = req.params - logger.log {project_id, doc_id, change_id}, "accepting change via http" - timer = new Metrics.Timer("http.acceptChange") - DocumentManager.acceptChangeWithLock project_id, doc_id, change_id, (error) -> + + acceptChanges: (req, res, next = (error) ->) -> + {project_id, doc_id} = req.params + change_ids = req.body?.change_ids + if !change_ids? + change_ids = [ req.params.change_id ] + logger.log {project_id, doc_id}, "accepting #{ change_ids.length } changes via http" + timer = new Metrics.Timer("http.acceptChanges") + DocumentManager.acceptChangesWithLock project_id, doc_id, change_ids, (error) -> timer.done() return next(error) if error? - logger.log {project_id, doc_id, change_id}, "accepted change via http" + logger.log {project_id, doc_id}, "accepted #{ change_ids.length } changes via http" res.send 204 # No Content deleteComment: (req, res, next = (error) ->) -> diff --git a/services/document-updater/app/coffee/RangesManager.coffee b/services/document-updater/app/coffee/RangesManager.coffee index ebef566424..d0653bb6a2 100644 --- a/services/document-updater/app/coffee/RangesManager.coffee +++ b/services/document-updater/app/coffee/RangesManager.coffee @@ -33,11 +33,11 @@ module.exports = RangesManager = logger.log {project_id, doc_id, changesCount: response.changes?.length, commentsCount: response.comments?.length}, "applied updates to ranges" callback null, response - acceptChange: (change_id, ranges, callback = (error, ranges) ->) -> + acceptChanges: (change_ids, ranges, callback = (error, ranges) ->) -> {changes, comments} = ranges - logger.log {change_id}, "accepting change in ranges" + logger.log "accepting #{ change_ids.length } changes in ranges" rangesTracker = new RangesTracker(changes, comments) - rangesTracker.removeChangeId(change_id) + rangesTracker.removeChangeIds(change_ids) response = RangesManager._getRanges(rangesTracker) callback null, response diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index 14193f628d..869d63159b 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -96,10 +96,42 @@ load = () -> break return change + getChanges: (change_ids) -> + changes_response = [] + ids_map = {} + + for change_id in change_ids + ids_map[change_id] = true + + for change in @changes + if ids_map[change.id] + delete ids_map[change.id] + changes_response.push change + + return changes_response + removeChangeId: (change_id) -> change = @getChange(change_id) return if !change? @_removeChange(change) + + removeChangeIds: (change_to_remove_ids) -> + return if !change_to_remove_ids?.length > 0 + i = @changes.length + remove_change_id = {} + for change_id in change_to_remove_ids + remove_change_id[change_id] = true + + remaining_changes = [] + + for change in @changes + if remove_change_id[change.id] + delete remove_change_id[change.id] + @_markAsDirty change, "change", "removed" + else + remaining_changes.push change + + @changes = remaining_changes validate: (text) -> for change in @changes diff --git a/services/document-updater/test/acceptance/coffee/FlushingDocsTests.coffee b/services/document-updater/test/acceptance/coffee/FlushingDocsTests.coffee index 47dddcd19a..f732d69bb6 100644 --- a/services/document-updater/test/acceptance/coffee/FlushingDocsTests.coffee +++ b/services/document-updater/test/acceptance/coffee/FlushingDocsTests.coffee @@ -63,7 +63,7 @@ describe "Flushing a doc to Mongo", -> lines: @lines version: @version } - sinon.stub MockWebApi, "setDocument", (project_id, doc_id, lines, version, callback = (error) ->) -> + sinon.stub MockWebApi, "setDocument", (project_id, doc_id, lines, version, ranges, callback = (error) ->) -> setTimeout callback, 30000 DocUpdaterClient.preloadDoc @project_id, @doc_id, done diff --git a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee index 0844c208eb..d5607220f0 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee @@ -319,17 +319,18 @@ describe "DocumentManager", -> describe "acceptChanges", -> beforeEach -> @change_id = "mock-change-id" + @change_ids = [ "mock-change-id-1", "mock-change-id-2", "mock-change-id-3", "mock-change-id-4" ] @version = 34 @lines = ["original", "lines"] @ranges = { entries: "mock", comments: "mock" } @updated_ranges = { entries: "updated", comments: "updated" } @DocumentManager.getDoc = sinon.stub().yields(null, @lines, @version, @ranges) - @RangesManager.acceptChange = sinon.stub().yields(null, @updated_ranges) + @RangesManager.acceptChanges = sinon.stub().yields(null, @updated_ranges) @RedisManager.updateDocument = sinon.stub().yields() - describe "successfully", -> + describe "successfully with a single change", -> beforeEach -> - @DocumentManager.acceptChange @project_id, @doc_id, @change_id, @callback + @DocumentManager.acceptChanges @project_id, @doc_id, [ @change_id ], @callback it "should get the document's current ranges", -> @DocumentManager.getDoc @@ -337,8 +338,8 @@ describe "DocumentManager", -> .should.equal true it "should apply the accept change to the ranges", -> - @RangesManager.acceptChange - .calledWith(@change_id, @ranges) + @RangesManager.acceptChanges + .calledWith([ @change_id ], @ranges) .should.equal true it "should save the updated ranges", -> @@ -349,10 +350,19 @@ describe "DocumentManager", -> it "should call the callback", -> @callback.called.should.equal true + describe "successfully with multiple changes", -> + beforeEach -> + @DocumentManager.acceptChanges @project_id, @doc_id, @change_ids, @callback + + it "should apply the accept change to the ranges", -> + @RangesManager.acceptChanges + .calledWith(@change_ids, @ranges) + .should.equal true + describe "when the doc is not found", -> beforeEach -> @DocumentManager.getDoc = sinon.stub().yields(null, null, null, null) - @DocumentManager.acceptChange @project_id, @doc_id, @change_id, @callback + @DocumentManager.acceptChanges @project_id, @doc_id, [ @change_id ], @callback it "should not save anything", -> @RedisManager.updateDocument.called.should.equal false @@ -397,7 +407,7 @@ describe "DocumentManager", -> describe "when the doc is not found", -> beforeEach -> @DocumentManager.getDoc = sinon.stub().yields(null, null, null, null) - @DocumentManager.acceptChange @project_id, @doc_id, @comment_id, @callback + @DocumentManager.acceptChanges @project_id, @doc_id, [ @comment_id ], @callback it "should not save anything", -> @RedisManager.updateDocument.called.should.equal false diff --git a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee index 69b40c85d2..617146e787 100644 --- a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee +++ b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee @@ -335,7 +335,7 @@ describe "HttpController", -> .calledWith(new Error("oops")) .should.equal true - describe "acceptChange", -> + describe "acceptChanges", -> beforeEach -> @req = params: @@ -343,14 +343,14 @@ describe "HttpController", -> doc_id: @doc_id change_id: @change_id = "mock-change-od-1" - describe "successfully", -> + describe "successfully with a single change", -> beforeEach -> - @DocumentManager.acceptChangeWithLock = sinon.stub().callsArgWith(3) - @HttpController.acceptChange(@req, @res, @next) + @DocumentManager.acceptChangesWithLock = sinon.stub().callsArgWith(3) + @HttpController.acceptChanges(@req, @res, @next) it "should accept the change", -> - @DocumentManager.acceptChangeWithLock - .calledWith(@project_id, @doc_id, @change_id) + @DocumentManager.acceptChangesWithLock + .calledWith(@project_id, @doc_id, [ @change_id ]) .should.equal true it "should return a successful No Content response", -> @@ -360,16 +360,34 @@ describe "HttpController", -> it "should log the request", -> @logger.log - .calledWith({@project_id, @doc_id, @change_id}, "accepting change via http") + .calledWith({@project_id, @doc_id}, "accepting 1 changes via http") .should.equal true it "should time the request", -> @Metrics.Timer::done.called.should.equal true + describe "succesfully with with multiple changes", -> + beforeEach -> + @change_ids = [ "mock-change-od-1", "mock-change-od-2", "mock-change-od-3", "mock-change-od-4" ] + @req.body = + change_ids: @change_ids + @DocumentManager.acceptChangesWithLock = sinon.stub().callsArgWith(3) + @HttpController.acceptChanges(@req, @res, @next) + + it "should accept the changes in the body payload", -> + @DocumentManager.acceptChangesWithLock + .calledWith(@project_id, @doc_id, @change_ids) + .should.equal true + + it "should log the request with the correct number of changes", -> + @logger.log + .calledWith({@project_id, @doc_id}, "accepting #{ @change_ids.length } changes via http") + .should.equal true + describe "when an errors occurs", -> beforeEach -> - @DocumentManager.acceptChangeWithLock = sinon.stub().callsArgWith(3, new Error("oops")) - @HttpController.acceptChange(@req, @res, @next) + @DocumentManager.acceptChangesWithLock = sinon.stub().callsArgWith(3, new Error("oops")) + @HttpController.acceptChanges(@req, @res, @next) it "should call next with the error", -> @next diff --git a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee index fd3bc8faec..b11c73489e 100644 --- a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee @@ -10,7 +10,7 @@ describe "RangesManager", -> @RangesManager = SandboxedModule.require modulePath, requires: "logger-sharelatex": @logger = { error: sinon.stub(), log: sinon.stub(), warn: sinon.stub() } - + @doc_id = "doc-id-123" @project_id = "project-id-123" @user_id = "user-id-123" @@ -178,4 +178,108 @@ describe "RangesManager", -> @callback.called.should.equal true [error, entries] = @callback.args[0] expect(error).to.not.be.null - expect(error.message).to.equal("Change ({\"op\":{\"i\":\"five\",\"p\":15},\"metadata\":{\"user_id\":\"user-id-123\"}}) doesn't match text (\"our \")") \ No newline at end of file + expect(error.message).to.equal("Change ({\"op\":{\"i\":\"five\",\"p\":15},\"metadata\":{\"user_id\":\"user-id-123\"}}) doesn't match text (\"our \")") + + describe "acceptChanges", -> + beforeEach -> + @RangesManager = SandboxedModule.require modulePath, + requires: + "logger-sharelatex": @logger = { error: sinon.stub(), log: sinon.stub(), warn: sinon.stub() } + "./RangesTracker":@RangesTracker = SandboxedModule.require "../../../../app/js/RangesTracker.js" + + @ranges = { + comments: [] + changes: [{ + id: "a1" + op: + i: "lorem" + p: 0 + }, { + id: "a2" + op: + i: "ipsum" + p: 10 + }, { + id: "a3" + op: + i: "dolor" + p: 20 + }, { + id: "a4" + op: + i: "sit" + p: 30 + }, { + id: "a5" + op: + i: "amet" + p: 40 + }] + } + @removeChangeIdsSpy = sinon.spy @RangesTracker.prototype, "removeChangeIds" + + describe "successfully with a single change", -> + beforeEach (done) -> + @change_ids = [ @ranges.changes[1].id ] + @RangesManager.acceptChanges @change_ids, @ranges, (err, ranges) => + @rangesResponse = ranges + done() + + it "should log the call with the correct number of changes", -> + @logger.log + .calledWith("accepting 1 changes in ranges") + .should.equal true + + it "should delegate the change removal to the ranges tracker", -> + @removeChangeIdsSpy + .calledWith(@change_ids) + .should.equal true + + it "should remove the change", -> + expect(@rangesResponse.changes + .find((change) => change.id == @ranges.changes[1].id)) + .to.be.undefined + + it "should return the original number of changes minus 1", -> + @rangesResponse.changes.length + .should.equal @ranges.changes.length - 1 + + it "should not touch other changes", -> + for i in [ 0, 2, 3, 4] + expect(@rangesResponse.changes + .find((change) => change.id == @ranges.changes[i].id)) + .to.deep.equal @ranges.changes[i] + + describe "successfully with multiple changes", -> + beforeEach (done) -> + @change_ids = [ @ranges.changes[1].id, @ranges.changes[3].id, @ranges.changes[4].id ] + @RangesManager.acceptChanges @change_ids, @ranges, (err, ranges) => + @rangesResponse = ranges + done() + + it "should log the call with the correct number of changes", -> + @logger.log + .calledWith("accepting #{ @change_ids.length } changes in ranges") + .should.equal true + + it "should delegate the change removal to the ranges tracker", -> + @removeChangeIdsSpy + .calledWith(@change_ids) + .should.equal true + + it "should remove the changes", -> + for i in [ 1, 3, 4] + expect(@rangesResponse.changes + .find((change) => change.id == @ranges.changes[1].id)) + .to.be.undefined + + it "should return the original number of changes minus the number of accepted changes", -> + @rangesResponse.changes.length + .should.equal @ranges.changes.length - 3 + + it "should not touch other changes", -> + for i in [ 0, 2 ] + expect(@rangesResponse.changes + .find((change) => change.id == @ranges.changes[i].id)) + .to.deep.equal @ranges.changes[i] +