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

Fixed issue with ActiveRecord serialize object as JSON #16059

Merged
merged 1 commit into from
Jul 5, 2014

Conversation

jenncoop
Copy link

@jenncoop jenncoop commented Jul 5, 2014

With 4.1.x serialize started returning a string when JSON was passed as the second attribute. It will now return a hash as per previous versions (prior to 4.1.x)

Example: An ActiveRecord model named Post with a comment column that uses the serialize method to convert a Comment object to JSON.

class Post < ActiveRecord::Base
serialize :comment, JSON
end

class Comment
  include ActiveModel::Model
  attr_accessor :category, :text
end

post = Post.create!
post.comment = Comment.new(category: "Animals", text: "This is a comment about squirrels.")
post.save!

# This will now return a hash
post.comment

When JSON is used with serialize, ActiveRecord will use the new ActiveRecord::Coders::JSON coder which delegates ActiveRecord::Coders::JSON.dump/load to ActiveSupport::JSON.encode/decode.

Fixes #15594

@chancancode chancancode added this to the 4.1.5 milestone Jul 5, 2014
@chancancode chancancode self-assigned this Jul 5, 2014
@chancancode
Copy link
Member

@jeremy @matthewd can one of you take a second look at this to see if it matches what you have in mind? If it seems fine with you we'll work on porting this to master.

@sgrif
Copy link
Contributor

sgrif commented Jul 5, 2014

👍 from me on this.

@@ -52,7 +52,9 @@ module ClassMethods
def serialize(attr_name, class_name_or_coder = Object)
include Behavior

coder = if [:load, :dump].all? { |x| class_name_or_coder.respond_to?(x) }
coder = if class_name_or_coder == JSON
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's not needed ::JSON may be nice here, to be very clear?

I think this probably also deserves a comment noting that we're consciously actually breaking our contract (by ignoring the #dump/#load present on the supplied object) to maintain better compatibility with 4.0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would anyone conceivably want to actually use the native ::JSON serializer which this implementation will suppress?

An alternate (not sure if good/even possible idea but just throwing out there) is to alias within ActiveRecord::Base namespace (or ActiveRecord in general, or Rails even more general) JSON to ActiveSupport::JSON, making users always use ::JSON if they want the native serializer.

@matthewd
Copy link
Member

matthewd commented Jul 5, 2014

👍

chancancode added a commit to chancancode/rails that referenced this pull request Jul 5, 2014
Fixed issue with ActiveRecord serialize object as JSON
@chancancode chancancode merged commit b1e30b3 into rails:4-1-stable Jul 5, 2014
chancancode added a commit that referenced this pull request Jul 5, 2014
Fixed issue with ActiveRecord serialize object as JSON

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/attribute_methods/serialization.rb
chancancode added a commit that referenced this pull request Jul 5, 2014
This reverts commit a030977.

This needs more work before it would work correctly on master.
chancancode added a commit to chancancode/rails that referenced this pull request Jul 15, 2014
…ain)

This reverts commit 6f3c64e.

Conflicts:
	activerecord/CHANGELOG.md
chancancode added a commit to chancancode/rails that referenced this pull request Jul 15, 2014
…ialized-attr""

This reverts commit 6f3c64e.

Conflicts:
	activerecord/CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants