New filter implementation causing problems in production ?

rubyonrails-core
1

Hi all,

I'm using a Pistonized Rails export from the 1.2 branch. I just updated to r6651 and got the new implementation. Running functional and integration tests reveals nothing, as well as running under Mongrel in development mode.

After deploying to a staging server (in production mode), I get the following failure:

ActionController::ActionControllerError in PartiesController#index

filter #<ActionController::Filters::ClassMethods::BeforeFilterProxy:0xb60ee668 @filter=#<ActionController::Filters::ClassMethods::SymbolFilter:0xb60ee6cc @filter=:login_from_cookie>> was in the wrong place!

RAILS_ROOT: /usr/local/www/xlsuite.com/config/.. Application Trace | Framework Trace | Full Trace

/usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/filters.rb:729:in `call_filters' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/filters.rb:675:in `perform_action_without_benchmark' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/benchmarking.rb:66:in `perform_action_without_rescue' /usr/lib/ruby/1.8/benchmark.rb:293:in `measure' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/benchmarking.rb:66:in `perform_action_without_rescue' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/rescue.rb:83:in `perform_action' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/base.rb:430:in `send' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/base.rb:430:in `process_without_filters' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/filters.rb:680:in `process_without_session_management_support' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/session_management.rb:114:in `process' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/base.rb:330:in `process' /usr/local/www/xlsuite.com/vendor/rails/railties/lib/dispatcher.rb:41:in `dispatch' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel/rails.rb:78:in `process' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel/rails.rb:76:in `synchronize' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel/rails.rb:76:in `process' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:618:in `process_client' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:617:in `each' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:617:in `process_client' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:736:in `run' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:736:in `initialize' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:736:in `new' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:736:in `run' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:720:in `initialize' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:720:in `new' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel.rb:720:in `run' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel/configurator.rb:271:in `run' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel/configurator.rb:270:in `each' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel/configurator.rb:270:in `run' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/bin/mongrel_rails:127:in `run' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/lib/mongrel/command.rb:211:in `run' /usr/lib/ruby/gems/1.8/gems/mongrel-1.0.1/bin/mongrel_rails:243

/usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/filters.rb:729:in `call_filters' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/filters.rb:675:in `perform_action_without_benchmark' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/benchmarking.rb:66:in `perform_action_without_rescue' /usr/lib/ruby/1.8/benchmark.rb:293:in `measure' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/benchmarking.rb:66:in `perform_action_without_rescue' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/rescue.rb:83:in `perform_action' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/base.rb:430:in `send' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/base.rb:430:in `process_without_filters' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/filters.rb:680:in `process_without_session_management_support' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/session_management.rb:114:in `process' /usr/local/www/xlsuite.com/vendor/rails/actionpack/lib/action_controller/base.rb:330:in `process' /usr/local/www/xlsuite.com/vendor/rails/railties/lib/dispatcher.rb:41:in `dispatch'

I looked at the code, and it seems the filter isn't put in the right spot ? The chain of events is:

class ApplicationController   include XlSuite::AuthenticatedSystem end

class XlSuite::AuthenticatedSystem   def self.included(base)     base.before_filter :login_from_cookie   end end

I received a similar failure for another filter that is added using prepend_before_filter.

Any change I did something wrong ?

Thanks !

2

I'm using a Pistonized Rails export from the 1.2 branch. I just updated to r6651 and got the new implementation. Running functional and integration tests reveals nothing, as well as running under Mongrel in development mode.

After deploying to a staging server (in production mode), I get the following failure:

Hmm, this could be a pain to track down. Can you reproduce it with a simple sample application?

I received a similar failure for another filter that is added using prepend_before_filter.

Any change I did something wrong ?

Seems ok to me from what you've described....

3

Hi,

There is a ticket with a failing test case attached at: http://dev.rubyonrails.org/ticket/8803

Cheers,

Nik

4

(repost from the ticket comment for the benefit of the mailing list)

I hit Filter in the wrong place error for a different scenario.

My scenario is that I have a before_filter that was raising an exception, and the around_filter rescuing it with a render / redirect. I have included test cases for this scenario as well as the scenario when the action itself raising an exception.

I have included a one-liner patch that basically just returns before it gets the chance to raise the Filter in wrong place exception if performed? is true. Seems to work, and all tests pass but I don't know if this is right.

I have also included the original test case above by nik.wakelin to test that this patch works.

http://dev.rubyonrails.org/ticket/8803

Regards, kamal

5

Stefan has a patch for these problems and more. Hopefully it'll show up on trac in the next few days.

6

The patch is here: http://dev.rubyonrails.org/ticket/8891

-- stefan

7

The patch is here: http://dev.rubyonrails.org/ticket/8891

I've forgotten why the return false was required for around filters. Couldn't we do:

yielded = false filter.call(self) do   yielded = true   # all remaining before and around filters will be run in this call   index = call_filters(chain, index.next, nesting.next) end halt_filter_chain(filter) unless yielded

8

Yes we could do that. The question in this case is whether we want to distinguish between around filters returning false and filters that simply don’t yield w.r.t. after filter processing.

we could say that an around filter returning false will stop after filter processing whereas filters that simply don’t yield will not stop the after filter chain.

and while we’re at it: I suggest that it should be possible for after filters to stop after filter chain processing by returning false.

9

Yes we could do that. The question in this case is whether we want to distinguish between around filters returning false and filters that simply don't yield w.r.t. after filter processing.

we could say that an around filter returning false will stop after filter processing whereas filters that simply don't yield will not stop the after filter chain.

and while we're at it: I suggest that it should be possible for after filters to stop after filter chain processing by returning false.

So assuming I've got this right, failing to yield halts the before/around filter chain, prevents the action from rendering but still runs the after filters. Failing to yield and returning false will behave as above, but *not* run the after filters.

Does anyone have any objections to this?