Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

:only option on has_many not working #1943

Open
HoneyryderChuck opened this issue Oct 7, 2016 · 12 comments
Open

:only option on has_many not working #1943

HoneyryderChuck opened this issue Oct 7, 2016 · 12 comments

Comments

@HoneyryderChuck
Copy link

Expected behavior vs actual behavior

I have the following set in a serializer:

Parent < ParentSerializer
  has_many :items, only: %i(hostname)
end

But instead of filtering all the other attributes, it returns them all. This was something that was working with version 0.9.3, but apparently it is not anymore in 0.10.2.

Is it a bug?

@bf4
Copy link
Member

bf4 commented Oct 9, 2016

(follows #1942 )

@TiagoCardoso1983 are you saying that only: [:hostname] has no effect? And that the effect you expect is the equivalent of object.items.as_json(only: [:hostname])?

Is this documented behavior on 0.10 somewhere? I don't recall seeing it.

This feature is related to a host of other issues, and I can't seem to find the canonical one:

@HoneyryderChuck
Copy link
Author

This was a working and documented feature in 0.9.x . I looked in the 0.10.x documentation and couldn't find it, so I guess it's maybe removed(?). Judging by the list and after having a superficial look on those, I guess I'm not the only one experiencing issues. Is version 1.x coming, so maybe breaking APIs will not happen unexpectedly?

I've used the :fieldsoption, but hasn't also worked. Is there anything similar in functionality in 0.10.x that might work as a drop-in replacement?

@scottohara
Copy link

I have been trying to figure out the best way to do something similar.

In my case, I have a Category model. A category can optionally have subcategories (and conversely, those subcategories have a relationship back to their parent category), e.g.

class Category < ApplicationRecord
  belongs_to :parent, class_name: 'Category', optional: true
  has_many :children, class_name: 'Category'
end
  • When serializing a "parent" category, I wish to include the child categories and all their attributes.
  • When serializing a "child" category, I wish to include the parent category and a subset of its attributes (only parent.id and parent.name).

For example, given the following sample data:

Vehicles:
  - Cars
  - Trucks

Serializing the Vehicles parent category should return something like this:

{
  "id": 1,
  "name": "Vehicles",
  "total": 800,
  "children": [
    {"id": 2, "name": "Cars", "total": 500},
    {"id": 3, "name": "Trucks", "total": 300}
  ]
}

Serializing the Cars child category should return something like this:

{
  "id": 2,
  "name": "Cars",
  "total": 500,
  "parent": {"id": 1, "name": "Vehicles"},
}

In 0.9.5, my CategorySerializer worked like this:

class CategorySerializer < ActiveModel::Serializer
  attributes :id, :name, :total, :parent
  has_many :children

  def parent
    # If there is a parent, serialize only id + name
    CategorySerializer.new object.parent, only: %i(id name) if object.parent
  end
end

In 0.10.2, this stopped working (the nested call to CategorySerializer.new in the parent method was causing an infinite loop).

I have managed to get it working again by changing the parent attribute/method to a belongs_to association, and using a block to filter the attributes to return:

class CategorySerializer < ActiveModel::Serializer
  attributes :id, :name, :total
  has_many :children, unless: -> { object.children.empty? }
  belongs_to :parent, if: -> { object.parent } do
    {
      id: object.parent.id,
      name: object.parent.name
    }
  end
end

But manually constructing a Hash to return from the block seems to defeat the point of AMS.

Instead of passing a block to the belongs_to :parent association, for the sole purpose of filtering attributes on the relationship, it would seem much simpler if we could specify an only: option:

class CategorySerializer < ActiveModel::Serializer
  attributes :id, :name, :total
  has_many :children, unless: -> { object.children.empty? }
  belongs_to :parent, if: -> { object.parent }, only: %i(id name)
end

@beauby
Copy link
Contributor

beauby commented Oct 21, 2016

The truth behind the attribute :foo, if: :bar and has_many :baz, only: [:foobar] is that you are in fact defining multiple serializers in one place. This may save you a few keystrokes, but increases complexity and mixes up unrelated concerns. It should be avoided.

@scottohara
Copy link

scottohara commented Oct 21, 2016

So is the recommendation therefore something like a nested serializer definition (for serializing a category within a category?

class CategorySerializer < ActiveModel::Serializer
  attributes :id, :name, :total
  has_many :children, unless: -> { object.children.empty? }
  belongs_to :parent, if: -> { object.parent }

  # This serializer used for category.parent and category.children
  class CategorySerializer < ActiveModel::Serializer
    attributes :id, :name
  end
end

@beauby
Copy link
Contributor

beauby commented Oct 21, 2016

In your case, why not have parent with nil data and children with value [] when applicable?

@scottohara
Copy link

I'm not so much concerned about the if and unless parts; I'm more interested in the correct way to restrict the attributes for a nested relation (that, in my case, happens to be the same class).

Here's the previous example again, without the if / unless, for clarity:

class CategorySerializer < ActiveModel::Serializer
  attributes :id, :name, :total
  has_many :children
  belongs_to :parent

  # This serializer used for category.parent and category.children
  class CategorySerializer < ActiveModel::Serializer
    attributes :id, :name
  end
end

(Unless I misunderstood the point you were making; which is entirely possible... 😄 )

@beauby
Copy link
Contributor

beauby commented Oct 21, 2016

Oh sorry – I was questioning the concept itself of having different representations for objects of the same type within a document. Is it necessary for you not to have :total in the children categories? Or do you want to avoid recursively serializing the children categories?

@scottohara
Copy link

That's a fair point...in the examples I provided, I only included a single additional attribute (:total) for brevity.

In my actual code, however, there are a number of additional attributes (5 or 6) that I would want excluded when serializing the nested relations.

Partly this is because I want to ship the smallest JSON down to the client as possible (in an extreme case of a Category that has, say, 25 children, excluding those 5 or 6 attributes that the client doesn't actually require to display the children would yield a noticeable reduction in the overall payload size).

And partly this is because some of those additional attributes are potentially expensive to calculate.

For example, let's assume that :total in my above example was, in fact, a calculated value, e.g.

class CategorySerializer < ActiveModel::Serializer
  attributes :id, :name, :total

  # ....

  def total
    # some non-trivial calculation here...
  end
end

Ideally I wouldn't want to incur the overhead of calculating the total for each nested child item if it would ultimately go unused. (I will admit that I haven't actually measured what the extra cost is of including those additional attributes, so one could argue that I'm guilty of premature optimization here).

@beauby
Copy link
Contributor

beauby commented Oct 21, 2016

For the expensive attributes, I think the right solution would be caching.
Regarding the payload size, I understand your concern, but maybe it could be addressed by compressing the transmitted data?

The reason I advocate against having different sets of fields for resources of the same type is that you do not have to subsequently fetch a "fuller" representation of a resource – once it has been fetched, you know all there is to know about it, and this makes keeping a local partial representation of your data in the client much easier. I understand it is not necessarily everybody's use case, but in that case, JSON API may also not be the best format there is, and an ad-hoc format may be more suitable (as JSON API is relatively verbose, in order to simply the use-case I mentioned above).

All that being said, part of the features you want are in AMS 0.10, and I believe the rest is being discussed somewhere (can't remember where).

@scottohara
Copy link

Thanks @beauby, you make some good points there. I should note that in my case I'm using the :attributes adapter (so not JSON API in my case).

It does seem to me that the direction AMS is heading is more and more towards JSON API as the primary use case, and for simple attribute serialization AMS may actually be overkill. I might be better off just crafting the appropriate #as_json methods on my models, to produce the exact JSON my front-end expects.

For now, I think I'll pin my Gemfile to gem 'active_model_serializers', '0.9.5' to retain the existing (working) behaviour; while keeping an eye on the open issues linked at the top of this thread.

@beauby
Copy link
Contributor

beauby commented Oct 21, 2016

Oh, I'm sorry I missed that. Yeah, you are right, AMS is headed more and more towards JSON API. The main reason for that is that it is a well-defined format, whereas the Attributes adapter builds a legacy format, that is not specified, and that has identified shortcomings (duplication, cycles, etc.).

In your case, I would probably use either an older version of AMS, as you suggested, or even use something like jbuilder to build a custom format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants