Remove special handling for ActiveRecordStore #45395

skipkayhil · 2022-06-18T04:28:57Z

Summary

activerecord-session_store was removed in 0ffe190, and has been
displaying a special error message when missing since Rails 4.0.

Replace the specific error message so that third party stores get nicer
error handling as well

jonathanhefner · 2022-06-18T15:12:38Z

guides/source/action_controller_overview.md

-Your application has a session for each user in which you can store small amounts of data that will be persisted between requests. The session is only available in the controller and the view and can use one of several of different storage mechanisms:
+Your application has a session for each user in which you can store small
+amounts of data that will be persisted between requests. The session is only
+available in the controller and the view. The storage mechanism can be
+configured to a store provided by an external gem or one that Rails provides by
+default:


Instead of changing this part, what if we added a mention of third-party gems as a bullet point to the list below? Maybe something like:

* A custom store or a store provided by a third-party gem.

jonathanhefner · 2022-06-18T15:14:52Z

guides/source/action_controller_overview.md


 * [`ActionDispatch::Session::CookieStore`][] - Stores everything on the client.
 * [`ActionDispatch::Session::CacheStore`][] - Stores the data in the Rails cache.
-* `ActionDispatch::Session::ActiveRecordStore` - Stores the data in a database using Active Record (requires the `activerecord-session_store` gem).


The activerecord-session_store gem is still being maintained under the rails org, so let's keep this. We can link to https://github.com/rails/activerecord-session_store though, and lean on its README for documentation purposes.

Hmm, I tried to find some context on the reasoning for extraction to a gem but came up short. I was curious what prompted it and whether that meant it should be recommended/documented.

Adding it back for now but I do think getting the context would be a big plus

jonathanhefner · 2022-06-18T15:16:54Z

guides/source/action_controller_overview.md

-# Use the database for sessions instead of the cookie-based default,
-# which shouldn't be used to store highly confidential information
-# (create the session table with "rails g active_record:session_migration")
-# Rails.application.config.session_store :active_record_store
+# Rails.application.config.session_store :cache_store


Good change! 👍 We might also consider linking to the configuration guide below this example. Something like:

