diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index 1b865a600d..233f5ad989 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -35,18 +35,25 @@ load = (EventEmitter) -> # * Inserts by another user will not combine with inserts by the first user. If they are in the # middle of a previous insert by the first user, the original insert will be split into two. constructor: (@changes = [], @comments = []) -> - # Change objects have the following structure: - # { - # id: ... # Uniquely generated by us - # op: { # ShareJs style op tracking the offset (p) and content inserted (i) or deleted (d) - # i: "..." - # p: 42 - # } - # } - # - # Ids are used to uniquely identify a change, e.g. for updating it in the database, or keeping in - # sync with Ace ranges. - @id = 0 + + @_increment: 0 + @newId: () -> + # Generate a Mongo ObjectId + # Reference: https://github.com/dreampulse/ObjectId.js/blob/master/src/main/javascript/Objectid.js + @_pid ?= Math.floor(Math.random() * (32767)) + @_machine ?= Math.floor(Math.random() * (16777216)) + timestamp = Math.floor(new Date().valueOf() / 1000) + @_increment++ + + timestamp = timestamp.toString(16) + machine = @_machine.toString(16) + pid = @_pid.toString(16) + increment = @_increment.toString(16) + + return '00000000'.substr(0, 8 - timestamp.length) + timestamp + + '000000'.substr(0, 6 - machine.length) + machine + + '0000'.substr(0, 4 - pid.length) + pid + + '000000'.substr(0, 6 - increment.length) + increment; getComment: (comment_id) -> comment = null @@ -56,19 +63,6 @@ load = (EventEmitter) -> break return comment - resolveCommentId: (comment_id, resolved_data) -> - comment = @getComment(comment_id) - return if !comment? - comment.metadata.resolved = true - comment.metadata.resolved_data = resolved_data - @emit "comment:resolved", comment - - unresolveCommentId: (comment_id) -> - comment = @getComment(comment_id) - return if !comment? - comment.metadata.resolved = false - @emit "comment:unresolved", comment - removeCommentId: (comment_id) -> comment = @getComment(comment_id) return if !comment? @@ -88,7 +82,7 @@ load = (EventEmitter) -> return if !change? @_removeChange(change) - applyOp: (op, metadata) -> + applyOp: (op, metadata = {}) -> metadata.ts ?= new Date() # Apply an op that has been applied to the document to our changes to keep them up to date if op.i? @@ -97,6 +91,8 @@ load = (EventEmitter) -> else if op.d? @applyDeleteToChanges(op, metadata) @applyDeleteToComments(op) + else if op.dr? + @applyDeleteRangeToChanges(op) else if op.c? @addComment(op, metadata) else @@ -105,7 +101,7 @@ load = (EventEmitter) -> addComment: (op, metadata) -> # TODO: Don't allow overlapping comments? @comments.push comment = { - id: @_newId() + id: RangesTracker.newId() op: # Copy because we'll modify in place c: op.c p: op.p @@ -394,28 +390,20 @@ load = (EventEmitter) -> if moved_changes.length > 0 @emit "changes:moved", moved_changes - _newId: () -> - # Generate a Mongo ObjectId - # Reference: https://github.com/dreampulse/ObjectId.js/blob/master/src/main/javascript/Objectid.js - @_pid ?= Math.floor(Math.random() * (32767)) - @_machine ?= Math.floor(Math.random() * (16777216)) - timestamp = Math.floor(new Date().valueOf() / 1000) - @_increment ?= 0 - @_increment++ - - timestamp = timestamp.toString(16) - machine = @_machine.toString(16) - pid = @_pid.toString(16) - increment = @_increment.toString(16) - - return '00000000'.substr(0, 8 - timestamp.length) + timestamp + - '000000'.substr(0, 6 - machine.length) + machine + - '0000'.substr(0, 4 - pid.length) + pid + - '000000'.substr(0, 6 - increment.length) + increment; + applyDeleteRangeToChanges: (op) -> + remove_changes = [] + for change in @changes + change_text = change.op.i or change.op.d + if op.p == change.op.p and op.dr == change_text + remove_changes.push change + if remove_changes.length == 0 + throw new Error("no range to remove") + for change in remove_changes + @_removeChange(change) _addOp: (op, metadata) -> change = { - id: @_newId() + id: RangesTracker.newId() op: op metadata: metadata } diff --git a/services/document-updater/app/coffee/sharejs/types/text.coffee b/services/document-updater/app/coffee/sharejs/types/text.coffee index 2a3b79997d..84303b3307 100644 --- a/services/document-updater/app/coffee/sharejs/types/text.coffee +++ b/services/document-updater/app/coffee/sharejs/types/text.coffee @@ -32,7 +32,8 @@ checkValidComponent = (c) -> i_type = typeof c.i d_type = typeof c.d c_type = typeof c.c - throw new Error 'component needs an i, d or c field' unless (i_type == 'string') ^ (d_type == 'string') ^ (c_type == 'string') + dr_type = typeof c.dr + throw new Error 'component needs an i, d, c or dr field' unless (i_type == 'string') ^ (d_type == 'string') ^ (c_type == 'string') ^ (dr_type == 'string') throw new Error 'position cannot be negative' unless c.p >= 0 @@ -40,6 +41,26 @@ checkValidOp = (op) -> checkValidComponent(c) for c in op true +componentText = (c) -> + if c.c? + text = c.c + if c.dr? + text = c.dr + throw new Error("invalid component") if !text? + return text + +duplicateComponent = (c) -> + newC = {} + for key, value of c + newC[key] = value + return newC + +setComponentText = (c, text) -> + if c.c? + c.c = text + if c.dr? + c.dr = text + text.apply = (snapshot, op) -> checkValidOp op for component in op @@ -49,9 +70,10 @@ text.apply = (snapshot, op) -> deleted = snapshot[component.p...(component.p + component.d.length)] throw new Error "Delete component '#{component.d}' does not match deleted text '#{deleted}'" unless component.d == deleted snapshot = snapshot[...component.p] + snapshot[(component.p + component.d.length)..] - else if component.c? - comment = snapshot[component.p...(component.p + component.c.length)] - throw new Error "Comment component '#{component.c}' does not match commented text '#{comment}'" unless component.c == comment + else if component.c? or component.dr? + c_text = componentText(component) + range = snapshot[component.p...(component.p + c_text.length)] + throw new Error "Range component '#{c_text}' does not match range text '#{range}'" unless c_text == range else throw new Error "Unknown op type" snapshot @@ -127,7 +149,7 @@ transformPosition = (pos, c, insertAfter) -> c.p else pos - c.d.length - else if c.c? + else if c.c? or c.dr? pos else throw new Error("unknown op type") @@ -187,46 +209,54 @@ text._tc = transformComponent = (dest, c, otherC, side) -> newC.p = transformPosition newC.p, otherC append dest, newC - else if otherC.c? + else if otherC.c? or otherC.dr? append dest, c else throw new Error("unknown op type") - else if c.c? # Comment + else if c.c? or c.dr? # Comment or delete range + c_text = componentText(c) if otherC.i? - if c.p < otherC.p < c.p + c.c.length + if c.p < otherC.p < c.p + c_text.length offset = otherC.p - c.p - new_c = (c.c[0..(offset-1)] + otherC.i + c.c[offset...]) - append dest, {c:new_c, p:c.p, t: c.t} + newText = (c_text[0..(offset-1)] + otherC.i + c_text[offset...]) + newC = duplicateComponent(c) + setComponentText(newC, newText) + append dest, newC else - append dest, {c:c.c, p:transformPosition(c.p, otherC, true), t: c.t} + newC = duplicateComponent(c) + newC.p = transformPosition(c.p, otherC, true) + append dest, newC else if otherC.d? if c.p >= otherC.p + otherC.d.length - append dest, {c:c.c, p:c.p - otherC.d.length, t: c.t} - else if c.p + c.c.length <= otherC.p + newC = duplicateComponent(c) + newC.p = c.p - otherC.d.length + append dest, newC + else if c.p + c_text.length <= otherC.p append dest, c else # Delete overlaps comment # They overlap somewhere. - newC = {c:'', p:c.p, t: c.t} + newC = duplicateComponent(c) + setComponentText(newC, '') if c.p < otherC.p - newC.c = c.c[...(otherC.p - c.p)] - if c.p + c.c.length > otherC.p + otherC.d.length - newC.c += c.c[(otherC.p + otherC.d.length - c.p)..] + setComponentText(newC, c_text[...(otherC.p - c.p)]) + if c.p + c_text.length > otherC.p + otherC.d.length + setComponentText(newC, componentText(newC) + c_text[(otherC.p + otherC.d.length - c.p)..]) # This is entirely optional - just for a check that the deleted # text in the two ops matches intersectStart = Math.max c.p, otherC.p - intersectEnd = Math.min c.p + c.c.length, otherC.p + otherC.d.length - cIntersect = c.c[intersectStart - c.p...intersectEnd - c.p] + intersectEnd = Math.min c.p + c_text.length, otherC.p + otherC.d.length + cIntersect = c_text[intersectStart - c.p...intersectEnd - c.p] otherIntersect = otherC.d[intersectStart - otherC.p...intersectEnd - otherC.p] - throw new Error 'Delete ops delete different text in the same region of the document' unless cIntersect == otherIntersect + throw new Error 'Delete op text does not match range being modified' unless cIntersect == otherIntersect newC.p = transformPosition newC.p, otherC append dest, newC - else if otherC.c? + else if otherC.c? or otherC.dr? append dest, c else diff --git a/services/document-updater/test/acceptance/coffee/RangesTests.coffee b/services/document-updater/test/acceptance/coffee/RangesTests.coffee index 0cee1598aa..a5cbee7569 100644 --- a/services/document-updater/test/acceptance/coffee/RangesTests.coffee +++ b/services/document-updater/test/acceptance/coffee/RangesTests.coffee @@ -1,6 +1,7 @@ sinon = require "sinon" chai = require("chai") chai.should() +expect = chai.expect async = require "async" rclient = require("redis").createClient() @@ -54,82 +55,105 @@ describe "Ranges", -> change.op.should.deep.equal { i: "456", p: 3 } change.metadata.user_id.should.equal @user_id done() + + describe "removing ranges", -> + it "should delete the range (and perform OT)", (done) -> + @conflicting_update = { + doc: @doc.id + op: [{ i: "X", p: 1 }] + v: 3 + meta: { user_id: @user_id } + } + @delete_range = { + doc: @doc.id + op: [{ dr: "456", p: 3 }] + v: 3 + meta: { user_id: @user_id } + } + DocUpdaterClient.sendUpdate @project_id, @doc.id, @conflicting_update, (error) => + throw error if error? + DocUpdaterClient.sendUpdate @project_id, @doc.id, @delete_range, (error) => + throw error if error? + DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) => + throw error if error? + expect(data.ranges.changes).to.be.undefined + done() - describe "Adding comments", -> - describe "standalone", -> - before (done) -> - @project_id = DocUpdaterClient.randomId() - @user_id = DocUpdaterClient.randomId() - @doc = { - id: DocUpdaterClient.randomId() - lines: ["foo bar baz"] - } - @updates = [{ - doc: @doc.id - op: [{ c: "bar", p: 4, t: @tid = DocUpdaterClient.randomId() }] - v: 0 - }] - MockWebApi.insertDoc @project_id, @doc.id, { - lines: @doc.lines - version: 0 - } - 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) => + describe "Adding comments", -> + describe "standalone", -> + before (done) -> + @project_id = DocUpdaterClient.randomId() + @user_id = DocUpdaterClient.randomId() + @doc = { + id: DocUpdaterClient.randomId() + lines: ["foo bar baz"] + } + @updates = [{ + doc: @doc.id + op: [{ c: "bar", p: 4, t: @tid = DocUpdaterClient.randomId() }] + v: 0 + }] + MockWebApi.insertDoc @project_id, @doc.id, { + lines: @doc.lines + version: 0 + } + 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? - async.series jobs, (error) -> - throw error if error? - setTimeout done, 200 - - it "should update the ranges", (done) -> - DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) => - throw error if error? - ranges = data.ranges - comment = ranges.comments[0] - comment.op.should.deep.equal { c: "bar", p: 4, t: @tid } - done() + setTimeout done, 200 + + it "should update the ranges", (done) -> + DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) => + throw error if error? + ranges = data.ranges + comment = ranges.comments[0] + comment.op.should.deep.equal { c: "bar", p: 4, t: @tid } + done() - describe "with conflicting ops needing OT", -> - before (done) -> - @project_id = DocUpdaterClient.randomId() - @user_id = DocUpdaterClient.randomId() - @doc = { - id: DocUpdaterClient.randomId() - lines: ["foo bar baz"] - } - @updates = [{ - doc: @doc.id - op: [{ i: "ABC", p: 3 }] - v: 0 - meta: { user_id: @user_id } - }, { - doc: @doc.id - op: [{ c: "bar", p: 4, t: @tid = DocUpdaterClient.randomId() }] - v: 0 - }] - MockWebApi.insertDoc @project_id, @doc.id, { - lines: @doc.lines - version: 0 - } - 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) => + describe "with conflicting ops needing OT", -> + before (done) -> + @project_id = DocUpdaterClient.randomId() + @user_id = DocUpdaterClient.randomId() + @doc = { + id: DocUpdaterClient.randomId() + lines: ["foo bar baz"] + } + @updates = [{ + doc: @doc.id + op: [{ i: "ABC", p: 3 }] + v: 0 + meta: { user_id: @user_id } + }, { + doc: @doc.id + op: [{ c: "bar", p: 4, t: @tid = DocUpdaterClient.randomId() }] + v: 0 + }] + MockWebApi.insertDoc @project_id, @doc.id, { + lines: @doc.lines + version: 0 + } + 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? - async.series jobs, (error) -> - throw error if error? - setTimeout done, 200 - - it "should update the comments with the OT shifted comment", (done) -> - DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) => - throw error if error? - ranges = data.ranges - comment = ranges.comments[0] - comment.op.should.deep.equal { c: "bar", p: 7, t: @tid } - done() + setTimeout done, 200 + + it "should update the comments with the OT shifted comment", (done) -> + DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) => + throw error if error? + ranges = data.ranges + comment = ranges.comments[0] + comment.op.should.deep.equal { c: "bar", p: 7, t: @tid } + done() describe "Loading ranges from persistence layer", -> before (done) -> diff --git a/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee index 81440bfe5b..2d9dcf94a0 100644 --- a/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee +++ b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee @@ -189,6 +189,95 @@ describe "ShareJS text type", -> dest = [] text._tc(dest, { c: "foo", p: 6 }, { c: "bar", p: 3 }) dest.should.deep.equal [{ c: "foo", p: 6 }] + + describe "comment / delete_range", -> + it "should not do anything", -> + dest = [] + text._tc(dest, { c: "foo", p: 6 }, { dr: "bar", p: 3 }) + dest.should.deep.equal [{ c: "foo", p: 6 }] + + describe "delete_range / insert", -> + it "with an insert before", -> + dest = [] + text._tc(dest, { dr: "foo", p: 9 }, { i: "bar", p: 3 }) + dest.should.deep.equal [{ dr: "foo", p: 12 }] + + it "with an insert after", -> + dest = [] + text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 9 }) + dest.should.deep.equal [{ dr: "foo", p: 3 }] + + it "with an insert at the left edge", -> + dest = [] + text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 3 }) + # RangesTracker doesn't inject inserts into comments on edges, so neither should we + dest.should.deep.equal [{ dr: "foo", p: 6 }] + + it "with an insert at the right edge", -> + dest = [] + text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 6 }) + # RangesTracker doesn't inject inserts into comments on edges, so neither should we + dest.should.deep.equal [{ dr: "foo", p: 3 }] + + it "with an insert in the middle", -> + dest = [] + text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 5 }) + dest.should.deep.equal [{ dr: "fobaro", p: 3 }] + + describe "delete_range / delete", -> + it "with a delete before", -> + dest = [] + text._tc(dest, { dr: "foo", p: 9 }, { d: "bar", p: 3 }) + dest.should.deep.equal [{ dr: "foo", p: 6 }] + + it "with a delete after", -> + dest = [] + text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 9 }) + dest.should.deep.equal [{ dr: "foo", p: 3 }] + + it "with a delete overlapping the comment content before", -> + dest = [] + text._tc(dest, { dr: "foobar", p: 6 }, { d: "123foo", p: 3 }) + dest.should.deep.equal [{ dr: "bar", p: 3 }] + + it "with a delete overlapping the comment content after", -> + dest = [] + text._tc(dest, { dr: "foobar", p: 6 }, { d: "bar123", p: 9 }) + dest.should.deep.equal [{ dr: "foo", p: 6 }] + + it "with a delete overlapping the comment content in the middle", -> + dest = [] + text._tc(dest, { dr: "foo123bar", p: 6 }, { d: "123", p: 9 }) + dest.should.deep.equal [{ dr: "foobar", p: 6 }] + + it "with a delete overlapping the whole comment", -> + dest = [] + text._tc(dest, { dr: "foo", p: 6 }, { d: "123foo456", p: 3 }) + dest.should.deep.equal [{ dr: "", p: 3 }] + + describe "delete_range / insert", -> + it "should not do anything", -> + dest = [] + text._tc(dest, { i: "foo", p: 6 }, { dr: "bar", p: 3 }) + dest.should.deep.equal [{ i: "foo", p: 6 }] + + describe "delete_range / delete", -> + it "should not do anything", -> + dest = [] + text._tc(dest, { d: "foo", p: 6 }, { dr: "bar", p: 3 }) + dest.should.deep.equal [{ d: "foo", p: 6 }] + + describe "delete_range / comment", -> + it "should not do anything", -> + dest = [] + text._tc(dest, { c: "foo", p: 6 }, { dr: "bar", p: 3 }) + dest.should.deep.equal [{ c: "foo", p: 6 }] + + describe "delete_range / delete_range", -> + it "should not do anything", -> + dest = [] + text._tc(dest, { dr: "foo", p: 6 }, { dr: "bar", p: 3 }) + dest.should.deep.equal [{ dr: "foo", p: 6 }] describe "apply", -> it "should apply an insert", -> @@ -199,6 +288,9 @@ describe "ShareJS text type", -> it "should do nothing with a comment", -> text.apply("foo123bar", [{ c: "123", p: 3 }]).should.equal "foo123bar" + + it "should do nothing with a delete_range", -> + text.apply("foo123bar", [{ dr: "123", p: 3 }]).should.equal "foo123bar" it "should throw an error when deleted content does not match", -> (() ->