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

EmbeddedRecords with ActiveModelSerializer should append _attributes to keys #1950

Closed
mehulkar opened this issue May 17, 2014 · 26 comments
Closed

Comments

@mehulkar
Copy link

When I embed records for a Rails backend, it's usually because I'm using the accepts_nested_attributes feature of ActiveRecord. Rails expects that embedded records will come in with the key [plural_model_name]_attributes.

Example:

{
  "author": {
      "name": "Mark Twain",
      "books_attributes": [{"id": "1", "title": "Tom Sawyer"}]
  }
}

The solution would be to do modify serializeHasMany in EmbeddedRecordsMixin to check if the ActiveModelSerializer and append the string.

Opening up this issue to see if this an appropriate addition. To implement, I'm not sure how to check what type of the ActiveModelSerializer (or an extension of it) is being used.

@rwjblue
Copy link
Member

rwjblue commented May 17, 2014

@danmcclain - I know we recently discussed the embedded records issues with rails, does this fall in line with our discussion?

@danmcclain
Copy link
Contributor

It does. In the project I am using the embedded mixin, I patched serializeHasMany to append _attributes

@mehulkar
Copy link
Author

I have a use case for opting out of this feature as well, so I've added a flag to the attrs hash:

// app/serializers/author.js
attrs: {
  books: { embedded: 'always', railsNested: true}
}

// app/mixiembedded_records_mixin.js

serializeHasMany: function(record, json, relationship) {
  var key   = relationship.key,
      attrs = get(this, 'attrs'),
      embed = attrs && attrs[key] && attrs[key].embedded === 'always',
      railsNested = attrs && attrs[key] && attrs[key].railsNested; // READ extra option

  if (embed) {
    // PATCH: Appends '_attributes' to hasMany embedded key if specified
    var keyForAttribute = this.keyForAttribute(key);
    keyForAttribute =  railsNested ? keyForAttribute + '_attributes' : keyForAttribute;

    json[keyForAttribute] = get(record, key).map(function(relation) {
      var data = relation.serialize(),
          primaryKey = get(this, 'primaryKey');

      data[primaryKey] = get(relation, primaryKey);

      return data;
    }, this);
  }
},

@watsonbox
Copy link

@mehulkar Presumably this doesn't work for deletions as that would require the _destroy key?

@mehulkar
Copy link
Author

@watsonbox I'm handling that by reassigning the entire has_many array server side. This automatically deletes records instead of having to deal with the _destroy key which would require munging the json even more. Something like this:

class AuthorController < ApplicationController
  def update
    @author = Author.find(params[:id])
    author_params = params[:author]
    if author_params[:books]
      books = author_params[:books].map {|x| Book.find_or_initialize_by_id(x[:book_id])}
      @author.books = books
    end
    @author.save
  end
end

Not ideal, but it's easier for me to follow than other solutions I've tried.

@watsonbox
Copy link

If you are using your proposed JavaScript, surely you'd have author_params[:books_attributes]? I did try using Rails' accepts_nested_attributes with Ember Data, but like you found the destroying of records to be a bit clunky. For that reason I decided to simply embed all child records when the parent is saved, and then update the parent on the server with something like what you have. Given that this approach works fairly well and what you propose doesn't support deletions, I'm not sure it's really worth supporting accepts_nested_attributes out of the box.

@mehulkar
Copy link
Author

@watsonbox Yes, you're right. I was typing out the pseudo code and used the wrong key. It should be author_params[:books_attributes].

Maybe, what's in order is not another feature for the EmbeddedRecords mixin, but a cookbook?

At the same time, I think this pattern will be used over and over. DS.ActiveModelSerializer and DS.EmbeddedRecordsMixin are meant to be used with Rails and accepts_nested_attributes_for. It doesn't make sense for the compatibility to be incomplete, IMO.

Maybe we can add the _destroy key based on the isDeleted status of hasMany records?

@digitalplaywright
Copy link

I bumped into this as well. Although the workaround used by @watsonbox seems pretty simple it requires special handling and a bit of code for every embedded model. I agree with @mehulkar that most users probably expect that DS.ActiveModelSerializer conform with accepts_nested_attributes_for. At least I did.

@bjorntrondsen
Copy link

I too expected ActiveModelSerializer to support accepts_nested_attributes out of the box and it took me a while to figure out a way to deal with the problem.

I ended up using @mehulkar 's approach with a couple of modifications. I added support for embedded: 'records', which does the same thing, and i include the child ids.

My only problem is that the nested records are still in a dirty state after a successful save, which Im currently solving in a very hackish way by manually cleaning the records.

//app/nest_attributes_like_rails.js
DS.EmbeddedRecordsMixin.reopen({
  serializeHasMany: function(record, json, relationship) {
    var attrs, embed, key, keyForAttribute, railsNested;
    key = relationship.key;
    attrs = Ember.get(this, "attrs");
    embed = attrs && attrs[key] && $.inArray(attrs[key].embedded, ['always', 'records']) > -1;
    railsNested = attrs && attrs[key] && attrs[key].railsNested;
    if (embed) {
      keyForAttribute = this.keyForAttribute(key);
      keyForAttribute = (railsNested ? keyForAttribute + "_attributes" : keyForAttribute);
      return json[keyForAttribute] = Ember.get(record, key).map(function(relation) {
        var data, primaryKey;
        data = relation.serialize({
          includeId: true
        });
        primaryKey = Ember.get(this, "primaryKey");
        data[primaryKey] = Ember.get(relation, primaryKey);
        return data;
      }, this);
    }
  }
});

@jdjkelly
Copy link
Contributor

Subbing to this - working on a PR for this

@EtienneDepaulis
Copy link

👍

@opsb
Copy link

opsb commented Jul 30, 2014

I've just spent a couple of hours scratching my head over this one, very glad I found the thread! I found the ActiveModelAdapter deviated from my expectations in a few ways.

  1. I assumed embedded record support would be out of the box, the DS.EmbeddedRecordsMixin should be included by default (I have to wire up a serializer at the moment).

  2. You have to list the relationships in your ActiveModelSerializer - I expected the config to be taken from the model definitions e.g.

App.Video = DS.Model.extend({
    archive: belongsTo('archive', {embedded:'always'})
});

rather than having to do

App.ApplicationSerializer = DS.ActiveModelSerializer.extend(DS.EmbeddedRecordsMixin, {
    attrs: {
        archive: { embedded: 'always' }
    }
});
  1. The topic of this thread, the missing _attributes

I think solving these 3 issues would really help people get going faster. There's a lot of unnecessary frustration at the moment (although to be fair we could just document these issues in the ember-data guide so people know what to do).

@EtienneDepaulis
Copy link

@opsb I posted a few hours ago a little hack very close to the one you are presenting : http://stackoverflow.com/questions/17431934/ember-js-with-rails-accepts-nested-attributes-for-and-polymorphic-has-many-belon/25035388#25035388

App.Post = DS.Model.extend
    comments: DS.hasMany('comment')

App.PostSerializer = DS.ActiveModelSerializer.extend( DS.EmbeddedRecordsMixin,
    attrs:
        comments: {embedded: 'always'}

    keyForAttribute: (attr) ->
        if attr == "comments"
            "comments_attributes"
        else
            @_super(attr)
)

I'm new to Ember and I think it's quite a dirty fix but it's working pretty well.

@opsb
Copy link

opsb commented Jul 30, 2014

@EtienneDepaulis Interesting idea, if you have access to the attrs hash in keyForAttribute you could decide whether or not to add _attributes based on whether or not the attr is embedded.

@opsb
Copy link

opsb commented Jul 30, 2014

@EtienneDepaulis the only problem with that approach is that the serializer calls the same method when serializing and deserializing. When you're deserializing you want it without the _attributes when you're serializing you want it with the _attributes.

@opsb
Copy link

opsb commented Jul 30, 2014

@EtienneDepaulis I've put together a modified version of the embedded-records-mixin that adds _attributes to the end of embedded attributes where appropriate, https://gist.github.com/opsb/730188df99173bff3fc7. You can just add it to your project and then use the mixin in place of the DS.EmbeddedRecordsMixin.

@EtienneDepaulis
Copy link

@opsb Thanks a lot, why not submit it as a PR ?
As for your comment on the deserializing action, I thought I was going to have problems but it's working :) (as I said I'm new to Ember so I don't understand everything yet)

@opsb
Copy link

opsb commented Jul 30, 2014

The main blocker is I haven't been able to get the tests running on ember-data :) I'll have another crack now.

@opsb
Copy link

opsb commented Jul 30, 2014

FYI #2138

@EtienneDepaulis
Copy link

👍

@NeuRA-Labs
Copy link

@EtienneDepaulis keyForAttribute works well because it's only invoked for serializing not and deserializing. Your method is a great drop in solution until hopefully @opsb PR gets merged. I am a little surprised that this isn't in DS.EmbeddedRecordsMixin.

I wonder if the lack of support for this is because REST API endpoints are really supposed to be designed with models as first class citizens and not nesting them like we've done in the past with rails.

@amedrz
Copy link

amedrz commented Dec 12, 2014

@EtienneDepaulis approach worked for me -- I'm sending data to a Rails API with accepts_nested_attributes_for enabled.

@arnaldo2792
Copy link

May be I'm wrong, but I think that this issue should be solved by what you guys said in #2138

@amedrz
Copy link

amedrz commented Apr 13, 2015

Yeah I actually opted for other approaches such as Form Objects and stop needing this.

@igorT
Copy link
Member

igorT commented Apr 13, 2015

This should now be much easier to customize, due to #2790 so you can customize the key in one direction. Maybe a change to AMS default(if we added keyForEmbeddedRecord?) or an addon would be great to get this out of the box.

@igorT igorT closed this as completed Apr 13, 2015
@mmahalwy
Copy link

Sorry guys, but any solution to this?

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