From 4ecb17b16e9041cc651ae5e9882a9ca7fed73583 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 11 Mar 2014 15:24:38 +0000 Subject: [PATCH] Put multiple ops in one update --- .../app/coffee/DiffGenerator.coffee | 47 ++++++++------- .../app/coffee/UpdateCompressor.coffee | 28 ++++++--- .../coffee/AppendingUpdatesTests.coffee | 52 ++++++++++++---- .../DiffGenerator/DiffGeneratorTests.coffee | 60 ++++++++++--------- .../UpdateCompressorTests.coffee | 29 ++++++++- 5 files changed, 147 insertions(+), 69 deletions(-) diff --git a/services/track-changes/app/coffee/DiffGenerator.coffee b/services/track-changes/app/coffee/DiffGenerator.coffee index fdec79d6f4..b8028d9f99 100644 --- a/services/track-changes/app/coffee/DiffGenerator.coffee +++ b/services/track-changes/app/coffee/DiffGenerator.coffee @@ -9,7 +9,11 @@ module.exports = DiffGenerator = ConsistencyError: ConsistencyError rewindUpdate: (content, update) -> - op = update.op + for op in update.op.slice().reverse() + content = DiffGenerator.rewindOp content, op + return content + + rewindOp: (content, op) -> if op.i? textToBeRemoved = content.slice(op.p, op.p + op.i.length) if op.i != textToBeRemoved @@ -33,9 +37,8 @@ module.exports = DiffGenerator = diff = DiffGenerator.applyUpdateToDiff diff, update return diff - applyUpdateToDiff: (diff, update) -> + applyOpToDiff: (diff, op, meta) -> position = 0 - op = update.op remainingDiff = diff.slice() {consumedDiff, remainingDiff} = DiffGenerator._consumeToOffset(remainingDiff, op.p) @@ -44,15 +47,19 @@ module.exports = DiffGenerator = if op.i? newDiff.push i: op.i - meta: update.meta + meta: meta else if op.d? - {consumedDiff, remainingDiff} = DiffGenerator._consumeDiffAffectedByDeleteUpdate remainingDiff, update + {consumedDiff, remainingDiff} = DiffGenerator._consumeDiffAffectedByDeleteOp remainingDiff, op, meta newDiff.push(consumedDiff...) newDiff.push(remainingDiff...) return newDiff + applyUpdateToDiff: (diff, update) -> + for op in update.op + diff = DiffGenerator.applyOpToDiff diff, op, update.meta + return diff _consumeToOffset: (remainingDiff, totalOffset) -> consumedDiff = [] @@ -76,25 +83,24 @@ module.exports = DiffGenerator = consumedDiff.push part throw new Error("Ran out of diff to consume. Offset is too small") - _consumeDiffAffectedByDeleteUpdate: (remainingDiff, deleteUpdate) -> + _consumeDiffAffectedByDeleteOp: (remainingDiff, deleteOp, meta) -> consumedDiff = [] - remainingUpdate = deleteUpdate - while remainingUpdate - {newPart, remainingDiff, remainingUpdate} = DiffGenerator._consumeDeletedPart remainingDiff, remainingUpdate + remainingOp = deleteOp + while remainingOp + {newPart, remainingDiff, remainingOp} = DiffGenerator._consumeDeletedPart remainingDiff, remainingOp, meta consumedDiff.push newPart if newPart? return { consumedDiff: consumedDiff remainingDiff: remainingDiff } - _consumeDeletedPart: (remainingDiff, deleteUpdate) -> + _consumeDeletedPart: (remainingDiff, op, meta) -> part = remainingDiff.shift() partLength = DiffGenerator._getLengthOfDiffPart part - op = deleteUpdate.op if part.d? # Skip existing deletes - remainingUpdate = deleteUpdate + remainingOp = op newPart = part else if partLength > op.d.length @@ -109,11 +115,11 @@ module.exports = DiffGenerator = if part.u? newPart = d: op.d - meta: deleteUpdate.meta + meta: meta else if part.i? newPart = null - remainingUpdate = null + remainingOp = null else if partLength == op.d.length # The entire part has been deleted, but it is the last part @@ -125,11 +131,11 @@ module.exports = DiffGenerator = if part.u? newPart = d: op.d - meta: deleteUpdate.meta + meta: meta else if part.i? newPart = null - remainingUpdate = null + remainingOp = null else if partLength < op.d.length # The entire part has been deleted and there is more @@ -142,18 +148,17 @@ module.exports = DiffGenerator = if part.u newPart = d: part.u - meta: deleteUpdate.meta + meta: meta else if part.i? newPart = null - remainingUpdate = - op: { p: op.p, d: op.d.slice(DiffGenerator._getLengthOfDiffPart(part)) } - meta: deleteUpdate.meta + remainingOp = + p: op.p, d: op.d.slice(DiffGenerator._getLengthOfDiffPart(part)) return { newPart: newPart remainingDiff: remainingDiff - remainingUpdate: remainingUpdate + remainingOp: remainingOp } _slicePart: (basePart, from, to) -> diff --git a/services/track-changes/app/coffee/UpdateCompressor.coffee b/services/track-changes/app/coffee/UpdateCompressor.coffee index e5830bde5a..5ba379a8d9 100644 --- a/services/track-changes/app/coffee/UpdateCompressor.coffee +++ b/services/track-changes/app/coffee/UpdateCompressor.coffee @@ -16,24 +16,38 @@ module.exports = UpdateCompressor = # op: op2 # meta: { start_ts: ... , end_ts: ..., user_id: ... } # }] - convertRawUpdatesToCompressedFormat: (updates) -> - normalizedUpdates = [] + convertToSingleOpUpdates: (updates) -> + splitUpdates = [] for update in updates for op in update.op - normalizedUpdates.push + splitUpdates.push op: op meta: start_ts: update.meta.start_ts or update.meta.ts end_ts: update.meta.end_ts or update.meta.ts user_id: update.meta.user_id v: update.v - return normalizedUpdates + return splitUpdates + + concatUpdatesWithSameVersion: (updates) -> + concattedUpdates = [] + for update in updates + lastUpdate = concattedUpdates[concattedUpdates.length - 1] + if lastUpdate? and lastUpdate.v == update.v + lastUpdate.op.push update.op + else + concattedUpdates.push + op: [ update.op ] + meta: update.meta + v: update.v + return concattedUpdates compressRawUpdates: (lastPreviousUpdate, rawUpdates) -> - updates = UpdateCompressor.convertRawUpdatesToCompressedFormat(rawUpdates) if lastPreviousUpdate? - updates.unshift(lastPreviousUpdate) - return UpdateCompressor.compressUpdates(updates) + rawUpdates = [lastPreviousUpdate].concat(rawUpdates) + updates = UpdateCompressor.convertToSingleOpUpdates(rawUpdates) + updates = UpdateCompressor.compressUpdates(updates) + return UpdateCompressor.concatUpdatesWithSameVersion(updates) compressUpdates: (updates) -> return [] if updates.length == 0 diff --git a/services/track-changes/test/acceptance/coffee/AppendingUpdatesTests.coffee b/services/track-changes/test/acceptance/coffee/AppendingUpdatesTests.coffee index 031659f24b..827d238239 100644 --- a/services/track-changes/test/acceptance/coffee/AppendingUpdatesTests.coffee +++ b/services/track-changes/test/acceptance/coffee/AppendingUpdatesTests.coffee @@ -34,9 +34,9 @@ describe "Appending doc ops to the history", -> done() it "should insert the compressed op into mongo", -> - expect(@updates[0].op).to.deep.equal { + expect(@updates[0].op).to.deep.equal [{ p: 3, i: "foo" - } + }] it "should insert the correct version number into mongo", -> expect(@updates[0].v).to.equal 5 @@ -84,9 +84,9 @@ describe "Appending doc ops to the history", -> done() it "should combine all the updates into one", -> - expect(@updates[0].op).to.deep.equal { + expect(@updates[0].op).to.deep.equal [{ p: 3, i: "foobar" - } + }] it "should insert the correct version number into mongo", -> expect(@updates[0].v).to.equal 8 @@ -114,26 +114,26 @@ describe "Appending doc ops to the history", -> done() it "should keep the updates separate", -> - expect(@updates[0].op).to.deep.equal { + expect(@updates[0].op).to.deep.equal [{ p: 3, i: "foo" - } - expect(@updates[1].op).to.deep.equal { + }] + expect(@updates[1].op).to.deep.equal [{ p: 6, i: "bar" - } + }] describe "when the updates need processing in batches", -> before (done) -> @doc_id = ObjectId().toString() @user_id = ObjectId().toString() updates = [] - @expectedOp = { p:0, i: "" } + @expectedOp = [{ p:0, i: "" }] for i in [0..250] updates.push { op: [{i: "a", p: 0}] meta: { ts: Date.now(), user_id: @user_id } v: i } - @expectedOp.i = "a" + @expectedOp.i + @expectedOp[0].i = "a" + @expectedOp[0].i TrackChangesClient.pushRawUpdates @doc_id, updates, (error) => throw error if error? @@ -147,3 +147,35 @@ describe "Appending doc ops to the history", -> it "should insert the correct version number into mongo", -> expect(@updates[0].v).to.equal 250 + + describe "when there are multiple ops in each update", -> + before (done) -> + @doc_id = ObjectId().toString() + @user_id = ObjectId().toString() + oneDay = 24 * 60 * 60 * 1000 + TrackChangesClient.pushRawUpdates @doc_id, [{ + op: [{ i: "f", p: 3 }, { i: "o", p: 4 }, { i: "o", p: 5 }] + meta: { ts: Date.now(), user_id: @user_id } + v: 3 + }, { + op: [{ i: "b", p: 6 }, { i: "a", p: 7 }, { i: "r", p: 8 }] + meta: { ts: Date.now() + oneDay, user_id: @user_id } + v: 4 + }], (error) => + throw error if error? + TrackChangesClient.flushAndGetCompressedUpdates @doc_id, (error, @updates) => + throw error if error? + done() + + it "should insert the compressed ops into mongo", -> + expect(@updates[0].op).to.deep.equal [{ + p: 3, i: "foo" + }] + expect(@updates[1].op).to.deep.equal [{ + p: 6, i: "bar" + }] + + it "should insert the correct version numbers into mongo", -> + expect(@updates[0].v).to.equal 3 + expect(@updates[1].v).to.equal 4 + diff --git a/services/track-changes/test/unit/coffee/DiffGenerator/DiffGeneratorTests.coffee b/services/track-changes/test/unit/coffee/DiffGenerator/DiffGeneratorTests.coffee index 18b2e637e1..9fd59d69f5 100644 --- a/services/track-changes/test/unit/coffee/DiffGenerator/DiffGeneratorTests.coffee +++ b/services/track-changes/test/unit/coffee/DiffGenerator/DiffGeneratorTests.coffee @@ -14,38 +14,40 @@ describe "DiffGenerator", -> start_ts: @ts, end_ts: @ts, user_id: @user_id } - describe "rewindUpdate", -> + describe "rewindOp", -> describe "rewinding an insert", -> it "should undo the insert", -> content = "hello world" - update = - op: { p: 6, i: "wo" } - rewoundContent = @DiffGenerator.rewindUpdate content, update + rewoundContent = @DiffGenerator.rewindOp content, { p: 6, i: "wo" } rewoundContent.should.equal "hello rld" describe "rewinding a delete", -> it "should undo the delete", -> content = "hello rld" - update = - op: { p: 6, d: "wo" } - rewoundContent = @DiffGenerator.rewindUpdate content, update + rewoundContent = @DiffGenerator.rewindOp content, { p: 6, d: "wo" } rewoundContent.should.equal "hello world" describe "with an inconsistent update", -> it "should throw an error", -> content = "hello world" - update = - op: { p: 6, i: "foo" } expect( () => - @DiffGenerator.rewindUpdate content, update + @DiffGenerator.rewindOp content, { p: 6, i: "foo" } ).to.throw(@DiffGenerator.ConsistencyError) + describe "rewindUpdate", -> + it "should rewind ops in reverse", -> + content = "aaabbbccc" + update = + op: [{ p: 3, i: "bbb" }, { p: 6, i: "ccc" }] + rewoundContent = @DiffGenerator.rewindUpdate content, update + rewoundContent.should.equal "aaa" + describe "rewindUpdates", -> it "should rewind updates in reverse", -> content = "aaabbbccc" updates = [ - { op: { p: 3, i: "bbb" } }, - { op: { p: 6, i: "ccc" } } + { op: [{ p: 3, i: "bbb" }] }, + { op: [{ p: 6, i: "ccc" }] } ] rewoundContent = @DiffGenerator.rewindUpdates content, updates rewoundContent.should.equal "aaa" @@ -83,7 +85,7 @@ describe "DiffGenerator", -> it "should insert into the middle of (u)nchanged text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foobar" } ], - { op: { p: 3, i: "baz" }, meta: @meta } + { op: [{ p: 3, i: "baz" }], meta: @meta } ) expect(diff).to.deep.equal([ { u: "foo" } @@ -94,7 +96,7 @@ describe "DiffGenerator", -> it "should insert into the start of (u)changed text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foobar" } ], - { op: { p: 0, i: "baz" }, meta: @meta } + { op: [{ p: 0, i: "baz" }], meta: @meta } ) expect(diff).to.deep.equal([ { i: "baz", meta: @meta } @@ -104,7 +106,7 @@ describe "DiffGenerator", -> it "should insert into the end of (u)changed text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foobar" } ], - { op: { p: 6, i: "baz" }, meta: @meta } + { op: [{ p: 6, i: "baz" }], meta: @meta } ) expect(diff).to.deep.equal([ { u: "foobar" } @@ -114,7 +116,7 @@ describe "DiffGenerator", -> it "should insert into the middle of (i)inserted text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { i: "foobar", meta: @meta } ], - { op: { p: 3, i: "baz" }, meta: @meta } + { op: [{ p: 3, i: "baz" }], meta: @meta } ) expect(diff).to.deep.equal([ { i: "foo", meta: @meta } @@ -128,7 +130,7 @@ describe "DiffGenerator", -> { d: "deleted", meta: @meta } { u: "foobar" } ], - { op: { p: 3, i: "baz" }, meta: @meta } + { op: [{ p: 3, i: "baz" }], meta: @meta } ) expect(diff).to.deep.equal([ { d: "deleted", meta: @meta } @@ -142,7 +144,7 @@ describe "DiffGenerator", -> it "should delete from the middle of (u)nchanged text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foobazbar" } ], - { op: { p: 3, d: "baz" }, meta: @meta } + { op: [{ p: 3, d: "baz" }], meta: @meta } ) expect(diff).to.deep.equal([ { u: "foo" } @@ -153,7 +155,7 @@ describe "DiffGenerator", -> it "should delete from the start of (u)nchanged text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foobazbar" } ], - { op: { p: 0, d: "foo" }, meta: @meta } + { op: [{ p: 0, d: "foo" }], meta: @meta } ) expect(diff).to.deep.equal([ { d: "foo", meta: @meta } @@ -163,7 +165,7 @@ describe "DiffGenerator", -> it "should delete from the end of (u)nchanged text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foobazbar" } ], - { op: { p: 6, d: "bar" }, meta: @meta } + { op: [{ p: 6, d: "bar" }], meta: @meta } ) expect(diff).to.deep.equal([ { u: "foobaz" } @@ -173,7 +175,7 @@ describe "DiffGenerator", -> it "should delete across multiple (u)changed text parts", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foo" }, { u: "baz" }, { u: "bar" } ], - { op: { p: 2, d: "obazb" }, meta: @meta } + { op: [{ p: 2, d: "obazb" }], meta: @meta } ) expect(diff).to.deep.equal([ { u: "fo" } @@ -187,7 +189,7 @@ describe "DiffGenerator", -> it "should delete from the middle of (i)nserted text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { i: "foobazbar", meta: @meta } ], - { op: { p: 3, d: "baz" }, meta: @meta } + { op: [{ p: 3, d: "baz" }], meta: @meta } ) expect(diff).to.deep.equal([ { i: "foo", meta: @meta } @@ -197,7 +199,7 @@ describe "DiffGenerator", -> it "should delete from the start of (u)nchanged text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { i: "foobazbar", meta: @meta } ], - { op: { p: 0, d: "foo" }, meta: @meta } + { op: [{ p: 0, d: "foo" }], meta: @meta } ) expect(diff).to.deep.equal([ { i: "bazbar", meta: @meta } @@ -206,7 +208,7 @@ describe "DiffGenerator", -> it "should delete from the end of (u)nchanged text", -> diff = @DiffGenerator.applyUpdateToDiff( [ { i: "foobazbar", meta: @meta } ], - { op: { p: 6, d: "bar" }, meta: @meta } + { op: [{ p: 6, d: "bar" }], meta: @meta } ) expect(diff).to.deep.equal([ { i: "foobaz", meta: @meta } @@ -215,7 +217,7 @@ describe "DiffGenerator", -> it "should delete across multiple (u)changed and (i)nserted text parts", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foo" }, { i: "baz", meta: @meta }, { u: "bar" } ], - { op: { p: 2, d: "obazb" }, meta: @meta } + { op: [{ p: 2, d: "obazb" }], meta: @meta } ) expect(diff).to.deep.equal([ { u: "fo" } @@ -228,7 +230,7 @@ describe "DiffGenerator", -> it "should delete across multiple (u)changed and (d)deleted text parts", -> diff = @DiffGenerator.applyUpdateToDiff( [ { u: "foo" }, { d: "baz", meta: @meta }, { u: "bar" } ], - { op: { p: 2, d: "ob" }, meta: @meta } + { op: [{ p: 2, d: "ob" }], meta: @meta } ) expect(diff).to.deep.equal([ { u: "fo" } @@ -243,7 +245,7 @@ describe "DiffGenerator", -> expect( () => @DiffGenerator.applyUpdateToDiff( [ { u: "foobazbar" } ], - { op: { p: 3, d: "xxx" }, meta: @meta } + { op: [{ p: 3, d: "xxx" }], meta: @meta } ) ).to.throw(@DiffGenerator.ConsistencyError) @@ -251,7 +253,7 @@ describe "DiffGenerator", -> expect( () => @DiffGenerator.applyUpdateToDiff( [ { u: "foobazbar" } ], - { op: { p: 0, d: "xxx" }, meta: @meta } + { op: [{ p: 0, d: "xxx" }], meta: @meta } ) ).to.throw(@DiffGenerator.ConsistencyError) @@ -259,7 +261,7 @@ describe "DiffGenerator", -> expect( () => @DiffGenerator.applyUpdateToDiff( [ { u: "foobazbar" } ], - { op: { p: 6, d: "xxx" }, meta: @meta } + { op: [{ p: 6, d: "xxx" }] , meta: @meta } ) ).to.throw(@DiffGenerator.ConsistencyError) diff --git a/services/track-changes/test/unit/coffee/UpdateCompressor/UpdateCompressorTests.coffee b/services/track-changes/test/unit/coffee/UpdateCompressor/UpdateCompressorTests.coffee index a4de76c5ac..ec41dcca30 100644 --- a/services/track-changes/test/unit/coffee/UpdateCompressor/UpdateCompressorTests.coffee +++ b/services/track-changes/test/unit/coffee/UpdateCompressor/UpdateCompressorTests.coffee @@ -13,9 +13,9 @@ describe "UpdateCompressor", -> @ts1 = Date.now() @ts2 = Date.now() + 1000 - describe "convertRawUpdatesToCompressedFormat", -> + describe "convertToSingleOpUpdates", -> it "should split grouped updates into individual updates", -> - expect(@UpdateCompressor.convertRawUpdatesToCompressedFormat [{ + expect(@UpdateCompressor.convertToSingleOpUpdates [{ op: [ @op1 = { p: 0, i: "Foo" }, @op2 = { p: 6, i: "bar"} ] meta: { ts: @ts1, user_id: @user_id } v: 42 @@ -38,6 +38,31 @@ describe "UpdateCompressor", -> v: 43 }] + describe "concatUpdatesWithSameVersion", -> + it "should concat updates with the same version", -> + expect(@UpdateCompressor.concatUpdatesWithSameVersion [{ + op: @op1 = { p: 0, i: "Foo" } + meta: { start_ts: @ts1, end_ts: @ts1, user_id: @user_id } + v: 42 + }, { + op: @op2 = { p: 6, i: "bar" } + meta: { start_ts: @ts1, end_ts: @ts1, user_id: @user_id } + v: 42 + }, { + op: @op3 = { p: 10, i: "baz" } + meta: { start_ts: @ts2, end_ts: @ts2, user_id: @other_user_id } + v: 43 + }]) + .to.deep.equal [{ + op: [ @op1, @op2 ] + meta: { start_ts: @ts1, end_ts: @ts1, user_id: @user_id } + v: 42 + }, { + op: [ @op3 ] + meta: { start_ts: @ts2, end_ts: @ts2, user_id: @other_user_id } + v: 43 + }] + describe "compress", -> describe "insert - insert", -> it "should append one insert to the other", ->