Allow alternate adapters to be used for serializing/deserializing

[mhuggins]

I'm working on an app that is intended to work with session between Rails and Node.js. The problem is, Node.js can't read a Ruby marshaled object from the session with redis-store's current implementation.

Looking through old issues, I see I'm not the only one with this problem, as issues #77 and #79 were both opened with a similar purpose. The problem is that both of those implementations could not properly unmarshal Ruby objects.

This pull request does two things. First, it creates the concept of an "adapter". An adapter is anything that can respond to load and dump, and it defaults to a new wrapper class that just uses Marshal. (This is to allow for backward compatibility without existing users having to worry about changing their app code.)

Second, it introduces a new JSON adapter. This adapter serializes Hash, Array, String, Numeric, TrueClass, FalseClass, and NilClass objects into their appropriate JSON representations. Any other non-native JSON class is converted to a String via Marshal.dump. This allows other languages that are sharing the session to parse the JSON session data itself, simply ignoring the native Ruby objects that aren't needed.

Example usage:

# each of the following will create a new redis store object that uses the new Marshal adapter
Redis::Store.new
Redis::Store.new(:adapter => :marshal)  # symbol shorthand
Redis::Store.new(:adapter => Redis::Store::Adapters::Marshal)  # actual class
Redis::Store.new(:adapter => "Redis::Store::Adapters::Marshal")  # string name representing class

# each of the following will create a new redis store object that uses the new Json adapter
Redis::Store.new(:adapter => :json)   # symbol shorthand
Redis::Store.new(:adapter => Redis::Store::Adapters::Json)  # actual class
Redis::Store.new(:adapter => "Redis::Store::Adapters::Json")  # string name representing class

Here is an example of how easy it is to create a custom adapter:

module RedisYamlAdapter
  def self.dump(object)
    ::YAML.dump(object)
  end

  def self.load(string)
    ::YAML.load(string)
  end
end

Redis::Store.new(:adapter => RedisYamlAdapter)

Of course, we didn't really even need to create an adapter for this very simple scenario. We could use the YAML class as is since it already responds to load/dump:

Redis::Store.new(:adapter => YAML)

And it's just as easy to integrate into your Rails app:

# Rails 3 example
MyApp::Application.config.session_store :redis_store, :servers => { :adapter => :json }

I would love to hear feedback on the approach. Some new unit tests could also be used (along with updated documentation), but I wanted to ensure you're open to the idea before moving forward with coding anything more into this branch. Thanks!

[jodosha]

Thanks for opening the discussion, this is an intersting topic. Here my considerations: first of all I don't like the idea of having ActiveSupport as dependency, just for #constantize. This will force developers to install this gem for a secondary feature, and also could prevent them to decide the Rails version the want to use (in the patch you're automatically excluding all the < 3.2.2 apps). A solution can be borrowing that code and consider it as a Ruby ext.

Second, does this JSON serializer guarantees ActiveRecord or Ruby objects to be transparently serialized/unserialized, including their internal state? Could be convenient to implement something similar to MongoId, where there is a more structured representations of the documents? I'm afraid that just invoking #to_json, isn't enough.

Third, by implementing this feature, I would like to see a little refactoring. Marshalling shound became an internal adapter (strartegy, if you will) of a more generic Serializer class. The _extend_marshalling should disappear, all the data will pass thru the Serializer, that by default will have a NoOp strategy, or, according to the conf, will change in marshalling/json/yaml/whatever.

Fourth, how to specify the adapter in the case of the configuration URI? Maybe redis+json:// ?

[mhuggins]

@jodosha - Thanks for the quick feedback, I really appreciate it.

After submitting this pull request and thinking about it in bed last night, I had some of the same thoughts about requiring ActiveSupport, as you mentioned. I starting thinking that it would make more sense to take that part out, not worry about constantizing strings, and only allow actual classes/objects that support dump/load to be supplied as values for the adapter param. This would allow the ActiveSupport dependency to be removed altogether.

There is a concern with this JSON serializer that if you have a Hash where the keys are strings, then you serialize/deserialize, you'll end up with a Hash where the keys are symbols. Strings will not have internal state information such as @html_safe instance variables being saved. Other objects that are extensions of Hash or Array will also lose any internal state they have prior to internal serialization with the current implementation. I''m not sure if there's really a way around this, unless a fake value is added to the serialized Hash/Array that can be interpreted back. I feel like that's kind of a hack though, personally.

I was wondering while coding this if the _extend_marshalling call should disappear, so I'm glad you mentioned that. Could you supply a little info on what you have in mind for the Serializer? Will it just be a placeholder class/module with dump/load methods that the other classes will extend or include? Also, why include a NoOp strategy as default when users currently expect Marshaling by default?

With regards to the configuration URI, I'm not familiar with this at all. Is this something Redis supports by default, or is it exposed by the gem? I haven't worked with Redis in enough detail to know more about it, unfortunately.

Thanks! :)

[mhuggins]

