From 9bc7594226688dd5b6d4116f7035c90432759170 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 2 Sep 2016 14:47:41 +0100 Subject: [PATCH] clean up redis query --- .../app/coffee/DocumentManager.coffee | 9 ++++----- .../app/coffee/RedisManager.coffee | 10 ++++++++-- .../flushDocIfLoadedTests.coffee | 8 ++++---- .../coffee/DocumentManager/getDocTests.coffee | 8 ++++---- .../RedisManager/RedisManagerTests.coffee | 19 +++++++++++++++++-- 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index 69311bd979..6bdffb76b8 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -7,13 +7,13 @@ Metrics = require "./Metrics" TrackChangesManager = require "./TrackChangesManager" module.exports = DocumentManager = - getDoc: (project_id, doc_id, _callback = (error, lines, version) ->) -> + getDoc: (project_id, doc_id, _callback = (error, lines, version, alreadyLoaded) ->) -> timer = new Metrics.Timer("docManager.getDoc") callback = (args...) -> timer.done() _callback(args...) - RedisManager.getDoc doc_id, (error, lines, version, alreadyLoaded) -> + RedisManager.getDoc project_id, doc_id, (error, lines, version) -> return callback(error) if error? if !lines? or !version? logger.log project_id: project_id, doc_id: doc_id, "doc not in redis so getting from persistence API" @@ -90,12 +90,11 @@ module.exports = DocumentManager = callback = (args...) -> timer.done() _callback(args...) - - RedisManager.getDoc doc_id, (error, lines, version) -> + RedisManager.getDoc project_id, doc_id, (error, lines, version) -> return callback(error) if error? if !lines? or !version? logger.log project_id: project_id, doc_id: doc_id, "doc is not loaded so not flushing" - callback null + callback null # TODO: return a flag to bail out, as we go on to remove doc from memory? else logger.log project_id: project_id, doc_id: doc_id, version: version, "flushing doc" PersistenceManager.setDoc project_id, doc_id, lines, version, (error) -> diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index 2e59fefd57..d7790dfe5e 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -45,11 +45,12 @@ module.exports = RedisManager = return callback(error) if error? rclient.srem keys.docsInProject(project_id:project_id), doc_id, callback - getDoc : (doc_id, callback = (error, lines, version) ->)-> + getDoc : (project_id, doc_id, callback = (error, lines, version, project_id) ->)-> timer = new metrics.Timer("redis.get-doc") multi = rclient.multi() multi.get keys.docLines(doc_id:doc_id) multi.get keys.docVersion(doc_id:doc_id) + multi.get keys.projectKey(doc_id:doc_id) multi.exec (error, result)-> timer.done() return callback(error) if error? @@ -58,7 +59,12 @@ module.exports = RedisManager = catch e return callback(e) version = parseInt(result[1] or 0, 10) - callback null, docLines, version + doc_project_id = result[2] + # check doc is in requested project + if doc_project_id? and doc_project_id isnt project_id + logger.error project_id: project_id, doc_id: doc_id, doc_project_id: doc_project_id, "doc not in project" + return callback(new Errors.NotFoundError("document not found")) + callback null, docLines, version, project_id getDocVersion: (doc_id, callback = (error, version) ->) -> rclient.get keys.docVersion(doc_id: doc_id), (error, version) -> diff --git a/services/document-updater/test/unit/coffee/DocumentManager/flushDocIfLoadedTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/flushDocIfLoadedTests.coffee index bda914999b..4a17e2b84c 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/flushDocIfLoadedTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/flushDocIfLoadedTests.coffee @@ -23,13 +23,13 @@ describe "DocumentManager.flushDocIfLoaded", -> describe "when the doc is in Redis", -> beforeEach -> - @RedisManager.getDoc = sinon.stub().callsArgWith(1, null, @lines, @version) + @RedisManager.getDoc = sinon.stub().callsArgWith(2, null, @lines, @version) @PersistenceManager.setDoc = sinon.stub().callsArgWith(4) @DocumentManager.flushDocIfLoaded @project_id, @doc_id, @callback it "should get the doc from redis", -> @RedisManager.getDoc - .calledWith(@doc_id) + .calledWith(@project_id, @doc_id) .should.equal true it "should write the doc lines to the persistence layer", -> @@ -45,14 +45,14 @@ describe "DocumentManager.flushDocIfLoaded", -> describe "when the document is not in Redis", -> beforeEach -> - @RedisManager.getDoc = sinon.stub().callsArgWith(1, null, null, null) + @RedisManager.getDoc = sinon.stub().callsArgWith(2, null, null, null) @PersistenceManager.setDoc = sinon.stub().callsArgWith(4) @DocOpsManager.flushDocOpsToMongo = sinon.stub().callsArgWith(2) @DocumentManager.flushDocIfLoaded @project_id, @doc_id, @callback it "should get the doc from redis", -> @RedisManager.getDoc - .calledWith(@doc_id) + .calledWith(@project_id, @doc_id) .should.equal true it "should not write anything to the persistence layer", -> diff --git a/services/document-updater/test/unit/coffee/DocumentManager/getDocTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/getDocTests.coffee index b11686ac3c..3edf4cb67d 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/getDocTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/getDocTests.coffee @@ -24,12 +24,12 @@ describe "DocumentUpdater.getDoc", -> describe "when the doc exists in Redis", -> beforeEach -> - @RedisManager.getDoc = sinon.stub().callsArgWith(1, null, @lines, @version) + @RedisManager.getDoc = sinon.stub().callsArgWith(2, null, @lines, @version) @DocumentManager.getDoc @project_id, @doc_id, @callback it "should get the doc from Redis", -> @RedisManager.getDoc - .calledWith(@doc_id) + .calledWith(@project_id, @doc_id) .should.equal true it "should call the callback with the doc info", -> @@ -40,14 +40,14 @@ describe "DocumentUpdater.getDoc", -> describe "when the doc does not exist in Redis", -> beforeEach -> - @RedisManager.getDoc = sinon.stub().callsArgWith(1, null, null, null) + @RedisManager.getDoc = sinon.stub().callsArgWith(2, null, null, null) @PersistenceManager.getDoc = sinon.stub().callsArgWith(2, null, @lines, @version) @RedisManager.putDocInMemory = sinon.stub().callsArg(4) @DocumentManager.getDoc @project_id, @doc_id, @callback it "should try to get the doc from Redis", -> @RedisManager.getDoc - .calledWith(@doc_id) + .calledWith(@project_id, @doc_id) .should.equal true it "should get the doc from the PersistenceManager", -> diff --git a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee index 6ca5250050..7e811e3858 100644 --- a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee @@ -38,8 +38,8 @@ describe "RedisManager", -> @jsonlines = JSON.stringify @lines @version = 42 @rclient.get = sinon.stub() - @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version]) - @RedisManager.getDoc @doc_id, @callback + @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @project_id]) + @RedisManager.getDoc @project_id, @doc_id, @callback it "should get the lines from redis", -> @rclient.get @@ -56,6 +56,21 @@ describe "RedisManager", -> .calledWith(null, @lines, @version) .should.equal true + describe "getDoc with an invalid project id", -> + beforeEach -> + @lines = ["one", "two", "three"] + @jsonlines = JSON.stringify @lines + @version = 42 + @another_project_id = "project-id-456" + @rclient.get = sinon.stub() + @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @another_project_id]) + @RedisManager.getDoc @project_id, @doc_id, @callback + + it 'should return an error', -> + @callback + .calledWith(new Errors.NotFoundError("not found")) + .should.equal true + describe "getPreviousDocOpsTests", -> describe "with a start and an end value", -> beforeEach ->