Skip to content

Do not allow editing middleware stack after it has been built. #5911

Closed
wants to merge 1 commit into from

2 participants

@arturaz
arturaz commented Apr 20, 2012

This prevents stupid mistakes, like this:

  config.after_initialize do
    # Whoops, doesn't do anything and will silently fail. Not quite good.
    config.middleware.use ExceptionNotifier,
      :email_prefix => "[N44 WEB ERROR] ",
      :sender_address => %{"Rails Notifier" <do-not-reply@nebula44.com>},
      :exception_recipients => EXCEPTION_RECIPIENTS # this is defined in config/initializers/config.rb
                                                    # and cannot be accessed before initialization.
                                                    # So junior programmer moves it to #after_initialize
                                                    # and things go boom.
  end
@arturaz arturaz Do not allow editing middleware stack after it has been built.
This prevents stupid mistakes, like this:

```ruby
  config.after_initialize do
    # Whoops, doesn't do anything and will silently fail. Not quite good.
    config.middleware.use ExceptionNotifier,
      :email_prefix => "[N44 WEB ERROR] ",
      :sender_address => %{"Rails Notifier" <do-not-reply@nebula44.com>},
      :exception_recipients => EXCEPTION_RECIPIENTS # this is defined in config/initializers/config.rb
                                                    # and cannot be accessed before initialization.
                                                    # So junior programmer moves it to #after_initialize
                                                    # and things go boom.
  end
```
398d920
@jeremy jeremy commented on the diff Apr 20, 2012
actionpack/lib/action_dispatch/middleware/stack.rb
@@ -120,5 +127,13 @@ def assert_index(index, where)
raise "No such middleware to insert #{where}: #{index.inspect}" unless i
i
end
+
+ private
+
+ def ensure_not_build_yet!
+ raise "Adding middlewares to stack after the application has been " +
+ "built is not allowed!" if @built
@jeremy
Ruby on Rails member
jeremy added a note Apr 20, 2012

Using a single string is clearer than splitting it to wrap within 80 chars.

@arturaz
arturaz added a note Apr 20, 2012

Depends on your style :) Doesn't matter to me.

@jeremy
Ruby on Rails member
jeremy added a note Apr 20, 2012

Cool - Rails style is to wrap the line instead of splitting a literal string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on the diff Apr 20, 2012
actionpack/lib/action_dispatch/middleware/stack.rb
@@ -110,7 +115,9 @@ def use(*args, &block)
def build(app = nil, &block)
app ||= block
raise "MiddlewareStack#build requires an app" unless app
- middlewares.reverse.inject(app) { |a, e| e.build(a) }
+ value = middlewares.reverse.inject(app) { |a, e| e.build(a) }
+ @built = true
+ value
@jeremy
Ruby on Rails member
jeremy added a note Apr 20, 2012

Can use .tap { @built = true } here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy
Ruby on Rails member
jeremy commented Apr 20, 2012

Good idea, though perhaps it should be a separate #freeze method call rather than implicit to #build.

So Rails would build the app and mark the stack as frozen.

@arturaz
arturaz commented Apr 20, 2012

Yeah, #freeze would be nice. I didn't know where this was actually done though :)

@arturaz
arturaz commented Apr 20, 2012

Can you do these tiny changes? Its friday and 21:03 here, so I'm kind of off :)

@jeremy
Ruby on Rails member
jeremy commented Apr 20, 2012

Rails::Engine builds the middleware stack: https://github.com/rails/rails/blob/master/railties/lib/rails/engine.rb#L467-472

Since the MiddlewareStack has an internal @middlewares array, freezing that array when the stack is built will catch all these cases without needing an explicit ensure_not_build_yet check.

@jeremy
Ruby on Rails member
jeremy commented Apr 20, 2012

@arturaz LOL sure thing :beers:

@arturaz
arturaz commented Apr 20, 2012

Yeah, but the error message won't be very user-friendly I suppose. And catching for RuntimeError would possibly catch other bugs.

@jeremy jeremy added a commit that closed this pull request Apr 20, 2012
@jeremy jeremy Freeze the middleware stack after it's built
So apps that accidentally add middlewares later aren't unwittingly dumping them in a black hole.

Closes #5911
42f6e9f
@jeremy jeremy closed this in 42f6e9f Apr 20, 2012
@romanvbabenko romanvbabenko added a commit to romanvbabenko/rails that referenced this pull request May 2, 2012
@jeremy jeremy Freeze the middleware stack after it's built
So apps that accidentally add middlewares later aren't unwittingly dumping them in a black hole.

Closes #5911
101f58c
@lwe lwe referenced this pull request in rails/sass-rails Feb 26, 2013
Open

sass-rails couldn't delete `Sass::Plugin::Rack` #136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.