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

A JSON serializer should be included by default #194

Closed
batter opened this issue Jan 22, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@batter
Copy link
Collaborator

commented Jan 22, 2013

Here is roughly what it would look like:

# lib/paper_trail/serializers/json.rb
require 'active_support/json'

module PaperTrail
  module Serializers
    class Json
      def self.load(string)
        ActiveSupport::JSON.decode string
      end

      def self.dump(hash)
        ActiveSupport::JSON.encode hash
      end
    end
  end
end

Or

# lib/paper_trail/serializers/json.rb
require 'json'

module PaperTrail
  module Serializers
    class Json
      def self.load(string)
        JSON.parse string
      end

      def self.dump(hash)
        hash.to_json
      end
    end
  end
end

@dwbutler - Can you elaborate a bit more on what you mean when you say "It's also more portable (for example, it will automagically pick up MultiJSON.)"? I don't see ActiveSupport::JSON listed as one of the supported JSON engines for MultiJSON (JSON is though). Just curious as to what you think the pro's and con's are of using ActiveSupport::JSON vs. the default JSON gem?

Perhaps it makes sense to just bundle MultiJSON into the library, although, I'm a bit weary of doing that when this will not be the default parser for PaperTrail (at least for now).

@ghost ghost assigned batter Jan 22, 2013

@bradleypriest

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2013

I thought about this when I put together the custom serializer code and my conclusion was that changing the serializer was a really uncommon use-case.
Adding the hooks had no downside, but adding an extra serializer to the gem itself IMHO was not really worth it.

My original usecase was to serialize the object_changes into an hstore for Postgres, but on further investigation, it doesn't really quite work straight out of the box as the hash values are all arrays. Having the keys usable is still useful though.

I'd love to hear more usecases, particularly for a JSON serializer. Would we save space by using JSON over YML?

@batter

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 22, 2013

Surprisingly, using JSON over YAML doesn't seem to save space. The strings generated from a JSON dump generally tend to be a bit longer (unless the hash is tiny) due to all the internal quotation marks used in JSON strings.

One argument for the case of providing a JSON parser is the confusion surrounding the fact that Ruby 1.9.2 and 1.9.3 provide two different YAML parsers that behave slightly differently. Due to the confusion that results from this, some users may wish to avoid YAML entirely.

Personally, I haven't run into any issues, but I am confused about what scenario's it still makes sense to use Syck over Psych (seemingly not many unless you are using the Delayed::Job gem but still). I kind of feel like Psych may have been pushed into production as the default YAML parser for Ruby a little bit sooner than it should have been but I don't really understand enough about the situation to speak with confidence.

I feel this is a legitimate argument for providing a JSON serializer.

@dwbutler

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2013

ActiveSupport::JSON vs JSON gem

Using ActiveSupport::JSON has several advantages over using the JSON gem or MultiJSON.

  • The JSON gem is not included in Ruby 1.8.7 by default, so this would introduce an extra dependency in Paper Trail.
  • As I found out by experiment, ActiveSupport::JSON correctly serializes HashWithIndifferentAccess in all platforms, whereas the JSON gem does not.
  • If MultiJSON is installed, ActiveSupport::JSON will pick it up and use it. This allows the user to plug in any library they want, without PaperTrail needing to explicitly depend on MultiJSON.

JSON vs YAML

The primary reasons I usually pick JSON over YAML are:

  • Serializing/deserializing JSON is much faster than YAML.
  • JSON is more portable to non-ruby environments.
  • JSON is human-readable; YAML, not so much.
  • YAML has security concerns due to the ability to embed ruby code.
  • In many applications, saving a little bit of space is not a huge concern.
@batter

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 22, 2013

Thanks a lot for the thoughtful response! Those are all good reasons that make a better case for ActiveSupport::JSON. I almost always run with 1.9 in my local environment so I probably wouldn't have realized the JSON is not included in 1.8.7 until the travis builds started failing.

I poked around in the ActiveSupport::JSON code and noticed they use MultiJSON for parsing, but not for dumping. After doing a fair amount of research on this and not finding obvious answers, I think I've concluded that parsing of JSON is much more resource-intensive than dumping, is that a fair assessment? I guess I was just curious as to why they don't use MultiJSON for dumping as well, but I'm assuming they made their own implementation in order to provide all of the nice features that can be used in the options argument of their dump method?

I also agree that a bit of space saved in the DB is not a huge concern, especially when serializing/deserializing are much faster (not to mention the other reasons you prefer JSON). These all seem like reasons enough for me (to warrant inclusion of JSON serializer in the gem by default)!

@dwbutler

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2013

That's interesting... I didn't realize ActiveSupport::JSON wasn't using MultiJson to serialize to JSON. I'm not sure what the reason is, but I'm sure the history of it must be convoluted. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.