Hi all, Recently I ran into an issue where a flash was being kept during a redirect chain, and it ended up that one redirect happened inside a before_filter that returned false. I chased down the issue, and indeed the flash is not swept if the filter chain is halted. Going down the svn logs I found a reference to a ticket where Kaes, among other things, revised my flash implementation. In this ticket (3527) he suggests that flash.sweep should always run, but the ticket is closed and there''s no further comment on the issue. Because I ran into trouble with the ways things are currently implemented, I''m wondering if maybe we should take Kaes suggestion and sweep unconditionally. The only con I can see is that, as things are now, with the sweep not running after an aborted filter chain, it''s possible to handle the situation manually, that is, just run flash.sweep inside your aborting filter. So, as things are now you have more control, but you have to remember to call it. I believe the expected behavior is for the flash to always be swept. So I''d give in some control for less surprise. What do you think?
On Jul 11, 2006, at 8:56 AM, Caio Chassot wrote:> I believe the expected behavior is for the flash to always be > swept. So I''d give in some control for less surprise.That''s certainly the behavior I would expect. -- Benjamin Curtis http://www.bencurtis.com/ http://www.tesly.com/ -- Collaborative test case management http://www.agilewebdevelopment.com/ -- Resources for the Rails community _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
I would expect the opposite of this. A lot of programs may halt a filter chain because a user is not logged in, set the flash to let them know "please login first" and then redirect to the login action which would then display the flash message. If the flash was swept that wouldn''t work any longer. -Marty On 7/11/06, Benjamin Curtis <rails@bencurtis.com> wrote:> > > > On Jul 11, 2006, at 8:56 AM, Caio Chassot wrote: > > I believe the expected behavior is for the flash to always be swept. So > I''d give in some control for less surprise. > > > That''s certainly the behavior I would expect. > > -- > Benjamin Curtis > http://www.bencurtis.com/ > http://www.tesly.com/ -- Collaborative test case management > http://www.agilewebdevelopment.com/ -- Resources for the Rails community > > > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > >_______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On 7/13/06, Martin Emde <martin.emde@gmail.com> wrote:> I would expect the opposite of this. A lot of programs may halt a filter > chain because a user is not logged in, set the flash to let them know > "please login first" and then redirect to the login action which would then > display the flash message. If the flash was swept that wouldn''t work any > longer. > > -MartyYou misinterpret what is meant by sweeping. The flash stays for two requests; current and next. However, if the current request is aborted by a filter, the flash now stays for both the page you redirect to and the page after that one. Obviously a bug. Isak
On 2006-07-13, at 15:14 , Martin Emde wrote:> I would expect the opposite of this. A lot of programs may halt a > filter chain because a user is not logged in, set the flash to let > them know "please login first" and then redirect to the login > action which would then display the flash message. If the flash > was swept that wouldn''t work any longer. >What Isak said. Here''s a quick example. Gerenate an app and create a test_controller with the following contents, then visit http:// localhost:3000/test/one and follow the links. class TestController < ApplicationController FLASH = "<pre><% {}.update(flash).to_yaml %><% {}.update(flash.instance_variable_get(''@used'')).to_yaml %></pre>" before_filter :redir, :only => ''skip'' def one render :inline => "#{FLASH}1. <%= link_to ''next'', :action => ''skip'' %>" end def skip render :inline => "#{FLASH}skip" end def two render :inline => "#{FLASH}2. <%= link_to ''next'', :action => ''three'' %>" end def three render :inline => "#{FLASH}3. see..." end private def redir flash[:foo] = ''bar'' redirect_to :action => ''two'' false end end The first yaml output is the flash. The second is an internal variable of the flash that keeps track of whether keys have been "used" or not. During sweep, unused keys are marked used, and kept in the flash, used keys are removed from the flash. That''s how keys are kept for one more action, and how you can use flash.keep(:key) to keep a key but not others.
> def redir > flash[:foo] = ''bar'' > redirect_to :action => ''two'' > false > endAnd if you want to see the behavior I expect, call flash.sweep in redir before returning false: def redir flash[:foo] = ''bar'' redirect_to :action => ''two'' flash.sweep false end
... speaking of flash sweeping... was a change made to the trunk that
broke sweeping after a redirect? Here is the error:
Processing AccountController#login (for 127.0.0.1 at 2006-07-14 11:11:02) [POST]
Session ID: a7a2b536b4d92aa3f633659b17f6e74c
Parameters: {"commit"=>"Log in",
"subdomain"=>"zackchandler",
"action"=>"login",
"controller"=>"account",
"password"=>"trackit"}
Account Columns (0.071569) SHOW FIELDS FROM accounts
Account Load (0.001490) SELECT * FROM accounts WHERE
(accounts.`subdomain` = ''zackchandler'' ) LIMIT 1
Redirected to http://zackchandler.localhost.com:3000
Completed in 0.08542 (11 reqs/sec) | DB: 0.07306 (85%) | 302 Found
[http://zackchandler.localhost.com/login]
You have a nil object when you didn''t expect it!
The error occurred while evaluating nil.sweep
./script/../config/../vendor/rails/actionpack/lib/action_controller/flash.rb:144:in
`process_cleanup_without_filters''
./script/../config/../vendor/rails/actionpack/lib/action_controller/filters.rb:434:in
`process_cleanup_without_session_management_support''
./script/../config/../vendor/rails/actionpack/lib/action_controller/session_management.rb:123:in
`process_cleanup_without_components''
./script/../config/../vendor/rails/actionpack/lib/action_controller/components.rb:177:in
`process_cleanup''
./script/../config/../vendor/rails/actionpack/lib/action_controller/base.rb:411:in
`process_without_filters''
./script/../config/../vendor/rails/actionpack/lib/action_controller/filters.rb:372:in
`process_without_session_management_support''
./script/../config/../vendor/rails/actionpack/lib/action_controller/session_management.rb:114:in
`process''
./script/../config/../vendor/rails/actionpack/lib/action_controller/base.rb:318:in
`process''
./script/../config/../vendor/rails/railties/lib/dispatcher.rb:38:in
`dispatch''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel/rails.rb:85:in
`process''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:563:in
`process_client''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:562:in
`process_client''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:648:in
`run''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:648:in
`run''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:637:in
`run''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:969:in
`run''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:968:in
`run''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/bin/mongrel_rails:119:in
`run''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel/command.rb:211:in
`run''
/usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/bin/mongrel_rails:227
./script/../config/../vendor/rails/activesupport/lib/active_support/dependencies.rb:140:in
`load''
./script/../config/../vendor/rails/railties/lib/commands/servers/mongrel.rb:48
/usr/local/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:21:in
`require''
./script/../config/../vendor/rails/activesupport/lib/active_support/dependencies.rb:147:in
`require''
./script/../config/../vendor/rails/railties/lib/commands/server.rb:39
./script/server:3
Here is the controller action
def login
if request.post?
account = Account.authenticate(params[:subdomain], params[:password])
if account
reset_session
session[:account_id] = account.id
session[:subdomain] = account.subdomain
session[:name] = account.name
domain =
request.env[''HTTP_HOST''].split(''.'')[1..-1].join(''.'')
# strip off subdomain
redirect_to "http://#{account.subdomain}.#{domain}"
else
flash[:error] = ''We were unable to login you in. Check your
web site name
and password and please try again.''
end
end
end
Any ideas?
Zack
On 7/13/06, Caio Chassot <lists@v2studio.com>
wrote:> > def redir
> > flash[:foo] = ''bar''
> > redirect_to :action => ''two''
> > false
> > end
>
>
> And if you want to see the behavior I expect, call flash.sweep in
> redir before returning false:
>
> def redir
> flash[:foo] = ''bar''
> redirect_to :action => ''two''
> flash.sweep
> false
> end
>
>
> _______________________________________________
> Rails-core mailing list
> Rails-core@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails-core
>
If you comment out the reset_session line the code works fine. Looks like a bug to me. I always like to reset the session upon login to protect against hijacked sessions. Should I submit a bug report when trac comes back to life? Zack On 7/14/06, Zack Chandler <zackchandler@gmail.com> wrote:> ... speaking of flash sweeping... was a change made to the trunk that > broke sweeping after a redirect? Here is the error: > > Processing AccountController#login (for 127.0.0.1 at 2006-07-14 11:11:02) [POST] > Session ID: a7a2b536b4d92aa3f633659b17f6e74c > Parameters: {"commit"=>"Log in", "subdomain"=>"zackchandler", > "action"=>"login", "controller"=>"account", "password"=>"trackit"} > Account Columns (0.071569) SHOW FIELDS FROM accounts > Account Load (0.001490) SELECT * FROM accounts WHERE > (accounts.`subdomain` = ''zackchandler'' ) LIMIT 1 > Redirected to http://zackchandler.localhost.com:3000 > Completed in 0.08542 (11 reqs/sec) | DB: 0.07306 (85%) | 302 Found > [http://zackchandler.localhost.com/login] > You have a nil object when you didn''t expect it! > The error occurred while evaluating nil.sweep > ./script/../config/../vendor/rails/actionpack/lib/action_controller/flash.rb:144:in > `process_cleanup_without_filters'' > ./script/../config/../vendor/rails/actionpack/lib/action_controller/filters.rb:434:in > `process_cleanup_without_session_management_support'' > ./script/../config/../vendor/rails/actionpack/lib/action_controller/session_management.rb:123:in > `process_cleanup_without_components'' > ./script/../config/../vendor/rails/actionpack/lib/action_controller/components.rb:177:in > `process_cleanup'' > ./script/../config/../vendor/rails/actionpack/lib/action_controller/base.rb:411:in > `process_without_filters'' > ./script/../config/../vendor/rails/actionpack/lib/action_controller/filters.rb:372:in > `process_without_session_management_support'' > ./script/../config/../vendor/rails/actionpack/lib/action_controller/session_management.rb:114:in > `process'' > ./script/../config/../vendor/rails/actionpack/lib/action_controller/base.rb:318:in > `process'' > ./script/../config/../vendor/rails/railties/lib/dispatcher.rb:38:in `dispatch'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel/rails.rb:85:in > `process'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:563:in > `process_client'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:562:in > `process_client'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:648:in `run'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:648:in `run'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:637:in `run'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:969:in `run'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel.rb:968:in `run'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/bin/mongrel_rails:119:in > `run'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/lib/mongrel/command.rb:211:in > `run'' > /usr/local/lib/ruby/gems/1.8/gems/mongrel-0.3.13.2/bin/mongrel_rails:227 > ./script/../config/../vendor/rails/activesupport/lib/active_support/dependencies.rb:140:in > `load'' > ./script/../config/../vendor/rails/railties/lib/commands/servers/mongrel.rb:48 > /usr/local/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:21:in `require'' > ./script/../config/../vendor/rails/activesupport/lib/active_support/dependencies.rb:147:in > `require'' > ./script/../config/../vendor/rails/railties/lib/commands/server.rb:39 > ./script/server:3 > > Here is the controller action > > def login > if request.post? > account = Account.authenticate(params[:subdomain], params[:password]) > if account > reset_session > session[:account_id] = account.id > session[:subdomain] = account.subdomain > session[:name] = account.name > domain = request.env[''HTTP_HOST''].split(''.'')[1..-1].join(''.'') > # strip off subdomain > redirect_to "http://#{account.subdomain}.#{domain}" > else > flash[:error] = ''We were unable to login you in. Check your > web site name > and password and please try again.'' > end > end > end > > Any ideas? > > Zack > > > On 7/13/06, Caio Chassot <lists@v2studio.com> wrote: > > > def redir > > > flash[:foo] = ''bar'' > > > redirect_to :action => ''two'' > > > false > > > end > > > > > > And if you want to see the behavior I expect, call flash.sweep in > > redir before returning false: > > > > def redir > > flash[:foo] = ''bar'' > > redirect_to :action => ''two'' > > flash.sweep > > false > > end > > > > > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > >
On 7/14/06, Zack Chandler <zackchandler@gmail.com> wrote:> If you comment out the reset_session line the code works fine. > Looks like a bug to me. I always like to reset the session upon login > to protect against hijacked sessions. > > Should I submit a bug report when trac comes back to life? > > ZackIf you could provide a failing action controller test, that''d be great :) I''ll try to get a look at this sometime this weekend if someone else doesn''t beat me to it. I''d say just throw a diff somewhere or even attach it to one of these mails. -- Rick Olson http://techno-weenie.net
Rick, Thanks... here is a quick diff of session_management_test.rb> class BasicController < ActionController::Base > > def reset > reset_session > render :text => ''reset'' > end > > end > > def test_reset_session > @controller = BasicController.new > get :reset > endHope this helps, Zack On 7/14/06, Rick Olson <technoweenie@gmail.com> wrote:> On 7/14/06, Zack Chandler <zackchandler@gmail.com> wrote: > > If you comment out the reset_session line the code works fine. > > Looks like a bug to me. I always like to reset the session upon login > > to protect against hijacked sessions. > > > > Should I submit a bug report when trac comes back to life? > > > > Zack > > If you could provide a failing action controller test, that''d be great > :) I''ll try to get a look at this sometime this weekend if someone > else doesn''t beat me to it. I''d say just throw a diff somewhere or > even attach it to one of these mails. > > -- > Rick Olson > http://techno-weenie.net > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >
On 7/15/06, Rick Olson <technoweenie@gmail.com> wrote:> > On 7/14/06, Zack Chandler <zackchandler@gmail.com> wrote: > > If you comment out the reset_session line the code works fine. > > Looks like a bug to me. I always like to reset the session upon login > > to protect against hijacked sessions. > > > > Should I submit a bug report when trac comes back to life? > > > > Zack > > If you could provide a failing action controller test, that''d be great > :) I''ll try to get a look at this sometime this weekend if someone > else doesn''t beat me to it. I''d say just throw a diff somewhere or > even attach it to one of these mails.Hi, I''m guessing this one isn''t sorted yet. I just got edge up to date (rev 4610) and I have the same error on two tests in the user_controller for AAA test_should_logout test_should_delete_token_on_logout I went back to rails 1.1.4 and no problem. Any news on this one? Cheers Dan --> Rick Olson > http://techno-weenie.net > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >_______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
Here''s a quick diff that should fix the flash issue. It fails one test in verification_test, but seeing how verifications rely on before_filters, it''s something to look into patching, probably. Also, maybe someone could transform my test controller into a real test for the issue being fixed here. _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
Any word on the reset_session bug in trunk? - I attached a new test as per Rick Olson''s request showing the failure. Here it is again (diff of session_management_test.rb). Let me know if there is anything else I can do. Thanks, Zack On 7/17/06, Daniel N <has.sox@gmail.com> wrote:> > > > On 7/15/06, Rick Olson <technoweenie@gmail.com> wrote: > > On 7/14/06, Zack Chandler <zackchandler@gmail.com> wrote: > > > If you comment out the reset_session line the code works fine. > > > Looks like a bug to me. I always like to reset the session upon login > > > to protect against hijacked sessions. > > > > > > Should I submit a bug report when trac comes back to life? > > > > > > Zack > > > > If you could provide a failing action controller test, that''d be great > > :) I''ll try to get a look at this sometime this weekend if someone > > else doesn''t beat me to it. I''d say just throw a diff somewhere or > > even attach it to one of these mails. > > > > Hi, I''m guessing this one isn''t sorted yet. > I just got edge up to date (rev 4610) and I have the same error on two tests > in the user_controller for AAA > > test_should_logout > test_should_delete_token_on_logout > > I went back to rails 1.1.4 and no problem. > > Any news on this one? > > Cheers > > Dan > > > > > -- > > Rick Olson > http://techno-weenie.net > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > >_______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
This looks to be the same issue as http://dev.rubyonrails.org/ticket/5584 which I reopened. I attached a failing testcase to it as well. -Lee On 7/19/06, Zack Chandler <zackchandler@gmail.com> wrote:> Any word on the reset_session bug in trunk? - I attached a new test as > per Rick Olson''s request showing the failure. Here it is again (diff > of session_management_test.rb). > Let me know if there is anything else I can do. > > Thanks, > Zack > > On 7/17/06, Daniel N <has.sox@gmail.com> wrote: > > > > > > > > On 7/15/06, Rick Olson <technoweenie@gmail.com> wrote: > > > On 7/14/06, Zack Chandler <zackchandler@gmail.com> wrote: > > > > If you comment out the reset_session line the code works fine. > > > > Looks like a bug to me. I always like to reset the session upon login > > > > to protect against hijacked sessions. > > > > > > > > Should I submit a bug report when trac comes back to life? > > > > > > > > Zack > > > > > > If you could provide a failing action controller test, that''d be great > > > :) I''ll try to get a look at this sometime this weekend if someone > > > else doesn''t beat me to it. I''d say just throw a diff somewhere or > > > even attach it to one of these mails. > > > > > > > > Hi, I''m guessing this one isn''t sorted yet. > > I just got edge up to date (rev 4610) and I have the same error on two tests > > in the user_controller for AAA > > > > test_should_logout > > test_should_delete_token_on_logout > > > > I went back to rails 1.1.4 and no problem. > > > > Any news on this one? > > > > Cheers > > > > Dan > > > > > > > > > -- > > > > Rick Olson > > http://techno-weenie.net > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > > > _______________________________________________ > > Rails-core mailing list > > Rails-core@lists.rubyonrails.org > > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > > > > > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core > > > >
On 2006-07-20, at 14:12 , Lee Marlow wrote:> This looks to be the same issue as > http://dev.rubyonrails.org/ticket/5584 which I reopened. I attached a > failing testcase to it as well.I don''t think it''s the same issue. Your patch doesn''t fix my problem, mine doesn''t pass your new test. I can, however, fix your test by defining reset_session_with_flash as such: def reset_session_with_flash reset_session_without_flash remove_instance_variable("@flash") flash(:refresh) end You might want to update that patch in #5584 to include this.
On 7/20/06, Lee Marlow <lmarlow@yahoo.com> wrote:> This looks to be the same issue as > http://dev.rubyonrails.org/ticket/5584 which I reopened. I attached a > failing testcase to it as well.Thanks for the patch Lee! http://dev.rubyonrails.org/changeset/4617 Aaaaand acts as authenticated is happy again. Now regarding this flash business, is it possible to do flash.keep if you want to keep the flash on a halted before filter (assuming the default is changed to just sweep unconditionally)? Personally I don''t care all that much since I don''t use flash a lot, but sweeping on each request makes sense to me. -- Rick Olson http://techno-weenie.net
On 2006-07-21, at 23:48 , Rick Olson wrote:> Now regarding this flash business, is it possible to do flash.keep if > you want to keep the flash on a halted before filter (assuming the > default is changed to just sweep unconditionally)?If, in the halting before filter, you need to keep the flash from the previous action, calling flash.keep will work fine. Here''s another contrived example that demonstrates that: class TestController < ApplicationController FLASH = "<pre><% {}.update(flash).to_yaml %><% {}.update(flash.instance_variable_get(''@used'')).to_yaml %></pre>" before_filter :redir, :only => ''skip'' def one flash[:foo] = ''bar'' render :inline => "#{FLASH}1. <%= link_to ''next'', :action => ''skip'' %>" end def skip render :inline => "#{FLASH}skip" end def two render :inline => "#{FLASH}2. <%= link_to ''next'', :action => ''three'' %>" end def three render :inline => "#{FLASH}3. see..." end private def redir flash.keep redirect_to :action => ''two'' false end end With my patch, the flash will be available to actions: one, skip, two. Without it, action three will still have the flash, which feels buggy.
> It fails one test in verification_test, but seeing how > verifications rely on before_filters, it''s something to look into > patching, probably.Actually, that revealed a bug in the flash. Here''s an updated patch that fixes this flash#update bug, along with the previous patch for the halted filter thing. _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core
On 2006-07-21, at 23:48 , Rick Olson wrote:> Now regarding this flash business... > > ... sweeping on each request makes sense to me.So... do we need anything else to get this patch commited? I''d love to stop using a locally patched rails. I''m sending the patch again so you don''t have to hunt for it. _______________________________________________ Rails-core mailing list Rails-core@lists.rubyonrails.org http://lists.rubyonrails.org/mailman/listinfo/rails-core