Make Allow middleware thread-safe without duping

- Drop support for passing config options to middleware

Gemfile

@@ -5,6 +5,8 @@ gemspec
 
 gem "actionpack", ">= 7.0.0"
 
+gem "benchmark-ips", "~> 2.14"
+
 gem "minitest", "~> 5.16"
 
 gem "rack-test", "~> 2.1"

bin/benchmark

@@ -0,0 +1,31 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+require "bundler/setup"
+require "strong_routes"
+
+require "benchmark/ips"
+
+StrongRoutes.config.allowed_routes = [
+  "/",
+  "/bar/sandwich",
+  "/comments/:id",
+  "/comments/:id/edit",
+  "/posts/:id",
+  "/posts/:id/edit",
+  "/posts/:post_id/comments",
+  "/posts/:post_id/comments/new",
+  "/trading-post",
+  "/user_posts/:id",
+  "/users",
+  "/users/:user_id/posts",
+  "/users/:user_id/posts/new"
+]
+StrongRoutes.reload! if StrongRoutes.respond_to?(:reload!)
+
+app = StrongRoutes::Allow.new(lambda { |env| [200, {"content-type" => "text/plain"}, ["OK"]] })
+
+Benchmark.ips do |x|
+  x.report("Hit") { app.call(Rack::PATH_INFO => "/users/1") }
+  x.report("Miss") { app.call(Rack::PATH_INFO => "/baz") }
+end

lib/strong_routes.rb

@@ -1,15 +1,23 @@
-require "rack/request"
+require "rack/response"
 
 require "strong_routes/allow"
 require "strong_routes/config"
 require "strong_routes/route_matcher"
 require "strong_routes/version"
 
 module StrongRoutes
+  def self.allowed?(route)
+    config.route_matchers.any? { |route_matcher| route_matcher =~ route }
+  end
+
   def self.config
     @config ||= Config.new
   end
 
+  def self.enabled?
+    config.enabled?
+  end
+
   # Initialize the config
   config
 end

lib/strong_routes/allow.rb

@@ -1,54 +1,23 @@
 module StrongRoutes
   class Allow
-    def initialize(app, options = {})
+    def initialize(app)
       @app = app
-      @options = options
     end
 
-    def _call(env)
-      return @app.call(env) unless enabled?
-
-      request = ::Rack::Request.new(env)
+    def call(env)
+      return @app.call(env) unless config.enabled?
 
-      if allowed?(request)
+      if StrongRoutes.allowed?(env[Rack::PATH_INFO].to_s)
         @app.call(env)
       else
         [404, {"Content-Type" => "text/html", "Content-Length" => config.message.length.to_s}, [config.message]]
       end
     end
 
-    # HACK: This makes this middleware threadsafe, but is not a very good pattern.
-    # We should rewrite this to use a separate class with instance-level stuff.
-    def call(env)
-      dup._call(env)
-    end
-
     private
 
-    def allowed_routes
-      @allowed_routes ||= begin
-        routes = [config.allowed_routes]
-        routes.flatten!
-        routes.compact!
-        routes.uniq!
-        routes
-      end
-    end
-
-    def allowed?(request)
-      route_matchers.any? { |route_matcher| route_matcher =~ request.path_info }
-    end
-
     def config
-      @config ||= StrongRoutes.config.merge(@options)
-    end
-
-    def enabled?
-      config.enabled?
-    end
-
-    def route_matchers
-      @route_matchers ||= allowed_routes.map { |allowed_route| ::StrongRoutes::RouteMatcher.new(allowed_route) }
+      StrongRoutes.config
     end
   end
 end

lib/strong_routes/config.rb

@@ -1,51 +1,35 @@
 module StrongRoutes
