diff --git a/services/document-updater/app/coffee/RangesManager.coffee b/services/document-updater/app/coffee/RangesManager.coffee index 7f430f476b..ebef566424 100644 --- a/services/document-updater/app/coffee/RangesManager.coffee +++ b/services/document-updater/app/coffee/RangesManager.coffee @@ -5,7 +5,7 @@ module.exports = RangesManager = MAX_COMMENTS: 500 MAX_CHANGES: 2000 - applyUpdate: (project_id, doc_id, entries = {}, updates = [], callback = (error, new_entries) ->) -> + applyUpdate: (project_id, doc_id, entries = {}, updates = [], newDocLines, callback = (error, new_entries) ->) -> {changes, comments} = entries rangesTracker = new RangesTracker(changes, comments) for update in updates @@ -21,6 +21,14 @@ module.exports = RangesManager = if rangesTracker.changes?.length > RangesManager.MAX_CHANGES or rangesTracker.comments?.length > RangesManager.MAX_COMMENTS return callback new Error("too many comments or tracked changes") + try + # This is a consistency check that all of our ranges and + # comments still match the corresponding text + rangesTracker.validate(newDocLines.join("\n")) + catch error + logger.error {err: error, project_id, doc_id, newDocLines, updates}, "error validating ranges" + return callback(error) + 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 diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index a9c43e9816..5f67a9561b 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -1,3 +1,6 @@ +# This file is shared between document-updater and web, so that the server and client share +# an identical track changes implementation. Do not edit it directly in web or document-updater, +# instead edit it at https://github.com/sharelatex/ranges-tracker, where it has a suite of tests load = () -> class RangesTracker # The purpose of this class is to track a set of inserts and deletes to a document, like @@ -90,6 +93,18 @@ load = () -> change = @getChange(change_id) return if !change? @_removeChange(change) + + validate: (text) -> + for change in @changes + if change.op.i? + content = text.slice(change.op.p, change.op.p + change.op.i.length) + if content != change.op.i + throw new Error("Change (#{JSON.stringify(change)}) doesn't match text (#{JSON.stringify(content)})") + for comment in @comments + content = text.slice(comment.op.p, comment.op.p + comment.op.c.length) + if content != comment.op.c + throw new Error("Comment (#{JSON.stringify(comment)}) doesn't match text (#{JSON.stringify(content)})") + return true applyOp: (op, metadata = {}) -> metadata.ts ?= new Date() diff --git a/services/document-updater/app/coffee/UpdateManager.coffee b/services/document-updater/app/coffee/UpdateManager.coffee index 2ad3281bfe..b6a5f98c4c 100644 --- a/services/document-updater/app/coffee/UpdateManager.coffee +++ b/services/document-updater/app/coffee/UpdateManager.coffee @@ -59,7 +59,7 @@ module.exports = UpdateManager = return callback(new Errors.NotFoundError("document not found: #{doc_id}")) ShareJsUpdateManager.applyUpdate project_id, doc_id, update, lines, version, (error, updatedDocLines, version, appliedOps) -> return callback(error) if error? - RangesManager.applyUpdate project_id, doc_id, ranges, appliedOps, (error, new_ranges) -> + RangesManager.applyUpdate project_id, doc_id, ranges, appliedOps, updatedDocLines, (error, new_ranges) -> return callback(error) if error? RedisManager.updateDocument doc_id, updatedDocLines, version, appliedOps, new_ranges, (error) -> return callback(error) if error? diff --git a/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee new file mode 100644 index 0000000000..0c0c556a38 --- /dev/null +++ b/services/document-updater/test/unit/coffee/RangesManager/RangesManagerTests.coffee @@ -0,0 +1,179 @@ +sinon = require('sinon') +chai = require('chai') +should = chai.should() +expect = chai.expect +modulePath = "../../../../app/js/RangesManager.js" +SandboxedModule = require('sandboxed-module') + +describe "RangesManager", -> + beforeEach -> + @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" + @callback = sinon.stub() + + describe "applyUpdate", -> + beforeEach -> + @updates = [{ + meta: + user_id: @user_id + op: [{ + i: "two " + p: 4 + }] + }] + @entries = { + comments: [{ + op: + c: "three " + p: 4 + metadata: + user_id: @user_id + }] + changes: [{ + op: + i: "five" + p: 15 + metadata: + user_id: @user_id + }] + } + @newDocLines = ["one two three four five"] # old is "one three four five" + + describe "successfully", -> + beforeEach -> + @RangesManager.applyUpdate @project_id, @doc_id, @entries, @updates, @newDocLines, @callback + + it "should return the modified the comments and changes", -> + @callback.called.should.equal true + [error, entries] = @callback.args[0] + expect(error).to.be.null + entries.comments[0].op.should.deep.equal { + c: "three " + p: 8 + } + entries.changes[0].op.should.deep.equal { + i: "five" + p: 19 + } + + describe "with empty comments", -> + beforeEach -> + @entries.comments = [] + @RangesManager.applyUpdate @project_id, @doc_id, @entries, @updates, @newDocLines, @callback + + it "should return an object with no comments", -> + # Save space in redis and don't store just {} + @callback.called.should.equal true + [error, entries] = @callback.args[0] + expect(error).to.be.null + expect(entries.comments).to.be.undefined + + describe "with empty changes", -> + beforeEach -> + @entries.changes = [] + @RangesManager.applyUpdate @project_id, @doc_id, @entries, @updates, @newDocLines, @callback + + it "should return an object with no changes", -> + # Save space in redis and don't store just {} + @callback.called.should.equal true + [error, entries] = @callback.args[0] + expect(error).to.be.null + expect(entries.changes).to.be.undefined + + describe "with too many comments", -> + beforeEach -> + @RangesManager.MAX_COMMENTS = 2 + @updates = [{ + meta: + user_id: @user_id + op: [{ + c: "one" + p: 0 + }] + }] + @entries = { + comments: [{ + op: + c: "three " + p: 4 + metadata: + user_id: @user_id + }, { + op: + c: "four " + p: 10 + metadata: + user_id: @user_id + }] + changes: [] + } + @RangesManager.applyUpdate @project_id, @doc_id, @entries, @updates, @newDocLines, @callback + + it "should return an error", -> + # Save space in redis and don't store just {} + @callback.called.should.equal true + [error, entries] = @callback.args[0] + expect(error).to.not.be.null + expect(error.message).to.equal("too many comments or tracked changes") + + describe "with too many changes", -> + beforeEach -> + @RangesManager.MAX_CHANGES = 2 + @updates = [{ + meta: + user_id: @user_id + tc: "track-changes-id-yes" + op: [{ + i: "one " + p: 0 + }] + }] + @entries = { + changes: [{ + op: + i: "three" + p: 4 + metadata: + user_id: @user_id + }, { + op: + i: "four" + p: 10 + metadata: + user_id: @user_id + }] + comments: [] + } + @newDocLines = ["one two three four"] + @RangesManager.applyUpdate @project_id, @doc_id, @entries, @updates, @newDocLines, @callback + + it "should return an error", -> + # Save space in redis and don't store just {} + @callback.called.should.equal true + [error, entries] = @callback.args[0] + expect(error).to.not.be.null + expect(error.message).to.equal("too many comments or tracked changes") + + describe "inconsistent changes", -> + beforeEach -> + @updates = [{ + meta: + user_id: @user_id + op: [{ + c: "doesn't match" + p: 0 + }] + }] + @RangesManager.applyUpdate @project_id, @doc_id, @entries, @updates, @newDocLines, @callback + + it "should return an error", -> + # Save space in redis and don't store just {} + @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 diff --git a/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee b/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee index 33578cb6f0..fbf9b21ddc 100644 --- a/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee @@ -179,7 +179,7 @@ describe "UpdateManager", -> it "should update the ranges", -> @RangesManager.applyUpdate - .calledWith(@project_id, @doc_id, @ranges, @appliedOps) + .calledWith(@project_id, @doc_id, @ranges, @appliedOps, @updatedDocLines) .should.equal true it "should save the document", ->