Rename Builder to AuthBuilder and create Builder wrapper

Previously, OmniAuth::Builder was a subclass of Rack::Builder.
Rack::Builder#call calls #to_app, rebuilding the Rack application on
each request, and, as it says in the Rack::Builder comments, this may
have a negative impact on performance.

This commit renames the original OmniAuth::Builder to OmniAuth::AuthBuilder
and replaces it with a wrapper to be used as middleware (as is suggested
in the documentation).

Using OmniAuth::Builder will now only build the application once.

Resolves omniauth#1083.

lib/omniauth.rb

@@ -9,11 +9,12 @@ module Strategies
     autoload :Developer, 'omniauth/strategies/developer'
   end
 
-  autoload :Builder,  'omniauth/builder'
-  autoload :Strategy, 'omniauth/strategy'
-  autoload :Test,     'omniauth/test'
-  autoload :Form,     'omniauth/form'
-  autoload :AuthHash, 'omniauth/auth_hash'
+  autoload :AuthBuilder, 'omniauth/auth_builder'
+  autoload :Builder,     'omniauth/builder'
+  autoload :Strategy,    'omniauth/strategy'
+  autoload :Test,        'omniauth/test'
+  autoload :Form,        'omniauth/form'
+  autoload :AuthHash,    'omniauth/auth_hash'
   autoload :FailureEndpoint, 'omniauth/failure_endpoint'
   autoload :AuthenticityTokenProtection, 'omniauth/authenticity_token_protection'
 

lib/omniauth/auth_builder.rb

@@ -0,0 +1,43 @@
+module OmniAuth
+  class AuthBuilder < ::Rack::Builder
+    def on_failure(&block)
+      OmniAuth.config.on_failure = block
+    end
+
+    def before_options_phase(&block)
+      OmniAuth.config.before_options_phase = block
+    end
+
+    def before_request_phase(&block)
+      OmniAuth.config.before_request_phase = block
+    end
+
+    def before_callback_phase(&block)
+      OmniAuth.config.before_callback_phase = block
+    end
+
+    def configure(&block)
+      OmniAuth.configure(&block)
+    end
+
+    def options(options = false)
+      return @options ||= {} if options == false
+
+      @options = options
+    end
+
+    def provider(klass, *args, **opts, &block)
+      if klass.is_a?(Class)
+        middleware = klass
+      else
+        begin
+          middleware = OmniAuth::Strategies.const_get(OmniAuth::Utils.camelize(klass.to_s).to_s, false)
+        rescue NameError
+          raise(LoadError.new("Could not find matching strategy for #{klass.inspect}. You may need to install an additional gem (such as omniauth-#{klass})."))
+        end
+      end
+
+      use middleware, *args, **options.merge(opts), &block
+    end
+  end
+end

lib/omniauth/builder.rb

@@ -1,47 +1,11 @@
 module OmniAuth
-  class Builder < ::Rack::Builder
-    def on_failure(&block)
-      OmniAuth.config.on_failure = block
+  class Builder
+    def initialize app, &block
+      @app = AuthBuilder.app(app, &block)
     end
 
-    def before_options_phase(&block)
-      OmniAuth.config.before_options_phase = block
-    end
-
-    def before_request_phase(&block)
-      OmniAuth.config.before_request_phase = block
-    end
-
-    def before_callback_phase(&block)
-      OmniAuth.config.before_callback_phase = block
-    end
-
-    def configure(&block)
-      OmniAuth.configure(&block)
-    end
-
-    def options(options = false)
-      return @options ||= {} if options == false
-
-      @options = options
-    end
-
-    def provider(klass, *args, **opts, &block)
-      if klass.is_a?(Class)
-        middleware = klass
-      else
-        begin
-          middleware = OmniAuth::Strategies.const_get(OmniAuth::Utils.camelize(klass.to_s).to_s, false)
-        rescue NameError
-          raise(LoadError.new("Could not find matching strategy for #{klass.inspect}. You may need to install an additional gem (such as omniauth-#{klass})."))
-        end
-      end
-
-      use middleware, *args, **options.merge(opts), &block
-    end
-
-    def call(env)
-      to_app.call(env)
+    def call env
+      @app.call(env)
     end
   end
 end

spec/omniauth/auth_builder_spec.rb

