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

Already on GitHub? Sign in to your account

Rack::Session remove inheritance from Hash #342

merged 6 commits into from Nov 3, 2012


None yet
5 participants

brainopia commented Feb 19, 2012

In discussions with @josevalim we've noticed that the current api provided by Rack::Session::Abstract::SessionHash is inconsistent. This class inherits a lot of methods directly from Hash and without redefinition they work incorrectly in case of unloaded session. For example , #replace, #each, #keys and a lot of others.

So in e5b4d96 I remove inheritance from Hash and define missing methods directly on SessionHash (since they were not working correctly previously anyway). This commit is backward-compatible. All rack, rails, sinatra, padrino tests are passing as they should.

But in this pull-request I've also made another change. There is Rack::Session::Abstract::OptionHash which inherits from Hash and redefines exactly one method [] in case we pass key :id there. Clearly it's not the best place to store session_id.

So in spirit of @evanphx I've removed OptionHash completely in 83a270d. Session options are now a simple hash and session_id is stored as instance variable of SessionHash (since the class is already tightly coupled to id). It promotes cleaner code and has advantage of being faster.

Since it's an implementation detail all rack, sinatra and padrino tests are still passing. But Rails tests use explicitly original location to test session. I've fixed them and made a few other changes we've discussed with @josevalim in brainopia/rails@8c9c4c0.

Still there is a possibility there may be some plugins which use such implementation detail to access session id directly and not through provided api. I think it's a risk worth taking. But I can provide pull-request with only SessionHash changes if needed.

brainopia added some commits Feb 18, 2012

@brainopia brainopia Remove OptionHash with options hash and SessionHash#id
There is no point in OptionHash to redefine exactly one method of Hash
to return session id. Session id belongs to SessionHash and by moving it
there we remove unnecessary class. This leads to better perfomance and
cleaner code.
@brainopia brainopia Use ENV_SESSION contants in #commit_session 740dbaa
@brainopia brainopia Make options accessible through SessionHash
It removes need to write long
@env[Rack::Session::Abstract::Id::ENV_SESSION_OPTIONS_KEY] by
abstracting it to the proper level.
@brainopia brainopia Remove SessionHash inheritance from Hash
Previous api provided by SessionHash was inconsistent. All methods
provided by Hash which were not rewritten in SessionHash were behaving
incorrectly when session was not loaded yet. For example, #replace, #each,
 #keep_if and many more. So by dropping Hash inheritance we provide
clean api with all methods working without regard to whether session was
loaded or not.
@brainopia brainopia SessionHash should be enumerable 564ba27
@brainopia brainopia Fix messed up identation a826b46

josevalim commented Feb 19, 2012

The only issue with removing the options hash is that I believe some people may expect session_options[:id] = "foo" to set the session id to foo. I am not sure if we have a test coverage for that, so we need to check it and add a test if not.

Overall, big 👍 but this should probably be part of a minor release. So I am unsure if I can merge this yet. @raggi ?


evanphx commented Feb 22, 2012

+1 here, but this probably should show up in 1.5 at least, not a bugfix of 1.4.


manveru commented Feb 26, 2012

+1 for 1.5

@raggi raggi added a commit that referenced this pull request Nov 3, 2012

@raggi raggi Merge pull request #342 from brainopia/e5b4d961e5cb310bffca18a2cf72a1…

Rack::Session remove inheritance from Hash

@raggi raggi merged commit 75b358c into rack:master Nov 3, 2012

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