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

Make low level session methods public #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsdalton
Copy link

@jsdalton jsdalton commented Jan 3, 2017

Recently, while writing some code to access session data at a low level, we ran into an issue with RedisSessionStore because it defines the following low level session methods (and aliases) as private:

  • #get_session
  • #find_session
  • #set_session
  • #write_session
  • #destroy_session
  • #delete_session

These methods are marked as private in the various inheritance trees of RedisSessionStore (via ActionDispatch::Session::AbstractStore):

  • In Rails 4, which uses Rack 1.6, the low level session methods are defined (and marked as private) in Rack::Session::Abstract::ID
  • In Rails 5, which uses Rack 2.0 the low level session methods are defined (and marked as private) in Rack::Session::Abstract::Persisted.

So at first glance, it seems that RedisSessionStore is correct in maintaining the private visibility level.

The problem, however, is that the vast majority of real world session store implementations override the parent visibility settings and mark these low level session methods as public.

A non exhaustive list of Rack-based session stores in the wild where these methods are public, not private:

As a counter example, ActiveRecordSession is the one session store I could find in the wild that preserves private visibility for these methods in https://github.com/rails/activerecord-session_store/blob/master/lib/action_dispatch/session/active_record_store.rb#L68-L120)

In short, RedisSessionStore's marking these methods as private, though consistent with its inheritance tree in both recent versions of Rails, is an outlier in the real world of session store implementations, including all of the persistent stores that are bundled with Rails.

The attached PR simply moves the low-level session methods to the public area of the RedisSessionStore class definition and adjusts the tests accordingly (i.e. so that they use the now public methods instead of send).

In addition to being consistent with other session store APIs in the wild, it's very helpful to be able to have proper access to the low level session store outside the context of a specific request. 😄

@iggant
Copy link
Contributor

iggant commented Oct 2, 2020

Hi, this is very important commit. @jsdalton can you please rebase it.
With master branch of rails we are receiving error

NoMethodError (private method delete_session' called for #RedisSessionStore:0x0000555c9eed60c0):`

and making methods public resolve this.

@iggant
Copy link
Contributor

iggant commented Oct 9, 2020

@jsdalton can you please rebase your branch and resolve conflict

@iggant
Copy link
Contributor

iggant commented Oct 21, 2020

@Jesterovskiy can you please review and change this PR?

iggant added a commit to iggant/redis-session-store that referenced this pull request Nov 20, 2020
Inspired by roidrage#83 

But also with Rails 6.1.0.rc1 there is  error 
NoMethodError (private method delete_session' called for #RedisSessionStore:0x0000555c9eed60c0):`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants