From 55959101225c1b0f8efbe7cde69ee5b3b9b1cfd1 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 20 Mar 2014 12:10:04 +0000 Subject: [PATCH] Return a summary of the entire project changes --- services/track-changes/app.coffee | 2 +- .../app/coffee/HttpController.coffee | 15 +-- .../app/coffee/UpdatesManager.coffee | 47 +++++--- .../coffee/GettingUpdatesTests.coffee | 17 ++- .../coffee/helpers/TrackChangesClient.coffee | 20 +++- .../HttpController/HttpControllerTests.coffee | 18 +-- .../UpdatesManager/UpdatesManagerTests.coffee | 103 +++++++++++------- 7 files changed, 136 insertions(+), 86 deletions(-) diff --git a/services/track-changes/app.coffee b/services/track-changes/app.coffee index 4e62a63c28..a0b6f3d5b3 100644 --- a/services/track-changes/app.coffee +++ b/services/track-changes/app.coffee @@ -14,7 +14,7 @@ app.post "/project/:project_id/doc/:doc_id/flush", HttpController.flushUpdatesWi app.get "/project/:project_id/doc/:doc_id/diff", HttpController.getDiff -app.get "/project/:project_id/doc/:doc_id/updates", HttpController.getUpdates +app.get "/project/:project_id/updates", HttpController.getUpdates app.post "/project/:project_id/doc/:doc_id/version/:version/restore", HttpController.restore diff --git a/services/track-changes/app/coffee/HttpController.coffee b/services/track-changes/app/coffee/HttpController.coffee index 66c2c294ad..abc2226fc5 100644 --- a/services/track-changes/app/coffee/HttpController.coffee +++ b/services/track-changes/app/coffee/HttpController.coffee @@ -31,17 +31,18 @@ module.exports = HttpController = res.send JSON.stringify(diff: diff) getUpdates: (req, res, next = (error) ->) -> - doc_id = req.params.doc_id project_id = req.params.project_id - if req.query.to? - to = parseInt(req.query.to, 10) - if req.query.limit? - limit = parseInt(req.query.limit, 10) + if req.query.before? + before = parseInt(req.query.before, 10) + if req.query.min_count? + min_count = parseInt(req.query.min_count, 10) - UpdatesManager.getSummarizedDocUpdates project_id, doc_id, to: to, limit: limit, (error, updates) -> + UpdatesManager.getSummarizedProjectUpdates project_id, before: before, min_count: min_count, (error, updates, nextBeforeTimestamp) -> return next(error) if error? - res.send JSON.stringify updates: updates + res.send JSON.stringify + updates: updates + nextBeforeTimestamp: nextBeforeTimestamp restore: (req, res, next = (error) ->) -> {doc_id, project_id, version} = req.params diff --git a/services/track-changes/app/coffee/UpdatesManager.coffee b/services/track-changes/app/coffee/UpdatesManager.coffee index 9c63f47de8..cf5a83ce71 100644 --- a/services/track-changes/app/coffee/UpdatesManager.coffee +++ b/services/track-changes/app/coffee/UpdatesManager.coffee @@ -86,39 +86,49 @@ module.exports = UpdatesManager = return callback(error) if error? callback null, updates - getSummarizedDocUpdates: (project_id, doc_id, options = {}, callback = (error, updates) ->) -> - options.limit ||= 25 + getSummarizedProjectUpdates: (project_id, options = {}, callback = (error, updates) ->) -> + options.min_count ||= 25 summarizedUpdates = [] - to = options.to + before = options.before do fetchNextBatch = () -> - UpdatesManager._extendBatchOfSummarizedUpdates project_id, doc_id, summarizedUpdates, to, options.limit, (error, updates, endOfDatabase) -> + UpdatesManager._extendBatchOfSummarizedUpdates project_id, summarizedUpdates, before, options.min_count, (error, updates, nextBeforeUpdate) -> return callback(error) if error? - if endOfDatabase or updates.length >= options.limit - callback null, updates + if !nextBeforeUpdate? or updates.length >= options.min_count + callback null, updates, nextBeforeUpdate else - to = updates[updates.length - 1].fromV - 1 + before = nextBeforeUpdate summarizedUpdates = updates fetchNextBatch() _extendBatchOfSummarizedUpdates: ( - project_id, - doc_id, + project_id, existingSummarizedUpdates, - to, desiredLength, + before, desiredLength, callback = (error, summarizedUpdates, endOfDatabase) -> ) -> - UpdatesManager.getDocUpdatesWithUserInfo project_id, doc_id, { to: to, limit: 3 * desiredLength }, (error, updates) -> + UpdatesManager.getProjectUpdatesWithUserInfo project_id, { before: before, limit: 3 * desiredLength }, (error, updates) -> return callback(error) if error? - if !updates? or updates.length == 0 - endOfDatabase = true + + # Suppose in this request we have fetch the solid updates. In the next request we need + # to fetch the dotted updates. These are defined by having an end timestamp less than + # the last update's end timestamp (updates are ordered by descending end_ts). I.e. + # start_ts--v v--end_ts + # doc1: |......| |...| |-------| + # doc2: |------------------| + # ^----- Next time, fetch all updates with an + # end_ts less than this + # + if updates? and updates.length > 0 + nextBeforeTimestamp = updates[updates.length - 1].meta.end_ts else - endOfDatabase = false + nextBeforeTimestamp = null + summarizedUpdates = UpdatesManager._summarizeUpdates( updates, existingSummarizedUpdates ) callback null, - summarizedUpdates.slice(0, desiredLength), - endOfDatabase + summarizedUpdates, + nextBeforeTimestamp fillUserInfo: (updates, callback = (error, updates) ->) -> users = {} @@ -165,6 +175,10 @@ module.exports = UpdatesManager = break if !userExists earliestUpdate.meta.users.push update.meta.user + + if update.doc_id.toString() not in earliestUpdate.doc_ids + earliestUpdate.doc_ids.push update.doc_id.toString() + earliestUpdate.meta.start_ts = Math.min(earliestUpdate.meta.start_ts, update.meta.start_ts) earliestUpdate.meta.end_ts = Math.max(earliestUpdate.meta.end_ts, update.meta.end_ts) earliestUpdate.fromV = update.v @@ -176,6 +190,7 @@ module.exports = UpdatesManager = end_ts: update.meta.end_ts fromV: update.v toV: update.v + doc_ids: [update.doc_id.toString()] if update.meta.user? newUpdate.meta.users.push update.meta.user diff --git a/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee b/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee index 4f2406c62e..1bfc939629 100644 --- a/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee +++ b/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee @@ -43,14 +43,16 @@ describe "Getting updates", -> TrackChangesClient.pushRawUpdates @doc_id, @updates, (error) => throw error if error? - done() + TrackChangesClient.flushUpdates @project_id, @doc_id, (error) => + throw error if error? + done() after: () -> MockWebApi.getUser.restore() describe "getting updates up to the limit", -> before (done) -> - TrackChangesClient.getUpdates @project_id, @doc_id, { to: 20, limit: 3 }, (error, body) => + TrackChangesClient.getUpdates @project_id, { before: @to + 1, min_count: 3 }, (error, body) => throw error if error? @updates = body.updates done() @@ -60,8 +62,9 @@ describe "Getting updates", -> .calledWith(@user_id) .should.equal true - it "should return the same number of summarized updates as the limit", -> - expect(@updates).to.deep.equal [{ + it "should return at least the min_count number of summarized updates", -> + expect(@updates.slice(0,3)).to.deep.equal [{ + doc_ids: [@doc_id] meta: start_ts: @to - 2 * @minutes end_ts: @to @@ -69,6 +72,7 @@ describe "Getting updates", -> toV: 20 fromV: 19 }, { + doc_ids: [@doc_id] meta: start_ts: @to - 1 * @days - 2 * @minutes end_ts: @to - 1 * @days @@ -76,6 +80,7 @@ describe "Getting updates", -> toV: 18 fromV: 17 }, { + doc_ids: [@doc_id] meta: start_ts: @to - 2 * @days - 2 * @minutes end_ts: @to - 2 * @days @@ -87,13 +92,14 @@ describe "Getting updates", -> describe "getting updates beyond the end of the database", -> before (done) -> - TrackChangesClient.getUpdates @project_id, @doc_id, { to: 4, limit: 30 }, (error, body) => + TrackChangesClient.getUpdates @project_id, { before: @to - 8 * @days + 1, min_count: 30 }, (error, body) => throw error if error? @updates = body.updates done() it "should return as many updates as it can", -> expect(@updates).to.deep.equal [{ + doc_ids: [@doc_id] meta: start_ts: @to - 8 * @days - 2 * @minutes end_ts: @to - 8 * @days @@ -101,6 +107,7 @@ describe "Getting updates", -> toV: 4 fromV: 3 }, { + doc_ids: [@doc_id] meta: start_ts: @to - 9 * @days - 2 * @minutes end_ts: @to - 9 * @days diff --git a/services/track-changes/test/acceptance/coffee/helpers/TrackChangesClient.coffee b/services/track-changes/test/acceptance/coffee/helpers/TrackChangesClient.coffee index 71efab30b8..f9e3a42e2f 100644 --- a/services/track-changes/test/acceptance/coffee/helpers/TrackChangesClient.coffee +++ b/services/track-changes/test/acceptance/coffee/helpers/TrackChangesClient.coffee @@ -4,14 +4,22 @@ rclient = require("redis").createClient() # Only works locally for now module.exports = TrackChangesClient = flushAndGetCompressedUpdates: (project_id, doc_id, callback = (error, updates) ->) -> + TrackChangesClient.flushUpdates project_id, doc_id, (error) -> + return callback(error) if error? + TrackChangesClient.getCompressedUpdates doc_id, callback + + flushUpdates: (project_id, doc_id, callback = (error) ->) -> request.post { url: "http://localhost:3015/project/#{project_id}/doc/#{doc_id}/flush" }, (error, response, body) => response.statusCode.should.equal 204 - db.docHistory - .find(doc_id: ObjectId(doc_id)) - .sort("meta.end_ts": 1) - .toArray callback + callback(error) + + getCompressedUpdates: (doc_id, callback = (error, updates) ->) -> + db.docHistory + .find(doc_id: ObjectId(doc_id)) + .sort("meta.end_ts": 1) + .toArray callback pushRawUpdates: (doc_id, updates, callback = (error) ->) -> rclient.rpush "UncompressedHistoryOps:#{doc_id}", (JSON.stringify(u) for u in updates)..., callback @@ -23,9 +31,9 @@ module.exports = TrackChangesClient = response.statusCode.should.equal 200 callback null, JSON.parse(body) - getUpdates: (project_id, doc_id, options, callback = (error, body) ->) -> + getUpdates: (project_id, options, callback = (error, body) ->) -> request.get { - url: "http://localhost:3015/project/#{project_id}/doc/#{doc_id}/updates?to=#{options.to}&limit=#{options.limit}" + url: "http://localhost:3015/project/#{project_id}/updates?before=#{options.before}&min_count=#{options.min_count}" }, (error, response, body) => response.statusCode.should.equal 200 callback null, JSON.parse(body) diff --git a/services/track-changes/test/unit/coffee/HttpController/HttpControllerTests.coffee b/services/track-changes/test/unit/coffee/HttpController/HttpControllerTests.coffee index 4d15670653..4caa1b83ef 100644 --- a/services/track-changes/test/unit/coffee/HttpController/HttpControllerTests.coffee +++ b/services/track-changes/test/unit/coffee/HttpController/HttpControllerTests.coffee @@ -64,28 +64,28 @@ describe "HttpController", -> describe "getUpdates", -> beforeEach -> - @to = 42 - @limit = 10 + @before = Date.now() + @nextBeforeTimestamp = @before - 100 + @min_count = 10 @req = params: - doc_id: @doc_id project_id: @project_id query: - to: @to.toString() - limit: @limit.toString() + before: @before.toString() + min_count: @min_count.toString() @res = send: sinon.stub() @updates = ["mock-summarized-updates"] - @UpdatesManager.getSummarizedDocUpdates = sinon.stub().callsArgWith(3, null, @updates) + @UpdatesManager.getSummarizedProjectUpdates = sinon.stub().callsArgWith(2, null, @updates, @nextBeforeTimestamp) @HttpController.getUpdates @req, @res, @next it "should get the updates", -> - @UpdatesManager.getSummarizedDocUpdates - .calledWith(@project_id, @doc_id, to: @to, limit: @limit) + @UpdatesManager.getSummarizedProjectUpdates + .calledWith(@project_id, before: @before, min_count: @min_count) .should.equal true it "should return the formatted updates", -> - @res.send.calledWith(JSON.stringify(updates: @updates)).should.equal true + @res.send.calledWith(JSON.stringify(updates: @updates, nextBeforeTimestamp: @nextBeforeTimestamp)).should.equal true describe "RestoreManager", -> beforeEach -> diff --git a/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee b/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee index 0673a08c8f..d5608f8bee 100644 --- a/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee +++ b/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee @@ -284,23 +284,26 @@ describe "UpdatesManager", -> describe "_extendBatchOfSummarizedUpdates", -> beforeEach -> - @to = 42 - @limit = 2 + @before = Date.now() + @min_count = 2 @existingSummarizedUpdates = ["summarized-updates-3"] @summarizedUpdates = ["summarized-updates-3", "summarized-update-2", "summarized-update-1"] describe "when there are updates to get", -> beforeEach -> - @updates = ["mock-updates"] + @updates = [ + {op: "mock-op-1", meta: end_ts: @before - 10}, + {op: "mock-op-1", meta: end_ts: @nextBeforeTimestamp = @before - 20} + ] @existingSummarizedUpdates = ["summarized-updates-3"] @summarizedUpdates = ["summarized-updates-3", "summarized-update-2", "summarized-update-1"] @UpdatesManager._summarizeUpdates = sinon.stub().returns(@summarizedUpdates) - @UpdatesManager.getDocUpdatesWithUserInfo = sinon.stub().callsArgWith(3, null, @updates) - @UpdatesManager._extendBatchOfSummarizedUpdates @project_id, @doc_id, @existingSummarizedUpdates, @to, @limit, @callback + @UpdatesManager.getProjectUpdatesWithUserInfo = sinon.stub().callsArgWith(2, null, @updates) + @UpdatesManager._extendBatchOfSummarizedUpdates @project_id, @existingSummarizedUpdates, @before, @min_count, @callback it "should get the updates", -> - @UpdatesManager.getDocUpdatesWithUserInfo - .calledWith(@project_id, @doc_id, { to: @to, limit: 3 * @limit }) + @UpdatesManager.getProjectUpdatesWithUserInfo + .calledWith(@project_id, { before: @before, limit: 3 * @min_count }) .should.equal true it "should summarize the updates", -> @@ -308,78 +311,81 @@ describe "UpdatesManager", -> .calledWith(@updates, @existingSummarizedUpdates) .should.equal true - it "should call the callback with the summarized updates and false for end-of-databse", -> - @callback.calledWith(null, @summarizedUpdates.slice(0, @limit), false).should.equal true + it "should call the callback with the summarized updates and the next before timestamp", -> + @callback.calledWith(null, @summarizedUpdates, @nextBeforeTimestamp).should.equal true describe "when there are no more updates", -> beforeEach -> @updates = [] @UpdatesManager._summarizeUpdates = sinon.stub().returns(@summarizedUpdates) - @UpdatesManager.getDocUpdatesWithUserInfo = sinon.stub().callsArgWith(3, null, @updates) - @UpdatesManager._extendBatchOfSummarizedUpdates @project_id, @doc_id, @existingSummarizedUpdates, @to, @limit, @callback + @UpdatesManager.getProjectUpdatesWithUserInfo = sinon.stub().callsArgWith(2, null, @updates) + @UpdatesManager._extendBatchOfSummarizedUpdates @project_id, @existingSummarizedUpdates, @before, @min_count, @callback - it "should call the callback with the summarized updates and true for end-of-database", -> - @callback.calledWith(null, @summarizedUpdates.slice(0, @limit), true).should.equal true + it "should call the callback with the summarized updates and null for nextBeforeTimestamp", -> + @callback.calledWith(null, @summarizedUpdates, null).should.equal true - describe "getSummarizedDocUpdates", -> + describe "getSummarizedProjectUpdates", -> describe "when one batch of updates is enough to meet the limit", -> beforeEach -> - @to = 42 - @limit = 2 + @before = Date.now() + @min_count = 2 @updates = ["summarized-updates-3", "summarized-updates-2"] - @UpdatesManager._extendBatchOfSummarizedUpdates = sinon.stub().callsArgWith(5, null, @updates) - @UpdatesManager.getSummarizedDocUpdates @project_id, @doc_id, { to: @to, limit: @limit }, @callback + @nextBeforeTimestamp = @before - 100 + @UpdatesManager._extendBatchOfSummarizedUpdates = sinon.stub().callsArgWith(4, null, @updates, @nextBeforeTimestamp) + @UpdatesManager.getSummarizedProjectUpdates @project_id, { before: @before, min_count: @min_count }, @callback it "should get the batch of summarized updates", -> @UpdatesManager._extendBatchOfSummarizedUpdates - .calledWith(@project_id, @doc_id, [], @to, @limit) + .calledWith(@project_id, [], @before, @min_count) .should.equal true it "should call the callback with the updates", -> - @callback.calledWith(null, @updates).should.equal true + @callback.calledWith(null, @updates, @nextBeforeTimestamp).should.equal true describe "when multiple batches are needed to meet the limit", -> beforeEach -> - @to = 6 - @limit = 4 + @before = Date.now() + @min_count = 4 @firstBatch = [{ toV: 6, fromV: 6 }, { toV: 5, fromV: 5 }] + @nextBeforeTimestamp = @before - 100 @secondBatch = [{ toV: 4, fromV: 4 }, { toV: 3, fromV: 3 }] - @UpdatesManager._extendBatchOfSummarizedUpdates = (project_id, doc_id, existingUpdates, to, limit, callback) => + @nextNextBeforeTimestamp = @before - 200 + @UpdatesManager._extendBatchOfSummarizedUpdates = (project_id, existingUpdates, before, desiredLength, callback) => if existingUpdates.length == 0 - callback null, @firstBatch, false + callback null, @firstBatch, @nextBeforeTimestamp else - callback null, @firstBatch.concat(@secondBatch), false + callback null, @firstBatch.concat(@secondBatch), @nextNextBeforeTimestamp sinon.spy @UpdatesManager, "_extendBatchOfSummarizedUpdates" - @UpdatesManager.getSummarizedDocUpdates @project_id, @doc_id, { to: @to, limit: @limit }, @callback + @UpdatesManager.getSummarizedProjectUpdates @project_id, { before: @before, min_count: @min_count }, @callback it "should get the first batch of summarized updates", -> @UpdatesManager._extendBatchOfSummarizedUpdates - .calledWith(@project_id, @doc_id, [], @to, @limit) + .calledWith(@project_id, [], @before, @min_count) .should.equal true it "should get the second batch of summarized updates", -> @UpdatesManager._extendBatchOfSummarizedUpdates - .calledWith(@project_id, @doc_id, @firstBatch, 4, @limit) + .calledWith(@project_id, @firstBatch, @nextBeforeTimestamp, @min_count) .should.equal true it "should call the callback with all the updates", -> - @callback.calledWith(null, @firstBatch.concat(@secondBatch)).should.equal true + @callback.calledWith(null, @firstBatch.concat(@secondBatch), @nextNextBeforeTimestamp).should.equal true describe "when the end of the database is hit", -> beforeEach -> - @to = 6 - @limit = 4 + @before = Date.now() + @min_count = 4 @updates = [{ toV: 6, fromV: 6 }, { toV: 5, fromV: 5 }] - @UpdatesManager._extendBatchOfSummarizedUpdates = sinon.stub().callsArgWith(5, null, @updates, true) - @UpdatesManager.getSummarizedDocUpdates @project_id, @doc_id, { to: @to, limit: @limit }, @callback + @UpdatesManager._extendBatchOfSummarizedUpdates = sinon.stub().callsArgWith(4, null, @updates, null) + @UpdatesManager.getSummarizedProjectUpdates @project_id, { before: @before, min_count: @min_count }, @callback it "should get the batch of summarized updates", -> @UpdatesManager._extendBatchOfSummarizedUpdates - .calledWith(@project_id, @doc_id, [], @to, @limit) + .calledWith(@project_id, [], @before, @min_count) .should.equal true it "should call the callback with the updates", -> - @callback.calledWith(null, @updates).should.equal true + @callback.calledWith(null, @updates, null).should.equal true describe "fillUserInfo", -> describe "with valid users", -> @@ -469,24 +475,31 @@ describe "UpdatesManager", -> op: "mock-op-2" }] - describe "_buildUpdatesView", -> + describe "_summarizeUpdates", -> beforeEach -> @now = Date.now() + @user_1 = { id: "mock-user-1" } + @user_2 = { id: "mock-user-2" } + @doc_id_1 = "mock-doc-id-1" + @doc_id_2 = "mock-doc-id-2" it "should concat updates that are close in time", -> expect(@UpdatesManager._summarizeUpdates [{ + doc_id: @doc_id_1 meta: - user: @user_1 = { id: "mock-user-1" } + user: @user_1 start_ts: @now + 20 end_ts: @now + 30 v: 5 }, { + doc_id: @doc_id_1 meta: - user: @user_2 = { id: "mock-user-2" } + user: @user_2 start_ts: @now end_ts: @now + 10 v: 4 }]).to.deep.equal [{ + doc_ids: [@doc_id_1] meta: users: [@user_1, @user_2] start_ts: @now @@ -498,18 +511,21 @@ describe "UpdatesManager", -> it "should leave updates that are far apart in time", -> oneDay = 1000 * 60 * 60 * 24 expect(@UpdatesManager._summarizeUpdates [{ + doc_id: @doc_id_1 meta: - user: @user_2 = { id: "mock-user-2" } + user: @user_2 start_ts: @now + oneDay end_ts: @now + oneDay + 10 v: 5 }, { + doc_id: @doc_id_1 meta: - user: @user_1 = { id: "mock-user-2" } + user: @user_1 start_ts: @now end_ts: @now + 10 v: 4 }]).to.deep.equal [{ + doc_ids: [@doc_id_1] meta: users: [@user_2] start_ts: @now + oneDay @@ -517,6 +533,7 @@ describe "UpdatesManager", -> fromV: 5 toV: 5 }, { + doc_ids: [@doc_id_1] meta: users: [@user_1] start_ts: @now @@ -526,21 +543,22 @@ describe "UpdatesManager", -> }] it "should concat onto existing summarized updates", -> - @user_1 = { id: "mock-user-1" } - @user_2 = { id: "mock-user-2" } expect(@UpdatesManager._summarizeUpdates [{ + doc_id: @doc_id_2 meta: user: @user_1 start_ts: @now + 20 end_ts: @now + 30 v: 5 }, { + doc_id: @doc_id_2 meta: user: @user_2 start_ts: @now end_ts: @now + 10 v: 4 }], [{ + doc_ids: [@doc_id_1] meta: users: [@user_1] start_ts: @now + 40 @@ -548,6 +566,7 @@ describe "UpdatesManager", -> fromV: 6 toV: 8 }]).to.deep.equal [{ + doc_ids: [@doc_id_1, @doc_id_2] meta: users: [@user_1, @user_2] start_ts: @now