Skip to content
This repository

psych doesn't handle instance variables in subclasses of Hash #43

Open
Fryguy opened this Issue · 9 comments

4 participants

Jason Frey Marcus Vorwaller Joe Rafaniello Aaron Patterson
Jason Frey

Psych.dump of a subclassed Hash with attributes loses those attribute values.

Failing test_hash.rb patch: https://gist.github.com/1482361

Marcus Vorwaller

For example:
Psych.dump({}.with_indifferent_access)
=> "--- {}\n"

Joe Rafaniello

@tenderlove Any idea on how you're thinking this may be fixed? I'm willing to monkey patch my own code but would like to do it in a way that won't be too painful to change once it's fixed in future versions of ruby.

Aaron Patterson
Owner

I'm not totally sure how to fix this one. I have to come up with a way to serialize instance variables without interfering with the hash serialization. TBH, I'm starting to lean towards "psych doesn't support hash subclasses".

Jason Frey
Fryguy commented

That's unfortunate. Also unfortunate is that the fix in issue 14 makes it more difficult to implement since it's already released in Ruby 1.9.3, and would probably require breaking that format if you wanted to do it more like what was done in issue 44.

At our company, while on Ruby 1.8.7, we had the same subclassing issue, so we hacked it by serializing the instance variables as just more hash entries with the key prefix __iv__. Then we hacked deserialization to remove any __iv__ prefixed entries and just set the instance variables directly. It's kind of ugly but it worked. I wanted to avoid doing any hacking like that for Ruby 1.9, as it seems like an actual psych issue that it can't handle subclasses.

Aaron Patterson
Owner

@Fryguy do you care what the actual serialized output is? The only nice solution I can think of is to basically wrap hash subclasses with ivars in a different object that knows how to serialize and deserialize the hash.

The problem is differentiating a hash key from a special key for ivars. For example, what if someone actually set a key on their hash of __iv__? What would the behavior be then?

Jason Frey
Fryguy commented

@tenderlove I only care about the serialized output because we have to store these structures in an ActiveRecord serialized column, and we will deserialize these later. Without getting into too much detail, the instance variables are needed because they store instance information about things like what type of Hash, where the Hash came from, etc. I guess we could change it use composition instead of inheritance, but that would affect a lot of code paths for us, particularly in how we use these objects when communicating with a third party API.

Your point about the potential hash key collisions is the exact reason I say it's an ugly hack. For us, it works out because in the subclassed Hashes we have, I know we won't have keys that will start with __iv__. For a general psych fix, you can't make that same assertion. I just thought I'd throw out there what we did in the past to get around it.

Aaron Patterson
Owner

@tenderlove I only care about the serialized output because we have to store these structures in an ActiveRecord serialized column, and we will deserialize these later. Without getting into too much detail, the instance variables are needed because they store instance information about things like what type of Hash, where the Hash came from, etc. I guess we could change it use composition instead of inheritance, but that would affect a lot of code paths for us, particularly in how we use these objects when communicating with a third party API.

I think if people don't care what the actual output YAML looks like, it should be possible to accomplish this.

Your point about the potential hash key collisions is the exact reason I say it's an ugly hack. For us, it works out because in the subclassed Hashes we have, I know we won't have keys that will start with iv. For a general psych fix, you can't make that same assertion. I just thought I'd throw out there what we did in the past to get around it.

Yup, totally understand. Thinking of how to solve the "general fix" is what's making me cringe. :cry:

Joe Rafaniello

No worries, the general fix is worth waiting for ;-) Feel free to share any thoughts or questions you have if you need to better understand our use case.

Joe Rafaniello

@tenderlove See this gist with our hack that works on 1.9.3p194's psych and on the current psych's master branch:
https://gist.github.com/2853827

It's not a solution but maybe it will give you some ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.