Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Builder from a class to a module and only build app once #1094

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

speckins
Copy link

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 changes OmniAuth::Builder from a class to a module and defines a ::new method that extends a Rack::Builder instance with that module, instance_evals any block, and returns the value of #to_app, the built application.

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

For users who have been subclassing OmniAuth::Builder, a fix would be subclassing Rack::Builder directly and including OmniAuth::Builder:

class MyBuilder < Rack::Builder
  include OmniAuth::Builder
  # ...
end

Resolves #1083.

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 changes OmniAuth::Builder from a class to a module and
defines a ::new method that extends a Rack::Builder instance with that
module, instance_evals any block, and returns the value of #to_app, the
built application.

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

For users who have been subclassing OmniAuth::Builder, a fix would be
subclassing Rack::Builder directly and including OmniAuth::Builder:

    class MyBuilder < Rack::Builder
      include OmniAuth::Builder
      # ...
    end

Resolves omniauth#1083.
Comment on lines +5 to +9
def self.new app, &block
builder = Rack::Builder.new(app).extend(self)
builder.instance_eval(&block) if block_given?
builder.to_app
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to pass an app in here? Can we default it to nil like Rack::Builder does in internal code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, no, but if no @run (in Rack::Builder internals) is defined by the time #to_app is called, an error will be raised, and the only way to add one would be to call #run in the block.

OmniAuth::Builder.new(nil) do
  provider :developer
  run proc {}
end

If you needed that, maybe we could extract the first two lines to a separate module method (e.g., ::build) that didn't require an argument.

def self.build(app = nil, &block)
  builder = Rack::Builder.new(app).extend(self)
  builder.instance_eval(&block) if block_given?
  builder
end

def self.new(app, &block)
  build(app, &block).to_app
end

module OmniAuth
class Builder < ::Rack::Builder
module Builder
def self.new app, &block
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def self.new app, &block
def self.new(app, &block)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rack::Builder#to_app is not designed to be called for every request.
2 participants