Merge pull request #49 from sharelatex/ja-record-snapshots-of-ranges

Record a snapshot to mongo when a doc's comments/changes get collapsed

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
+		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

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"

services/document-updater/app/coffee/SnapshotManager.coffee

@@ -0,0 +1,42 @@
+{db, ObjectId} = require "./mongojs"
+
+module.exports = SnapshotManager =
+  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, 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?
+    
+    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

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,10 +77,11 @@ 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?
@@ -88,7 +90,19 @@ module.exports = UpdateManager =
 						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})

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
+

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": "lodash@4.17.4",
       "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": "mongojs@2.6.0",
+      "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",

services/document-updater/package.json

@@ -23,16 +23,17 @@
     "async": "^2.5.0",
     "coffee-script": "~1.7.0",
     "express": "3.3.4",
+    "lodash": "^4.17.4",
     "logger-sharelatex": "^1.6.0",
     "lynx": "0.0.11",
     "metrics-sharelatex": "^2.1.1",
+    "mongojs": "^2.6.0",
     "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",

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()
+				done()
+
+	describe "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()

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 ->

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,

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,25 @@ 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,
+						@pathname,
+						@lines,
+						@ranges
+					)
+					.should.equal true
+
+
 	describe "_addProjectHistoryMetadataToOps", ->
 		it "should add projectHistoryId, pathname and doc_length metadata to the ops", ->
 			lines = [