-  module Accessorable
-    # Creates an accessor that simply sets and reads a key in the hash:
-    #
-    #   class Config < Hash
-    #     hash_accessor :app
-    #   end
-    #
-    #   config = Config.new
-    #   config.app = Foo
-    #   config[:app] #=> Foo
-    #
-    #   config[:app] = Bar
-    #   config.app #=> Bar
-    #
-    def hash_accessor(*names) # :nodoc:
-      names.each do |name|
-        class_eval <<-METHOD, __FILE__, __LINE__ + 1
-          def #{name}
-            self[:#{name}]
-          end
+  class Config
+    attr_accessor :enabled,
+      :insert_after,
+      :insert_before,
+      :message
 
-          def #{name}=(value)
-            self[:#{name}] = value
-          end
+    attr_reader :allowed_routes, :route_matchers
 
-          def #{name}?
-            !! self[:#{name}]
-          end
-        METHOD
-      end
+    def initialize
+      @allowed_routes = Set.new
+      @enabled = true
+      @message = "Resource Not Found"
+      @route_matchers = []
     end
-  end
 
-  class Config < Hash
-    extend Accessorable
+    def allowed_routes=(value)
+      raise TypeError, "allowed routes must be an Enumerable" unless value.is_a?(Enumerable)
+      @allowed_routes = value.to_set
+      @route_matchers = allowed_routes.map { |route| StrongRoutes::RouteMatcher.new(route) }
+    end
 
-    hash_accessor :allowed_routes,
-      :enabled,
-      :insert_after,
-      :insert_before,
-      :message
+    def enabled?
+      !!enabled
+    end
 
-    def initialize(*)
-      super
+    def insert_after?
+      !!insert_after
+    end
 
-      self[:enabled] = true if self[:enabled].nil?
-      self[:message] = "Resource Not Found" if self[:message].nil?
+    def insert_before?
+      !!insert_before
     end
   end
 end

lib/strong_routes/rails/railtie.rb

@@ -24,8 +24,7 @@ class Railtie < ::Rails::Railtie
         # in after routes are loaded
         app.reload_routes!
 
-        config.strong_routes.allowed_routes ||= []
-        config.strong_routes.allowed_routes += RouteMapper.map(app.routes)
+        config.strong_routes.allowed_routes = RouteMapper.map(app.routes)
       end
     end
   end

test/strong_routes/allow_test.rb

@@ -7,6 +7,10 @@
   let(:stack) { lambda { |env| [200, {"Content-Type" => "text/plain"}, ["Good"]] } }
 
   context "without allowed routes set" do
+    before do
+      StrongRoutes.config.allowed_routes = []
+    end
+
     it "does not allow access to /users" do
       get "/users"
       _(last_response).wont_be :ok?
@@ -34,14 +38,8 @@
   end
 
   context "with allowed routes set" do
-    let(:users_path) { [/\/users/i] }
-
     before do
-      StrongRoutes.config.allowed_routes = users_path
-    end
-
-    after do
-      StrongRoutes.config.allowed_routes = nil
+      StrongRoutes.config.allowed_routes = ["/users"]
     end
 
     it "allows access to /users" do
@@ -60,31 +58,12 @@
     end
   end
 
-  context "enabled option is false" do
-    let(:app) { StrongRoutes::Allow.new(stack, {enabled: false}) }
-
+  context "when enabled option is false" do
     it "passes request to the next app" do
-      get "/users/profile/anything?stuff=12"
-      _(last_response).must_be :ok?
-    end
-  end
-
-  context "allowed_routes passed to initializer" do
-    let(:app) { StrongRoutes::Allow.new(stack, {allowed_routes: [:users]}) }
-
-    it "allows /users with :users" do
-      get "/users"
-      _(last_response).must_be :ok?
-    end
-
-    it "does not allow /user :user" do
-      get "/user"
-      _(last_response).wont_be :ok?
-    end
-
-    it "allows access to /users/profile/anything?stuff=12 with :users" do
-      get "/users/profile/anything?stuff=12"
-      _(last_response).must_be :ok?
+      StrongRoutes.config.stub :enabled?, false do
+        get "/users/profile/anything?stuff=12"
+        _(last_response).must_be :ok?
+      end
     end
   end
 end

test/strong_routes/config_test.rb

@@ -16,14 +16,29 @@
       _(subject.respond_to?(:insert_after)).must_equal true
     end
 
+    it "supports :insert_after?" do
+      _(subject.respond_to?(:insert_after?)).must_equal true
+    end
+
     it "supports :insert_before" do
       _(subject.respond_to?(:insert_before)).must_equal true
     end
+
+    it "supports :insert_before?" do
+      _(subject.respond_to?(:insert_before?)).must_equal true
+    end
   end
 
   describe "#enabled" do
     it "is enabled by default" do
       _(subject).must_be :enabled
     end
   end
+
+  describe "#allowed_routes=" do
+    it "maps allowed routes to matchers" do
+      subject.allowed_routes = ["/users"]
+      _(subject.route_matchers).must_equal [StrongRoutes::RouteMatcher.new("/users")]
+    end
+  end
 end