Fix #to_json for BasicObject Enumerables #10278

Merged
merged 1 commit into from Mar 14, 2014

Projects

None yet

3 participants

@codeodor

Objects which include Enumerable already get the as_json method, but they do not get to_json.

That means it would get to_json from Object in most cases, however, not if they inherit from BasicObject.

You might think, "who does that?!" 😕 (and rightfully so).

That is just the simplest case where we'd have an issue. For a real-life use case, consider someone who uses SimpleDelegator and includes Enumerable. In that case, #to_json will forward on to the wrapped object, which means it will end up calling #as_json on each element in the wrapped collection, instead of each element in the collection we actually want to call it on.

Delegation is a much more common pattern than inheriting directly from BasicObject and we ran into this issue in DisplayCase.

Now, it could be the case that we should just say, "if you don't want to forward the #to_json message to the other object, then write a method to define it."

I thought about that a lot, but since as_json is already on Enumerable I thought #to_json probably should be as well. (Especially since there was already a test case for it, but it was not testing against a custom Enumerable, it was testing against an array of hashes).

(And we certainly don't want to pollute BasicObject with the method, even though it is the simplest test case I could come up with!)

In addition to testing the case I mentioned above, this pull request also adds tests to demonstrate:

  1. The classes to which Rails adds #to_json get the method. (On the face of it, I thought that's really not testing much, but it could be useful to catch regressions for objects that expect it to be defined).
  2. That we actually test (to|as)_json against an enumerable instead of a hash.

👍 ?

@chancancode
Ruby on Rails member

cc @jeremy

@jeremy
Ruby on Rails member

Good argument, @codeodor. This also casts some doubt on whether Enumerable should have #as_json at all. What do you think, @chancancode?

@chancancode
Ruby on Rails member

Sorry for the hold up guys, I have been repeatedly thinking about this for the last few days. It took me a while to wrap my head around the "BasicObject subclass that includes Enumerable" use case (I'm still not 100% sure if I understand it :P) and what's the developer's intent/expectation when they do that.

I think @codeodor's suggestion of adding the #to_json entry point to Enumerable seems to make sense, and I can't really think of much drawbacks. So I think I'm a 👍 on that.

As for whether Enumerable should have #as_json, I'm not too sure either. Besides this there are some other weird cases as well. For example, not every enumerable can be converted into an array. While I don't know what you can reasonable expect to happen when you try to serialize an infinite sequence, the current behaviour (infinite loop) is probably not ideal for a web application.

That being said, I don't think the theoretical concerns (I can't think of too many real world use case where defaulting the serialization behaviour to to_a is wrong) could justify the removal given the cans of worms it could potentially open. We'll have to add #as_json back to a lot of things that people depend on, like AR::Relation, and will probably miss many cases in the transition.

So, probably best to just patch this edge case and move on. Maybe Enumerable shouldn't have #as_json added in the beginning and people should have explicitly opt-ed in, but I tend to think it's too late to fix now.

@codeodor

So just to follow up on the discussion, does that mean this pull request is good to go, or should I make any changes?

@chancancode chancancode self-assigned this Mar 13, 2014
@chancancode
Ruby on Rails member

@codeodor sorry for the delay again :( I think this is good to go, do you think you'll have time to rebase (might be closer to a rework by now) against master?

@codeodor

No worries, I can probably do it tomorrow or Saturday. (I'm in US Central time zone) 😄

@chancancode
Ruby on Rails member

❤️ thank you so much

@codeodor

Rebased on current master!

@chancancode chancancode and 1 other commented on an outdated diff Mar 14, 2014
activesupport/test/core_ext/object_and_class_ext_test.rb
+
+class ObjectJSONTest < ActiveSupport::TestCase
+ class BasicEnumerable < BasicObject
+ include ::Enumerable
+ def initialize(values=[]); @values = values; end
+ def each; @values.each{ |v| yield v }; end
+ def is_a?(klass); false; end
+ end
+
+ def test_ruby_objects_should_respond_to_to_json
+ [Object, Array, FalseClass, Float, Hash, Integer, NilClass, String, TrueClass, BasicEnumerable].each do |klass|
+ instance = klass.respond_to?(:new) ? klass.new : klass
+ assert_nothing_raised { instance.to_json }
+ end
+ end
+end
@chancancode
chancancode Mar 14, 2014

Hmm, I don't see much value in duplicating the list here, seeing that we already tests the behaviour in encoding_test.rb, thoughts?

@codeodor
codeodor Mar 14, 2014

I'm ambivalent. I just thought it would be nice to catch regressions but it could be removed.

@chancancode
chancancode Mar 14, 2014

I think we should probably remove this but double check to make sure the behaviour test those potential regressions (I think it should). WDYT?

@codeodor
codeodor Mar 14, 2014

I see. I think the behavior test in encoding covers it, but given this test file for object and class extensions was there, and the JSON extensions were not tested here, I thought it would be nice to at least start something.

It just seemed odd to me a lot of the other extensions were tested here, so I added it.

However, I'll go ahead and remove it.

@chancancode chancancode and 1 other commented on an outdated diff Mar 14, 2014
activesupport/test/json/encoding_test.rb
- { :name => 'Jean', :address => { :city => 'Paris' , :country => 'France' }}
+ People = Class.new(BasicObject) do
+ include Enumerable
+ def initialize()
+ @people = [
+ { :name => 'John', :address => { :city => 'London', :country => 'UK' }},
+ { :name => 'Jean', :address => { :city => 'Paris' , :country => 'France' }}
+ ]
+ end
+ def each(*, &blk)
+ @people.each do |p|
+ yield p if blk
+ p
+ end.each
+ end
+ def is_a?(thing); false; end
@chancancode
chancancode Mar 14, 2014

What does this do? :)

@codeodor
codeodor Mar 14, 2014

The old test tested a hash, even though it claimed to be testing an Enumerable. So this one tests the described problem described in the Pull Request: Enumerable of BasicObjects.

@chancancode
chancancode Mar 14, 2014

I mean the def is_a?(thing); false; end specifically – just out of curiosity, cause that's not very obvious to me :)

@codeodor
codeodor Mar 14, 2014

I don't remember. The only thing I can I think of, is maybe at one point somewhere inside one of the methods Rails was calling is_a?(Hash) or some other class in order to decide what to do with the conversion, so I just wanted to be explicit about this object is not going to say it's anything if asked what it is, to be sure we went down the expected path of code.

However, after reviewing the code a bit, I don't see that, so I'll remove this is_a? thing.

@codeodor

Ok, I made the suggested changes. Let me know if you have any other feedback, and thanks @chancancode!

@codeodor

Hold a sec. I didn't look at my diffs before I pushed. My crappy editor seems to have messed with the character encoding.

@codeodor

That issue has been fixed.

@jeremy
Ruby on Rails member

👍 here. Thank you @codeodor.

@chancancode chancancode merged commit 6d5724d into rails:master Mar 14, 2014
@chancancode
Ruby on Rails member

❤️ 💚 💙 💛 💜

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