@@ -0,0 +1,140 @@
+require 'helper'
+
+describe OmniAuth::AuthBuilder do
+
+  let(:app) { proc { [204, {}, ['']] } }
+
+  describe '#provider' do
+    it 'translates a symbol to a constant' do
+      expect(OmniAuth::Strategies).to receive(:const_get).with('MyStrategy', false).and_return(Class.new)
+      OmniAuth::AuthBuilder.new(app) do
+        provider :my_strategy
+      end
+    end
+
+    it 'accepts a class' do
+      class ExampleClass; end
+
+      expect do
+        OmniAuth::AuthBuilder.new(app) do
+          provider ::ExampleClass
+        end
+      end.not_to raise_error
+    end
+
+    it "raises a helpful LoadError message if it can't find the class" do
+      expect do
+        OmniAuth::AuthBuilder.new(app) do
+          provider :lorax
+        end
+      end.to raise_error(LoadError, 'Could not find matching strategy for :lorax. You may need to install an additional gem (such as omniauth-lorax).')
+    end
+
+    it "doesn't translate a symbol to a top-level constant" do
+      class MyStrategy; end
+
+      expect do
+        OmniAuth::AuthBuilder.new(app) do
+          provider :my_strategy
+        end
+      end.to raise_error(LoadError, 'Could not find matching strategy for :my_strategy. You may need to install an additional gem (such as omniauth-my_strategy).')
+    end
+  end
+
+  describe '#options' do
+    it 'merges provided options in' do
+      k = Class.new
+      b = OmniAuth::AuthBuilder.new(app)
+      expect(b).to receive(:use).with(k, :foo => 'bar', :baz => 'tik')
+
+      b.options :foo => 'bar'
+      b.provider k, :baz => 'tik'
+    end
+
+    it 'adds an argument if no options are provided' do
+      k = Class.new
+      b = OmniAuth::AuthBuilder.new(app)
+      expect(b).to receive(:use).with(k, :foo => 'bar')
+
+      b.options :foo => 'bar'
+      b.provider k
+    end
+  end
+
+  describe '#on_failure' do
+    it 'passes the block to the config' do
+      prok = proc {}
+
+      with_config_reset(:on_failure) do
+        OmniAuth::AuthBuilder.new(app).on_failure(&prok)
+
+        expect(OmniAuth.config.on_failure).to eq(prok)
+      end
+    end
+  end
+
+  describe '#before_options_phase' do
+    it 'passes the block to the config' do
+      prok = proc {}
+
+      with_config_reset(:before_options_phase) do
+        OmniAuth::AuthBuilder.new(app).before_options_phase(&prok)
+
+        expect(OmniAuth.config.before_options_phase).to eq(prok)
+      end
+    end
+  end
+
+  describe '#before_request_phase' do
+    it 'passes the block to the config' do
+      prok = proc {}
+
+      with_config_reset(:before_request_phase) do
+        OmniAuth::AuthBuilder.new(app).before_request_phase(&prok)
+
+        expect(OmniAuth.config.before_request_phase).to eq(prok)
+      end
+    end
+  end
+
+  describe '#before_callback_phase' do
+    it 'passes the block to the config' do
+      prok = proc {}
+
+      with_config_reset(:before_callback_phase) do
+        OmniAuth::AuthBuilder.new(app).before_callback_phase(&prok)
+
+        expect(OmniAuth.config.before_callback_phase).to eq(prok)
+      end
+    end
+  end
+
+  describe '#configure' do
+    it 'passes the block to the config' do
+      prok = proc {}
+      allow(OmniAuth).to receive(:configure).and_call_original
+
+      OmniAuth::AuthBuilder.new(app).configure(&prok)
+
+      expect(OmniAuth).to have_received(:configure) do |&block|
+        expect(block).to eq(prok)
+      end
+    end
+  end
+
+  describe '#call' do
+    it 'passes env to to_app.call' do
+      app = lambda { |env| [200, {}, [env['CUSTOM_ENV_VALUE']]] }
+      builder = OmniAuth::AuthBuilder.new(app)
+      env = {'REQUEST_METHOD' => 'GET', 'PATH_INFO' => '/some/path', 'CUSTOM_ENV_VALUE' => 'VALUE'}
+
+      expect(builder.call(env)).to eq([200, {}, ['VALUE']])
+    end
+  end
+
+  def with_config_reset(option)
+    old_config = OmniAuth.config.send(option)
+    yield
+    OmniAuth.config.send("#{option}=", old_config)
+  end
+end

spec/omniauth/builder_spec.rb

@@ -1,124 +1,6 @@
 require 'helper'
 
 describe OmniAuth::Builder do
-  describe '#provider' do
-    it 'translates a symbol to a constant' do
-      expect(OmniAuth::Strategies).to receive(:const_get).with('MyStrategy', false).and_return(Class.new)
-      OmniAuth::Builder.new(nil) do
-        provider :my_strategy
-      end
-    end
-
-    it 'accepts a class' do
-      class ExampleClass; end
-
-      expect do
-        OmniAuth::Builder.new(nil) do
-          provider ::ExampleClass
-        end
-      end.not_to raise_error
-    end
-
-    it "raises a helpful LoadError message if it can't find the class" do
-      expect do
-        OmniAuth::Builder.new(nil) do
-          provider :lorax
-        end
-      end.to raise_error(LoadError, 'Could not find matching strategy for :lorax. You may need to install an additional gem (such as omniauth-lorax).')
-    end
-
-    it "doesn't translate a symbol to a top-level constant" do
-      class MyStrategy; end
-
-      expect do
-        OmniAuth::Builder.new(nil) do
-          provider :my_strategy
-        end
-      end.to raise_error(LoadError, 'Could not find matching strategy for :my_strategy. You may need to install an additional gem (such as omniauth-my_strategy).')
-    end
-  end
-
-  describe '#options' do
-    it 'merges provided options in' do
-      k = Class.new
-      b = OmniAuth::Builder.new(nil)
-      expect(b).to receive(:use).with(k, :foo => 'bar', :baz => 'tik')
-
-      b.options :foo => 'bar'
-      b.provider k, :baz => 'tik'
-    end
-
-    it 'adds an argument if no options are provided' do
-      k = Class.new
-      b = OmniAuth::Builder.new(nil)
-      expect(b).to receive(:use).with(k, :foo => 'bar')
-
-      b.options :foo => 'bar'
-      b.provider k
-    end
-  end
-
-  describe '#on_failure' do
-    it 'passes the block to the config' do
-      prok = proc {}
-
-      with_config_reset(:on_failure) do
-        OmniAuth::Builder.new(nil).on_failure(&prok)
-
-        expect(OmniAuth.config.on_failure).to eq(prok)
-      end
-    end
-  end
-
-  describe '#before_options_phase' do
-    it 'passes the block to the config' do
-      prok = proc {}
-
-      with_config_reset(:before_options_phase) do
-        OmniAuth::Builder.new(nil).before_options_phase(&prok)
-
-        expect(OmniAuth.config.before_options_phase).to eq(prok)
-      end
-    end
-  end
-
-  describe '#before_request_phase' do
-    it 'passes the block to the config' do
-      prok = proc {}
-
-      with_config_reset(:before_request_phase) do
-        OmniAuth::Builder.new(nil).before_request_phase(&prok)
-
-        expect(OmniAuth.config.before_request_phase).to eq(prok)
-      end
-    end
-  end
-
-  describe '#before_callback_phase' do
-    it 'passes the block to the config' do
-      prok = proc {}
-
-      with_config_reset(:before_callback_phase) do
-        OmniAuth::Builder.new(nil).before_callback_phase(&prok)
-
-        expect(OmniAuth.config.before_callback_phase).to eq(prok)
-      end
-    end
-  end
-
-  describe '#configure' do
-    it 'passes the block to the config' do
-      prok = proc {}
-      allow(OmniAuth).to receive(:configure).and_call_original
-
-      OmniAuth::Builder.new(nil).configure(&prok)
-
-      expect(OmniAuth).to have_received(:configure) do |&block|
-        expect(block).to eq(prok)
-      end
-    end
-  end
-
   describe '#call' do
     it 'passes env to to_app.call' do
       app = lambda { |env| [200, {}, env['CUSTOM_ENV_VALUE']] }
@@ -128,10 +10,4 @@ class MyStrategy; end
       expect(builder.call(env)).to eq([200, {}, 'VALUE'])
     end
   end
-
-  def with_config_reset(option)
-    old_config = OmniAuth.config.send(option)
-    yield
-    OmniAuth.config.send("#{option}=", old_config)
-  end
 end