From dcd7649badcbbedfd2488fae55d1e8d25ff786ab Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 15 Nov 2019 16:53:16 +0000 Subject: [PATCH] filter track-changes updates for projects using project-history --- .../app/coffee/DocumentManager.coffee | 8 +++-- .../app/coffee/HistoryManager.coffee | 29 +++++++++++++----- .../app/coffee/PersistenceManager.coffee | 4 +-- .../app/coffee/RedisManager.coffee | 30 ++++++++++++++----- .../config/settings.defaults.coffee | 5 ++-- .../DocumentManagerTests.coffee | 9 +++++- .../HistoryManager/HistoryManagerTests.coffee | 25 ++++++++++++---- .../RedisManager/RedisManagerTests.coffee | 1 + 8 files changed, 82 insertions(+), 29 deletions(-) diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index 5183a3aaea..59db98e97f 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -23,12 +23,14 @@ module.exports = DocumentManager = return callback(error) if error? if !lines? or !version? logger.log {project_id, doc_id}, "doc not in redis so getting from persistence API" - PersistenceManager.getDoc project_id, doc_id, (error, lines, version, ranges, pathname, projectHistoryId) -> + PersistenceManager.getDoc project_id, doc_id, (error, lines, version, ranges, pathname, projectHistoryId, projectHistoryType) -> return callback(error) if error? - logger.log {project_id, doc_id, lines, version, pathname, projectHistoryId}, "got doc from persistence API" + logger.log {project_id, doc_id, lines, version, pathname, projectHistoryId, projectHistoryType}, "got doc from persistence API" RedisManager.putDocInMemory project_id, doc_id, lines, version, ranges, pathname, projectHistoryId, (error) -> return callback(error) if error? - callback null, lines, version, ranges, pathname, projectHistoryId, null, false + RedisManager.setHistoryType doc_id, projectHistoryType, (error) -> + return callback(error) if error? + callback null, lines, version, ranges, pathname, projectHistoryId, null, false else callback null, lines, version, ranges, pathname, projectHistoryId, unflushedTime, true diff --git a/services/document-updater/app/coffee/HistoryManager.coffee b/services/document-updater/app/coffee/HistoryManager.coffee index 6b68b4b676..286292e6b3 100644 --- a/services/document-updater/app/coffee/HistoryManager.coffee +++ b/services/document-updater/app/coffee/HistoryManager.coffee @@ -11,14 +11,19 @@ module.exports = HistoryManager = if !Settings.apis?.trackchanges? logger.warn { doc_id }, "track changes API is not configured, so not flushing" return - - url = "#{Settings.apis.trackchanges.url}/project/#{project_id}/doc/#{doc_id}/flush" - logger.log { project_id, doc_id, url }, "flushing doc in track changes api" - request.post url, (error, res, body)-> - if error? - logger.error { error, doc_id, project_id}, "track changes doc to track changes api" - else if res.statusCode < 200 and res.statusCode >= 300 - logger.error { doc_id, project_id }, "track changes api returned a failure status code: #{res.statusCode}" + RedisManager.getHistoryType doc_id, (err, projectHistoryType) -> + if err? + logger.error {err, doc_id}, "error getting history type" + else if projectHistoryType is "project-history" + logger.debug {doc_id, projectHistoryType}, "skipping track-changes flush" + else + url = "#{Settings.apis.trackchanges.url}/project/#{project_id}/doc/#{doc_id}/flush" + logger.log { project_id, doc_id, url, projectHistoryType }, "flushing doc in track changes api" + request.post url, (error, res, body)-> + if error? + logger.error { error, doc_id, project_id}, "track changes doc to track changes api" + else if res.statusCode < 200 and res.statusCode >= 300 + logger.error { doc_id, project_id }, "track changes api returned a failure status code: #{res.statusCode}" # flush changes in the background flushProjectChangesAsync: (project_id) -> @@ -52,6 +57,7 @@ module.exports = HistoryManager = if ops.length == 0 return callback() + # record updates for project history if Settings.apis?.project_history?.enabled if HistoryManager.shouldFlushHistoryOps(project_ops_length, ops.length, HistoryManager.FLUSH_PROJECT_EVERY_N_OPS) # Do this in the background since it uses HTTP and so may be too @@ -59,6 +65,13 @@ module.exports = HistoryManager = logger.log { project_ops_length, project_id }, "flushing project history api" HistoryManager.flushProjectChangesAsync project_id + # if the doc_ops_length is undefined it means the project is not using track-changes + # so we can bail out here + if typeof(doc_ops_length) is 'undefined' + logger.debug { project_id, doc_id}, "skipping flush to track-changes, only using project-history" + return callback() + + # record updates for track-changes HistoryRedisManager.recordDocHasHistoryOps project_id, doc_id, ops, (error) -> return callback(error) if error? if HistoryManager.shouldFlushHistoryOps(doc_ops_length, ops.length, HistoryManager.FLUSH_DOC_EVERY_N_OPS) diff --git a/services/document-updater/app/coffee/PersistenceManager.coffee b/services/document-updater/app/coffee/PersistenceManager.coffee index ee80453137..543b9f0c22 100644 --- a/services/document-updater/app/coffee/PersistenceManager.coffee +++ b/services/document-updater/app/coffee/PersistenceManager.coffee @@ -13,7 +13,7 @@ request = (require("requestretry")).defaults({ MAX_HTTP_REQUEST_LENGTH = 5000 # 5 seconds module.exports = PersistenceManager = - getDoc: (project_id, doc_id, _callback = (error, lines, version, ranges, pathname, projectHistoryId) ->) -> + getDoc: (project_id, doc_id, _callback = (error, lines, version, ranges, pathname, projectHistoryId, projectHistoryType) ->) -> timer = new Metrics.Timer("persistenceManager.getDoc") callback = (args...) -> timer.done() @@ -44,7 +44,7 @@ module.exports = PersistenceManager = return callback(new Error("web API response had no valid doc version")) if !body.pathname? return callback(new Error("web API response had no valid doc pathname")) - return callback null, body.lines, body.version, body.ranges, body.pathname, body.projectHistoryId + return callback null, body.lines, body.version, body.ranges, body.pathname, body.projectHistoryId, body.projectHistoryType else if res.statusCode == 404 return callback(new Errors.NotFoundError("doc not not found: #{url}")) else diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index 12227fae21..7729486986 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -78,6 +78,7 @@ module.exports = RedisManager = multi.del keys.ranges(doc_id:doc_id) multi.del keys.pathname(doc_id:doc_id) multi.del keys.projectHistoryId(doc_id:doc_id) + multi.del keys.projectHistoryType(doc_id:doc_id) multi.del keys.unflushedTime(doc_id:doc_id) multi.del keys.lastUpdatedAt(doc_id: doc_id) multi.del keys.lastUpdatedBy(doc_id: doc_id) @@ -154,11 +155,11 @@ module.exports = RedisManager = logger.error project_id: project_id, doc_id: doc_id, doc_project_id: doc_project_id, "doc missing from docsInProject set" callback null, docLines, version, ranges, pathname, projectHistoryId, unflushedTime, lastUpdatedAt, lastUpdatedBy - getDocVersion: (doc_id, callback = (error, version) ->) -> - rclient.get keys.docVersion(doc_id: doc_id), (error, version) -> + getDocVersion: (doc_id, callback = (error, version, projectHistoryType) ->) -> + rclient.mget keys.docVersion(doc_id: doc_id), keys.projectHistoryType(doc_id:doc_id), (error, version, projectHistoryType) -> return callback(error) if error? version = parseInt(version, 10) - callback null, version + callback null, version, projectHistoryType getDocLines: (doc_id, callback = (error, version) ->) -> rclient.get keys.docLines(doc_id: doc_id), (error, docLines) -> @@ -200,10 +201,18 @@ module.exports = RedisManager = return callback(error) callback null, ops + getHistoryType: (doc_id, callback = (error, projectHistoryType) ->) -> + rclient.get keys.projectHistoryType(doc_id:doc_id), (error, projectHistoryType) -> + return callback(error) if error? + callback null, projectHistoryType + + setHistoryType: (doc_id, projectHistoryType, callback = (error) ->) -> + rclient.set keys.projectHistoryType(doc_id:doc_id), projectHistoryType, callback + DOC_OPS_TTL: 60 * minutes DOC_OPS_MAX_LENGTH: 100 updateDocument : (project_id, doc_id, docLines, newVersion, appliedOps = [], ranges, updateMeta, callback = (error) ->)-> - RedisManager.getDocVersion doc_id, (error, currentVersion) -> + RedisManager.getDocVersion doc_id, (error, currentVersion, projectHistoryType) -> return callback(error) if error? if currentVersion + appliedOps.length != newVersion error = new Error("Version mismatch. '#{doc_id}' is corrupted.") @@ -249,7 +258,11 @@ module.exports = RedisManager = multi.rpush keys.docOps(doc_id: doc_id), jsonOps... # index 5 # expire must come after rpush since before it will be a no-op if the list is empty multi.expire keys.docOps(doc_id: doc_id), RedisManager.DOC_OPS_TTL # index 6 - multi.rpush historyKeys.uncompressedHistoryOps(doc_id: doc_id), jsonOps... # index 7 + if projectHistoryType is "project-history" + logger.debug {doc_id}, "skipping push of uncompressed ops for project using project-history" + else + # project is using old track-changes history service + multi.rpush historyKeys.uncompressedHistoryOps(doc_id: doc_id), jsonOps... # index 7 # Set the unflushed timestamp to the current time if the doc # hasn't been modified before (the content in mongo has been # valid up to this point). Otherwise leave it alone ("NX" flag). @@ -262,8 +275,11 @@ module.exports = RedisManager = multi.exec (error, result) -> return callback(error) if error? - # length of uncompressedHistoryOps queue (index 7) - docUpdateCount = result[7] + if projectHistoryType is 'project-history' + docUpdateCount = undefined # only using project history, don't bother with track-changes + else + # project is using old track-changes history service + docUpdateCount = result[7] # length of uncompressedHistoryOps queue (index 7) if jsonOps.length > 0 && Settings.apis?.project_history?.enabled ProjectHistoryRedisManager.queueOps project_id, jsonOps..., (error, projectUpdateCount) -> diff --git a/services/document-updater/config/settings.defaults.coffee b/services/document-updater/config/settings.defaults.coffee index 6711b3c3bf..e8804dfe3e 100755 --- a/services/document-updater/config/settings.defaults.coffee +++ b/services/document-updater/config/settings.defaults.coffee @@ -70,13 +70,14 @@ module.exports = unflushedTime: ({doc_id}) -> "UnflushedTime:{#{doc_id}}" pathname: ({doc_id}) -> "Pathname:{#{doc_id}}" projectHistoryId: ({doc_id}) -> "ProjectHistoryId:{#{doc_id}}" + projectHistoryType: ({doc_id}) -> "ProjectHistoryType:{#{doc_id}}" projectState: ({project_id}) -> "ProjectState:{#{project_id}}" pendingUpdates: ({doc_id}) -> "PendingUpdates:{#{doc_id}}" lastUpdatedBy: ({doc_id}) -> "lastUpdatedBy:{#{doc_id}}" lastUpdatedAt: ({doc_id}) -> "lastUpdatedAt:{#{doc_id}}" pendingUpdates: ({doc_id}) -> "PendingUpdates:{#{doc_id}}" flushAndDeleteQueue: () -> "DocUpdaterFlushAndDeleteQueue" - + max_doc_length: 2 * 1024 * 1024 # 2mb dispatcherCount: process.env["DISPATCHER_COUNT"] @@ -91,4 +92,4 @@ module.exports = continuousBackgroundFlush: process.env['CONTINUOUS_BACKGROUND_FLUSH'] or false - smoothingOffset: process.env['SMOOTHING_OFFSET'] or 1000 # milliseconds \ No newline at end of file + smoothingOffset: process.env['SMOOTHING_OFFSET'] or 1000 # milliseconds diff --git a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee index dc57022a5a..76ad7f5af5 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee @@ -27,6 +27,7 @@ describe "DocumentManager", -> "./RangesManager": @RangesManager = {} @project_id = "project-id-123" @projectHistoryId = "history-id-123" + @projectHistoryType = "project-history" @doc_id = "doc-id-123" @user_id = 1234 @callback = sinon.stub() @@ -178,8 +179,9 @@ describe "DocumentManager", -> describe "when the doc does not exist in Redis", -> beforeEach -> @RedisManager.getDoc = sinon.stub().callsArgWith(2, null, null, null, null, null, null) - @PersistenceManager.getDoc = sinon.stub().callsArgWith(2, null, @lines, @version, @ranges, @pathname, @projectHistoryId) + @PersistenceManager.getDoc = sinon.stub().callsArgWith(2, null, @lines, @version, @ranges, @pathname, @projectHistoryId, @projectHistoryType) @RedisManager.putDocInMemory = sinon.stub().yields() + @RedisManager.setHistoryType = sinon.stub().yields() @DocumentManager.getDoc @project_id, @doc_id, @callback it "should try to get the doc from Redis", -> @@ -197,6 +199,11 @@ describe "DocumentManager", -> .calledWith(@project_id, @doc_id, @lines, @version, @ranges, @pathname, @projectHistoryId) .should.equal true + it "should set the history type in Redis", -> + @RedisManager.setHistoryType + .calledWith(@doc_id, @projectHistoryType) + .should.equal true + it "should call the callback with the doc info", -> @callback.calledWith(null, @lines, @version, @ranges, @pathname, @projectHistoryId, null, false).should.equal true diff --git a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee index 07c3577a91..20d5d0358b 100644 --- a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee @@ -15,7 +15,7 @@ describe "HistoryManager", -> trackchanges: url: "http://trackchanges.example.com" } - "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } + "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub(), debug: sinon.stub() } "./DocumentManager": @DocumentManager = {} "./HistoryRedisManager": @HistoryRedisManager = {} "./RedisManager": @RedisManager = {} @@ -28,12 +28,25 @@ describe "HistoryManager", -> beforeEach -> @request.post = sinon.stub().callsArgWith(1, null, statusCode: 204) - @HistoryManager.flushDocChangesAsync @project_id, @doc_id + describe "when the project uses track changes", -> + beforeEach -> + @RedisManager.getHistoryType = sinon.stub().yields(null, 'track-changes') + @HistoryManager.flushDocChangesAsync @project_id, @doc_id - it "should send a request to the track changes api", -> - @request.post - .calledWith("#{@Settings.apis.trackchanges.url}/project/#{@project_id}/doc/#{@doc_id}/flush") - .should.equal true + it "should send a request to the track changes api", -> + @request.post + .calledWith("#{@Settings.apis.trackchanges.url}/project/#{@project_id}/doc/#{@doc_id}/flush") + .should.equal true + + describe "when the project uses project history", -> + beforeEach -> + @RedisManager.getHistoryType = sinon.stub().yields(null, 'project-history') + @HistoryManager.flushDocChangesAsync @project_id, @doc_id + + it "should not send a request to the track changes api", -> + @request.post + .called + .should.equal false describe "flushProjectChangesAsync", -> beforeEach -> diff --git a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee index 5491922efb..508a9ba0f7 100644 --- a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee @@ -34,6 +34,7 @@ describe "RedisManager", -> ranges: ({doc_id}) -> "Ranges:#{doc_id}" pathname: ({doc_id}) -> "Pathname:#{doc_id}" projectHistoryId: ({doc_id}) -> "ProjectHistoryId:#{doc_id}" + projectHistoryType: ({doc_id}) -> "ProjectHistoryType:#{doc_id}" projectState: ({project_id}) -> "ProjectState:#{project_id}" unflushedTime: ({doc_id}) -> "UnflushedTime:#{doc_id}" lastUpdatedBy: ({doc_id}) -> "lastUpdatedBy:#{doc_id}"