New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix request.reset_session for API controllers #24229

Merged
merged 1 commit into from Mar 21, 2016

Conversation

Projects
None yet
4 participants
@maclover7
Member

maclover7 commented Mar 17, 2016

Due to that ActionDispatch::Flash (the flash API's middleware) is not
included for API controllers, the request.reset_session method, which
relies on there being a flash= method which is in fact defined by the
middleware, was previously breaking. Similarly to how
add4648 created a method to be
overriden by the flash middleware in order to ensure non-breakage, this
is how flashes are now reset.

Fixes #24222

@sikachu

View changes

actionpack/lib/action_dispatch/http/request.rb Outdated
@@ -403,6 +403,9 @@ def logger
def commit_flash
end
def reset_flash

This comment has been minimized.

@sikachu

sikachu Mar 18, 2016

Member

Should this and #commit_flash above be :nodoc: too?

@kaspth

View changes

actionpack/lib/action_dispatch/http/request.rb Outdated
@@ -337,7 +337,7 @@ def reset_session
else
self.session = {}
end
self.flash = nil
reset_flash

This comment has been minimized.

@kaspth

kaspth Mar 18, 2016

Member

Shouldn't the flash middleware just override reset_session, call super and reset the flash?

@kaspth

View changes

actionpack/lib/action_dispatch/middleware/flash.rb Outdated
@@ -70,6 +70,10 @@ def commit_flash # :nodoc:
session.delete('flash')
end
end
def reset_flash # :nodoc:
set_header(Flash::KEY, nil)

This comment has been minimized.

@kaspth

kaspth Mar 18, 2016

Member

Shouldn't we use the exposed API instead of writing "new" code?

@kaspth

View changes

railties/test/application/middleware/flash_test.rb Outdated
assert_equal(200, last_response.status)
assert_equal('It worked!', last_response.body)
refute(middleware.include?('ActionDispatch::Flash'))

This comment has been minimized.

@kaspth

kaspth Mar 18, 2016

Member

Prefer assert_not. I'd also remove the parens around the assertion calls (and Rack::Test calls).

This comment has been minimized.

@kaspth

kaspth Mar 18, 2016

Member

Also do we really want to assert on the internal middleware stack? Seems iffy.

@kaspth

View changes

railties/test/application/middleware/flash_test.rb Outdated
private
def middleware
Rails.application.middleware.map(&:klass).map(&:name)

This comment has been minimized.

@kaspth

kaspth Mar 18, 2016

Member

Why map to class name? Won't middleware.include?(ActionDispatch::Flash) work?

Fix request.reset_session for API controllers
Due to that `ActionDispatch::Flash` (the flash API's middleware) is not
included for API controllers, the `request.reset_session` method, which
relies on there being a `flash=` method which is in fact defined by the
middleware, was previously breaking. Similarly to how
add4648 created a method to be
overridden by the flash middleware in order to ensure non-breakage, this
is how flashes are now reset.

Fixes #24222

@maclover7 maclover7 force-pushed the maclover7:fix-24222 branch to 6c6a221 Mar 20, 2016

@maclover7

This comment has been minimized.

Member

maclover7 commented Mar 20, 2016

Update per code review comments. I wanted to assert that ActionDispatch::Flash is not in the middleware stack, to show that part of the bug is precisely that this middleware isn't in the ActionDispatch::MiddlewareStack for API applications.

refute Rails.application.middleware.include?(ActionDispatch::Flash)
end
end
end

This comment has been minimized.

@kaspth

kaspth Mar 20, 2016

Member

This test still feels misplaced to me. It's placed in middleware/flash_test.rb but its whole point is that the flash middleware isn't there 🤔

This is really testing calling reset_session in API only apps works. So aren't there some API app tests we can put this with?

This comment has been minimized.

@maclover7

maclover7 Mar 20, 2016

Member

From what I've been able to see inside railties/test, there's no centralized API-specific testing stuff...

@rafaelfranca rafaelfranca merged commit 6c6a221 into rails:master Mar 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Mar 21, 2016

Merge pull request #24229 from maclover7/fix-24222
Fix request.reset_session for API controllers

@maclover7 maclover7 deleted the maclover7:fix-24222 branch Mar 21, 2016

@maclover7

This comment has been minimized.

Member

maclover7 commented Mar 21, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment