Check that return values from different redis backends match

services/document-updater/app/coffee/RedisBackend.coffee

@@ -1,5 +1,7 @@
 Settings = require "settings-sharelatex"
 async = require "async"
+_ = require "underscore"
+logger = require "logger-sharelatex"
 
 class Client
 	constructor: (@clients) ->
@@ -10,6 +12,7 @@ class Client
 				rclient: client.rclient.multi()
 				key_schema: client.key_schema
 				primary: client.primary
+				driver: client.driver
 			}
 		)
 
@@ -19,56 +22,84 @@ class MultiClient
 	exec: (callback) ->
 		jobs = @clients.map (client) ->
 			(cb) ->
-				console.error "EXEC", client.rclient.queue
-				client.rclient.exec (result...) ->
-					console.error "EXEC RESULT", result
+				client.rclient.exec (error, result) ->
+					if client.driver == "ioredis"
+						# ioredis returns an results like:
+						# [ [null, 42], [null, "foo"] ]
+						# where the first entries in each 2-tuple are
+						# presumably errors for each individual command,
+						# and the second entry is the result. We need to transform
+						# this into the same result as the old redis driver:
+						# [ 42, "foo" ]
+						filtered_result = []
+						for entry in result or []
+							if entry[0]?
+								return cb(entry[0])
+							else
+								filtered_result.push entry[1]
+						result = filtered_result
+						
 					if client.primary
 						# Return this result as the actual result
-						callback(result...)
+						callback(error, result)
 					# Send the rest through for comparison
-					cb(result...)
+					cb(error, result)
 		async.parallel jobs, (error, results) ->
-			console.error "EXEC RESULTS", results
+			if error?
+				logger.error {err: error}, "error in redis backend"
+			else
+				compareResults(results)
 
-COMMANDS = [
-	"get", "smembers", "set", "srem", "sadd", "del", "lrange",
-	"llen", "rpush", "expire", "ltrim", "incr"
-]
-for command in COMMANDS
-	do (command) ->
-		Client.prototype[command] = (key_builder, args..., callback) ->
-			async.parallel @clients.map (client) ->
+COMMANDS = {
+	"get": 0,
+	"smembers": 0,
+	"set": 0,
+	"srem": 0,
+	"sadd": 0,
+	"del": 0,
+	"lrange": 0,
+	"llen": 0,
+	"rpush": 0,
+	"expire": 0,
+	"ltrim": 0,
+	"incr": 0,
+	"eval": 2
+}
+for command, key_pos of COMMANDS
+	do (command, key_pos) ->
+		Client.prototype[command] = (args..., callback) ->
+			jobs = @clients.map (client) ->
 				(cb) ->
+					key_builder = args[key_pos]
 					key = key_builder(client.key_schema)
-					console.error "COMMAND", command, key, args
-					client.rclient[command] key, args..., (result...) ->
-						console.log "RESULT", command, result
+					args_with_key = args.slice(0)
+					args_with_key[key_pos] = key
+					client.rclient[command] args_with_key..., (error, result...) ->
 						if client.primary
 							# Return this result as the actual result
-							callback?(result...)
+							callback(error, result...)
 						# Send the rest through for comparison
-						cb(result...)
-			, (error, results) ->
-				console.log "#{command} RESULTS", results
+						cb(error, result...)
+			async.parallel jobs, (error, results) ->
+				if error?
+					logger.error {err: error}, "error in redis backend"
+				else
+					compareResults(results)
 
-		MultiClient.prototype[command] = (key_builder, args...) ->
+		MultiClient.prototype[command] = (args...) ->
 			for client in @clients
+				key_builder = args[key_pos]
 				key = key_builder(client.key_schema)
-				console.error "MULTI COMMAND", command, key, args
+				args_with_key = args.slice(0)
+				args_with_key[key_pos] = key
 				client.rclient[command] key, args...
 
-Client::eval = (script, pos, key_builder, args..., callback) ->
-	async.parallel @clients.map (client) ->
-		(cb) ->
-			key = key_builder(client.key_schema)
-			client.rclient.eval script, pos, key, args..., (result...) ->
-				if client.primary
-					# Return this result as the actual result
-					callback(result...)
-				# Send the rest through for comparison
-				cb(result...)
-	, (error, results) ->
-		console.log "#{command} RESULTS", results
+compareResults = (results) ->
+	return if results.length < 2
+	first = results[0]
+	for result in results.slice(1)
+		if not _.isEqual(first, result)
+			logger.warn { results }, "redis return values do not match"
 
 module.exports =
 	createClient: () ->
@@ -80,9 +111,15 @@ module.exports =
 			if config.cluster?
 				Redis = require("ioredis")
 				rclient = new Redis.Cluster(config.cluster)
+				driver = "ioredis"
 			else
-				rclient = require("redis-sharelatex").createClient(config)
-			rclient: rclient
-			key_schema: config.key_schema
-			primary: config.primary
+				{host, port, password} = config
+				rclient = require("redis-sharelatex").createClient({host, port, password})
+				driver = "redis"
+			return {
+				rclient: rclient
+				key_schema: config.key_schema
+				primary: config.primary
+				driver: driver
+			}
 		return new Client(clients)

services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee

@@ -0,0 +1,307 @@
+sinon = require('sinon')
+chai = require('chai')
+should = chai.should()
+modulePath = "../../../../app/js/RedisBackend.js"
+SandboxedModule = require('sandboxed-module')
+RedisKeyBuilder = require "../../../../app/js/RedisKeyBuilder"
+
+describe "RedisBackend", ->
+	beforeEach ->
+		@Settings =
+			redis:
+				documentupdater: [{
+					primary: true
+					port: "6379"
+					host: "localhost"
+					password: "single-password"
+					key_schema:
+						blockingKey: ({doc_id}) -> "Blocking:#{doc_id}"
+						docLines: ({doc_id}) -> "doclines:#{doc_id}"
+						docOps: ({doc_id}) -> "DocOps:#{doc_id}"
+						docVersion: ({doc_id}) -> "DocVersion:#{doc_id}"
+						projectKey: ({doc_id}) -> "ProjectId:#{doc_id}"
+						pendingUpdates: ({doc_id}) -> "PendingUpdates:#{doc_id}"
+						docsInProject: ({project_id}) -> "DocsIn:#{project_id}"
+				}, {
+					cluster: [{
+						port: "7000"
+						host: "localhost"
+					}]
+					password: "cluster-password"
+					key_schema:
+						blockingKey: ({doc_id}) -> "Blocking:{#{doc_id}}"
+						docLines: ({doc_id}) -> "doclines:{#{doc_id}}"
+						docOps: ({doc_id}) -> "DocOps:{#{doc_id}}"
+						docVersion: ({doc_id}) -> "DocVersion:{#{doc_id}}"
+						projectKey: ({doc_id}) -> "ProjectId:{#{doc_id}}"
+						pendingUpdates: ({doc_id}) -> "PendingUpdates:{#{doc_id}}"
+						docsInProject: ({project_id}) -> "DocsIn:{#{project_id}}"
+				}]
+
+		test_context = @
+		class Cluster
+			constructor: (@config) ->
+				test_context.rclient_ioredis = @
+
+		@RedisBackend = SandboxedModule.require modulePath, requires:
+			"settings-sharelatex": @Settings
+			"logger-sharelatex": @logger = { error: sinon.stub(), log: sinon.stub(), warn: sinon.stub() }
+			"redis-sharelatex": @redis =
+				createClient: sinon.stub().returns @rclient_redis = {}
+			"ioredis": @ioredis =
+				Cluster: Cluster
+		@client = @RedisBackend.createClient()
+		
+		@doc_id = "mock-doc-id"
+	
+	it "should create a redis client", ->
+		@redis.createClient
+			.calledWith({
+				port: "6379"
+				host: "localhost"
+				password: "single-password"
+			})
+			.should.equal true
+	
+	it "should create an ioredis cluster client", ->
+		@rclient_ioredis.config.should.deep.equal [{
+			port: "7000"
+			host: "localhost"
+		}]
+
+	describe "individual commands", ->
+		describe "with the same results", ->
+			beforeEach (done) ->
+				@content = "bar"
+				@rclient_redis.get = sinon.stub()
+				@rclient_redis.get.withArgs("doclines:#{@doc_id}").yields(null, @content)
+				@rclient_ioredis.get = sinon.stub()
+				@rclient_ioredis.get.withArgs("doclines:{#{@doc_id}}").yields(null, @content)
+				@client.get RedisKeyBuilder.docLines({doc_id: @doc_id}), (error, @result) =>
+					setTimeout () -> # Let all background requests complete
+						done(error)
+			
+			it "should return the result", ->
+				@result.should.equal @content
+			
+			it "should have called the redis client with the appropriate key", ->
+				@rclient_redis.get
+					.calledWith("doclines:#{@doc_id}")
+					.should.equal true
+				
+			it "should have called the ioredis cluster client with the appropriate key", ->
+				@rclient_ioredis.get
+					.calledWith("doclines:{#{@doc_id}}")
+					.should.equal true
+
+		describe "with different results", ->
+			beforeEach (done) ->
+				@rclient_redis.get = sinon.stub()
+				@rclient_redis.get.withArgs("doclines:#{@doc_id}").yields(null, "primary-result")
+				@rclient_ioredis.get = sinon.stub()
+				@rclient_ioredis.get.withArgs("doclines:{#{@doc_id}}").yields(null, "secondary-result")
+				@client.get RedisKeyBuilder.docLines({doc_id: @doc_id}), (error, @result) =>
+					setTimeout () -> # Let all background requests complete
+						done(error)
+			
+			it "should return the primary result", ->
+				@result.should.equal "primary-result"
+			
+			it "should log out the difference", ->
+				@logger.warn
+					.calledWith({
+						results: [
+							"primary-result",
+							"secondary-result"
+						]
+					}, "redis return values do not match")
+					.should.equal true
+
+		describe "when the secondary errors", ->
+			beforeEach (done) ->
+				@rclient_redis.get = sinon.stub()
+				@rclient_redis.get.withArgs("doclines:#{@doc_id}").yields(null, "primary-result")
+				@rclient_ioredis.get = sinon.stub()
+				@rclient_ioredis.get.withArgs("doclines:{#{@doc_id}}").yields(@error = new Error("oops"))
+				@client.get RedisKeyBuilder.docLines({doc_id: @doc_id}), (error, @result) =>
+					setTimeout () -> # Let all background requests complete
+						done(error)
+			
+			it "should return the primary result", ->
+				@result.should.equal "primary-result"
+			
+			it "should log out the secondary error", ->
+				@logger.error
+					.calledWith({
+						err: @error
+					}, "error in redis backend")
+					.should.equal true
+
+		describe "when the primary errors", ->
+			beforeEach (done) ->
+				@rclient_redis.get = sinon.stub()
+				@rclient_redis.get.withArgs("doclines:#{@doc_id}").yields(@error = new Error("oops"))
+				@rclient_ioredis.get = sinon.stub()
+				@rclient_ioredis.get.withArgs("doclines:{#{@doc_id}}").yields(null, "secondary-result")
+				@client.get RedisKeyBuilder.docLines({doc_id: @doc_id}), (@returned_error, @result) =>
+					setTimeout () -> # Let all background requests complete
+						done()
+			
+			it "should return the error", ->
+				@returned_error.should.equal @error
+			
+			it "should log out the error", ->
+				@logger.error
+					.calledWith({
+						err: @error
+					}, "error in redis backend")
+					.should.equal true
+		
+		describe "when the command has the key in a non-zero argument index", ->
+			beforeEach (done) ->
+				@script = "mock-script"
+				@key_count = 1
+				@value = "mock-value"
+				@rclient_redis.eval = sinon.stub()
+				@rclient_redis.eval.withArgs(@script, @key_count, "Blocking:#{@doc_id}", @value).yields(null)
+				@rclient_ioredis.eval = sinon.stub()
+				@rclient_ioredis.eval.withArgs(@script, @key_count, "Blocking:{#{@doc_id}}", @value).yields(null, @content)
+				@client.eval @script, @key_count, RedisKeyBuilder.blockingKey({doc_id: @doc_id}), @value, (error) =>
+					setTimeout () -> # Let all background requests complete
+						done(error)
+			
+			it "should have called the redis client with the appropriate key", ->
+				@rclient_redis.eval
+					.calledWith(@script, @key_count, "Blocking:#{@doc_id}", @value)
+					.should.equal true
+				
+			it "should have called the ioredis cluster client with the appropriate key", ->
+				@rclient_ioredis.eval
+					.calledWith(@script, @key_count, "Blocking:{#{@doc_id}}", @value)
+					.should.equal true
+
+	describe "multi commands", ->
+		beforeEach ->
+			# We will test with:
+			# rclient.multi()
+			#     .get("doclines:foo")
+			#     .get("DocVersion:foo")
+			#     .exec (...) ->
+			@doclines = "mock-doclines"
+			@version = "42"
+			@rclient_redis.multi = sinon.stub().returns @rclient_redis
+			@rclient_ioredis.multi = sinon.stub().returns @rclient_ioredis
+
+		describe "with the same results", ->
+			beforeEach (done) ->
+				@rclient_redis.get = sinon.stub()
+				@rclient_redis.exec = sinon.stub().yields(null, [@doclines, @version])
+				@rclient_ioredis.get = sinon.stub()
+				@rclient_ioredis.exec = sinon.stub().yields(null, [ [null, @doclines], [null, @version] ])
+				
+				multi = @client.multi()
+				multi.get RedisKeyBuilder.docLines({doc_id: @doc_id})
+				multi.get RedisKeyBuilder.docVersion({doc_id: @doc_id})
+				multi.exec (error, @result) =>
+					setTimeout () ->
+						done(error)
+			
+			it "should return the result", ->
+				@result.should.deep.equal [@doclines, @version]
+			
+			it "should have called the redis client with the appropriate keys", ->
+				@rclient_redis.get
+					.calledWith("doclines:#{@doc_id}")
+					.should.equal true
+				@rclient_redis.get
+					.calledWith("DocVersion:#{@doc_id}")
+					.should.equal true
+				@rclient_ioredis.exec
+					.called
+					.should.equal true
+				
+			it "should have called the ioredis cluster client with the appropriate keys", ->
+				@rclient_ioredis.get
+					.calledWith("doclines:{#{@doc_id}}")
+					.should.equal true
+				@rclient_ioredis.get
+					.calledWith("DocVersion:{#{@doc_id}}")
+					.should.equal true
+				@rclient_ioredis.exec
+					.called
+					.should.equal true
+
+		describe "with different results", ->
+			beforeEach (done) ->
+				@rclient_redis.get = sinon.stub()
+				@rclient_redis.exec = sinon.stub().yields(null, [@doclines, @version])
+				@rclient_ioredis.get = sinon.stub()
+				@rclient_ioredis.exec = sinon.stub().yields(null, [ [null, "different-doc-lines"], [null, @version] ])
+				
+				multi = @client.multi()
+				multi.get RedisKeyBuilder.docLines({doc_id: @doc_id})
+				multi.get RedisKeyBuilder.docVersion({doc_id: @doc_id})
+				multi.exec (error, @result) =>
+					setTimeout () ->
+						done(error)
+			
+			it "should return the primary result", ->
+				@result.should.deep.equal [@doclines, @version]
+			
+			it "should log out the difference", ->
+				@logger.warn
+					.calledWith({
+						results: [
+							[@doclines, @version],
+							["different-doc-lines", @version]
+						]
+					}, "redis return values do not match")
+					.should.equal true
+
+		describe "when the secondary errors", ->
+			beforeEach (done) ->
+				@rclient_redis.get = sinon.stub()
+				@rclient_redis.exec = sinon.stub().yields(null, [@doclines, @version])
+				@rclient_ioredis.get = sinon.stub()
+				@rclient_ioredis.exec = sinon.stub().yields(@error = new Error("oops"))
+				
+				multi = @client.multi()
+				multi.get RedisKeyBuilder.docLines({doc_id: @doc_id})
+				multi.get RedisKeyBuilder.docVersion({doc_id: @doc_id})
+				multi.exec (error, @result) =>
+					setTimeout () ->
+						done(error)
+			
+			it "should return the primary result", ->
+				@result.should.deep.equal [@doclines, @version]
+			
+			it "should log out the secondary error", ->
+				@logger.error
+					.calledWith({
+						err: @error
+					}, "error in redis backend")
+					.should.equal true
+
+		describe "when the secondary errors", ->
+			beforeEach (done) ->
+				@rclient_redis.get = sinon.stub()
+				@rclient_redis.exec = sinon.stub().yields(@error = new Error("oops"))
+				@rclient_ioredis.get = sinon.stub()
+				@rclient_ioredis.exec = sinon.stub().yields([ [null, @doclines], [null, @version] ])
+				
+				multi = @client.multi()
+				multi.get RedisKeyBuilder.docLines({doc_id: @doc_id})
+				multi.get RedisKeyBuilder.docVersion({doc_id: @doc_id})
+				multi.exec (@returned_error) =>
+					setTimeout () -> done()
+			
+			it "should return the error", ->
+				@returned_error.should.equal @error
+			
+			it "should log out the error", ->
+				@logger.error
+					.calledWith({
+						err: @error
+					}, "error in redis backend")
+					.should.equal true
+