Add configurable serializer #61
Conversation
Credit to [redis-session-store](https://github.com/roidrage/redis-session-store/blob/master/lib/redis-session-store.rb) for the auto-migrating serialization scheme.
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Huh, the Travis build fails with the Rails 4.0 Gemfile: But the same method Any tips? |
|
@rafaelfranca Howdy, Rafael. I'm not sure why that Travis build is failing, can you give me a pointer as to what I should look at to fix it? |
|
Just my humble suggestion: Instead of using cattr_accessor, you could do this inside the module. This will work. I am using something similar in one of my projects that use Rails 4. class << self
attr_accessor :serializer
endHope it helps :) I would really like to see this merged :D |
|
Thanks, @bogdanRada, but Your suggestion did prompt me to look around a bit more and I found Updated. |
|
Ugh. The ruby-2.2 build failed trying to install rack: ¯_(ツ)_/¯ The ruby-head build failed failed because of : But locally I get a failure with ruby-head because of a deprecation warning: |
|
Can you update |
|
No change, but those are classified by Travis as Allowable Failures…so, good? |
|
If not, I'll keep putzing around with things. |
|
I know this is a prerelease, but i have been trying to use this in one of my application, because i am trying to implement something and i need the sessions to be in JSON format , however it seems the deserialization when using JSON always returns hash with string keys. Or maybe that is supposed to be happening? |
|
@bogdanRada Glad you're using it—the best way to good software is early feedback. A hash is indeed the intent (you're getting data returned from JSON.parse()), as that that is the opposite of serialization for JSON storage (JSON.generate()). |
|
Yeah. I know about that. Maybe i wasn't clear...i was trying to say if maybe could be possible when deserializing JSON to return a HashWithIndifferentAccess from ActiveSupport or something similar or is this intended behaviour? The reason why i am asking because in most of my applications i use symbol keys in hashes but since now the deserialization from JSON always returns string keys it breaks everything and i have to refactor and i would really want to see if there could be a solution for this. Or maybe there are some security concerns regarding this? |
|
Hmm, I played around with HWIA and it wasn't as simple as magicking everything we get back into HWIA.new() (namely, if the data is just a string, it's not a hash to be put into a hash). Since you have a use case, could you give this change a try? Maybe it's just storing the object type alongside the object for later reinflation. I'm not well versed in Ruby serialization. |
|
well i was able to solve this using this patch: require 'json'
require 'active_support/core_ext/hash/keys'
ActiveRecord::SessionStore::ClassMethods::JsonSerializer.class_eval do
singleton_class.send(:alias_method, :original_load, :load)
singleton_class.send(:alias_method, :original_dump, :dump)
def self.load(value)
hash = JSON.parse(value, quirks_mode: true).with_indifferent_access
hash[:value]
end
def self.dump(value)
JSON.generate({ value: value }, quirks_mode: true)
end
endThis way you can return a HashWithIndifferentAccess , and it really does not matter what type the variable value has. I tested this with my own application and it does work, but would be nice if this could be part of the gem itself . Let me know what you think This gem is dependent on actionpack, which has dependency to activesupport, so activesupport can be used easily. If the type of the value variable is something else than Hash(or object ), same type will be returned, but if the value is a Hash, or an object than when you will deserialize will return a HashWithIndifferentAccess, which will solve this problem. Or would be even better if we could use the gem 'oj'* (or maybe some other gem like yajl-ruby which seems faster and was part of Rails core until got removed rails/rails#13068 or we could use multi_json which seems now to be in the Rails core ) for JSON serializing , because that will handle also nested objects . But that is just a suggestion. |
|
I tested also with multi_json gem using this patch: require 'multi_json'
ActiveRecord::SessionStore::ClassMethods::JsonSerializer.class_eval do
singleton_class.send(:alias_method, :original_load, :load)
singleton_class.send(:alias_method, :original_dump, :dump)
def self.load(value)
hash = MultiJson.load(value)
hash[:value]
end
def self.dump(value)
MultiJson.dump({ value: value })
end
endand this already returns a HashWithIndifferentAccess (if the value variable is a Hash or object ) and handles also nested objects. I would suggest we stick with this. I tried using gem 'oj' also, but that kinda has some issues when i tested locally . I haven't tested though with yajl-ruby, because this seems to work really good. I had in session nested objects and it did serialize them correctly. The previous patch was not handling nested objects correctly |
|
@bogdanRada Thanks for clarifying. I continued to use JSON since it's been in core; my Ruby doesn't have multi_json. |
|
Yes but since this gem is part of Rails projects and Rails has multi_json in his core would be better if this would be used here also. And like i said earlier JSON from Ruby does not serialize correctly nested objects and since sessions often do contain nested objects would be really nice if this would be fixed now otherwise other people will rely on this gem to make the serialization properly and this issue will continue to arrive unless is fixed in a early stage before releasing this changes. Would you not agree with this? And multi_json is a gem, could be easily added as dependency in the gemspec file. This way will work even with non-Rails projects But if you dont consider this serialization issue important i am ok with this . I guess i can always fork the project and make my own patches although i would like to avoid that. That's why i felt obligated to report this issue before was released and i have tried to provide solutions because i really want this to be merged as soon as possible. It is indeed a awesome work and i really want to use this in production. |
|
Nope I definitely consider it important—the problems are simply my lack of understanding & experience in this area. I did this, but end up with intermittent test failures: Any ideas? |
|
Have you tried adding multi_json as a runtime dependency to the gemspec file? That should solve your problem |
|
I finally figured it out. Here are all my changes: From 3324b64025836134ecfef2270e40a1d0b2129c7f Mon Sep 17 00:00:00 2001
From: Rada Bogdan Raul <raoul_ice@yahoo.com>
Date: Tue, 19 Jan 2016 09:20:11 +0200
Subject: fix tests
diff --git a/Rakefile b/Rakefile
index 04cdda8..a28d89d 100644
--- a/Rakefile
+++ b/Rakefile
@@ -1,7 +1,9 @@
#!/usr/bin/env rake
+require 'bundler/setup'
require "bundler/gem_tasks"
require 'rake/testtask'
+
Rake::TestTask.new do |t|
t.libs = ["test"]
t.pattern = "test/**/*_test.rb"
diff --git a/activerecord-session_store.gemspec b/activerecord-session_store.gemspec
index b12b947..e158cd3 100644
--- a/activerecord-session_store.gemspec
+++ b/activerecord-session_store.gemspec
@@ -20,6 +20,7 @@ Gem::Specification.new do |s|
s.add_dependency('activerecord', '>= 4.0.0', '< 5')
s.add_dependency('actionpack', '>= 4.0.0', '< 5')
s.add_dependency('railties', '>= 4.0.0', '< 5')
+ s.add_dependency('multi_json', '~> 1.11', '>= 1.11.2')
s.add_development_dependency('sqlite3')
s.add_development_dependency('appraisal')
diff --git a/lib/active_record/session_store.rb b/lib/active_record/session_store.rb
index 731f6a5..7d647de 100644
--- a/lib/active_record/session_store.rb
+++ b/lib/active_record/session_store.rb
@@ -1,7 +1,7 @@
require 'action_dispatch/session/active_record_store'
require "active_record/session_store/extension/logger_silencer"
require 'active_support/core_ext/hash/keys'
-
+require 'multi_json'
module ActiveRecord
module SessionStore
module ClassMethods # :nodoc:
@@ -57,12 +57,12 @@ module ActiveRecord
# Uses built-in JSON library to encode/decode session
class JsonSerializer
def self.load(value)
- hash = JSON.parse(value, quirks_mode: true).with_indifferent_access
+ hash = MultiJson.load(value).with_indifferent_access
hash[:value]
end
def self.dump(value)
- JSON.generate({value: value}, quirks_mode: true)
+ MultiJson.dump(value: value)
end
end
@@ -78,6 +78,7 @@ module ActiveRecord
end
end
+
def self.needs_migration?(value)
value.start_with?(MARSHAL_SIGNATURE)
end
diff --git a/test/action_controller_test.rb b/test/action_controller_test.rb
index 9db3d6a..0aaa6b9 100644
--- a/test/action_controller_test.rb
+++ b/test/action_controller_test.rb
@@ -2,6 +2,7 @@ require 'helper'
class ActionControllerTest < ActionDispatch::IntegrationTest
class TestController < ActionController::Base
+ protect_from_forgery
def no_session_access
head :ok
end
diff --git a/test/session_test.rb b/test/session_test.rb
index 1a9a6aa..e904e34 100644
--- a/test/session_test.rb
+++ b/test/session_test.rb
@@ -13,6 +13,7 @@ module ActiveRecord
ActiveRecord::Base.connection.schema_cache.clear!
Session.drop_table! if Session.table_exists?
@session_klass = Class.new(Session)
+ ActiveRecord::SessionStore::Session.serializer = :json
end
def test_data_column_name
@@ -38,7 +39,8 @@ module ActiveRecord
s = session_klass.create!(:data => 'world', :session_id => '7')
sessions = ActiveRecord::Base.connection.execute("SELECT * FROM #{Session.table_name}")
- assert_equal JSON.generate({value: s.data}, quirks_mode: true), sessions[0][Session.data_column_name]
+ data = Session.deserialize(sessions[0][Session.data_column_name])
+ assert_equal s.data, data
end
def test_hybrid_serialization
@@ -54,7 +56,7 @@ module ActiveRecord
# Check that first was serialized with Marshal and second as JSON
sessions = ActiveRecord::Base.connection.execute("SELECT * FROM #{Session.table_name}")
assert_equal ::Base64.encode64(Marshal.dump(s1.data)), sessions[0][Session.data_column_name]
- assert_equal JSON.generate({value: s2.data}, quirks_mode: true), sessions[1][Session.data_column_name]
+ assert_equal s2.data, Session.deserialize(sessions[1][Session.data_column_name])
end
def test_hybrid_deserialization
@@ -77,7 +79,7 @@ module ActiveRecord
# and reserializes as JSON
session.save
sessions = ActiveRecord::Base.connection.execute("SELECT * FROM #{Session.table_name}")
- assert_equal JSON.generate({value: s.data}, quirks_mode: true), sessions[0][Session.data_column_name]
+ assert_equal s.data,Session.deserialize(sessions[0][Session.data_column_name])
end
def test_find_by_sess_id_compatNow i get zero failures: # Running:
.......................................
Finished in 0.279490s, 139.5396 runs/s, 468.7100 assertions/s.
39 runs, 131 assertions, 0 failures, 0 errors, 0 skips
Tested several times, and now it works , and i have also tested this changes with my own application and it works |
|
@dinomite have you tried to apply the changes i showed you? Those changes were made using your branch and it does fix the problem with the tests |
|
@bogdanRada Have not yet, sorry I hadn't responded. I'll get to it today, thanks for keeping on this! |
From bogdanRada's patch rails#61 (comment)
|
Sweet, that works, you were right about the missing runtime dep, and thanks for making the tests better, too! I think this is ready now. |
|
@dinomite could make just one small change to the deserialize method in JsonSerializer ? Like this: def deserialize(data)
hash = Marshal.load(value)
If hash.is_a?(Hash)
hash[:value].with_indifferent_access
else
hash
end
endThis is because if you are unauthenticated. The data argument will be NIL and will fail when trying to call method with_indifferent_access Other than that everything is working fine . i did some intensive testing and found only this issue. Thank you very much for being so patient . i really appreciate it. |
|
@bogdanRada Thank you for the insights and testing! Updated. @rafaelfranca This is ready. |
|
You're welcome. I am glad i was helpful :) |
|
Hey @rafaelfranca —does this PR need anything else to be merged? |
|
@rafaelfranca I merged master back into this branch, so it's ready to merge (or for your comments) again. |
|
@sikachu Can you take a look at this and let me know if it needs anything else to be merged? |
| @@ -3,6 +3,7 @@ | |||
| .bundle | |||
| .config | |||
| .yardoc | |||
| .idea | |||
There was a problem hiding this comment.
I don't think this line should be in your patch.
- Remove duplicate line in sql_bypass.rb - Rename determine_serializer as serializer_class for more natural sounding calls
|
@sikachu Thanks for the comments, updated! |
|
@sikachu Anything else needed here? |
|
I think this is good. I'm holding off merging stuff into |
|
Great, thanks for the update! |
|
@sikachu Any thoughts of a new release? I'm suddenly interested again—I had originally written this up just for debugging, but now I want to share sessions with a non-Ruby system and this would make that super easy. |
|
Recently migrated to redis and do not wait a miracle |
|
@sikachu Is this going to wait for Rails 5.0.1? |
Add redis-session-store inspired JSON serialization.