Fixed collection name for empty collections. #119

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

Contributor

I didn't want to change too much here, since I'm not sure of the original motivations, so this is the minimal change to fix the problem.

Possibly a better change would be to just take the symbol name in all cases? I would think this would be the least surprising option.

Owner
nesquena commented Oct 2, 2011

Which problem in particular were you solving? Can we add a regression test along with this fix.

Contributor

I'll try to add a test soon, but I'd at least first like to decide what the
right fix is.

The problem is this, if you have a model User with an association :things,
and now you come across a user with no things, and do:

child :things do
attributes :foo
end

Instead of the desired JSON, which would be something like
{things: []}

you instead get

{[]}

but if there's at least one thing, you'll get

{things: [
thing: {}
]}

Owner
nesquena commented Oct 2, 2011

Ah yes that issue. I usually solve that by doing:

child :things => :things do
  attributes :foo
end

which I agree 100% should not be necessary. It is a bit of a tricky case to define the behavior because in this case:

child @things => :things do
  attributes :foo
end

it is clear (to me) that the naming will go things => thing. But in this case:

child @things do
  attributes :foo
end

I would expect it to use whatever the class name is so if it was a blank collection, I am not sure of the correct behavior for the outer node if not specified.

My first reaction is that this case:

child :things do
  attributes :foo
end

would use the symbol as the name all the time as you mentioned things => thing. What is the downside to doing that? heterogeneous collections?

Contributor

I see the problem here now. If I understand what you're saying, then I think
the commit I proposed leaves what you're worried about ambiguous and fixes
the case that has a definite solution.

In the 'child @things do' case, my preference, if was was driving this
thing, would be to either complain loudly, or to even disallow (again
warning loudly) that specific construction without a name, unless they gave
us something with #model_name, whether or now we use #model_name. If you
choose the latter, I would have noticed this even before I tried to render
an object with an empty collection, since the part of the broken usage that
'works' is also disallowed, and loudly.

You could also choose to only warn when trying to render an object with an
empty collection, I'd say that's the minimum here, but the least intrusive.

On Sun, Oct 2, 2011 at 6:36 PM, Nathan Esquenazi <
reply@reply.github.com>wrote:

Ah yes that issue. I usually solve that by doing:

child :things => :things do
 attributes :foo
end

which I agree 100% should not be necessary. It is a bit of a tricky
case to define the behavior because in this case:

child @things => :things do
 attributes :foo
end

it is clear (to me) that the naming will go things => thing. But in this
case:

child @things do
 attributes :foo
end

I would expect it to use whatever the class name is so if it was a blank
collection, I am not sure of the correct behavior for the outer node if not
specified.

My first reaction is that this case:

child :things do
 attributes :foo
end
``

would use the symbol as the name all the time as you mentioned. What is the
downside to doing that? heterogeneous collections?

--
Reply to this email directly or view it on GitHub:
https://github.com/nesquena/rabl/pull/119#issuecomment-2267465
Owner
nesquena commented Oct 3, 2011

Yes, I think your solution should work fine for most cases. I also want to document our choices with tests as we move forward. I am a fan of minimalist warnings at the start unless a more serious warning is needed. In this case I would err towards minimalism with warning them if the collection is empty. However, I am not opposed to a more bold warning in general being the direction this is going.

jeyb commented Oct 9, 2011

Similar problem exists if the collection itself is an empty array, as opposed to child collections.

Example:

collection @users
attribute :id

Assume @users is an empty array. The following exception is raised:

undefined method `pluralize' for nil:NilClass`

Which comes from this definition in lib/rabl/helpers.rb:

def data_name(data)
  return nil unless data # nil or false
  return data.values.first if data.is_a?(Hash) # @user => :user
  data = @_object.send(data) if data.is_a?(Symbol) && @_object # :address
  if data.respond_to?(:first)
    data_name(data.first).pluralize
  else # actual data object
    object_name = @_collection_name.to_s.singularize if @_collection_name
    object_name ||= data.class.respond_to?(:model_name) ? data.class.model_name.element : data.class.to_s.downcase
    object_name
  end
end

I would prefer the JSON response to look like { users: [] }

Suggestions?

Owner
nesquena commented Oct 9, 2011

@jeyb Yes, just do this:

collection @users => :users

by explicitly setting the symbol, it should fix that issue.

jeyb commented Oct 9, 2011

That doesn't solve the issue. If @users is an empty array, then data_object receives [] => :users and returns the first key, which is []. Since that's an array, respond_to?(:first) evaluates to true and returns nil within the if condition. Hence the pluralize call on nil.

Dagnan commented Feb 22, 2012

The problem with the empty root collection seems to be still present in the master :

{ "items" => [ {} ] }

doing

collection @items => 'items'
attributes :id, :name
Dagnan commented Feb 22, 2012

Please disregard my comment. All went fine when I deleted

 => 'items'
Collaborator
databyte commented Apr 6, 2012

Closing. For further conversations about syntax changes, see Issue 216

@databyte databyte closed this Apr 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment