dewski / json_builder

Rails provides an excellent XML Builder by default to build RSS and ATOM feeds, but nothing to help you build complex and custom JSON data structures. JSON Builder is here to help.
http://garrettbjerkhoel.com/json_builder/
MIT License
244 stars 52 forks source link

Can't use ActiveRecord scopes directly #8

Closed lagartoflojo closed 12 years ago

lagartoflojo commented 12 years ago

With a model like this:

# app/models/child.rb
class Child < ActiveRecord::Base
  validates_presence_of :name
  def self.not_null # contrived example
    where("name is not null")
  end
end

And a template like this:

# index.json.json_builder
children Child.not_null do |c|
  id c.id
end

Rails throws:

undefined method `id' for #<ActiveRecord::Relation:0x0000000548ca60>

This is fixed by adding .all to the where call (i.e. where("name is not null").all), but that would break chained scopes (i.e. Child.not_null.order_by_name), so it's not a solution, or by adding .all when calling the scope (i.e. Child.not_null.all), but it shouldn't be necessary.

I believe this error is line 13 in Member which asks if argument.is_a?(Array), to which an ActiveRecord::Relation returns false.

I guess the solution would be to check whether argument is "array-like" rather than an actual array (maybe something like if argument.respond_to? :each, but that would catch Hashes and anything else that has an each method), or adding || argument.is_a?(ActiveRecord::Relation) (but that would be a Rails-specific solution).

dewski commented 12 years ago

I'd like to hear if you have anything against this. Is checking explicitly for an Array and ActiveRecord::Relation too narrow? Would explicitly leaving out Hash been too open?

lagartoflojo commented 12 years ago

Yeah, I was thinking of a solution like that; I just didn't know if you wanted to have ActiveRecord-specific code in the library (for example, maybe this code won't work with another ORM).

Actually, at first I tried using the array method, which worked fine, except that by using it I can't include anything else (this had me scratching my head for a while, by the way). I think array is nice, because you are explicitly telling the builder to treat the object as an array, so the user can throw whatever he wants in there and it will work as long as it responds to each.

So my idea is: maybe add an option to the array method accepting a custom key? Something like:

array @children, key: :children do |child|
  id child.id
end

Or you could even forego "key" and go with array @children, :children.

What do you think?

dewski commented 12 years ago

Per the README, the array method will build an array with JSON objects, where each object defines their own keys.

What I think you're trying to do is:

children @child do |child|
  id child.id
end

JSON Builder tries to be smart and as you can tell, checks to see if the passed object is an array, if so, create an array member and return it as such. But if you want to create an array without a key attached to it, that's when you'd use the array method.

lagartoflojo commented 12 years ago

(PS: I edited my previous comment (@child => @children) to make it clear that the object represents "many children".)

Yes, that's what I understood from reading the README. That's when I replaced "array" with "children", and it didn't work because my array wasn't an array but an ActiveRecord::Relation.

Your commit will work fine for me and most people I think, but it will break when the object is not an array nor an ActiveRecord::Relation, but some other array-like object.

So I had another idea: How about checking if a block is given, and if the .arity of the block is 1, then build an array and treat the object as an array?

dewski commented 12 years ago

Oh ok, I thought that comment was a separate issue that wasn't really related to the first.

If you'd like to provide a pull request with that I'd be more than happy to merge it in, but for right now I feel it is handling the arrays for a large number of people correctly. Could you see of any other situation where passing in anything but an Array or query/relation may need the actual array block?

lagartoflojo commented 12 years ago

Yes, the way it is now is much simpler, and it probably solves the problem for 95% of the use cases. Plus, I really can't think of an array-like object that's not an array/enumerable nor an ActiveRecord::Relation; my "arity" idea was to future-proof the code =) For now, I guess it's not really necessary, but I'll try to make a patch just for fun.