From 9bc30387288ef06971d21e3460ec319685e1101b Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Wed, 2 Aug 2017 14:32:01 +0900
Subject: [PATCH] Fix Redis command errors when trying to start app with a
 readonly Redis.

---
 app/models/global_setting.rb             |  2 +
 config/initializers/006-mini_profiler.rb |  4 +-
 lib/discourse_redis.rb                   | 23 +++++---
 spec/components/discourse_redis_spec.rb  | 68 +++++++++++++++++++++++-
 4 files changed, 86 insertions(+), 11 deletions(-)

diff --git a/app/models/global_setting.rb b/app/models/global_setting.rb
index 84750ecc695..e141e151e1f 100644
--- a/app/models/global_setting.rb
+++ b/app/models/global_setting.rb
@@ -48,6 +48,8 @@ class GlobalSetting
       end
       token
     end
+  rescue Redis::CommandError => e
+    @safe_secret_key_base = SecureRandom.hex(64) if e.message =~ /READONLY/
   end
 
   def self.load_defaults
diff --git a/config/initializers/006-mini_profiler.rb b/config/initializers/006-mini_profiler.rb
index a04c888fceb..8a0ceb8a1a8 100644
--- a/config/initializers/006-mini_profiler.rb
+++ b/config/initializers/006-mini_profiler.rb
@@ -19,7 +19,9 @@ if defined?(Rack::MiniProfiler)
   #   raw_connection means results are not namespaced
   #
   # namespacing gets complex, cause mini profiler is in the rack chain way before multisite
-  Rack::MiniProfiler.config.storage_instance = Rack::MiniProfiler::RedisStore.new(connection:  DiscourseRedis.raw_connection)
+  Rack::MiniProfiler.config.storage_instance = Rack::MiniProfiler::RedisStore.new(
+    connection:  DiscourseRedis.new(nil, namespace: false)
+  )
 
   skip = [
     /^\/message-bus/,
diff --git a/lib/discourse_redis.rb b/lib/discourse_redis.rb
index f646cba58e8..bffba0860af 100644
--- a/lib/discourse_redis.rb
+++ b/lib/discourse_redis.rb
@@ -135,9 +135,10 @@ class DiscourseRedis
     options.dup.merge!(host: options[:slave_host], port: options[:slave_port])
   end
 
-  def initialize(config = nil)
+  def initialize(config = nil, namespace: true)
     @config = config || DiscourseRedis.config
     @redis = DiscourseRedis.raw_connection(@config)
+    @namespace = namespace
   end
 
   def self.fallback_handler
@@ -183,29 +184,35 @@ class DiscourseRedis
    :sunion, :ttl, :type, :watch, :zadd, :zcard, :zcount, :zincrby, :zrange, :zrangebyscore, :zrank, :zrem, :zremrangebyrank,
    :zremrangebyscore, :zrevrange, :zrevrangebyscore, :zrevrank, :zrangebyscore].each do |m|
     define_method m do |*args|
-      args[0] = "#{namespace}:#{args[0]}"
+      args[0] = "#{namespace}:#{args[0]}" if @namespace
       DiscourseRedis.ignore_readonly { @redis.send(m, *args) }
     end
   end
 
   def mget(*args)
-    args.map! { |a| "#{namespace}:#{a}" }
+    args.map! { |a| "#{namespace}:#{a}" }  if @namespace
     DiscourseRedis.ignore_readonly { @redis.mget(*args) }
   end
 
   def del(k)
     DiscourseRedis.ignore_readonly do
-      k = "#{namespace}:#{k}"
+      k = "#{namespace}:#{k}"  if @namespace
       @redis.del k
     end
   end
 
   def keys(pattern = nil)
     DiscourseRedis.ignore_readonly do
-      len = namespace.length + 1
-      @redis.keys("#{namespace}:#{pattern || '*'}").map {
-        |k| k[len..-1]
-      }
+      pattern = pattern || '*'
+      pattern = "#{namespace}:#{pattern}" if @namespace
+      keys = @redis.keys(pattern)
+
+      if @namespace
+        len = namespace.length + 1
+        keys.map! { |k| k[len..-1] }
+      end
+
+      keys
     end
   end
 
diff --git a/spec/components/discourse_redis_spec.rb b/spec/components/discourse_redis_spec.rb
index a6568427ef9..471752965a7 100644
--- a/spec/components/discourse_redis_spec.rb
+++ b/spec/components/discourse_redis_spec.rb
@@ -10,6 +10,70 @@ describe DiscourseRedis do
 
   let(:fallback_handler) { DiscourseRedis::FallbackHandler.instance }
 
+  describe 'redis commands' do
+    let(:raw_redis) { Redis.new(DiscourseRedis.config) }
+
+    before do
+      raw_redis.flushall
+    end
+
+    after do
+      raw_redis.flushall
+    end
+
+    describe 'when namespace is enabled' do
+      let(:redis) { DiscourseRedis.new }
+
+      it 'should append namespace to the keys' do
+        redis.set('key', 1)
+
+        expect(raw_redis.get('default:key')).to eq('1')
+        expect(redis.keys).to eq(['key'])
+
+        redis.del('key')
+
+        expect(raw_redis.get('default:key')).to eq(nil)
+
+        raw_redis.set('default:key1', '1')
+        raw_redis.set('default:key2', '2')
+
+        expect(redis.mget('key1', 'key2')).to eq(['1', '2'])
+      end
+    end
+
+    describe 'when namespace is disabled' do
+      let(:redis) { DiscourseRedis.new(nil, namespace: false) }
+
+      it 'should not append any namespace to the keys' do
+        redis.set('key', 1)
+
+        expect(raw_redis.get('key')).to eq('1')
+        expect(redis.keys).to eq(['key'])
+
+        redis.del('key')
+
+        expect(raw_redis.get('key')).to eq(nil)
+
+        raw_redis.set('key1', '1')
+        raw_redis.set('key2', '2')
+
+        expect(redis.mget('key1', 'key2')).to eq(['1', '2'])
+      end
+
+      it 'should noop a readonly redis' do
+        expect(Discourse.recently_readonly?).to eq(false)
+
+        redis.without_namespace
+          .expects(:set)
+          .raises(Redis::CommandError.new("READONLY"))
+
+        redis.set('key', 1)
+
+        expect(Discourse.recently_readonly?).to eq(true)
+      end
+    end
+  end
+
   context '.slave_host' do
     it 'should return the right config' do
       slave_config = DiscourseRedis.slave_config(config)
@@ -22,9 +86,9 @@ describe DiscourseRedis do
     it 'should check the status of the master server' do
       begin
         fallback_handler.master = false
-        $redis.without_namespace.expects(:get).raises(Redis::CommandError.new("READONLY"))
+        $redis.without_namespace.expects(:set).raises(Redis::CommandError.new("READONLY"))
         fallback_handler.expects(:verify_master).once
-        $redis.get('test')
+        $redis.set('test', '1')
       ensure
         fallback_handler.master = true
       end