Alright, I went ahead and made some additional changes to this from what you suggested. It's using the verbiage "strategy" instead of "adapter" now. There are four possible options built in:

• :marshal (default): uses Ruby's built-in Marshal dump/load methods
• :json: builds and parses JSON in the manner I described in my original comment
• :yaml. builds and parses YAML
• false: acts the same as :marshalling => false previously did. (I removed the :marshalling option in the process.)

I removed the ActiveSupport dependency, and I also added tests for the new strategies.

Let me know what you think so far, and also if you could provide more insight about configuration URI's, that would be great so I could maybe take a look at that as well.

Lastly, if you decide you want to pull this in after all is said and done, I can squash the commits first if needed. Just let me know! Thanks again :)

[mhuggins]

Hmm, I seem to be getting a "Session collision" error quite a bit now, even though I didn't change any of the underlying code. I'll make sure it's nothing I've changed, but if you see anything that might have caused an issue in my changes, please let me know!

[travisbot]

This pull request fails (merged ca5079a into 6644a83).

[mhuggins]

Well, I fixed the errors I had previously, which related to the session collisions. However, I now have an issue with 'json' only being available in Ruby 1.9+, but not Ruby 1.8 or non-MRI Rubies it seems. I suppose I could do the following, though I could use some input from you on how you might want this to go.

In the redis-store.gemspec file:

s.add_dependency 'json',  '>= 1.7.0' if RUBY_VERSION < '1.9'

In the json.rb strategy module:

require 'rubygems'
require 'json'

Thoughts?

[mhuggins]

@jodosha - Just wanted to follow up and see if you have any input regarding my above questions. I would love to follow through on this and ideally have it pulled into the redis-store gem if that's an option (after ironing out the questions and json gem issue).

[mhoran]

@mhuggins, the problem with the RUBY_VERSION requirement in the gemspec is that the requirement gets compiled when the gem is pushed, and we'll only push the gem once so it will get compiled with whatever Ruby we're running at build time. Perhaps you could look into how Rails works around this?

I'm not sure we need to be too concerned about the loss of internal state resulting from serialization. This should be left as a caveat to those choosing an alternative marshaling strategy. The default state, however, should maintain internal state to preserve backwards compatibility.

Regarding URI configuration, perhaps we could pass the strategy (and possibly future options) as "query parameters" in the URI. For example, "redis://localhost/namespace?strategy=json".

[jodosha]

Sorry guys, just back from holidays.
I think that specifying the strategy as query string is an elegant solution, but I'm still not convinced about the loss of internal the state. For instance flash messages which are containing some markup declared secure with html_safe or memoized vars for object cached with Rails.cache, will be lost after the JSON is being loaded from the store.

[mhuggins]

@jodosha - Given that feedback, I wonder then if a simpler approach could be implemented compared to what I have here. Consider this:

• remove all strategy classes from my current pull request
• keep the idea of a strategy, but instead of passing in a symbol that represents the strategy to use, pass in an object reference that responds to dump/load (e.g.: pass Marshal instead of :marshal).

That way, redis-store does not have to be concerned with issues such as the html_safe issue when it comes to serializing JSON, as you pointed out. I can optionally create my own JSON encoder and pass that as the strategy option if I prefer.

[mhuggins]

Then again, that would make it difficult to handle the "query parameter" idea in the URI configuration.

[hayesgm]

Just FYI, I created a NodeJS module a while ago to unmarshall Ruby serialized data. This could be a quick-fix alternative to the problem presented in this thread: https://gist.github.com/4417808

[alainbloch]

im surprised this wasn't accepted.

@jodosha - im wondering why you care so much if JSON doesn't properly de/serialize objects in session. People shouldn't be saving objects into session anyways. The REAL problem is that redis-store only uses ruby Marshal which is a non-standard way of serializing data. You shouldn't care what strategy/adapter people use to load/dump into redis. If a developer creates a crazy adapter and it breaks their app, then it breaks their app.

[alainbloch]

This won't work. You need to add an asterisk so the MARSHAL_INDICATORS Array is used as multiple arguments for the star_with? method :

object.start_with?(*MARSHAL_INDICATORS)

I suggest adding a test for it. I noticed that there wasn't a test covering marshalling Ruby Marshalled strings from the store.

[mhuggins]

You're right, good catch. It could use some more tests, but it doesn't seem like this pull request is going anywhere for me to commit the time.

[alainbloch]

Well, if it means anything to you, Im using your code on a fairly large website. ;) Thanks for taking the time those months ago to do this.

[mhuggins]

😄 👍

[steventen]

+1

This is a good feature, JSON is a standard way when the session is not used only in rails/ruby

[Gargron]

👍

[radar]

Hi @mhuggins.

There seems to be some interest in getting this PR merged still. If so, can you please update it to make it mergeable with the current master?

Anyone else is welcome also to take this PR and do the same and just open a new PR.

Thanks!

[mhuggins]

Sure thing, I'll try to take a look this weekend. :)