redis-store / redis-actionpack

Redis stores for ActionPack
http://redis-store.org/redis-actionpack
MIT License
76 stars 44 forks source link

Incompatible with rails_admin 1.4.x #22

Closed PikachuEXE closed 6 years ago

PikachuEXE commented 6 years ago

Just updated to rails_admin 1.4.2 and got this error on start up:

Required middlewares for RailsAdmin are not added
To fix tihs, add

  config.middleware.use ActionDispatch::Session::RedisStore, {:redis_server=>{:host=>"127.0.0.1", :port=>6379, :password=>nil, :namespace=>"spacious:development:sessions"}, :key=>"_spacious_development_sessions_v2017_01_04_1426"}

to config/application.rb.

error raised from https://github.com/sferik/rails_admin/blob/v1.4.2/lib/rails_admin/engine.rb#L46

Not sure if this is this gem's issue or rails_admin's issue

tubbo commented 6 years ago

This seems due to the fact that ActionDispatch::Session::RedisStore is actually a subclass of Rack::Session::Redis, not ActionDispatch::Session::AbstractStore.

This line right here should return true because the middleware is indeed a session store which follows the entire API, but it's testing whether it's a direct subclass of the AbstractStore, which it is not, thus it fails.

The solution here is to extract the common functionality from Rack::Session::Redis into modules, allowing both Rack::Session::Redis and ActionDispatch::Session::AbstractStore to share the same functionality. I'll make an issue on redis-store/redis-rack that accompanies this one.

tubbo commented 6 years ago

Upon retrospect I think there’s a simpler fix that was eluding me at first. I’ll write more about it in an upcoming PR to redis-rack, and close the current one I have open.

On Oct 7, 2018 at 10:04 PM, <PikachuEXE (mailto:notifications@github.com)> wrote:

Just updated to rails_admin 1.4.2 and got this error on start up:

Required middlewares for RailsAdmin are not added To fix tihs, add config.middleware.use ActionDispatch::Session::RedisStore, {:redis_server=>{:host=>"127.0.0.1", :port=>6379, :password=>nil, :namespace=>"spacious:development:sessions"}, :key=>"_spacious_development_sessions_v2017_01_04_1426"} to config/application.rb.

error raised from https://github.com/sferik/rails_admin/blob/v1.4.2/lib/rails_admin/engine.rb#L46

Not sure if this is this gem's issue or rails_admin's issue

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub (https://github.com/redis-store/redis-actionpack/issues/22), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAG5grSEfFrL3XEDYCEA7uLNhxmllCJ1ks5uirKUgaJpZM4XMJ-X).

tubbo commented 6 years ago

Looking at Dalli's implementation of Rack::Session, I believe the problem has more to do with how RailsAdmin is choosing to figure out which of the middleware is your session store. Instead of testing whether the middleware inherits from some base class, RailsAdmin could be looking for a middleware with the class name "ActionDispatch::Session::#{camelized_session_store_name_from_rails_config}", and then it wouldn't matter what the inheritance chain of that middleware was. Additionally, there's nothing preventing two session stores from being in the Rails middleware chain, technically, so this fix doesn't actually solve for all edge cases.

I don't think there's anything to be done in redis-actionpack, and instead the issue should be raised as to how rails-admin is testing for session store middleware in the chain. Gonna close this unless you feel otherwise.

mshibuya commented 6 years ago

@tubbo Thanks for the suggestion, I've updated the middleware check logic accordingly.