Attempt to improve code legibility/clarity/semantics (#357)

* attempt to improve semantics for legibility

* Attempt to improve legibility by simplifying

* Make it more clear that we're calling procs/blocks here

* Enable rubocop Style/BlockDelimiters cop

* Prefer 'request' over 'req' abbreviation for legibility/clarity

* Instances of Track named 'track' not 'tracker'

.rubocop.yml

@@ -40,6 +40,9 @@ Security:
 Lint:
   Enabled: true
 
+Style/BlockDelimiters:
+  Enabled: true
+
 Style/BracesAroundHashParameters:
   Enabled: true
 

lib/rack/attack.rb

@@ -81,40 +81,40 @@ class Rack::Attack
       blocklists
     end
 
-    def safelisted?(req)
+    def safelisted?(request)
-      ip_safelists.any? { |safelist| safelist.match?(req) } ||
+      ip_safelists.any? { |safelist| safelist.matched_by?(request) } ||
-        safelists.any? { |_name, safelist| safelist.match?(req) }
+        safelists.any? { |_name, safelist| safelist.matched_by?(request) }
     end
 
-    def whitelisted?(req)
+    def whitelisted?(request)
       warn "[DEPRECATION] 'Rack::Attack.whitelisted?' is deprecated.  Please use 'safelisted?' instead."
-      safelisted?(req)
+      safelisted?(request)
     end
 
-    def blocklisted?(req)
+    def blocklisted?(request)
-      ip_blocklists.any? { |blocklist| blocklist.match?(req) } ||
+      ip_blocklists.any? { |blocklist| blocklist.matched_by?(request) } ||
-        blocklists.any? { |_name, blocklist| blocklist.match?(req) }
+        blocklists.any? { |_name, blocklist| blocklist.matched_by?(request) }
     end
 
-    def blacklisted?(req)
+    def blacklisted?(request)
       warn "[DEPRECATION] 'Rack::Attack.blacklisted?' is deprecated.  Please use 'blocklisted?' instead."
-      blocklisted?(req)
+      blocklisted?(request)
     end
 
-    def throttled?(req)
+    def throttled?(request)
       throttles.any? do |_name, throttle|
-        throttle[req]
+        throttle.matched_by?(request)
       end
     end
 
-    def tracked?(req)
+    def tracked?(request)
-      tracks.each_value do |tracker|
+      tracks.each_value do |track|
-        tracker[req]
+        track.matched_by?(request)
       end
     end
 
-    def instrument(req)
+    def instrument(request)
-      notifier.instrument('rack.attack', req) if notifier
+      notifier.instrument('rack.attack', request) if notifier
     end
 
     def cache
@@ -167,16 +167,16 @@ class Rack::Attack
 
   def call(env)
     env['PATH_INFO'] = PathNormalizer.normalize_path(env['PATH_INFO'])
-    req = Rack::Attack::Request.new(env)
+    request = Rack::Attack::Request.new(env)
 
-    if safelisted?(req)
+    if safelisted?(request)
       @app.call(env)
-    elsif blocklisted?(req)
+    elsif blocklisted?(request)
       self.class.blocklisted_response.call(env)
-    elsif throttled?(req)
+    elsif throttled?(request)
       self.class.throttled_response.call(env)
     else
-      tracked?(req)
+      tracked?(request)
       @app.call(env)
     end
   end

lib/rack/attack/check.rb

@@ -7,17 +7,15 @@ module Rack
         @type = options.fetch(:type, nil)
       end
 
-      def [](req)
+      def matched_by?(request)
-        block[req].tap { |match|
+        block.call(request).tap do |match|
           if match
-            req.env["rack.attack.matched"] = name
+            request.env["rack.attack.matched"] = name
-            req.env["rack.attack.match_type"] = type
+            request.env["rack.attack.match_type"] = type
-            Rack::Attack.instrument(req)
+            Rack::Attack.instrument(request)
+          end
         end
-        }
       end
-
-      alias_method :match?, :[]
     end
   end
 end

lib/rack/attack/throttle.rb

@@ -18,12 +18,12 @@ module Rack
         Rack::Attack.cache
       end
 
-      def [](req)
+      def matched_by?(request)
-        discriminator = block[req]
+        discriminator = block.call(request)
         return false unless discriminator
 
-        current_period = period.respond_to?(:call) ? period.call(req) : period
+        current_period = period.respond_to?(:call) ? period.call(request) : period
-        current_limit  = limit.respond_to?(:call) ? limit.call(req) : limit
+        current_limit  = limit.respond_to?(:call) ? limit.call(request) : limit
         key            = "#{name}:#{discriminator}"
         count          = cache.count(key, current_period)
 
@@ -32,15 +32,15 @@ module Rack
           :period => current_period,
           :limit => current_limit
         }