See the [configuration guide](configuring.html#config-session-store) for more information.

jonathanhefner · 2022-06-18T15:25:36Z

railties/lib/rails/application/configuration.rb

          @session_store = new_session_store
          @session_options = options || {}
        else
          case @session_store
          when :disabled
            nil
-          when :active_record_store
-            ActionDispatch::Session::ActiveRecordStore
          when Symbol
            ActionDispatch::Session.const_get(@session_store.to_s.camelize)


The idea here is to just rely on the logic in the when Symbol branch to resolve :active_record_store, yes? Sounds good! 👍

Yep, I left the test to ensure it keeps working too: https://github.com/rails/rails/blob/b88de15888b2ced7cac03508e1c28845e3d30034/railties/test/application/configuration_test.rb#L1641

jonathanhefner · 2022-06-18T15:42:21Z

railties/lib/rails/application/configuration.rb

-          if new_session_store == :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
-          end
-


I'm on the fence about removing this. Without it, the error would be just "uninitialized constant ActionDispatch::Session::ActiveRecordStore (NameError)", which is not nearly as helpful.

What about adding a helpful, general error message by rescuing NameError raised by the ActionDispatch::Session.const_get further below. Roughly something like:

Could not resolve session store #{@session_store.inspect}. #{@session_store.inspect} resolves to ActionDispatch::Session::#{@session_store.to_s.camelize}, but ActionDispatch::Session::#{@session_store.to_s.camelize} is not defined. Is #{@session_store.inspect} spelled correctly, and are the necessary gems installed?

It might be possible to add some kind of did-you-mean suggestions based on the constants under ActionDispatch::Session, but that may be more trouble than it's worth. (And could be a separate PR.)

I like the idea for a DYM error, I'll take a stab at that in a follow up PR

skipkayhil · 2022-06-18T20:55:21Z

My most recent commit's test failure is real:

Failure:
ApplicationTests::ConfigurationTest#test_ActionDispatch::Request.return_only_media_type_on_content_type_can_be_configured_in_the_new_framework_defaults [/home/hartley/src/github.com/skipkayhil/rails/railties/test/application/configuration_test.rb:3552]:
Expected: false
  Actual: true

It looks like trying to load ActionDispatch::Session::CookieStore triggers ActionDispatch::Request to load, which prevents any configuration from being applied to it in later initializers... going to investigate this some more.

Edit: CookieStore < AbstractSecureStore, and that file requires 'action_dispatch/request/session' which will autoload ActionDispatch::Request I believe. I'm honestly not sure how activerecord-session_store doesn't encounter this same error right now.

skipkayhil · 2022-06-18T22:17:08Z

https://gist.github.com/skipkayhil/1897d8bfcea72003c58a9ebcdf17f3c7
So it looks like even including the activerecord-session_store gem prevents configuring ActionDispatch::Request. I'm going to push an update that doesn't try to constantize the store until it gets used (in default_middleware_stack) but I'm not sure its the best solution.

jonathanhefner

I'm going to push an update that doesn't try to constantize the store until it gets used (in default_middleware_stack) but I'm not sure its the best solution.

That was what I originally had in mind — I hadn't really thought about doing an eager check. I think it would be possible by using ActionDispatch::Session.const_defined?, which shouldn't autoload. But I'm not sure if it's worth the extra code. What you have now isn't much worse for :active_record_store, and is definitely better for e.g. :activerecord_store or :memcache_store.

Also, I have not looked, but do we have adequate test coverage for when config.session_store (and thus ActionDispatch::Session.resolve_store) fails to resolve a store?

jonathanhefner · 2022-06-19T19:13:03Z

actionpack/lib/action_dispatch.rb

+    def self.resolve_store(session_store)
+      self.const_get(session_store.to_s.camelize)
+    rescue NameError
+      raise <<~ERROR
+        Unable to resolve session store :#{session_store}.
+
+        :#{session_store} resolves to ActionDispatch::Session::#{session_store.to_s.camelize},
+        but that class is undefined.
+
+        Is :#{session_store} spelled correctly, and are the necessary gems installed?
+      ERROR
+    end


I like this extraction! 😍

I wanted to point out a subtle difference between ":#{session_store}" and "#{session_store.inspect}". Symbols can include arbitrary characters, including slashes which are validly "camelized" to "::":

symbol = :"my_namespace/my_store" puts symbol.to_s.camelize # => MyNamespace::MyStore puts ":#{symbol} versus #{symbol.inspect}" # => :my_namespace/my_store versus :"my_namespace/my_store"

So it might be better to use session_store.inspect here.

activerecord-session_store was removed in 0ffe190, and has been displaying a special error message when missing since Rails 4.0. Replace the specific error message so that third party stores get nicer error handling as well

jonathanhefner · 2022-06-20T19:06:32Z

Thank you, @skipkayhil! 👍

rails-bot bot added docs railties labels Jun 18, 2022

jonathanhefner reviewed Jun 18, 2022

View reviewed changes

skipkayhil force-pushed the rm-ar-store-special branch from b88de15 to ec7ec44 Compare June 18, 2022 19:43

rails-bot bot added the actionpack label Jun 18, 2022

skipkayhil force-pushed the rm-ar-store-special branch 2 times, most recently from 2843fd0 to a00c37d Compare June 18, 2022 22:53

rails deleted a comment Jun 19, 2022

jonathanhefner reviewed Jun 19, 2022

View reviewed changes

skipkayhil force-pushed the rm-ar-store-special branch from a00c37d to 782a263 Compare June 20, 2022 02:02

Remove special handling for ActiveRecordStore

27285e7

activerecord-session_store was removed in 0ffe190, and has been displaying a special error message when missing since Rails 4.0. Replace the specific error message so that third party stores get nicer error handling as well

skipkayhil force-pushed the rm-ar-store-special branch from 782a263 to 27285e7 Compare June 20, 2022 04:35

jonathanhefner merged commit 949a5e4 into rails:main Jun 20, 2022

skipkayhil deleted the rm-ar-store-special branch June 20, 2022 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove special handling for ActiveRecordStore #45395

Remove special handling for ActiveRecordStore #45395

skipkayhil commented Jun 18, 2022 •

edited

jonathanhefner Jun 18, 2022

jonathanhefner Jun 18, 2022

skipkayhil Jun 18, 2022 •

edited

jonathanhefner Jun 18, 2022

jonathanhefner Jun 18, 2022

skipkayhil Jun 18, 2022

jonathanhefner Jun 18, 2022

skipkayhil Jun 18, 2022

skipkayhil commented Jun 18, 2022 •

edited

skipkayhil commented Jun 18, 2022

jonathanhefner left a comment

jonathanhefner Jun 19, 2022

jonathanhefner commented Jun 20, 2022

Remove special handling for ActiveRecordStore #45395

Remove special handling for ActiveRecordStore #45395

Conversation

skipkayhil commented Jun 18, 2022 • edited

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skipkayhil Jun 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skipkayhil commented Jun 18, 2022 • edited

skipkayhil commented Jun 18, 2022

jonathanhefner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanhefner commented Jun 20, 2022

skipkayhil commented Jun 18, 2022 •

edited

skipkayhil Jun 18, 2022 •

edited

skipkayhil commented Jun 18, 2022 •

edited