From 3d76f4b9bfdd6d90d4b6c9c9e55b19996df43a6a Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 11 Apr 2019 13:25:03 +0100 Subject: [PATCH 1/3] Record a snapshot to mongo when a doc's comments/changes get collapsed --- .../app/coffee/RangesManager.coffee | 24 ++++-- .../app/coffee/RedisManager.coffee | 1 - .../app/coffee/SnapshotManager.coffee | 39 +++++++++ .../app/coffee/UpdateManager.coffee | 11 ++- .../app/coffee/mongojs.coffee | 7 ++ services/document-updater/npm-shrinkwrap.json | 86 +++++++++++++++++-- services/document-updater/package.json | 5 +- .../test/acceptance/coffee/RangesTests.coffee | 66 +++++++++++++- .../ProjectManager/updateProjectTests.coffee | 2 +- .../RangesManager/RangesManagerTests.coffee | 33 ++++++- .../UpdateManager/UpdateManagerTests.coffee | 21 ++++- 11 files changed, 274 insertions(+), 21 deletions(-) create mode 100644 services/document-updater/app/coffee/SnapshotManager.coffee create mode 100644 services/document-updater/app/coffee/mongojs.coffee diff --git a/services/document-updater/app/coffee/RangesManager.coffee b/services/document-updater/app/coffee/RangesManager.coffee index d0653bb6a2..bcb16a39c9 100644 --- a/services/document-updater/app/coffee/RangesManager.coffee +++ b/services/document-updater/app/coffee/RangesManager.coffee @@ -1,13 +1,15 @@ RangesTracker = require "./RangesTracker" logger = require "logger-sharelatex" +_ = require "lodash" module.exports = RangesManager = MAX_COMMENTS: 500 MAX_CHANGES: 2000 - applyUpdate: (project_id, doc_id, entries = {}, updates = [], newDocLines, callback = (error, new_entries) ->) -> - {changes, comments} = entries + applyUpdate: (project_id, doc_id, entries = {}, updates = [], newDocLines, callback = (error, new_entries, ranges_were_collapsed) ->) -> + {changes, comments} = _.cloneDeep(entries) rangesTracker = new RangesTracker(changes, comments) + emptyRangeCountBefore = RangesManager._emptyRangesCount(rangesTracker) for update in updates rangesTracker.track_changes = !!update.meta.tc if !!update.meta.tc @@ -29,9 +31,11 @@ module.exports = RangesManager = logger.error {err: error, project_id, doc_id, newDocLines, updates}, "error validating ranges" return callback(error) + emptyRangeCountAfter = RangesManager._emptyRangesCount(rangesTracker) + rangesWereCollapsed = emptyRangeCountAfter > emptyRangeCountBefore response = RangesManager._getRanges rangesTracker - logger.log {project_id, doc_id, changesCount: response.changes?.length, commentsCount: response.comments?.length}, "applied updates to ranges" - callback null, response + logger.log {project_id, doc_id, changesCount: response.changes?.length, commentsCount: response.comments?.length, rangesWereCollapsed}, "applied updates to ranges" + callback null, response, rangesWereCollapsed acceptChanges: (change_ids, ranges, callback = (error, ranges) ->) -> {changes, comments} = ranges @@ -59,4 +63,14 @@ module.exports = RangesManager = if rangesTracker.comments?.length > 0 response ?= {} response.comments = rangesTracker.comments - return response \ No newline at end of file + return response + + _emptyRangesCount: (ranges) -> + count = 0 + for comment in (ranges.comments or []) + if comment.op.c == "" + count++ + for change in (ranges.changes or []) when change.op.i? + if change.op.i == "" + count++ + return count \ No newline at end of file diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index 25dbafc6e7..76c7a9a8d0 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -1,6 +1,5 @@ Settings = require('settings-sharelatex') rclient = require("redis-sharelatex").createClient(Settings.redis.documentupdater) -_ = require('underscore') logger = require('logger-sharelatex') metrics = require('./Metrics') Errors = require "./Errors" diff --git a/services/document-updater/app/coffee/SnapshotManager.coffee b/services/document-updater/app/coffee/SnapshotManager.coffee new file mode 100644 index 0000000000..5a756b34db --- /dev/null +++ b/services/document-updater/app/coffee/SnapshotManager.coffee @@ -0,0 +1,39 @@ +{db, ObjectId} = require "./mongojs" + +module.exports = SnapshotManager = + recordSnapshot: (project_id, doc_id, version, lines, ranges, callback) -> + try + project_id = ObjectId(project_id) + doc_id = ObjectId(doc_id) + catch error + return callback(error) + db.docSnapshots.insert { + project_id, doc_id, version, lines + ranges: SnapshotManager.jsonRangesToMongo(ranges), + ts: new Date() + }, callback + + jsonRangesToMongo: (ranges) -> + return null if !ranges? + + updateMetadata = (metadata) -> + if metadata?.ts? + metadata.ts = new Date(metadata.ts) + if metadata?.user_id? + metadata.user_id = SnapshotManager._safeObjectId(metadata.user_id) + + for change in ranges.changes or [] + change.id = SnapshotManager._safeObjectId(change.id) + updateMetadata(change.metadata) + for comment in ranges.comments or [] + comment.id = SnapshotManager._safeObjectId(comment.id) + if comment.op?.t? + comment.op.t = SnapshotManager._safeObjectId(comment.op.t) + updateMetadata(comment.metadata) + return ranges + + _safeObjectId: (data) -> + try + return ObjectId(data) + catch error + return data diff --git a/services/document-updater/app/coffee/UpdateManager.coffee b/services/document-updater/app/coffee/UpdateManager.coffee index bfcfb806ca..5f9dcf5317 100644 --- a/services/document-updater/app/coffee/UpdateManager.coffee +++ b/services/document-updater/app/coffee/UpdateManager.coffee @@ -4,13 +4,14 @@ RealTimeRedisManager = require "./RealTimeRedisManager" ShareJsUpdateManager = require "./ShareJsUpdateManager" HistoryManager = require "./HistoryManager" Settings = require('settings-sharelatex') -_ = require("underscore") +_ = require("lodash") async = require("async") logger = require('logger-sharelatex') Metrics = require "./Metrics" Errors = require "./Errors" DocumentManager = require "./DocumentManager" RangesManager = require "./RangesManager" +SnapshotManager = require "./SnapshotManager" Profiler = require "./Profiler" module.exports = UpdateManager = @@ -76,13 +77,19 @@ module.exports = UpdateManager = return callback(error) if error? if !lines? or !version? return callback(new Errors.NotFoundError("document not found: #{doc_id}")) + previousVersion = version ShareJsUpdateManager.applyUpdate project_id, doc_id, update, lines, version, (error, updatedDocLines, version, appliedOps) -> profile.log("sharejs.applyUpdate") return callback(error) if error? - RangesManager.applyUpdate project_id, doc_id, ranges, appliedOps, updatedDocLines, (error, new_ranges) -> + RangesManager.applyUpdate project_id, doc_id, ranges, appliedOps, updatedDocLines, (error, new_ranges, ranges_were_collapsed) -> UpdateManager._addProjectHistoryMetadataToOps(appliedOps, pathname, projectHistoryId, lines) profile.log("RangesManager.applyUpdate") return callback(error) if error? + if ranges_were_collapsed + logger.log {project_id, doc_id, previousVersion, lines, ranges, update}, "update collapsed some ranges, snapshotting previous content" + SnapshotManager.recordSnapshot project_id, doc_id, previousVersion, lines, ranges, (error) -> + if error? + logger.error {err: error, project_id, doc_id, version, lines, ranges}, "error recording snapshot" RedisManager.updateDocument project_id, doc_id, updatedDocLines, version, appliedOps, new_ranges, (error, doc_ops_length, project_ops_length) -> profile.log("RedisManager.updateDocument") return callback(error) if error? diff --git a/services/document-updater/app/coffee/mongojs.coffee b/services/document-updater/app/coffee/mongojs.coffee new file mode 100644 index 0000000000..8f8f1a9ab9 --- /dev/null +++ b/services/document-updater/app/coffee/mongojs.coffee @@ -0,0 +1,7 @@ +Settings = require "settings-sharelatex" +mongojs = require "mongojs" +db = mongojs(Settings.mongo.url, ["docSnapshots"]) +module.exports = + db: db + ObjectId: mongojs.ObjectId + diff --git a/services/document-updater/npm-shrinkwrap.json b/services/document-updater/npm-shrinkwrap.json index 095e360c00..0f4f0fb697 100644 --- a/services/document-updater/npm-shrinkwrap.json +++ b/services/document-updater/npm-shrinkwrap.json @@ -291,6 +291,11 @@ "resolved": "https://registry.npmjs.org/browser-stdout/-/browser-stdout-1.3.1.tgz", "dev": true }, + "bson": { + "version": "1.0.9", + "from": "bson@~1.0.4", + "resolved": "https://registry.npmjs.org/bson/-/bson-1.0.9.tgz" + }, "buffer-crc32": { "version": "0.2.1", "from": "buffer-crc32@0.2.1", @@ -301,6 +306,11 @@ "from": "buffer-equal-constant-time@1.0.1", "resolved": "https://registry.npmjs.org/buffer-equal-constant-time/-/buffer-equal-constant-time-1.0.1.tgz" }, + "buffer-shims": { + "version": "1.0.0", + "from": "buffer-shims@>=1.0.0 <1.1.0", + "resolved": "https://registry.npmjs.org/buffer-shims/-/buffer-shims-1.0.0.tgz" + }, "builtin-modules": { "version": "3.0.0", "from": "builtin-modules@>=3.0.0 <4.0.0", @@ -364,7 +374,7 @@ }, "coffee-script": { "version": "1.7.1", - "from": "coffee-script@>=1.7.0 <1.8.0", + "from": "coffee-script@1.7.1", "resolved": "https://registry.npmjs.org/coffee-script/-/coffee-script-1.7.1.tgz" }, "combined-stream": { @@ -491,6 +501,11 @@ "from": "duplexify@>=3.6.0 <4.0.0", "resolved": "https://registry.npmjs.org/duplexify/-/duplexify-3.6.1.tgz" }, + "each-series": { + "version": "1.0.0", + "from": "each-series@^1.0.0", + "resolved": "https://registry.npmjs.org/each-series/-/each-series-1.0.0.tgz" + }, "ecc-jsbn": { "version": "0.1.1", "from": "ecc-jsbn@>=0.1.1 <0.2.0", @@ -816,7 +831,7 @@ }, "lodash": { "version": "4.17.4", - "from": "lodash@>=4.14.0 <5.0.0", + "from": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz", "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz" }, "lodash.defaults": { @@ -1093,6 +1108,43 @@ "from": "module-details-from-path@>=1.0.3 <2.0.0", "resolved": "https://registry.npmjs.org/module-details-from-path/-/module-details-from-path-1.0.3.tgz" }, + "mongodb": { + "version": "2.2.36", + "from": "mongodb@^2.2.31", + "resolved": "https://registry.npmjs.org/mongodb/-/mongodb-2.2.36.tgz", + "dependencies": { + "es6-promise": { + "version": "3.2.1", + "from": "es6-promise@3.2.1", + "resolved": "https://registry.npmjs.org/es6-promise/-/es6-promise-3.2.1.tgz" + }, + "process-nextick-args": { + "version": "1.0.7", + "from": "process-nextick-args@>=1.0.6 <1.1.0", + "resolved": "https://registry.npmjs.org/process-nextick-args/-/process-nextick-args-1.0.7.tgz" + }, + "readable-stream": { + "version": "2.2.7", + "from": "readable-stream@2.2.7", + "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.2.7.tgz" + }, + "string_decoder": { + "version": "1.0.3", + "from": "string_decoder@>=1.0.0 <1.1.0", + "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.0.3.tgz" + } + } + }, + "mongodb-core": { + "version": "2.1.20", + "from": "mongodb-core@2.1.20", + "resolved": "https://registry.npmjs.org/mongodb-core/-/mongodb-core-2.1.20.tgz" + }, + "mongojs": { + "version": "2.6.0", + "from": "https://registry.npmjs.org/mongojs/-/mongojs-2.6.0.tgz", + "resolved": "https://registry.npmjs.org/mongojs/-/mongojs-2.6.0.tgz" + }, "ms": { "version": "2.0.0", "from": "ms@2.0.0", @@ -1158,6 +1210,11 @@ "from": "parse-duration@>=0.1.1 <0.2.0", "resolved": "https://registry.npmjs.org/parse-duration/-/parse-duration-0.1.1.tgz" }, + "parse-mongo-url": { + "version": "1.1.1", + "from": "parse-mongo-url@^1.1.1", + "resolved": "https://registry.npmjs.org/parse-mongo-url/-/parse-mongo-url-1.1.1.tgz" + }, "parse-ms": { "version": "2.0.0", "from": "parse-ms@>=2.0.0 <3.0.0", @@ -1405,6 +1462,11 @@ } } }, + "require_optional": { + "version": "1.0.1", + "from": "require_optional@~1.0.0", + "resolved": "https://registry.npmjs.org/require_optional/-/require_optional-1.0.1.tgz" + }, "require-in-the-middle": { "version": "3.1.0", "from": "require-in-the-middle@>=3.0.0 <4.0.0", @@ -1420,6 +1482,11 @@ "from": "resolve@>=1.5.0 <2.0.0", "resolved": "https://registry.npmjs.org/resolve/-/resolve-1.10.0.tgz" }, + "resolve-from": { + "version": "2.0.0", + "from": "resolve-from@^2.0.0", + "resolved": "https://registry.npmjs.org/resolve-from/-/resolve-from-2.0.0.tgz" + }, "retry-axios": { "version": "0.3.2", "from": "retry-axios@>=0.3.2 <0.4.0", @@ -1585,12 +1652,22 @@ "from": "through2@>=2.0.3 <3.0.0", "resolved": "https://registry.npmjs.org/through2/-/through2-2.0.5.tgz" }, + "thunky": { + "version": "1.0.3", + "from": "thunky@^1.0.2", + "resolved": "https://registry.npmjs.org/thunky/-/thunky-1.0.3.tgz" + }, "timekeeper": { "version": "2.0.0", "from": "timekeeper@>=2.0.0 <3.0.0", "resolved": "https://registry.npmjs.org/timekeeper/-/timekeeper-2.0.0.tgz", "dev": true }, + "to-mongodb-core": { + "version": "2.0.0", + "from": "to-mongodb-core@^2.0.0", + "resolved": "https://registry.npmjs.org/to-mongodb-core/-/to-mongodb-core-2.0.0.tgz" + }, "tough-cookie": { "version": "2.3.3", "from": "tough-cookie@>=2.3.3 <2.4.0", @@ -1612,11 +1689,6 @@ "from": "uid2@0.0.2", "resolved": "https://registry.npmjs.org/uid2/-/uid2-0.0.2.tgz" }, - "underscore": { - "version": "1.2.2", - "from": "underscore@1.2.2", - "resolved": "https://registry.npmjs.org/underscore/-/underscore-1.2.2.tgz" - }, "uri-js": { "version": "4.2.2", "from": "uri-js@>=4.2.2 <5.0.0", diff --git a/services/document-updater/package.json b/services/document-updater/package.json index fa95246d21..f0682a7c30 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -23,16 +23,17 @@ "async": "^2.5.0", "coffee-script": "~1.7.0", "express": "3.3.4", + "lodash": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz", "logger-sharelatex": "^1.6.0", "lynx": "0.0.11", "metrics-sharelatex": "^2.1.1", + "mongojs": "https://registry.npmjs.org/mongojs/-/mongojs-2.6.0.tgz", "redis-sharelatex": "^1.0.5", "request": "2.25.0", "requestretry": "^1.12.0", "sandboxed-module": "~0.2.0", "settings-sharelatex": "^1.1.0", - "sinon": "~1.5.2", - "underscore": "1.2.2" + "sinon": "~1.5.2" }, "devDependencies": { "bunyan": "~0.22.1", diff --git a/services/document-updater/test/acceptance/coffee/RangesTests.coffee b/services/document-updater/test/acceptance/coffee/RangesTests.coffee index 95c80440c0..c6df55b459 100644 --- a/services/document-updater/test/acceptance/coffee/RangesTests.coffee +++ b/services/document-updater/test/acceptance/coffee/RangesTests.coffee @@ -4,11 +4,15 @@ chai.should() expect = chai.expect async = require "async" +{db, ObjectId} = require "../../../app/js/mongojs" MockWebApi = require "./helpers/MockWebApi" DocUpdaterClient = require "./helpers/DocUpdaterClient" DocUpdaterApp = require "./helpers/DocUpdaterApp" describe "Ranges", -> + before (done) -> + DocUpdaterApp.ensureRunning done + describe "tracking changes from ops", -> before (done) -> @project_id = DocUpdaterClient.randomId() @@ -305,4 +309,64 @@ describe "Ranges", -> throw error if error? ranges = data.ranges expect(ranges.changes).to.be.undefined - done() \ No newline at end of file + done() + + describe.only "deleting text surrounding a comment", -> + before (done) -> + @project_id = DocUpdaterClient.randomId() + @user_id = DocUpdaterClient.randomId() + @doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc @project_id, @doc_id, { + lines: ["foo bar baz"] + version: 0 + ranges: { + comments: [{ + op: { c: "a", p: 5, tid: @tid = DocUpdaterClient.randomId() } + metadata: + user_id: @user_id + ts: new Date() + }] + } + } + @updates = [{ + doc: @doc_id + op: [{ d: "foo ", p: 0 }] + v: 0 + meta: { user_id: @user_id } + }, { + doc: @doc_id + op: [{ d: "bar ", p: 0 }] + v: 1 + meta: { user_id: @user_id } + }] + jobs = [] + for update in @updates + do (update) => + jobs.push (callback) => DocUpdaterClient.sendUpdate @project_id, @doc_id, update, callback + DocUpdaterClient.preloadDoc @project_id, @doc_id, (error) => + throw error if error? + async.series jobs, (error) -> + throw error if error? + setTimeout () => + DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, data) => + throw error if error? + done() + , 200 + + it "should write a snapshot from before the destructive change", (done) -> + DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, data) => + return done(error) if error? + db.docSnapshots.find { + project_id: ObjectId(@project_id), + doc_id: ObjectId(@doc_id) + }, (error, docSnapshots) => + return done(error) if error? + expect(docSnapshots.length).to.equal 1 + expect(docSnapshots[0].version).to.equal 1 + expect(docSnapshots[0].lines).to.deep.equal ["bar baz"] + expect(docSnapshots[0].ranges.comments[0].op).to.deep.equal { + c: "a", + p: 1, + tid: @tid + } + done() diff --git a/services/document-updater/test/unit/coffee/ProjectManager/updateProjectTests.coffee b/services/document-updater/test/unit/coffee/ProjectManager/updateProjectTests.coffee index 3ed0109be7..635e562669 100644 --- a/services/document-updater/test/unit/coffee/ProjectManager/updateProjectTests.coffee +++ b/services/document-updater/test/unit/coffee/ProjectManager/updateProjectTests.coffee @@ -3,7 +3,7 @@ chai = require('chai') should = chai.should() modulePath = "../../../../app/js/ProjectManager.js" SandboxedModule = require('sandboxed-module') -_ = require('underscore') +_ = require('lodash') describe "ProjectManager", -> beforeEach -> diff --git a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee index b11c73489e..93d5d26e2f 100644 --- a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee @@ -50,8 +50,9 @@ describe "RangesManager", -> it "should return the modified the comments and changes", -> @callback.called.should.equal true - [error, entries] = @callback.args[0] + [error, entries, ranges_were_collapsed] = @callback.args[0] expect(error).to.be.null + expect(ranges_were_collapsed).to.equal false entries.comments[0].op.should.deep.equal { c: "three " p: 8 @@ -180,6 +181,36 @@ describe "RangesManager", -> 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 \")") + + describe "with an update that collapses a range", -> + beforeEach -> + @updates = [{ + meta: + user_id: @user_id + op: [{ + d: "one" + p: 0 + t: "thread-id-1" + }] + }] + @entries = { + comments: [{ + op: + c: "n" + p: 1 + t: "thread-id-2" + metadata: + user_id: @user_id + }] + changes: [] + } + @RangesManager.applyUpdate @project_id, @doc_id, @entries, @updates, @newDocLines, @callback + + it "should return ranges_were_collapsed == true", -> + @callback.called.should.equal true + [error, entries, ranges_were_collapsed] = @callback.args[0] + expect(ranges_were_collapsed).to.equal true + describe "acceptChanges", -> beforeEach -> @RangesManager = SandboxedModule.require modulePath, diff --git a/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee b/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee index 383bd1848e..32c305aedd 100644 --- a/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee @@ -23,6 +23,7 @@ describe "UpdateManager", -> "settings-sharelatex": @Settings = {} "./DocumentManager": @DocumentManager = {} "./RangesManager": @RangesManager = {} + "./SnapshotManager": @SnapshotManager = {} "./Profiler": class Profiler log: sinon.stub().returns { end: sinon.stub() } end: sinon.stub() @@ -169,7 +170,7 @@ describe "UpdateManager", -> @project_ops_length = sinon.stub() @pathname = '/a/b/c.tex' @DocumentManager.getDoc = sinon.stub().yields(null, @lines, @version, @ranges, @pathname, @projectHistoryId) - @RangesManager.applyUpdate = sinon.stub().yields(null, @updated_ranges) + @RangesManager.applyUpdate = sinon.stub().yields(null, @updated_ranges, false) @ShareJsUpdateManager.applyUpdate = sinon.stub().yields(null, @updatedDocLines, @version, @appliedOps) @RedisManager.updateDocument = sinon.stub().yields(null, @doc_ops_length, @project_ops_length) @RealTimeRedisManager.sendData = sinon.stub() @@ -239,6 +240,24 @@ describe "UpdateManager", -> it "should call the callback with the error", -> @callback.calledWith(@error).should.equal true + describe "when ranges get collapsed", -> + beforeEach -> + @RangesManager.applyUpdate = sinon.stub().yields(null, @updated_ranges, true) + @SnapshotManager.recordSnapshot = sinon.stub().yields() + @UpdateManager.applyUpdate @project_id, @doc_id, @update, @callback + + it "should call SnapshotManager.recordSnapshot", -> + @SnapshotManager.recordSnapshot + .calledWith( + @project_id, + @doc_id, + @version, + @lines, + @ranges + ) + .should.equal true + + describe "_addProjectHistoryMetadataToOps", -> it "should add projectHistoryId, pathname and doc_length metadata to the ops", -> lines = [ From 33478f95fdbabd8e8f2d0668770c28246f65c3cb Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 11 Apr 2019 16:32:31 +0100 Subject: [PATCH 2/3] Fix package.json versions --- services/document-updater/npm-shrinkwrap.json | 4 ++-- services/document-updater/package.json | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/document-updater/npm-shrinkwrap.json b/services/document-updater/npm-shrinkwrap.json index 0f4f0fb697..f30ce4f581 100644 --- a/services/document-updater/npm-shrinkwrap.json +++ b/services/document-updater/npm-shrinkwrap.json @@ -831,7 +831,7 @@ }, "lodash": { "version": "4.17.4", - "from": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz", + "from": "lodash@4.17.4", "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz" }, "lodash.defaults": { @@ -1142,7 +1142,7 @@ }, "mongojs": { "version": "2.6.0", - "from": "https://registry.npmjs.org/mongojs/-/mongojs-2.6.0.tgz", + "from": "mongojs@2.6.0", "resolved": "https://registry.npmjs.org/mongojs/-/mongojs-2.6.0.tgz" }, "ms": { diff --git a/services/document-updater/package.json b/services/document-updater/package.json index f0682a7c30..386f48e7ba 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -23,11 +23,11 @@ "async": "^2.5.0", "coffee-script": "~1.7.0", "express": "3.3.4", - "lodash": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz", + "lodash": "^4.17.4", "logger-sharelatex": "^1.6.0", "lynx": "0.0.11", "metrics-sharelatex": "^2.1.1", - "mongojs": "https://registry.npmjs.org/mongojs/-/mongojs-2.6.0.tgz", + "mongojs": "^2.6.0", "redis-sharelatex": "^1.0.5", "request": "2.25.0", "requestretry": "^1.12.0", From 52f3596e53b85661d6ae2de961620870da6d8ad7 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 16 Apr 2019 11:05:17 +0100 Subject: [PATCH 3/3] Review feedback --- .../app/coffee/SnapshotManager.coffee | 7 +++++-- .../app/coffee/UpdateManager.coffee | 19 +++++++++++++------ .../test/acceptance/coffee/RangesTests.coffee | 2 +- .../UpdateManager/UpdateManagerTests.coffee | 1 + 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/services/document-updater/app/coffee/SnapshotManager.coffee b/services/document-updater/app/coffee/SnapshotManager.coffee index 5a756b34db..86670b648d 100644 --- a/services/document-updater/app/coffee/SnapshotManager.coffee +++ b/services/document-updater/app/coffee/SnapshotManager.coffee @@ -1,17 +1,20 @@ {db, ObjectId} = require "./mongojs" module.exports = SnapshotManager = - recordSnapshot: (project_id, doc_id, version, lines, ranges, callback) -> + recordSnapshot: (project_id, doc_id, version, pathname, lines, ranges, callback) -> try project_id = ObjectId(project_id) doc_id = ObjectId(doc_id) catch error return callback(error) db.docSnapshots.insert { - project_id, doc_id, version, lines + project_id, doc_id, version, lines, pathname, ranges: SnapshotManager.jsonRangesToMongo(ranges), ts: new Date() }, callback + # Suggested indexes: + # db.docSnapshots.createIndex({project_id:1, doc_id:1}) + # db.docSnapshots.createIndex({ts:1},{expiresAfterSeconds: 30*24*3600)) # expires after 30 days jsonRangesToMongo: (ranges) -> return null if !ranges? diff --git a/services/document-updater/app/coffee/UpdateManager.coffee b/services/document-updater/app/coffee/UpdateManager.coffee index 5f9dcf5317..2fe7508fed 100644 --- a/services/document-updater/app/coffee/UpdateManager.coffee +++ b/services/document-updater/app/coffee/UpdateManager.coffee @@ -85,17 +85,24 @@ module.exports = UpdateManager = UpdateManager._addProjectHistoryMetadataToOps(appliedOps, pathname, projectHistoryId, lines) profile.log("RangesManager.applyUpdate") return callback(error) if error? - if ranges_were_collapsed - logger.log {project_id, doc_id, previousVersion, lines, ranges, update}, "update collapsed some ranges, snapshotting previous content" - SnapshotManager.recordSnapshot project_id, doc_id, previousVersion, lines, ranges, (error) -> - if error? - logger.error {err: error, project_id, doc_id, version, lines, ranges}, "error recording snapshot" RedisManager.updateDocument project_id, doc_id, updatedDocLines, version, appliedOps, new_ranges, (error, doc_ops_length, project_ops_length) -> profile.log("RedisManager.updateDocument") return callback(error) if error? HistoryManager.recordAndFlushHistoryOps project_id, doc_id, appliedOps, doc_ops_length, project_ops_length, (error) -> profile.log("recordAndFlushHistoryOps") - callback(error) + return callback(error) if error? + if ranges_were_collapsed + logger.log {project_id, doc_id, previousVersion, lines, ranges, update}, "update collapsed some ranges, snapshotting previous content" + # Do this last, since it's a mongo call, and so potentially longest running + # If it overruns the lock, it's ok, since all of our redis work is done + SnapshotManager.recordSnapshot project_id, doc_id, previousVersion, pathname, lines, ranges, (error) -> + if error? + logger.error {err: error, project_id, doc_id, version, lines, ranges}, "error recording snapshot" + return callback(error) + else + callback() + else + callback() lockUpdatesAndDo: (method, project_id, doc_id, args..., callback) -> profile = new Profiler("lockUpdatesAndDo", {project_id, doc_id}) diff --git a/services/document-updater/test/acceptance/coffee/RangesTests.coffee b/services/document-updater/test/acceptance/coffee/RangesTests.coffee index c6df55b459..52946f4823 100644 --- a/services/document-updater/test/acceptance/coffee/RangesTests.coffee +++ b/services/document-updater/test/acceptance/coffee/RangesTests.coffee @@ -311,7 +311,7 @@ describe "Ranges", -> expect(ranges.changes).to.be.undefined done() - describe.only "deleting text surrounding a comment", -> + describe "deleting text surrounding a comment", -> before (done) -> @project_id = DocUpdaterClient.randomId() @user_id = DocUpdaterClient.randomId() diff --git a/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee b/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee index 32c305aedd..0fdbbb9728 100644 --- a/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee @@ -252,6 +252,7 @@ describe "UpdateManager", -> @project_id, @doc_id, @version, + @pathname, @lines, @ranges )