From c57d3ce31c025605ad9ca467b697368068292d1f Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 17 Feb 2017 12:29:15 +0000 Subject: [PATCH 1/2] compute hash on write in redis server --- .../app/coffee/RedisManager.coffee | 24 +++++++++++++++---- .../RedisManager/RedisManagerTests.coffee | 14 ++++++----- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index ab95d1129f..f3d6360a0d 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -11,6 +11,13 @@ crypto = require "crypto" # Make times easy to read minutes = 60 # seconds for Redis expire +# LUA script to write document and return hash +# arguments: docLinesKey docLines +setScript = """ + redis.call('set', KEYS[1], ARGV[1]) + return redis.sha1hex(ARGV[1]) +""" + module.exports = RedisManager = rclient: rclient @@ -24,7 +31,7 @@ module.exports = RedisManager = logger.log project_id:project_id, doc_id:doc_id, version: version, hash:docHash, "putting doc in redis" ranges = RedisManager._serializeRanges(ranges) multi = rclient.multi() - multi.set keys.docLines(doc_id:doc_id), docLines + multi.eval setScript, 1, keys.docLines(doc_id:doc_id), docLines multi.set keys.projectKey({doc_id:doc_id}), project_id multi.set keys.docVersion(doc_id:doc_id), version multi.set keys.docHash(doc_id:doc_id), docHash @@ -32,8 +39,13 @@ module.exports = RedisManager = multi.set keys.ranges(doc_id:doc_id), ranges else multi.del keys.ranges(doc_id:doc_id) - multi.exec (error) -> + multi.exec (error, result) -> return callback(error) if error? + # check the hash computed on the redis server + writeHash = result?[0] + if writeHash? and writeHash isnt docHash + logger.error project_id: project_id, doc_id: doc_id, writeHash: writeHash, origHash: docHash, "hash mismatch on putDocInMemory" + # update docsInProject set rclient.sadd keys.docsInProject(project_id:project_id), doc_id, callback removeDocFromMemory : (project_id, doc_id, _callback)-> @@ -136,7 +148,7 @@ module.exports = RedisManager = multi = rclient.multi() newDocLines = JSON.stringify(docLines) newHash = RedisManager._computeHash(newDocLines) - multi.set keys.docLines(doc_id:doc_id), newDocLines + multi.eval setScript, 1, keys.docLines(doc_id:doc_id), newDocLines multi.set keys.docVersion(doc_id:doc_id), newVersion multi.set keys.docHash(doc_id:doc_id), newHash if jsonOps.length > 0 @@ -148,8 +160,12 @@ module.exports = RedisManager = multi.set keys.ranges(doc_id:doc_id), ranges else multi.del keys.ranges(doc_id:doc_id) - multi.exec (error, replys) -> + multi.exec (error, result) -> return callback(error) if error? + # check the hash computed on the redis server + writeHash = result?[0] + if writeHash? and writeHash isnt newHash + logger.error doc_id: doc_id, writeHash: writeHash, origHash: newHash, "hash mismatch on updateDocument" return callback() getDocIdsInProject: (project_id, callback = (error, doc_ids) ->) -> diff --git a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee index daad278174..618537f819 100644 --- a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee @@ -197,6 +197,7 @@ describe "RedisManager", -> @rclient.expire = sinon.stub() @rclient.ltrim = sinon.stub() @rclient.del = sinon.stub() + @rclient.eval = sinon.stub() @RedisManager.getDocVersion = sinon.stub() @lines = ["one", "two", "three"] @@ -218,8 +219,8 @@ describe "RedisManager", -> .should.equal true it "should set the doclines", -> - @rclient.set - .calledWith("doclines:#{@doc_id}", JSON.stringify(@lines)) + @rclient.eval + .calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines)) .should.equal true it "should set the version", -> @@ -279,8 +280,8 @@ describe "RedisManager", -> .should.equal false it "should still set the doclines", -> - @rclient.set - .calledWith("doclines:#{@doc_id}", JSON.stringify(@lines)) + @rclient.eval + .calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines)) .should.equal true describe "with empty ranges", -> @@ -303,6 +304,7 @@ describe "RedisManager", -> @rclient.set = sinon.stub() @rclient.sadd = sinon.stub().yields() @rclient.del = sinon.stub() + @rclient.eval = sinon.stub() @rclient.exec.yields() @lines = ["one", "two", "three"] @version = 42 @@ -314,8 +316,8 @@ describe "RedisManager", -> @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, done it "should set the lines", -> - @rclient.set - .calledWith("doclines:#{@doc_id}", JSON.stringify @lines) + @rclient.eval + .calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines)) .should.equal true it "should set the version", -> From 62165ddeab91ad92ea1e9334a2c44142e5c7dfdd Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 20 Feb 2017 15:33:19 +0000 Subject: [PATCH 2/2] add unit tests --- .../RedisManager/RedisManagerTests.coffee | 35 +++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee index 618537f819..ccf9d389c9 100644 --- a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee @@ -206,7 +206,7 @@ describe "RedisManager", -> @hash = crypto.createHash('sha1').update(JSON.stringify(@lines)).digest('hex') @ranges = { comments: "mock", entries: "mock" } - @rclient.exec = sinon.stub().callsArg(0) + @rclient.exec = sinon.stub().callsArg(0, null, [@hash]) describe "with a consistent version", -> beforeEach -> @@ -255,6 +255,10 @@ describe "RedisManager", -> it "should call the callback", -> @callback.called.should.equal true + + it 'should not log any errors', -> + @logger.error.calledWith() + .should.equal false describe "with an inconsistent version", -> beforeEach -> @@ -299,16 +303,30 @@ describe "RedisManager", -> .calledWith("Ranges:#{@doc_id}") .should.equal true + describe "with a corrupted write", -> + beforeEach -> + @badHash = "INVALID-HASH-VALUE" + @rclient.exec = sinon.stub().callsArgWith(0, null, [@badHash]) + @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length) + @RedisManager.updateDocument @doc_id, @lines, @version, @ops, @ranges, @callback + + it 'should log a hash error', -> + @logger.error.calledWith() + .should.equal true + + it "should call the callback", -> + @callback.called.should.equal true + describe "putDocInMemory", -> beforeEach -> @rclient.set = sinon.stub() @rclient.sadd = sinon.stub().yields() @rclient.del = sinon.stub() @rclient.eval = sinon.stub() - @rclient.exec.yields() @lines = ["one", "two", "three"] @version = 42 @hash = crypto.createHash('sha1').update(JSON.stringify(@lines)).digest('hex') + @rclient.exec = sinon.stub().callsArgWith(0, null, [@hash]) @ranges = { comments: "mock", entries: "mock" } describe "with non-empty ranges", -> @@ -344,6 +362,10 @@ describe "RedisManager", -> @rclient.sadd .calledWith("DocsIn:#{@project_id}", @doc_id) .should.equal true + + it 'should not log any errors', -> + @logger.error.calledWith() + .should.equal false describe "with empty ranges", -> beforeEach (done) -> @@ -359,6 +381,15 @@ describe "RedisManager", -> .calledWith("Ranges:#{@doc_id}", JSON.stringify(@ranges)) .should.equal false + describe "with a corrupted write", -> + beforeEach (done) -> + @rclient.exec = sinon.stub().callsArgWith(0, null, ["INVALID-HASH-VALUE"]) + @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, done + + it 'should log a hash error', -> + @logger.error.calledWith() + .should.equal true + describe "removeDocFromMemory", -> beforeEach (done) -> @rclient.del = sinon.stub()