Eagerly check for store validity (#714)

Ref: #315

`respond_to?` is a relatively expensive method that doesn't
benefit from inline caching, so it has to keep looking up
the method every time. Hence it's best to avoid it in hotspots.

In this case, `@store` can only change through the `store=` method,
so we might as well eagerly validate it there.

First because this way it's a one time fixed cost, but also because
this way the error is raised early during boot/configuration rather
than at runtime where it has availability impact.

Author: byroot

lib/rack/attack/cache.rb

@@ -26,6 +26,9 @@ def store=(store)
           else
             store
           end
+        if @store
+          check_store_methods_presence(:read, :write, :delete, :increment)
+        end
       end
 
       def count(unprefixed_key, period)
@@ -34,13 +37,14 @@ def count(unprefixed_key, period)
       end
 
       def read(unprefixed_key)
-        enforce_store_presence!
-        enforce_store_method_presence!(:read)
+        raise Rack::Attack::MissingStoreError if store.nil?
 
         store.read("#{prefix}:#{unprefixed_key}")
       end
 
       def write(unprefixed_key, value, expires_in)
+        raise Rack::Attack::MissingStoreError if store.nil?
+
         store.write("#{prefix}:#{unprefixed_key}", value, expires_in: expires_in)
       end
 
@@ -74,32 +78,22 @@ def key_and_expiry(unprefixed_key, period)
       end
 
       def do_count(key, expires_in)
-        enforce_store_presence!
-        enforce_store_method_presence!(:increment)
+        raise Rack::Attack::MissingStoreError if store.nil?
 
         result = store.increment(key, 1, expires_in: expires_in)
 
         # NB: Some stores return nil when incrementing uninitialized values
         if result.nil?
-          enforce_store_method_presence!(:write)
-
           store.write(key, 1, expires_in: expires_in)
         end
         result || 1
       end
 
-      def enforce_store_presence!
-        if store.nil?
-          raise Rack::Attack::MissingStoreError
-        end
-      end
-
-      def enforce_store_method_presence!(method_name)
-        if !store.respond_to?(method_name)
-          raise(
-            Rack::Attack::MisconfiguredStoreError,
-            "Configured store #{store.class.name} doesn't respond to ##{method_name} method"
-          )
+      def check_store_methods_presence(*method_names)
+        missing = method_names.reject { |m| store.respond_to?(m) }
+        unless missing.empty?
+          missing = missing.map { |m| "##{m}" }.join(", ")
+          warn "[rack-attack] Configured store #{store.class.name} doesn't respond to #{missing}"
         end
       end
     end

spec/acceptance/cache_store_config_for_allow2ban_spec.rb

@@ -20,67 +20,14 @@
     end
   end
 
-  it "gives semantic error if store is missing #read method" do
-    raised_exception = nil
-
-    fake_store_class = Class.new do
-      def write(key, value); end
-
-      def increment(key, count, options = {}); end
+  it "display warning if store is missing methods" do
+    warning = "[rack-attack] Configured store Object doesn't respond to #read, #write, #delete, #increment\n"
+    assert_output("", warning) do
+      Rack::Attack.cache.store = Object.new
     end
-
-    Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
-      raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/scarce-resource"
-      end
-    end
-
-    assert_equal "Configured store FakeStore doesn't respond to #read method", raised_exception.message
   end
 
-  it "gives semantic error if store is missing #write method" do
-    raised_exception = nil
-
-    fake_store_class = Class.new do
-      def read(key); end
-
-      def increment(key, count, options = {}); end
-    end
-
-    Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
-      raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/scarce-resource"
-      end
-    end
-
-    assert_equal "Configured store FakeStore doesn't respond to #write method", raised_exception.message
-  end
-
-  it "gives semantic error if store is missing #increment method" do
-    raised_exception = nil
-
-    fake_store_class = Class.new do
-      def read(key); end
-
-      def write(key, value); end
-    end
-
-    Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
-      raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/scarce-resource"
-      end
-    end
-
-    assert_equal "Configured store FakeStore doesn't respond to #increment method", raised_exception.message
-  end
-
-  it "works with any object that responds to #read, #write and #increment" do
+  it "works with any object that responds to #read, #write, #delete and #increment" do
     fake_store_class = Class.new do
       attr_accessor :backend
 
@@ -100,6 +47,10 @@ def increment(key, _count, _options = {})
         @backend[key] ||= 0
         @backend[key] += 1
       end
+
+      def delete(key)
+        @backend.delete(key)
+      end
     end
 
     Object.stub_const(:FakeStore, fake_store_class) do

spec/acceptance/cache_store_config_for_fail2ban_spec.rb

@@ -20,67 +20,14 @@
     end
   end
 
-  it "gives semantic error if store is missing #read method" do
-    raised_exception = nil
-
-    fake_store_class = Class.new do
-      def write(key, value); end
-
-      def increment(key, count, options = {}); end
+  it "display warning if store is missing methods" do
+    warning = "[rack-attack] Configured store Object doesn't respond to #read, #write, #delete, #increment\n"
+    assert_output("", warning) do
+      Rack::Attack.cache.store = Object.new
     end
-
-    Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
-      raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/private-place"
-      end
-    end
-
-    assert_equal "Configured store FakeStore doesn't respond to #read method", raised_exception.message
   end
 
-  it "gives semantic error if store is missing #write method" do
-    raised_exception = nil
-
-    fake_store_class = Class.new do
-      def read(key); end
-
-      def increment(key, count, options = {}); end
-    end
-
-    Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
-      raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/private-place"
-      end
-    end
-
-    assert_equal "Configured store FakeStore doesn't respond to #write method", raised_exception.message
-  end
-
-  it "gives semantic error if store is missing #increment method" do
-    raised_exception = nil
-
-    fake_store_class = Class.new do
-      def read(key); end
-
-      def write(key, value); end
-    end
-
-    Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
-      raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/private-place"
-      end
-    end
-
-    assert_equal "Configured store FakeStore doesn't respond to #increment method", raised_exception.message
-  end
-
-  it "works with any object that responds to #read, #write and #increment" do
+  it "works with any object that responds to #read, #write, #delete and #increment" do
     fake_store_class = Class.new do
       attr_accessor :backend
 
@@ -100,6 +47,10 @@ def increment(key, _count, _options = {})
         @backend[key] ||= 0
         @backend[key] += 1
       end
+
+      def delete(key)
+        @backend.delete(key)
+      end
     end
 
     Rack::Attack.cache.store = fake_store_class.new

spec/acceptance/cache_store_config_for_throttle_spec.rb

@@ -18,10 +18,9 @@
   end
 
   it "gives semantic error if incompatible store was configured" do
-    Rack::Attack.cache.store = Object.new
-
-    assert_raises(Rack::Attack::MisconfiguredStoreError) do
-      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+    warning = "[rack-attack] Configured store Object doesn't respond to #read, #write, #delete, #increment\n"
+    assert_output("", warning) do
+      Rack::Attack.cache.store = Object.new
     end
   end
 
@@ -33,10 +32,22 @@ def initialize
         @counts = {}
       end
 
+      def read(key)
+        @counts[key]
+      end
+
+      def write(key, count)
+        @counts[key] = count
+      end
+
       def increment(key, _count, _options)
         @counts[key] ||= 0
         @counts[key] += 1
       end
+
+      def delete(key)
+        @counts.delete(key)
+      end
     end
 
     Rack::Attack.cache.store = basic_store_class.new

spec/rack_attack_reset_spec.rb

@@ -4,9 +4,21 @@
 
 describe "Rack::Attack.reset!" do
   it "raises an error when is not supported by cache store" do
-    Rack::Attack.cache.store = Class.new
-    assert_raises(Rack::Attack::IncompatibleStoreError) do
-      Rack::Attack.reset!
+    fake_store_class = Class.new do
+      def read(key); end
+
+      def write(key, value); end
+
+      def increment(key, count, options = {}); end
+
+      def delete(key); end
+    end
+
+    Object.stub_const(:FakeStore, fake_store_class) do
+      Rack::Attack.cache.store = FakeStore.new
+      assert_raises(Rack::Attack::IncompatibleStoreError) do
+        Rack::Attack.reset!
+      end
     end
   end