Skip to content
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

Setup default session store internally, no longer through an initializer #25438

Merged

Conversation

@prathamesh-sonpatki
Copy link
Member

@prathamesh-sonpatki prathamesh-sonpatki commented Jun 19, 2016

  • By default the session store will be set to cookie store with application name as session key.
  • Based on comment by DHH here - #25181 (comment).
@kamipo
kamipo reviewed Jun 19, 2016
View changes
actionpack/lib/action_controller/railtie.rb Outdated

initializer "setup session store" do |app|
app_name = app.railtie_name.sub(/_application$/, '')
app.config.session_store :cookie_store, key: "'_#{app_name}_session'"

This comment has been minimized.

@kamipo

kamipo Jun 19, 2016
Member

Should be "_#{app_name}_session"?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 19, 2016
Author Member

Fixed, thanks

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:rm-session-store-initializer branch Jun 19, 2016
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jun 20, 2016

Tests are broken.

@rafaelfranca rafaelfranca self-assigned this Jun 20, 2016
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:rm-session-store-initializer branch Jun 20, 2016
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:rm-session-store-initializer branch 2 times, most recently Jul 2, 2016
@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Jul 6, 2016

@rafaelfranca Please review. Build is green.

@rafaelfranca
rafaelfranca reviewed Jul 6, 2016
View changes
actionpack/lib/action_controller/railtie.rb Outdated
@@ -67,5 +67,12 @@ class Railtie < Rails::Railtie #:nodoc:
config.compile_methods! if config.respond_to?(:compile_methods!)
end
end

initializer "setup session store" do |app|
ActiveSupport.on_load(:action_controller) do

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 6, 2016
Member

Why does it need to be inside on_load hook if it sets a config?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 6, 2016
Member

This config lives in the railties so the initializer should be there too, not in Action Controller.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 6, 2016
Author Member

We have lot of tests which set the session_store in config/application.rb. If this is not in the inside of on_load hook, then it gets run after the application.rb is run -- overriding the config set in application.rb.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 6, 2016
Member

This is just masking a problem. It only works because ActionController::Base is not loaded in those tests.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 6, 2016
Author Member

This config lives in the railties so the initializer should be there too, not in Action Controller.

@rafaelfranca I was actually confused here, I thought of railties also but where do we place it in railties?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 6, 2016
Author Member

@rafaelfranca We can move it to Rails::Application::Configuration class. What do you think?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 6, 2016
Member

I mean railties folder. This configuration have nothing to do with Action Controller. All work is being done in Application::Configuration class so this initializers should be in the bootstrap.rb or finisher.rb files.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 6, 2016
Member

Rails::Application::Configuration doesn't define any initializer. It should be in bootstrap or finisher. I believe finisher is the right place to put since we only want to set it if it was not already set.

@kaspth
kaspth reviewed Jul 6, 2016
View changes
actionpack/lib/action_controller/railtie.rb Outdated

initializer "setup session store" do |app|
ActiveSupport.on_load(:action_controller) do
app_name = app.class.name ? app.railtie_name.sub(/_application$/, '') : ''

This comment has been minimized.

@kaspth

kaspth Jul 6, 2016
Member

Could make good use of chomp: app.railtie_name.chomp('_application').

Also when won't the app class have a name? Is it likely that Rails::Application subclasses are an anonymous class?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 6, 2016
Author Member

For few tests I noticed that the app_name was coming out to be empty. I will revert this locally and post which were those tests.

This comment has been minimized.

