Fix json serialization of nested hash when using root elements with a Grape::Entity presenter #181

Merged
merged 8 commits into from Jul 17, 2012

Conversation

Projects
None yet
5 participants
@benrosenblum
Contributor

benrosenblum commented Jun 4, 2012

Previously, when a root element is used with an Entity, the presenter
wraps the array of Entity objects in a plain hash, and the Entities end
up serialized as plain strings of the class name
(i.e., {"root":["#<EntityFoo...>","#<EntityBar...>"]})

The new code recursively checks for any elements of the hash that respond
to serializable_hash and calls it if present.

Correctly serializes nested hash elements when a root element is used…
… with an Entity.

Previously, when a root element is used with an Entity, the presenter
wraps the array of Entity object in a plain hash, and the Entities end
up serialized as plain strings of the class name
(i.e., {"root":["#<EntityFoo...>","#<EntityBar...>"])

The new code recursively checks for any elements of the hash that respond
to serializable_hash and calls it if present.
@dblock

This comment has been minimized.

Show comment Hide comment
@dblock

dblock Jun 12, 2012

Member

Could you please update the new CHANGELOG.markdown with this? I'll merge it. Thanks!

Member

dblock commented Jun 12, 2012

Could you please update the new CHANGELOG.markdown with this? I'll merge it. Thanks!

@benrosenblum

This comment has been minimized.

Show comment Hide comment
@benrosenblum

benrosenblum Jun 13, 2012

Contributor

There you go. Let me know if you need any changes!

Contributor

benrosenblum commented Jun 13, 2012

There you go. Let me know if you need any changes!

Grape::Entity#serializable_hash should call #serializable_hash if the…
… data returned from #value_for responds to that method so that nested Grape::Entity presentation works correctly.
@benrosenblum

This comment has been minimized.

Show comment Hide comment
@benrosenblum

benrosenblum Jun 22, 2012

Contributor

Kyle added some additional changes that will also correctly serialize nested Entity objects in addition to nested Hashes.

Contributor

benrosenblum commented Jun 22, 2012

Kyle added some additional changes that will also correctly serialize nested Entity objects in addition to nested Hashes.

lib/grape/middleware/base.rb
- MultiJson.dump(object.map {|o| o.serializable_hash })
- elsif object.respond_to? :to_json
+ serialized_hash = serialize_recurse(object)
+ if (object == serialized_hash) && (object.respond_to? :to_json)

This comment has been minimized.

Show comment Hide comment
@dblock

dblock Jun 23, 2012

Member

Could you explain the comparison here? I am also afraid that seriazlie_recurse, which is quite hefty for large JSONs, is going to run for every JSON output, then get thrown out.

@dblock

dblock Jun 23, 2012

Member

Could you explain the comparison here? I am also afraid that seriazlie_recurse, which is quite hefty for large JSONs, is going to run for every JSON output, then get thrown out.

This comment has been minimized.

Show comment Hide comment
@benrosenblum

benrosenblum Jun 25, 2012

Contributor

The idea is that if the objects supports serializable_hash, or is an array or hash of those objects, then we need to build a new array or hash of the results of the serializable_hash calls. Otherwise, we want the results of to_json as our final output. The comparison checks if the unmodified object has been returned, and is an equivalent to the final condition in the previous code. You're right that this is unclear, and I'll refactor it to have everything inside a single serialize_object method or similar.

It is hefty for deeply nested hashes, but how else can we ensure they are properly serialized?

@benrosenblum

benrosenblum Jun 25, 2012

Contributor

The idea is that if the objects supports serializable_hash, or is an array or hash of those objects, then we need to build a new array or hash of the results of the serializable_hash calls. Otherwise, we want the results of to_json as our final output. The comparison checks if the unmodified object has been returned, and is an equivalent to the final condition in the previous code. You're right that this is unclear, and I'll refactor it to have everything inside a single serialize_object method or similar.

It is hefty for deeply nested hashes, but how else can we ensure they are properly serialized?

This comment has been minimized.

Show comment Hide comment
@dblock

dblock Jun 26, 2012

Member

I think my biggest problem with this is that we call serialize_recurse every time, then compare the object to the result of the serialization. So I am not arguing the purpose, but the implementation has to be unwrapped into something that looks less obscure. Maybe something in this genre?

def serializable?(object)
   object.respond_to? :serializable_hash
     || object.kind_of?(Array) && !object.map {|o| o.respond_to? :serializable_hash }.include?(false)
     || object.kind_of?(Hash)
end

def serialize(object)
 # just like now
end

def encode_json(object)
 return object if object.is_a?(String)
 return MultiJson.dump(serialize(object)) if  serializable?(object)
 ...
end

I can take a stab at this if you don't find anything workable.

@dblock

dblock Jun 26, 2012

Member

I think my biggest problem with this is that we call serialize_recurse every time, then compare the object to the result of the serialization. So I am not arguing the purpose, but the implementation has to be unwrapped into something that looks less obscure. Maybe something in this genre?

def serializable?(object)
   object.respond_to? :serializable_hash
     || object.kind_of?(Array) && !object.map {|o| o.respond_to? :serializable_hash }.include?(false)
     || object.kind_of?(Hash)
end

def serialize(object)
 # just like now
end

def encode_json(object)
 return object if object.is_a?(String)
 return MultiJson.dump(serialize(object)) if  serializable?(object)
 ...
end

I can take a stab at this if you don't find anything workable.

This comment has been minimized.

Show comment Hide comment
@benrosenblum

benrosenblum Jun 26, 2012

Contributor

Right, I agree. Your implementation is much cleaner :) I'll try to find some time in the next day or two to tidy this up.

@benrosenblum

benrosenblum Jun 26, 2012

Contributor

Right, I agree. Your implementation is much cleaner :) I'll try to find some time in the next day or two to tidy this up.

Grape::Entity#serializable_hash should call #serializable_hash elemen…
…twise if the data returned from #value_for is an array for which each element responds to that method so that nested Grape::Entity presentation works correctly with arrays of values.
@twoism-dev

This comment has been minimized.

Show comment Hide comment
@twoism-dev

twoism-dev Jul 9, 2012

Is there any update on this one yet - I'm happy to give a patch a go, but if you are close I won't bother ;-)

Is there any update on this one yet - I'm happy to give a patch a go, but if you are close I won't bother ;-)

@benrosenblum

This comment has been minimized.

Show comment Hide comment
@benrosenblum

benrosenblum Jul 11, 2012

Contributor

Yeah, I'll try and get the last update to this pushed up tomorrow.

Contributor

benrosenblum commented Jul 11, 2012

Yeah, I'll try and get the last update to this pushed up tomorrow.

@benrosenblum

This comment has been minimized.

Show comment Hide comment
@benrosenblum

benrosenblum Jul 16, 2012

Contributor

There we go. Let me know if you need any other changes. Thanks!

Contributor

benrosenblum commented Jul 16, 2012

There we go. Let me know if you need any other changes. Thanks!

@@ -257,6 +257,50 @@
fresh_class.expose :name
expect{ fresh_class.new(nil).serializable_hash }.not_to raise_error
end
+
+ it 'should serialize embedded objects which respond to #serializable_hash' do
+ class EmbeddedExample

This comment has been minimized.

Show comment Hide comment
@dblock

dblock Jul 16, 2012

Member

You should know that these definitions become global in Ruby, and notably pollute other tests. Either use subject that gets cleared every time or move the classes into spec/support with a better defined name.

@dblock

dblock Jul 16, 2012

Member

You should know that these definitions become global in Ruby, and notably pollute other tests. Either use subject that gets cleared every time or move the classes into spec/support with a better defined name.

@dblock

This comment has been minimized.

Show comment Hide comment
@dblock

dblock Jul 16, 2012

Member

This looks like it's in the middle of #203, could you also please take a look at that one and comment? Appreciated.

Member

dblock commented Jul 16, 2012

This looks like it's in the middle of #203, could you also please take a look at that one and comment? Appreciated.

dblock added a commit that referenced this pull request Jul 17, 2012

@dblock dblock merged commit 18419c4 into ruby-grape:master Jul 17, 2012

@dblock

This comment has been minimized.

Show comment Hide comment
@dblock

dblock Jul 17, 2012

Member

I merged this with some minor spec changes. Thanks everybody.

Member

dblock commented Jul 17, 2012

I merged this with some minor spec changes. Thanks everybody.

@tobert

This comment has been minimized.

Show comment Hide comment
@tobert

tobert Aug 2, 2012

I was looking at adding pretty-print support for JSON and noticed:

It looks like :serializable? always returns true, which will force all hashes to be copied before serialization. It's not a huge deal, but I sometimes serve up multi-megabyte JSON docs where that would have a measurable cost.

https://github.com/intridea/grape/blob/master/lib/grape/middleware/base.rb#L116

Maybe I'm missing something? In any case that long boolean expression should have some grouping to make it easier to read.

tobert commented Aug 2, 2012

I was looking at adding pretty-print support for JSON and noticed:

It looks like :serializable? always returns true, which will force all hashes to be copied before serialization. It's not a huge deal, but I sometimes serve up multi-megabyte JSON docs where that would have a measurable cost.

https://github.com/intridea/grape/blob/master/lib/grape/middleware/base.rb#L116

Maybe I'm missing something? In any case that long boolean expression should have some grouping to make it easier to read.

@dblock

This comment has been minimized.

Show comment Hide comment
@dblock

dblock Aug 2, 2012

Member

@tobert => You are probably right about the serializable issue and aren't missing anything - the cost may be very high here. We've been increasingly calling to_json in our API to make results strings and avoiding automatic serialization. It also plays better with caching implementations, such as garner.

Feel free to make pull requests that make this better, including noop changes for readability.

Member

dblock commented Aug 2, 2012

@tobert => You are probably right about the serializable issue and aren't missing anything - the cost may be very high here. We've been increasingly calling to_json in our API to make results strings and avoiding automatic serialization. It also plays better with caching implementations, such as garner.

Feel free to make pull requests that make this better, including noop changes for readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment