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

[ioquatix]

omniauth/lib/omniauth/builder.rb

Line 44 in 9cd7aad

to_app.call(env)

It can have less than ideal performance implications.

[BobbyMcWho]

Thanks for the report, this is an area of the library I haven't looked much into. Would memoizing the to_app call be sufficient?

[ioquatix]

I think you'd want to do it at build time because memoization is not thread safe.

[speckins]

It's also not necessary to redefine #call since it's the same as the superclass.

https://github.com/rack/rack/blob/ab9cd7416c1b5253cd280d75704bafd87510c120/lib/rack/builder.rb#L261-L263

If OmniAuth::Builder is intended to be used as shown in the README, maybe overridding ::new would be a low-impact way of resolving this?

module OmniAuth
  class Builder < ::Rack::Builder
    def self.new(default_app = nil, &block)
      builder = super
      block ? builder.to_app : builder
    end
  end
end

[ioquatix]

module OmniAuth
  module Builder
    def self.new(default_app = nil, &block)
      ::Rack::Builder.new(default_app, &block).to_app
    end
  end
end

maybe consider this???

[speckins]

Except that OmniAuth::Builder adds a few methods of its own, of which, the #provider method is probably the main reason for using it.

omniauth/lib/omniauth/builder.rb

Line 29 in 7d90ba2

def provider(klass, *args, **opts, &block)

What about this:

module OmniAuth
  class Builder < Rack::Builder
    def initialize default_app = nil, &block
      super
      @app = to_app if default_app
    end

    def call env
      (@app || to_app).call(env)
    end

    # The rest same as current OmniAuth::Builder class,
    # not duplicated here....
  end
end

It ought to cover people who are currently using it in both ways, as middleware as suggested in the docs or as Rack::Builder is expected to be used.

[ioquatix]

I don't think side effects in #initialize are a good idea.

I like your original proposal: #1083 (comment)

I'd even suggest just doing it like this:

module OmniAuth
  module Builder
    def self.build(default_app = nil, &block)
      new(default_app, &block).to_app
    end

    # All the existing code.
  end
end

Then use OmniAuth::Builder.build(...).

[BobbyMcWho]

I'm not opposed to a major bump if need be, so the change can be as breaking as needed

[speckins]

The problem is that omniauth has been telling people to use OmniAuth::Builder as middleware. E.g.,

use OmniAuth::Builder do
  provider :developer
end

Any Rack-like use(middleware, app, *args, &block) implementation is going to push those arguments onto a stack, and when it finally assembles the Rack app, it's going to call OmniAuth::Builder.new(app, *args, &block). So that means that OmniAuth has no control over which class method is called, and the only two places to fix this in existing applications written this way are ::new or #initialize. (And Rack::Builder already has a class method, ::app, that calls #to_app.)

What about...

module OmniAuth
  module Builder
    def self.new(app, *args, &block)
      builder = Rack::Builder.new(app).extend(self)
      builder.instance_eval(&block) if block_given?
      builder.to_app
    end

    def provider(klass, *args, **opts, &block)
      # ...
    end

    def options(options = false)
      # ...
    end

    # ...etc
  end
end

This would work for applications using OmniAuth::Builder as middleware with no changes needed. It would break for people (like me) who have been subclassing Builder, but at least it wouldn't be subtle (TypeError (superclass must be a Class (Module given))) and could be resolved by subclassing Rack::Builder and including the OmniAuth::Builder module.

It would break for people who have been instantiating Builder and using the instance directly, though.

[speckins]

One more option -- rename the current Builder class to something else and define a new Builder to use it internally.

module OmniAuth
  class AuthBuilder < Rack::Builder
    # ...same as current OmniAuth::Builder, except no #call

    undef_method :call
  end

  class Builder
    def initialize app, &block
      @app = AuthBuilder.app(app, &block)
    end

    def call env
      @app.call(env)
    end
  end
end

[speckins]

If you had a preference on how you wanted this resolved (or the priorities to take into consideration, e.g., what level of backward-compatibility, etc), I'd be willing to take a crack at it and submit a PR.

[BobbyMcWho]

I think the biggest downstream consumer is devise, so we'd wanna look if/how devise uses the builder and try and at least keep backwards compatibility for that

[ioquatix]

If you want an easy way to test external dependencies, you might like https://github.com/ioquatix/bake-test-external

[speckins]

I searched the devise code. They don't use Builder. The only reference to "builder" is a form builder. I also ran their test suite against the code in the PR just in case, and everything passes.

[speckins]

I opened another PR, with the Builder-as-a-module idea. I don't really have a preference between them, but the module version means there won't be an extra do-nothing piece of middleware in the stack since it just returns the built app.

[BobbyMcWho]

Thanks @speckins I will take a look as soon as I have some time, I've been very busy

[jcmfernandes]

I tripped on this earlier today. How can I help get #1094 merged?

[BobbyMcWho]

I've just not had the time to manage a large refactor at the moment, I'd love to be able to take care of a few different long standing issues w/ omniauth in the next major release, I just haven't had the time to work on OmniAuth outside of critical maintenance. Namely I'd like to resolve this issue, completely remove the hashie dependency, not dup the entire strategy w/ every request, and shortcircuit out of omniauth early when the request is not on a path that omniauth cares about.

[jcmfernandes]

I've just not had the time to manage a large refactor at the moment, I'd love to be able to take care of a few different long standing issues w/ omniauth in the next major release, I just haven't had the time to work on OmniAuth outside of critical maintenance. Namely I'd like to resolve this issue, completely remove the hashie dependency, not dup the entire strategy w/ every request, and shortcircuit out of omniauth early when the request is not on a path that omniauth cares about.

Makes sense! What do you think of doing those changes incrementally? I can help.