From 0199f2e1297bde87106db5760b39de867fe93395 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 14 May 2014 13:28:17 +0100 Subject: [PATCH] Use version from web, with fallback to old mongo collection --- services/document-updater/Gruntfile.coffee | 3 +- .../app/coffee/DocOpsManager.coffee | 1 + .../app/coffee/PersistenceManager.coffee | 29 +++++- .../app/coffee/mongojs.coffee | 6 ++ .../coffee/ApplyingUpdatesToADocTests.coffee | 90 +++++++++++++++--- .../DocOpsManager/DocOpsManagerTests.coffee | 8 +- .../getDocFromWebTests.coffee | 86 +++++++++++++++++ .../PersistenceManager/getDocTests.coffee | 94 ++++++++----------- .../getDocVersionInMongoTests.coffee | 46 +++++++++ .../test/unit/js/module-loader.js | 29 ------ 10 files changed, 284 insertions(+), 108 deletions(-) create mode 100644 services/document-updater/app/coffee/mongojs.coffee create mode 100644 services/document-updater/test/unit/coffee/PersistenceManager/getDocFromWebTests.coffee create mode 100644 services/document-updater/test/unit/coffee/PersistenceManager/getDocVersionInMongoTests.coffee delete mode 100644 services/document-updater/test/unit/js/module-loader.js diff --git a/services/document-updater/Gruntfile.coffee b/services/document-updater/Gruntfile.coffee index 8c96ea0650..3497455a57 100644 --- a/services/document-updater/Gruntfile.coffee +++ b/services/document-updater/Gruntfile.coffee @@ -52,6 +52,7 @@ module.exports = (grunt) -> clean: app: ["app/js"] acceptance_tests: ["test/acceptance/js"] + unit_tests: ["test/unit/js"] mochaTest: unit: @@ -102,7 +103,7 @@ module.exports = (grunt) -> grunt.registerTask 'help', 'Display this help list', 'availabletasks' grunt.registerTask 'compile:server', 'Compile the server side coffee script', ['clean:app', 'coffee:app', 'coffee:app_dir'] - grunt.registerTask 'compile:unit_tests', 'Compile the unit tests', ['coffee:unit_tests'] + grunt.registerTask 'compile:unit_tests', 'Compile the unit tests', ['clean:unit_tests', 'coffee:unit_tests'] grunt.registerTask 'compile:acceptance_tests', 'Compile the acceptance tests', ['clean:acceptance_tests', 'coffee:acceptance_tests'] grunt.registerTask 'compile:tests', 'Compile all the tests', ['compile:acceptance_tests', 'compile:unit_tests'] grunt.registerTask 'compile', 'Compiles everything need to run document-updater-sharelatex', ['compile:server'] diff --git a/services/document-updater/app/coffee/DocOpsManager.coffee b/services/document-updater/app/coffee/DocOpsManager.coffee index a8896f8b12..a85a1e18ee 100644 --- a/services/document-updater/app/coffee/DocOpsManager.coffee +++ b/services/document-updater/app/coffee/DocOpsManager.coffee @@ -14,3 +14,4 @@ module.exports = DocOpsManager = return callback(error) if error? callback null, version + diff --git a/services/document-updater/app/coffee/PersistenceManager.coffee b/services/document-updater/app/coffee/PersistenceManager.coffee index 03cbe78cbe..6ac999629a 100644 --- a/services/document-updater/app/coffee/PersistenceManager.coffee +++ b/services/document-updater/app/coffee/PersistenceManager.coffee @@ -2,9 +2,35 @@ request = require "request" Settings = require "settings-sharelatex" Errors = require "./Errors" Metrics = require "./Metrics" +{db, ObjectId} = require("./mongojs") module.exports = PersistenceManager = - getDoc: (project_id, doc_id, _callback = (error, lines) ->) -> + getDoc: (project_id, doc_id, callback = (error, lines, version) ->) -> + PersistenceManager.getDocFromWeb project_id, doc_id, (error, lines, version) -> + return callback(error) if error? + if version? + callback null, lines, version + else + PersistenceManager.getDocVersionInMongo doc_id, (error, version) -> + return callback(error) if error? + if version? + callback null, lines, version + else + callback null, lines, 0 + + getDocVersionInMongo: (doc_id, callback = (error, version) ->) -> + db.docOps.find { + doc_id: ObjectId(doc_id) + }, { + version: 1 + }, (error, docs) -> + return callback(error) if error? + if docs.length < 1 or !docs[0].version? + return callback null, null + else + return callback null, docs[0].version + + getDocFromWeb: (project_id, doc_id, _callback = (error, lines, version) ->) -> timer = new Metrics.Timer("persistenceManager.getDoc") callback = (args...) -> timer.done() @@ -62,6 +88,5 @@ module.exports = PersistenceManager = return callback(new Errors.NotFoundError("doc not not found: #{url}")) else return callback(new Error("error accessing web API: #{url} #{res.statusCode}")) - diff --git a/services/document-updater/app/coffee/mongojs.coffee b/services/document-updater/app/coffee/mongojs.coffee new file mode 100644 index 0000000000..cf9f5fec86 --- /dev/null +++ b/services/document-updater/app/coffee/mongojs.coffee @@ -0,0 +1,6 @@ +Settings = require "settings-sharelatex" +mongojs = require "mongojs" +db = mongojs.connect(Settings.mongo.url, ["docOps"]) +module.exports = + db: db + ObjectId: mongojs.ObjectId diff --git a/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee b/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee index 1810d222e5..94730b406c 100644 --- a/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee +++ b/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee @@ -3,6 +3,7 @@ chai = require("chai") chai.should() async = require "async" rclient = require("redis").createClient() +{db, ObjectId} = require "../../../app/js/mongojs" MockTrackChangesApi = require "./helpers/MockTrackChangesApi" MockWebApi = require "./helpers/MockWebApi" @@ -92,9 +93,9 @@ describe "Applying updates to a doc", -> describe "when the ops come in a single linear order", -> before -> [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] - @lines = ["", "", ""] + lines = ["", "", ""] MockWebApi.insertDoc @project_id, @doc_id, { - lines: @lines + lines: lines version: 0 } @@ -111,7 +112,7 @@ describe "Applying updates to a doc", -> { doc_id: @doc_id, v: 9, op: [i: "l", p: 9 ] } { doc_id: @doc_id, v: 10, op: [i: "d", p: 10] } ] - @result = ["hello world", "", ""] + @my_result = ["hello world", "", ""] it "should be able to continue applying updates when the project has been deleted", (done) -> actions = [] @@ -126,7 +127,7 @@ describe "Applying updates to a doc", -> async.series actions, (error) => throw error if error? DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, doc) => - doc.lines.should.deep.equal @result + doc.lines.should.deep.equal @my_result done() it "should push the applied updates to the track changes api", (done) -> @@ -142,9 +143,9 @@ describe "Applying updates to a doc", -> describe "when older ops come in after the delete", -> before -> [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] - @lines = ["", "", ""] + lines = ["", "", ""] MockWebApi.insertDoc @project_id, @doc_id, { - lines: @lines + lines: lines version: 0 } @@ -156,7 +157,7 @@ describe "Applying updates to a doc", -> { doc_id: @doc_id, v: 4, op: [i: "o", p: 4 ] } { doc_id: @doc_id, v: 0, op: [i: "world", p: 1 ] } ] - @result = ["hello", "world", ""] + @my_result = ["hello", "world", ""] it "should be able to continue applying updates when the project has been deleted", (done) -> actions = [] @@ -171,7 +172,7 @@ describe "Applying updates to a doc", -> async.series actions, (error) => throw error if error? DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, doc) => - doc.lines.should.deep.equal @result + doc.lines.should.deep.equal @my_result done() describe "with a broken update", -> @@ -191,28 +192,91 @@ describe "Applying updates to a doc", -> done() describe "with enough updates to flush to the track changes api", -> - beforeEach (done) -> + before (done) -> [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] MockWebApi.insertDoc @project_id, @doc_id, { lines: @lines version: 0 } - @updates = [] + updates = [] for v in [0..99] # Should flush after 50 ops - @updates.push + updates.push doc_id: @doc_id, op: [i: v.toString(), p: 0] v: v sinon.spy MockTrackChangesApi, "flushDoc" - DocUpdaterClient.sendUpdates @project_id, @doc_id, @updates, (error) => + DocUpdaterClient.sendUpdates @project_id, @doc_id, updates, (error) => throw error if error? setTimeout done, 200 - afterEach -> + after -> MockTrackChangesApi.flushDoc.restore() it "should flush the doc twice", -> MockTrackChangesApi.flushDoc.calledTwice.should.equal true + describe "when the document does not have a version in the web api but does in Mongo", -> + 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.sendUpdate @project_id, @doc_id, @update, (error) -> + throw error if error? + setTimeout done, 200 + + it "should update the doc (using the mongo version)", (done) -> + DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, doc) => + doc.lines.should.deep.equal @result + done() + + describe "when the document version in the web api is ahead of Mongo", -> + before (done) -> + [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] + MockWebApi.insertDoc @project_id, @doc_id, { + lines: @lines + version: @version + } + + db.docOps.insert { + doc_id: ObjectId(@doc_id) + version: @version - 20 + }, (error) => + throw error if error? + DocUpdaterClient.sendUpdate @project_id, @doc_id, @update, (error) -> + throw error if error? + setTimeout done, 200 + + it "should update the doc (using the web version)", (done) -> + DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, doc) => + doc.lines.should.deep.equal @result + done() + + describe "when there is no version yet", -> + before (done) -> + [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] + MockWebApi.insertDoc @project_id, @doc_id, { + lines: @lines + } + + update = + doc: @doc_id + op: @update.op + v: 0 + DocUpdaterClient.sendUpdate @project_id, @doc_id, update, (error) -> + throw error if error? + setTimeout done, 200 + + it "should update the doc (using version = 0)", (done) -> + DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, doc) => + doc.lines.should.deep.equal @result + done() + diff --git a/services/document-updater/test/unit/coffee/DocOpsManager/DocOpsManagerTests.coffee b/services/document-updater/test/unit/coffee/DocOpsManager/DocOpsManagerTests.coffee index 04814241e4..6f6094c855 100644 --- a/services/document-updater/test/unit/coffee/DocOpsManager/DocOpsManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/DocOpsManager/DocOpsManagerTests.coffee @@ -6,15 +6,11 @@ SandboxedModule = require('sandboxed-module') describe "DocOpsManager", -> beforeEach -> - @doc_id = "doc-id" - @project_id = "project-id" + @doc_id = ObjectId().toString() + @project_id = ObjectId().toString() @callback = sinon.stub() @DocOpsManager = SandboxedModule.require modulePath, requires: "./RedisManager": @RedisManager = {} - "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } - "./Metrics": @Metrics = - Timer: class Timer - done: sinon.stub() "./TrackChangesManager": @TrackChangesManager = {} describe "getPreviousDocOps", -> diff --git a/services/document-updater/test/unit/coffee/PersistenceManager/getDocFromWebTests.coffee b/services/document-updater/test/unit/coffee/PersistenceManager/getDocFromWebTests.coffee new file mode 100644 index 0000000000..b2aebdd84d --- /dev/null +++ b/services/document-updater/test/unit/coffee/PersistenceManager/getDocFromWebTests.coffee @@ -0,0 +1,86 @@ +sinon = require('sinon') +chai = require('chai') +should = chai.should() +modulePath = "../../../../app/js/PersistenceManager.js" +SandboxedModule = require('sandboxed-module') +Errors = require "../../../../app/js/Errors" + +describe "PersistenceManager.getDocFromWeb", -> + beforeEach -> + @PersistenceManager = SandboxedModule.require modulePath, requires: + "request": @request = sinon.stub() + "settings-sharelatex": @Settings = {} + "./Metrics": @Metrics = + Timer: class Timer + done: sinon.stub() + @project_id = "project-id-123" + @doc_id = "doc-id-123" + @lines = ["one", "two", "three"] + @version = 42 + @callback = sinon.stub() + @Settings.apis = + web: + url: @url = "www.example.com" + user: @user = "sharelatex" + pass: @pass = "password" + + describe "with a successful response from the web api", -> + beforeEach -> + @request.callsArgWith(1, null, {statusCode: 200}, JSON.stringify(lines: @lines, version: @version)) + @PersistenceManager.getDocFromWeb(@project_id, @doc_id, @callback) + + it "should call the web api", -> + @request + .calledWith({ + url: "#{@url}/project/#{@project_id}/doc/#{@doc_id}" + method: "GET" + headers: + "accept": "application/json" + auth: + user: @user + pass: @pass + sendImmediately: true + jar: false + }) + .should.equal true + + it "should call the callback with the doc lines and version", -> + @callback.calledWith(null, @lines, @version).should.equal true + + it "should time the execution", -> + @Metrics.Timer::done.called.should.equal true + + describe "when request returns an error", -> + beforeEach -> + @request.callsArgWith(1, @error = new Error("oops"), null, null) + @PersistenceManager.getDocFromWeb(@project_id, @doc_id, @callback) + + it "should return the error", -> + @callback.calledWith(@error).should.equal true + + it "should time the execution", -> + @Metrics.Timer::done.called.should.equal true + + describe "when the request returns 404", -> + beforeEach -> + @request.callsArgWith(1, null, {statusCode: 404}, "") + @PersistenceManager.getDocFromWeb(@project_id, @doc_id, @callback) + + it "should return a NotFoundError", -> + @callback.calledWith(new Errors.NotFoundError("not found")).should.equal true + + it "should time the execution", -> + @Metrics.Timer::done.called.should.equal true + + describe "when the request returns an error status code", -> + beforeEach -> + @request.callsArgWith(1, null, {statusCode: 500}, "") + @PersistenceManager.getDocFromWeb(@project_id, @doc_id, @callback) + + it "should return an error", -> + @callback.calledWith(new Error("web api error")).should.equal true + + it "should time the execution", -> + @Metrics.Timer::done.called.should.equal true + + diff --git a/services/document-updater/test/unit/coffee/PersistenceManager/getDocTests.coffee b/services/document-updater/test/unit/coffee/PersistenceManager/getDocTests.coffee index 0bb881b3ee..50ba0984ac 100644 --- a/services/document-updater/test/unit/coffee/PersistenceManager/getDocTests.coffee +++ b/services/document-updater/test/unit/coffee/PersistenceManager/getDocTests.coffee @@ -3,7 +3,7 @@ chai = require('chai') should = chai.should() modulePath = "../../../../app/js/PersistenceManager.js" SandboxedModule = require('sandboxed-module') -Errors = require "../../../../app/js/Errors" +{ObjectId} = require("mongojs") describe "PersistenceManager.getDoc", -> beforeEach -> @@ -13,74 +13,54 @@ describe "PersistenceManager.getDoc", -> "./Metrics": @Metrics = Timer: class Timer done: sinon.stub() - @project_id = "project-id-123" - @doc_id = "doc-id-123" - @lines = ["one", "two", "three"] - @version = 42 + "./mongojs": + db: @db = { docOps: {} } + ObjectId: ObjectId + + @project_id = ObjectId().toString() + @doc_id = ObjectId().toString() @callback = sinon.stub() - @Settings.apis = - web: - url: @url = "www.example.com" - user: @user = "sharelatex" - pass: @pass = "password" + @lines = ["mock", "doc", "lines"] + @version = 42 - describe "with a successful response from the web api", -> + describe "when the version is set in the web api", -> beforeEach -> - @request.callsArgWith(1, null, {statusCode: 200}, JSON.stringify(lines: @lines, version: @version)) - @PersistenceManager.getDoc(@project_id, @doc_id, @callback) + @PersistenceManager.getDocFromWeb = sinon.stub().callsArgWith(2, null, @lines, @version) + @PersistenceManager.getDocVersionInMongo = sinon.stub() + @PersistenceManager.getDoc @project_id, @doc_id, @callback - it "should call the web api", -> - @request - .calledWith({ - url: "#{@url}/project/#{@project_id}/doc/#{@doc_id}" - method: "GET" - headers: - "accept": "application/json" - auth: - user: @user - pass: @pass - sendImmediately: true - jar: false - }) + it "should look up the doc in the web api", -> + @PersistenceManager.getDocFromWeb + .calledWith(@project_id, @doc_id) .should.equal true - it "should call the callback with the doc lines and version", -> + it "should not look up the version in Mongo", -> + @PersistenceManager.getDocVersionInMongo + .called.should.equal false + + it "should call the callback with the lines and version", -> @callback.calledWith(null, @lines, @version).should.equal true - it "should time the execution", -> - @Metrics.Timer::done.called.should.equal true - - describe "when request returns an error", -> + describe "when the version is not set in the web api, but is in Mongo", -> beforeEach -> - @request.callsArgWith(1, @error = new Error("oops"), null, null) - @PersistenceManager.getDoc(@project_id, @doc_id, @callback) + @PersistenceManager.getDocFromWeb = sinon.stub().callsArgWith(2, null, @lines, null) + @PersistenceManager.getDocVersionInMongo = sinon.stub().callsArgWith(1, null, @version) + @PersistenceManager.getDoc @project_id, @doc_id, @callback - it "should return the error", -> - @callback.calledWith(@error).should.equal true + it "should look up the version in Mongo", -> + @PersistenceManager.getDocVersionInMongo + .calledWith(@doc_id) + .should.equal true - it "should time the execution", -> - @Metrics.Timer::done.called.should.equal true + it "should call the callback with the lines and version", -> + @callback.calledWith(null, @lines, @version).should.equal true - describe "when the request returns 404", -> + describe "when the version is not set", -> beforeEach -> - @request.callsArgWith(1, null, {statusCode: 404}, "") - @PersistenceManager.getDoc(@project_id, @doc_id, @callback) - - it "should return a NotFoundError", -> - @callback.calledWith(new Errors.NotFoundError("not found")).should.equal true + @PersistenceManager.getDocFromWeb = sinon.stub().callsArgWith(2, null, @lines, null) + @PersistenceManager.getDocVersionInMongo = sinon.stub().callsArgWith(1, null, null) + @PersistenceManager.getDoc @project_id, @doc_id, @callback - it "should time the execution", -> - @Metrics.Timer::done.called.should.equal true + it "should call the callback with the lines and version = 0", -> + @callback.calledWith(null, @lines, 0).should.equal true - describe "when the request returns an error status code", -> - beforeEach -> - @request.callsArgWith(1, null, {statusCode: 500}, "") - @PersistenceManager.getDoc(@project_id, @doc_id, @callback) - - it "should return an error", -> - @callback.calledWith(new Error("web api error")).should.equal true - - it "should time the execution", -> - @Metrics.Timer::done.called.should.equal true - - diff --git a/services/document-updater/test/unit/coffee/PersistenceManager/getDocVersionInMongoTests.coffee b/services/document-updater/test/unit/coffee/PersistenceManager/getDocVersionInMongoTests.coffee new file mode 100644 index 0000000000..bbe6c43c48 --- /dev/null +++ b/services/document-updater/test/unit/coffee/PersistenceManager/getDocVersionInMongoTests.coffee @@ -0,0 +1,46 @@ +sinon = require('sinon') +chai = require('chai') +should = chai.should() +modulePath = "../../../../app/js/PersistenceManager.js" +SandboxedModule = require('sandboxed-module') +Errors = require "../../../../app/js/Errors" +{ObjectId} = require("mongojs") + +describe "PersistenceManager.getDocVersionInMongo", -> + beforeEach -> + @PersistenceManager = SandboxedModule.require modulePath, requires: + "request": @request = sinon.stub() + "settings-sharelatex": @Settings = {} + "./Metrics": @Metrics = + Timer: class Timer + done: sinon.stub() + "./mongojs": + db: @db = { docOps: {} } + ObjectId: ObjectId + + @doc_id = ObjectId().toString() + @callback = sinon.stub() + + describe "getDocVersionInMongo", -> + describe "when the doc exists", -> + beforeEach -> + @doc = + version: @version = 42 + @db.docOps.find = sinon.stub().callsArgWith(2, null, [@doc]) + @PersistenceManager.getDocVersionInMongo @doc_id, @callback + + it "should look for the doc in the database", -> + @db.docOps.find + .calledWith({ doc_id: ObjectId(@doc_id) }, {version: 1}) + .should.equal true + + it "should call the callback with the version", -> + @callback.calledWith(null, @version).should.equal true + + describe "when the doc doesn't exist", -> + beforeEach -> + @db.docOps.find = sinon.stub().callsArgWith(2, null, []) + @PersistenceManager.getDocVersionInMongo @doc_id, @callback + + it "should call the callback with null", -> + @callback.calledWith(null, null).should.equal true \ No newline at end of file diff --git a/services/document-updater/test/unit/js/module-loader.js b/services/document-updater/test/unit/js/module-loader.js deleted file mode 100644 index ac4cae7601..0000000000 --- a/services/document-updater/test/unit/js/module-loader.js +++ /dev/null @@ -1,29 +0,0 @@ -var vm = require('vm'); -var fs = require('fs'); -var path = require('path'); - -module.exports.loadModule = function(filePath, mocks) { - mocks = mocks || {}; - - // this is necessary to allow relative path modules within loaded file - // i.e. requiring ./some inside file /a/b.js needs to be resolved to /a/some - var resolveModule = function(module) { - if (module.charAt(0) !== '.') return module; - return path.resolve(path.dirname(filePath), module); - }; - - var exports = {}; - var context = { - require: function(name) { - return mocks[name] || require(resolveModule(name)); - }, - console: console, - exports: exports, - module: { - exports: exports - } - }; - file = fs.readFileSync(filePath); - vm.runInNewContext(file, context); - return context; -};