-        (req.env['rack.attack.throttle_data'] ||= {})[name] = data
+        (request.env['rack.attack.throttle_data'] ||= {})[name] = data
 
         (count > current_limit).tap do |throttled|
           if throttled
-            req.env['rack.attack.matched']             = name
+            request.env['rack.attack.matched']             = name
-            req.env['rack.attack.match_discriminator'] = discriminator
+            request.env['rack.attack.match_discriminator'] = discriminator
-            req.env['rack.attack.match_type']          = type
+            request.env['rack.attack.match_type']          = type
-            req.env['rack.attack.match_data']          = data
+            request.env['rack.attack.match_data']          = data
-            Rack::Attack.instrument(req)
+            Rack::Attack.instrument(request)
           end
         end
       end

lib/rack/attack/track.rb

@@ -1,8 +1,6 @@
 module Rack
   class Attack
     class Track
-      extend Forwardable
-
       attr_reader :filter
 
       def initialize(name, options = {}, block)
@@ -15,7 +13,9 @@ module Rack
         end
       end
 
-      def_delegator :@filter, :[]
+      def matched_by?(request)
+        filter.matched_by?(request)
+      end
     end
   end
 end

spec/integration/offline_spec.rb

@@ -20,24 +20,24 @@ end
 describe 'when Redis is offline' do
   include OfflineExamples
 
-  before {
+  before do
     @cache = Rack::Attack::Cache.new
     # Use presumably unused port for Redis client
     @cache.store = ActiveSupport::Cache::RedisStore.new(:host => '127.0.0.1', :port => 3333)
-  }
+  end
 end
 
 describe 'when Memcached is offline' do
   include OfflineExamples
 
-  before {
+  before do
     Dalli.logger.level = Logger::FATAL
 
     @cache = Rack::Attack::Cache.new
     @cache.store = Dalli::Client.new('127.0.0.1:22122')
-  }
+  end
 
-  after {
+  after do
     Dalli.logger.level = Logger::INFO
-  }
+  end
 end

spec/integration/rack_attack_cache_spec.rb

@@ -37,13 +37,13 @@ describe Rack::Attack::Cache do
     store = Rack::Attack::StoreProxy.build(store)
 
     describe "with #{store.class}" do
-      before {
+      before do
         @cache = Rack::Attack::Cache.new
         @key = "rack::attack:cache-test-key"
         @expires_in = 1
         @cache.store = store
         delete(@key)
-      }
+      end
 
       after { delete(@key) }
 

spec/rack_attack_track_spec.rb

@@ -47,15 +47,15 @@ describe 'Rack::Attack.track' do
 
   describe "without limit and period options" do
     it "should assign the track filter to a Check instance" do
-      tracker = Rack::Attack.track("homepage") { |req| req.path == "/" }
+      track = Rack::Attack.track("homepage") { |req| req.path == "/" }
-      tracker.filter.class.must_equal Rack::Attack::Check
+      track.filter.class.must_equal Rack::Attack::Check
     end
   end
 
   describe "with limit and period options" do
     it "should assign the track filter to a Throttle instance" do
-      tracker = Rack::Attack.track("homepage", :limit => 10, :period => 10) { |req| req.path == "/" }
+      track = Rack::Attack.track("homepage", :limit => 10, :period => 10) { |req| req.path == "/" }
-      tracker.filter.class.must_equal Rack::Attack::Throttle
+      track.filter.class.must_equal Rack::Attack::Throttle
     end
   end
 end

spec/spec_helper.rb

@@ -36,14 +36,14 @@ class MiniTest::Spec
   end
 
   def app
-    Rack::Builder.new {
+    Rack::Builder.new do
       # Use Rack::Lint to test that rack-attack is complying with the rack spec
       use Rack::Lint
       use Rack::Attack
       use Rack::Lint
 
       run lambda { |_env| [200, {}, ['Hello World']] }
-    }.to_app
+    end.to_app
   end
 
   def self.it_allows_ok_requests