@kaspth
kaspth reviewed Jul 6, 2016
View changes
guides/source/configuring.md Outdated
@@ -142,7 +142,7 @@ defaults to `:debug` for all environments. The available log levels are: `:debug

* `config.public_file_server.enabled` configures Rails to serve static files from the public directory. This option defaults to `true`, but in the production environment it is set to `false` because the server software (e.g. NGINX or Apache) used to run the application should serve static files instead. If you are running or testing your app in production mode using WEBrick (it is not recommended to use WEBrick in production) set the option to `true.` Otherwise, you won't be able to use page caching and request for files that exist under the public directory.

* `config.session_store` is usually set up in `config/initializers/session_store.rb` and specifies what class to use to store the session. Possible values are `:cookie_store` which is the default, `:mem_cache_store`, and `:disabled`. The last one tells Rails not to deal with sessions. Custom session stores can also be specified:
* `config.session_store` specifies what class to use to store the session. Possible values are `:cookie_store` which is the default, `:mem_cache_store`, and `:disabled`. The last one tells Rails not to deal with sessions. Defaults to cookie store with application name as session key. Custom session stores can also be specified:

This comment has been minimized.

@kaspth

kaspth Jul 6, 2016
Member

Defaults to a cookie store with the application name as the session key 😁

@kaspth
kaspth reviewed Jul 6, 2016
View changes
railties/lib/rails/application/configuration.rb Outdated
when Symbol
ActionDispatch::Session.const_get(@session_store.to_s.camelize)
else
@session_store
end
else
@session_store = args.shift

if @session_store == :active_record_store

This comment has been minimized.

@kaspth

kaspth Jul 6, 2016
Member

Curious, why do we have to move this?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 6, 2016
Member

Why this was moved?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 6, 2016
Author Member

Please check c73107e 😄

This comment has been minimized.

@kaspth

kaspth Jul 6, 2016
Member

Faster than @rafaelfranca! Woo \o/

skaermbillede 2016-07-06 kl 19 03 28

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 6, 2016
Author Member

I have another proposal of changing the config.session_store to not act as accessor as well as setter at the same time, but I was planning to propose it after this PR :)

This comment has been minimized.

@kaspth

kaspth Jul 10, 2016
Member

Does that benefit outweigh a deprecation cycle? Seems a bit to quick in my opinion.

I don't quite understand your reasoning behind the move. I'm curious which tests were failing?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 17, 2016
Author Member

Does that benefit outweigh a deprecation cycle? Seems a bit to quick in my opinion.

Yup, it does. EDIT The new proposal definititely needs deprecation. This PR not.

I'm curious which tests were failing?

With my earlier implementation, the test

test "config.session_store with :active_record_store without activerecord-session_store gem" do
was failing. With the current implementation, it no longer fails but I have kept the commit as it is. Because I feel the error about missing activerecord-session_store gem should be shown when someone tries to set config.session_store to :activerecord_store and not when somebody tries to access config.session_store.

Currently the same session_store method is used for accessing as well as setting the session store.

def session_store(*args)
  if args.empty?
    # Access session store
    case @session_store
    when :disabled
      nil
    when :active_record_store
      begin
        ActionDispatch::Session::ActiveRecordStore
      rescue NameError
        raise "`ActiveRecord::SessionStore` is extracted out of Rails into a gem. " \
          "Please add `activerecord-session_store` to your Gemfile to use it."
      end
    when Symbol
      ActionDispatch::Session.const_get(@session_store.to_s.camelize)
    else
      @session_store
    end
  else
    # Set session store
    @session_store = args.shift
    @session_options = args.shift || {}
  end
end

This is the implementation from master branch which raises the error in the code path of accessing session store. This change moves the error from accessing session store to setting session store code path. The behavior is same. IMO it will be good if we raise as early as possible.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:rm-session-store-initializer branch 3 times, most recently Jul 7, 2016
@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Jul 7, 2016

@rafaelfranca Please review.

@prathamesh-sonpatki
prathamesh-sonpatki reviewed Jul 7, 2016
View changes
railties/lib/rails/application/finisher.rb Outdated
@@ -33,6 +33,14 @@ module Finisher
end
end

# Setup default session store if not already set in config/application.rb
initializer :setup_default_session_store, before: :build_middleware_stack do |app|
unless app.config.session_store

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 7, 2016
Author Member

This introduces one issue. If user has set the session_store to :disabled then this change overwrites it.

This comment has been minimized.

@kaspth

kaspth Jul 10, 2016
Member

Why not just use app.config.session_store != :disabled? 😁

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jul 17, 2016
Author Member

@kaspth Because app.config.session_store returns nil when it is set to disabled.

There is no way as of now to see if the session store is not set without touching internal instance variables.

@kaspth
kaspth reviewed Jul 10, 2016
View changes
railties/lib/rails/application/configuration.rb Outdated
@@ -170,20 +168,23 @@ def session_store(*args)
case @session_store

This comment has been minimized.

@kaspth

kaspth Jul 10, 2016
Member

Could also shift to kw args while we're here:

def session_store(new_session_store = nil, **options)
  unless new_session_store
    case @session_store
    # ...
  else
    @session_store   = new_session_store
    @session_options = options
  end
end
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:rm-session-store-initializer branch Jul 17, 2016
…ser or by Rails

- We need to ability to check whether the session store it is set or
   not so that we can set it ourselves in an internal initializer.
- We can't rely on return value of `config.session_store` as it can
  return `nil` when set to `disabled` and we will accidentally override it
  and set to default cookie store.
- So introduced new method which just tells us whether it is set or not.
…ly when set to activerecord session store

- Use keyword args as it is possible to use them now.
- The error message for activerecord-session_store gem was added in 1807384.
- But it was added for a code path which gets called when we try to
  **access** the session store, not when we **set** it.
- So the test expecting the exception started failing because now the
  session store is set via railtie again **after** setting it first with
  :active_record_store in the test.
- As the error is not raised while setting the store to
  :active_record_store, the store gets overwritten by railtie and when
  we access it via `session_store` while building the default middleware
  stack, the exception is not raised.
- This commit moves the code for raising the exception to the path where
  we try to set the store.
…ion initializer

- By default the session store will be set to cookie store with
  application name as session key.
- Older apps are not affected as they will have the session store
  initializer generated by Rails in older versions, and Rails will not
  overwrite the session store if it is already set or disabled.
- But new apps will not have the initializer, instead the session store
  will be set to cookie store by default.
- Based on comment by DHH here - #25181 (comment).
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:rm-session-store-initializer branch to e5a6f7e Jul 17, 2016
@prathamesh-sonpatki
Copy link
Member Author

@prathamesh-sonpatki prathamesh-sonpatki commented Jul 17, 2016

@kaspth @rafaelfranca Ready for review.

when :active_record_store
def session_store(new_session_store = nil, **options)
if new_session_store

This comment has been minimized.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jul 20, 2016

Just a really small comment, but this really looks good 👏

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jul 20, 2016

Merged

@rafaelfranca rafaelfranca merged commit e5a6f7e into rails:master Jul 20, 2016
2 checks passed
2 checks passed
codeclimate Code Climate has skipped analysis of this commit.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rafaelfranca added a commit that referenced this pull request Jul 20, 2016
…nitializer

Setup default session store internally, no longer through an initializer
@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:rm-session-store-initializer branch Jul 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.