Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

after_filter not halted

#5648

Expected behavior:

When a before filter render or redirects the action itself and after filters should not be executed.

Wrong behavior:

After filter gets executed although the before filter rendered or redirected.

Example:

In following application_controller.rb ...

class ApplicationController < ActionController::Base

  before_filter :do_before
  after_filter  :do_after

  def should_halt
    Rails.logger.debug "Executed: should_halt"
    render :text => 'not halted'
  end

  def do_before
    Rails.logger.debug "Executed: do_before"
    render :text => 'before'
  end

  def do_after
    Rails.logger.debug "Executed: do_after"
  end

end

expected debug output should be:

Executed: do_before

but was

Executed: do_before
Executed: do_after

Note:

Rails 2.0 documentation indicated that after filters should not get called when before filters returned false, rendered or redirected. The current rails documentation (3.0) does not document the behavior of the controller callbacks. Either indicate that the behavior has changed or that this bug needs to be fixed.

Reported by Buts Johan · September 17th, 2010 @ 01:39 PM

State: resolved
Milestone: 3.0.2
Assigned to: José Valim José Valim
Importance: Low

Activity

  1. Bertg
    Bertg

    I'm seeing and expecting the same behaviour. I'm noticing that the compiled callback does not put any conditions on an after filter.

    _conditional_callback_around_21(halted) do
  unless halted
    result = do_before
    halted = (response_body)
  end

  value = yield if block_given? && !halted
  do_after
end
halted ? false : (block_given? ? value : true)

    I'm expecting that the last compiled line should be used as a condition on the "do_after" call.

    When adding a :if statement to the after_filter the output slightly changes on the last lines:

      if _callback_after_21(self)
    do_after
  end
end
halted ? false : (block_given? ? value : true)

    It seems to me that the last line should be included in the condition.

      if (halted ? false : (block_given? ? value : true)) && _callback_after_21(self)
    do_after
  end
end

      if halted ? false : (block_given? ? value : true)
    do_after
  end
end

    September 17th, 2010 @ 01:57 PM

  2. Bertg
    Bertg

    Surprised to see no response. Thinking this would be quite a serious issue...

    September 30th, 2010 @ 05:29 PM

  3. Marcelo Giorgi
    Marcelo Giorgi
    • Assigned user set to Santiago Pastorino
    • Tag changed from rails 3.0.0, actionpack, after_filter, before_filter, callbacks, chain to rails 3.0.0, actionpack, after_filter, before_filter, callbacks, chain, patch

    Hi Bertg,

    I agree with you, I would expect Rails 3 to inherit the behavior of Rails 2.3.x filters.

    For that, I've wrote tests and patch to make it work as you suggested.

    BTW: This patch applies cleanly on master now.

    October 3rd, 2010 @ 04:51 AM

  4. Neeraj Singh
    Neeraj Singh
    • Importance changed from to Low

    Interestingly I am not able to reproduce this error. I tested with rails edge. In both render :text and redirect_to cases the after_filter is not invoked.

    Here is my test code

    class UsersController < ApplicationController

  before_filter :do_before
  after_filter  :do_after

  def do_before
    puts 'before >>>' * 10
    #render :text => 'filter should be halted'
    redirect_to surveys_path
  end

  def do_after
    puts 'after <<<' * 10
  end

  def index
    @users = User.all
  end

end

    I'm sure I'm missing something. Not sure what? :-)

    October 3rd, 2010 @ 10:40 AM

  5. José Valim
    José Valim
    • State changed from new to open
    • Milestone cleared.
    • Assigned user changed from Santiago Pastorino to José Valim

    Hey guys,

    If this bug still exists, I would recommend to use a solution similar to the one in ActiveModel::Callbacks:

    http://github.com/rails/rails/blob/master/activemodel/lib/active_mo...

    In other words, simple adding "!halted" as condition to :if in after_filter will fix the issue. I prefer this solution because AS::Callbacks is already too complex and adding another option will just make it worse.

    October 3rd, 2010 @ 01:00 PM

  6. Marcelo Giorgi
    Marcelo Giorgi

    Hi Neeraj and Jose,

    Neeraj, I run your code with rails edge and got the following output for this url http://localhost:3000/users:

    before >>>before >>>before >>>before >>>before >>>before >>>before >>>before >>>before >>>before >>>
    after <<

    So, for me it is reaching the after filter either when I redirect or render from the before_filter called do_before.

    Jose, Yeah I agree, AS::Callbacks is complex enough. So, I wrote a new patch here.

    Let me know what you think.

    October 3rd, 2010 @ 03:34 PM

  7. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2

    October 15th, 2010 @ 11:02 PM

  8. Ryan Bigg
    Ryan Bigg
    • Tag changed from rails 3.0.0, sheepskin boots, actionpack, after_filter, before_filter, callbacks, chain, patch to actionpack after_filter before_filter callbacks chain patch rails 3.0.0

    Automatic cleanup of spam.

    October 16th, 2010 @ 02:28 AM

  9. Ryan Bigg
    Ryan Bigg
    • Tag cleared.

    Automatic cleanup of spam.

    October 19th, 2010 @ 08:35 AM

  10. Evgeniy Dolzhenko
    Evgeniy Dolzhenko

    +1 having this issue with 3.0.1 application

    October 22nd, 2010 @ 11:58 AM

  11. José Valim
    José Valim

    Great, I will apply this patch in the following days.

    October 22nd, 2010 @ 12:22 PM

  12. Neeraj Singh
    Neeraj Singh

    I still can't reproduce this problem with rails edge.

    I don't see boom in the following case.

    class UsersController < ApplicationController
  before_filter :f1
  before_filter :f2

  def f2
    raise 'boom'
  end

  def f1
    render :text => 'finished'
  end

  def index
    @users = User.all
  end
end

    November 11th, 2010 @ 03:48 PM

  13. Evgeniy Dolzhenko
    Evgeniy Dolzhenko

    It should be after_filter :f2 not before_filter :f2

    November 11th, 2010 @ 03:55 PM

  14. Repository
    Repository
    • State changed from open to resolved

    (from [2bb1c202b48c4f1c8d81927fb3e5fc00806231f9]) Make after_filter halt when before_filter renders or redirects [#5648 after_filter not halted state:resolved]

    Signed-off-by: José Valim jose.valim@gmail.com
    https://github.com/rails/rails/commit/2bb1c202b48c4f1c8d81927fb3e5f...

    November 11th, 2010 @ 04:07 PM

  15. Repository
    Repository

    (from [0de95dfb2c99f188c5e4c98aa59f6e8f2736f70a]) Make after_filter halt when before_filter renders or redirects [#5648 after_filter not halted state:resolved]

    Signed-off-by: José Valim jose.valim@gmail.com
    https://github.com/rails/rails/commit/0de95dfb2c99f188c5e4c98aa59f6...

    November 11th, 2010 @ 04:07 PM

  16. Neeraj Singh
    Neeraj Singh

    My bad. Anyway it is committed now.

    November 11th, 2010 @ 04:23 PM