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

Problem with nil values in #reify #219

Merged
merged 1 commit into from May 13, 2013

Conversation

dwbutler
Copy link
Contributor

I'm using a custom serializer that does not store nil values. I noticed that the attributes are stored correctly, but then not correctly reified back. For example:

previous version data: {id: 123}
current version data:  {id: 123, name: "Bob"}

model
#=> <#Person id: 123, name: "Bob">

model.previous_version
#Expected => <#Person id: 123, name: nil>
#Actual => <#Person id: 123, name: "Bob">

After some digging, I found that the problem is in #reify. The current version of the model is reused, and each attribute from the previous version is set on the current model. However, since "name" is not a key in the previous version, name was left as "Bob."

This was not a problem in the default serializers provided. Because they store everything, both versions of the models have all the same keys. For example:

previous version data: {id: 123, name: nil, dob: nil}
current version data:  {id: 123, name: "Bob", dob: nil}

This patch compares the keys between the current model and the previous version. If any keys are found to be missing, it puts them in as nil. This allows those attributes to be set to nil on the reified model.

I updated the serializer tests so they separately test the built-in JSON serializer as well as a custom serializer which doesn't store nil keys.

@batter batter merged commit 0546800 into paper-trail-gem:master May 13, 2013
@dwbutler dwbutler deleted the nil_attribute_fix branch May 13, 2013 17:28
@dwbutler
Copy link
Contributor Author

Thanks for the merge!

Just curious, do you plan to release a new version of the gem soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants