dewski/json_builder

Can't use ActiveRecord scopes directly

lagartoflojo opened this issue · 6 comments

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).

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?

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?

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.

(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?

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?

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.