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

undefined method `embed' #938

Closed
tommyblue opened this issue Jun 4, 2015 · 24 comments
Closed

undefined method `embed' #938

tommyblue opened this issue Jun 4, 2015 · 24 comments

Comments

@tommyblue
Copy link

I'm using the 0.10.0.rc1 version because 0.9.x doesn't work, but with this definition of the serializer:

class Api::V1::ItinerarySerializer < ActiveModel::Serializer
  attributes :id, :title

  embed :ids, include: true
  has_many :pois, serializer: Api::V1::PoiSerializer
end

I get this error:

NoMethodError (undefined method `embed' for Api::V1::ItinerarySerializer:Class):
  app/serializers/api/v1/itinerary_serializer.rb:4:in `<class:ItinerarySerializer>'
  app/serializers/api/v1/itinerary_serializer.rb:1:in `<top (required)>'
  app/controllers/api/v1/itineraries_controller.rb:29:in `update'

Embed is deprecated? How can I generate the "old" JSON including associated objects, like this:

{
  "itinerary": {
    "poi_ids": [1,4,5]
  },
  { "pois": [
    // POI serialized objects
  ]}
}
@groyoh
Copy link
Member

groyoh commented Jun 7, 2015

I'm afraid that is not currently possible with 0.10. @joaomdmoura is this a planned feature for 0.10?
In the meantime, you could workaround it by creating your own adapter based on the json adapter. If you need some help with that let me know.

@tommyblue
Copy link
Author

ok, thanks.
Shouldn't this an important option? I have an habtm association and embedding object is a good way to avoid data replication inside the single json response or avoiding multiple requests (one for each id).
Isn't it?

@joaomdmoura
Copy link
Member

Hey @tommyblue, indeed, @groyoh is right!

Its possible to accomplish something close to it by using JSON-API, but would be a different implementation and not the '"old" JSON'.

btw why not use only:

 has_many :pois, serializer: Api::V1::PoiSerializer

This will return the serialized objects, isn't waht you need? You can map the ids if you want to, on a virtual attribute:

class Api::V1::ItinerarySerializer < ActiveModel::Serializer
  attributes :id, :title, : poi_ids
  has_many :pois, serializer: Api::V1::PoiSerializer

  def poi_ids
    object.pois.map {|p| p.id}
  end
end

@tommyblue
Copy link
Author

If I'm not wrong, your solution puts the serialized POI objects inside their Itinerary "fathers". This means that there will be a lot of replicated POI objects inside different itineraries. This is not a problem with few objects but with a lot of objects is a lot of useless weigth into the JSON

@bf4
Copy link
Member

bf4 commented Jun 11, 2015

@tommyblue Which adapter are you using? Json or JsonApi? You might want to subclass the adapter you're using to extend it.

@tommyblue
Copy link
Author

@bf4 I could, but I'd like to understand from @joaomdmoura if I'm saying something wrong stating that the use of embed :ids, include: true is preferable when having a HABTM association with a lot of records since it can reduce the weight of the response.
I don't know the background discussion which led to removing embed, but I think it's useful so I'd like to understand why isn't present any more :)

thanks!

@joaomdmoura
Copy link
Member

@tommyblue indeed I didn't realised the whole scenario. It makes sense.
Actually we haven't decided to remove embed, we just haven't implemented or discussed about it yet. 😄

The good news is that I'm willing to review a PR implementing it, but the bad one is that I'm busy into some other new features, so we can't work on this right now, but we might do it in a near future!! 😄 Meanwhile @kurko do you have any thoughts about it?

@tommyblue
Copy link
Author

thanks @joaomdmoura

@joaomdmoura joaomdmoura self-assigned this Jun 12, 2015
@joaomdmoura joaomdmoura added this to the 0.10 milestone Jun 12, 2015
@NullVoxPopuli
Copy link
Contributor

side loading is the preferred method of retrieving data for ember.
it reduces redundancy when nested models would otherwise reference the same model.

for example, if your json references "tag" objects, you don't want to include the same tag data twice, so you just make an id reference.

@ZackMattor
Copy link

👍 I also need this functionality... I'll probably have to roll back to 0.9 for now.

@NullVoxPopuli
Copy link
Contributor

I'm going to work on this a bit today, as I absolutely NEED it for communicating with ember.

@NullVoxPopuli
Copy link
Contributor

I have a pull request #1088 which I hope addresses this for most people

@NullVoxPopuli
Copy link
Contributor

as for embedding ids only, would it make sense to have embed_ids: true on the associations declaration?

@ZackMattor
Copy link

Didn't v0.9 do it on the class level? But I do see the appeal of being able to change the behavior per association.

@mattholtom
Copy link

Here's my workaround for now, similar to @joaomdmoura above:

class PostSerializer < ActiveModel::Serializer
  attributes :id, :comment_ids

  def comment_ids
    object.comments.pluck(:id)
  end

end

@andrewhavens
Copy link

Sounds like #1127 added support for nested associations and #1084 is a work in progress, trying to introduce a backwards compatible "Legacy Adapter". As far as I know, embedding IDs is the default when using the JSON-API adapter. Just wanted to update the breadcrumb trail. =]

@bf4
Copy link
Member

bf4 commented Feb 24, 2016

@andrewhavens about right. embed was terminology from an earlier iteration of the json api spec, iirc. Subclassing an adapter (and then sharing it!) would probably be the simplest way to have the exact same behavior

@NullVoxPopuli
Copy link
Contributor

Closing for now --

It looks like there have been a couple different strategies for achieving the functionality of embed_ids, so further action could be in the form of a pull request, or expanding on the LegacyJSON PR, or something similar.

@aandis
Copy link

aandis commented Jul 10, 2016

+1. This is a necessity for active-model-adapter. Would really like it back.

@bf4
Copy link
Member

bf4 commented Jul 10, 2016

@aandis This macro won't be re-added as it was from an earlier iteration of JSON API see #938 (comment)

The docs you link to are because that repo hasn't been updated adopted-ember-addons/active-model-adapter#66

@Jarred-Sumner
Copy link

+1. I need this too.

@NullVoxPopuli
Copy link
Contributor

some work on this is actually being done here: #1845

@alexloginov
Copy link

+1. Need this too.

@NullVoxPopuli
Copy link
Contributor

@alexloginov in the latest version of AMS, include be used from the render call :-)

atd added a commit to singularities/circular-work that referenced this issue Nov 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests