diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index 258784a48c..1b1afeda3c 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -12,7 +12,7 @@ module.exports = DocumentManager = timer.done() _callback(args...) - RedisManager.getDoc doc_id, (error, lines, version) -> + RedisManager.getDoc doc_id, (error, lines, version, alreadyLoaded) -> return callback(error) if error? if !lines? or !version? logger.log project_id: project_id, doc_id: doc_id, "doc not in redis so getting from persistence API" @@ -21,9 +21,9 @@ module.exports = DocumentManager = logger.log project_id: project_id, doc_id: doc_id, lines: lines, version: version, "got doc from persistence API" RedisManager.putDocInMemory project_id, doc_id, lines, version, (error) -> return callback(error) if error? - callback null, lines, version + callback null, lines, version, false else - callback null, lines, version + callback null, lines, version, true getDocAndRecentOps: (project_id, doc_id, fromVersion, _callback = (error, lines, version, recentOps) ->) -> timer = new Metrics.Timer("docManager.getDocAndRecentOps") @@ -50,7 +50,7 @@ module.exports = DocumentManager = return callback(new Error("No lines were provided to setDoc")) UpdateManager = require "./UpdateManager" - DocumentManager.getDoc project_id, doc_id, (error, oldLines, version) -> + DocumentManager.getDoc project_id, doc_id, (error, oldLines, version, alreadyLoaded) -> return callback(error) if error? if oldLines? and oldLines.length > 0 and oldLines[0].text? @@ -70,9 +70,18 @@ module.exports = DocumentManager = user_id: user_id UpdateManager.applyUpdates project_id, doc_id, [update], (error) -> return callback(error) if error? - DocumentManager.flushDocIfLoaded project_id, doc_id, (error) -> - return callback(error) if error? - callback null + # If the document was loaded already, then someone has it open + # in a project, and the usual flushing mechanism will happen. + # Otherwise we should remove it immediately since nothing else + # is using it. + if alreadyLoaded + DocumentManager.flushDocIfLoaded project_id, doc_id, (error) -> + return callback(error) if error? + callback null + else + DocumentManager.flushAndDeleteDoc project_id, doc_id, (error) -> + return callback(error) if error? + callback null flushDocIfLoaded: (project_id, doc_id, _callback = (error) ->) -> diff --git a/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee b/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee index f213458168..e9b5da5b1d 100644 --- a/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee +++ b/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee @@ -360,7 +360,7 @@ describe "Applying updates to a large doc (uses compression)", -> throw error if error? DocUpdaterClient.sendUpdates @project_id, @doc_id, updates, (error) => throw error if error? - setTimeout done, 200 + setTimeout done, 500 after -> MockTrackChangesApi.flushDoc.restore() diff --git a/services/document-updater/test/acceptance/coffee/SettingADocumentTests.coffee b/services/document-updater/test/acceptance/coffee/SettingADocumentTests.coffee index 5dfc5d95f0..cf0d224995 100644 --- a/services/document-updater/test/acceptance/coffee/SettingADocumentTests.coffee +++ b/services/document-updater/test/acceptance/coffee/SettingADocumentTests.coffee @@ -1,14 +1,16 @@ sinon = require "sinon" chai = require("chai") chai.should() +expect = require("chai").expect {db, ObjectId} = require "../../../app/js/mongojs" +rclient = require("redis").createClient() +MockTrackChangesApi = require "./helpers/MockTrackChangesApi" MockWebApi = require "./helpers/MockWebApi" DocUpdaterClient = require "./helpers/DocUpdaterClient" describe "Setting a document", -> - before (done) -> - [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] + before -> @lines = ["one", "two", "three"] @version = 42 @update = @@ -23,30 +25,31 @@ describe "Setting a document", -> @source = "dropbox" @user_id = "user-id-123" - MockWebApi.insertDoc @project_id, @doc_id, lines: @lines - db.docOps.insert { - doc_id: ObjectId(@doc_id) - version: @version - }, (error) => - throw error if error? - done() + sinon.spy MockTrackChangesApi, "flushDoc" + sinon.spy MockWebApi, "setDocumentLines" + + after -> + MockWebApi.setDocumentLines.restore() + MockTrackChangesApi.flushDoc.restore() - describe "when the updated doc exists in the doc updater", -> before (done) -> - sinon.spy MockWebApi, "setDocumentLines" - DocUpdaterClient.preloadDoc @project_id, @doc_id, (error) => + [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] + MockWebApi.insertDoc @project_id, @doc_id, lines: @lines + db.docOps.insert { + doc_id: ObjectId(@doc_id) + version: @version + }, (error) => throw error if error? - DocUpdaterClient.sendUpdate @project_id, @doc_id, @update, (error) => + DocUpdaterClient.preloadDoc @project_id, @doc_id, (error) => throw error if error? - setTimeout () => - DocUpdaterClient.setDocLines @project_id, @doc_id, @newLines, @source, @user_id, (error, res, body) => - @statusCode = res.statusCode - done() - , 200 - - after -> - MockWebApi.setDocumentLines.restore() + DocUpdaterClient.sendUpdate @project_id, @doc_id, @update, (error) => + throw error if error? + setTimeout () => + DocUpdaterClient.setDocLines @project_id, @doc_id, @newLines, @source, @user_id, (error, res, body) => + @statusCode = res.statusCode + done() + , 200 it "should return a 204 status code", -> @statusCode.should.equal 204 @@ -65,4 +68,39 @@ describe "Setting a document", -> DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, doc) => doc.version.should.equal @version + 2 done() + + it "should leave the document in redis", (done) -> + rclient.get "doclines:#{@doc_id}", (error, lines) => + throw error if error? + expect(JSON.parse(lines)).to.deep.equal @newLines + done() + + describe "when the updated doc does not exist in the doc updater", -> + before (done) -> + [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] + MockWebApi.insertDoc @project_id, @doc_id, lines: @lines + db.docOps.insert { + doc_id: ObjectId(@doc_id) + version: @version + }, (error) => + throw error if error? + DocUpdaterClient.setDocLines @project_id, @doc_id, @newLines, @source, @user_id, (error, res, body) => + @statusCode = res.statusCode + done() + + it "should return a 204 status code", -> + @statusCode.should.equal 204 + it "should send the updated doc lines to the web api", -> + MockWebApi.setDocumentLines + .calledWith(@project_id, @doc_id, @newLines) + .should.equal true + + it "should flush track changes"#, -> + # MockTrackChangesApi.flushDoc.calledWith(@doc_id).should.equal true + + it "should remove the document from redis", (done) -> + rclient.get "doclines:#{@doc_id}", (error, lines) => + throw error if error? + expect(lines).to.not.exist + done() diff --git a/services/document-updater/test/unit/coffee/DocumentManager/getDocTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/getDocTests.coffee index ea68890199..0db51ddba8 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/getDocTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/getDocTests.coffee @@ -32,7 +32,7 @@ describe "DocumentUpdater - getDoc", -> .should.equal true it "should call the callback with the doc info", -> - @callback.calledWith(null, @lines, @version).should.equal true + @callback.calledWith(null, @lines, @version, true).should.equal true it "should time the execution", -> @Metrics.Timer::done.called.should.equal true @@ -60,7 +60,7 @@ describe "DocumentUpdater - getDoc", -> .should.equal true it "should call the callback with the doc info", -> - @callback.calledWith(null, @lines, @version).should.equal true + @callback.calledWith(null, @lines, @version, false).should.equal true it "should time the execution", -> @Metrics.Timer::done.called.should.equal true diff --git a/services/document-updater/test/unit/coffee/DocumentManager/setDocTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/setDocTests.coffee index b827b584f8..76067f943e 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/setDocTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/setDocTests.coffee @@ -29,13 +29,14 @@ describe "DocumentManager - setDoc", -> beforeEach -> @beforeLines = ["before", "lines"] @afterLines = ["after", "lines"] + @DocumentManager.getDoc = sinon.stub().callsArgWith(2, null, @beforeLines, @version, true) + @DiffCodec.diffAsShareJsOp = sinon.stub().callsArgWith(2, null, @ops) + @UpdateManager.applyUpdates = sinon.stub().callsArgWith(3, null) + @DocumentManager.flushDocIfLoaded = sinon.stub().callsArg(2) + @DocumentManager.flushAndDeleteDoc = sinon.stub().callsArg(2) - describe "successfully", -> + describe "when already loaded", -> beforeEach -> - @DocumentManager.getDoc = sinon.stub().callsArgWith(2, null, @beforeLines, @version) - @DiffCodec.diffAsShareJsOp = sinon.stub().callsArgWith(2, null, @ops) - @UpdateManager.applyUpdates = sinon.stub().callsArgWith(3, null) - @DocumentManager.flushDocIfLoaded = sinon.stub().callsArg(2) @DocumentManager.setDoc @project_id, @doc_id, @afterLines, @source, @user_id, @callback it "should get the current doc lines", -> @@ -76,17 +77,26 @@ describe "DocumentManager - setDoc", -> it "should time the execution", -> @Metrics.Timer::done.called.should.equal true + + describe "when not already loaded", -> + beforeEach -> + @DocumentManager.getDoc = sinon.stub().callsArgWith(2, null, @beforeLines, @version, false) + @DocumentManager.setDoc @project_id, @doc_id, @afterLines, @source, @user_id, @callback - describe "without new lines", -> - beforeEach -> - @DocumentManager.getDoc = sinon.stub().callsArgWith(2, null, @beforeLines, @version) - @DocumentManager.setDoc @project_id, @doc_id, null, @callback + it "should flush and delete the doc from the doc updater", -> + @DocumentManager.flushAndDeleteDoc + .calledWith(@project_id, @doc_id) + .should.equal true - it "should return teh callback with an error", -> - @callback.calledWith(new Error("No lines were passed to setDoc")) - - it "should not try to get the doc lines", -> - @DocumentManager.getDoc.called.should.equal false + describe "without new lines", -> + beforeEach -> + @DocumentManager.setDoc @project_id, @doc_id, null, @source, @user_id, @callback + + it "should return the callback with an error", -> + @callback.calledWith(new Error("No lines were passed to setDoc")) + + it "should not try to get the doc lines", -> + @DocumentManager.getDoc.called.should.equal false