Follow rails conventions for serializing embedded objects

[opsb]

In #1950 there was a lot of talk about how people expected the embedded objects to be serialized with _attributes when they're embedded (as per the rails convention). This PR will add _attributes to any embedded objects when the EmbeddedRecordsMixin is used.

It does break the existing behaviour but I think it's fair to say the existing behaviour is broken for rails. If someone does need the existing behaviour they can revert to it by overriding the formatEmbeddedKey method e.g.

App.ApplicationSerializer = DS.ActiveModelSerializer.extend(DS.EmbeddedRecordsMixin, {
  formatEmbeddedKey: function(key){
    return key;
  },

  attrs: {
    video: { embedded: 'always' }
  }
});

[igorT]

I think it might make sense to default the ActiveModelSerializer to append '_attribute' while the Rest/Json serializer shouldn't. However, apparently Rails only expects '_attribute' when serializing, but not when deserializing?? Can someone explain/link to the reasons behind that design decision.
CC @pixelhandler

[bradleypriest]

@igorT in the 'conventions' being discussed here, serializing and deserializing are provided by two completely separate pieces.

Serialization is provided by the ActiveModelSerializers gem, and deserializing is provided by the accepts_nested_attributes_for helper inside ActiveRecord.

nested_attributes_for has been getting a lot of bad press lately, and a lot of people are really pushing the use of FormObjects instead.

And ActiveModelSerializers is a gem, not technically part of Rails.

Personally, I don't think this a good default, maybe the solution is just better documentation on how to update this, or possibly adding in the proposed hook, but nooping it by default.

[bradleypriest]

To clear it up, I am personally using the accepts_nested_attributes_for helper myself and translating it in the controller, but our API is public, and it didn't seem correct to have the embedded objects serialized and deserialized under different keys.

[jdjkelly]

To add to @bradleypriest 's comments here are the docs:

http://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html
http://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html#method-i-accepts_nested_attributes_for

One bit of feedback I have on the discussion so far:

Personally, I don't think this a good default, maybe the solution is just better documentation on how to update this, or possibly adding in the proposed hook, but nooping it by default.

I disagree. This commit is against the ActiveModelAdapter, which means the end developer is using Rails. If they're using Rails, they're going to be passing params hashes to methods like update_attributes which expect this format.

EDIT: In general, I think this is a smart default for this particular adapter, and reflects the reality of developing a Rails API today.

[igorT]

Paging @tomdale @wycats as well

[pixelhandler]

@igorT I have not used accepts_nested_attributes_for, I've used a document storage db and didn't need accepts_nested_attributes_for; the document is saved for me with a gem (mongoid). Sounds like there is a need for the serializing using the _attributes convention for people who use accepts_nested_attributes_for. At a quick glance it sounds like... a feature of the (ember data) active model serializer that can be enabled or disabled for naming the payload properties using the _attributes convention. Would using some branching in keyForAttribute based on a config like attr: {embedded: 'always', nestedAttributes: true} work?

[opsb]

@pixelhandler I originally didn't use keyForAttribute because I thought it was used for deserializing as well (and obviously the serialization and deserialization aren't symetrical). It looks like keyForAttribute is only used for serialization but I'm not sure whether that will always be the case. If it's the case that it will only ever be used for serialization then it would probably make more sense then calling a separate method.

[pixelhandler]

@opsb yeah just looked at the JSONSerializer and keyForAttribute is used in the normalize methods as well as serialize methods perhaps we do need a keyForNestedAttribute method as well to support accepts_nested_attributes_for. Since the embedded records mixin was moved out of the active model serializer package and into the ember-data package... the support it provides will need to be compatible with RESTSerializer, JSONSerializer and the ActiveModelSerializer. It seems that the accepts_nested_attributes_for support belongs in the ActiveModelSerializer or perhaps a new mixin.

[lytbulb]

@pixelhandler That's why I used a hook for the formatEmbeddedKey, by default it's a noop but the ActiveModelSerializer overrides it to append the "_attributes" to the key. There's a couple of specs at https://github.com/opsb/data/blob/embedded_attributes_postfix/packages/activemodel-adapter/tests/integration/embedded_records_mixin_test.js#L1009 which test the default behaviour of EmbeddedRecordsMixin against the RestSerializer.

[opsb]

Rebased against master. If it's going to be quicker to get this merged in perhaps it would be best to use the existing defaults and just document that you need to specify the formatEmbeddedKey? I can open a separate issue regarding whether or not the default should be changed. I'd like to get this and the docs at emberjs/website#1634 merged in because at the moment getting started with the ActiveModelAdapter is more work than it should be.

[juggy]

Do the embedded records get their dirty flag changed? It seems this PR only sends the embedded record, but I think ember data does not change it to the saved state. Am I right?

Also what happens when you remove records? It won't be serialized at all in the request? Separate calls to destroy will happen on save?

[opsb]

@juggy I presumed that EmbeddedRecordsMixin already take care of those things, this is just to change the serialization format.

[juggy]

Understood. The EmbeddedRecordsMixin is just the serializer part, as far as I know it doesn't touch any of the model state. I do not see anything built for this scenario in the store or the model.

The goal of _attributes is actually to save the embedded records, if they stay in dirty state it will have some drawbacks (for example rollbacks).

[sandstrom]

I agree with @bradleypriest, we're also using accepts_nested_attribute_for and are manually appending _attributes in the controller. Similarly, I don't think this Rails-pattern is great, and I'd prefer if EmbeddedRecordsMixin keeps its current behaviour.

I think the addition of formatEmbeddedKey is great, but I'd vote for not changing the default (i.e. make this change an opt-in).

[opsb]

@sandstrom personally I don't think it matters whether the rails conventions are good or not, this adapter is called the ActiveModelAdapter and it should behave as such. That said if there were a formatEmbeddedKey option and documentation then the getting started path will be easy enough(this was my motivation for the PR, people are regularly finding it a PITA to get started with ember-data with rails).

[sandstrom]

@opsb that's a good point! And, on the flip-side, if there were a formatEmbeddedKey method, then people like me could also easily revert, so opt-in or opt-out, it'll be useful either way.

[jurre]

I'm also running into this, and I'm unsure on wether I should handle this on the ember side or the rails side.. I've made a non_stupid_params module that I mix in into my controllers but it feels pretty hacky. It would be awesome if it was a bit more clear on how to handle this!

[fivetanley]

We discussed this PR at the Ember Data meeting today.

Yehuda and Igor are going to discuss this within the next few days. One question we had is "where does _attributes" come from? I think it comes from accepts_nested_attributes_for. Yehuda brought up the example of jbuilder that it will use posts and not posts_attributes.

To be clear, is this for sending, receiving or both?

[jurre]

(in my case) this is for sending from the client to the server (POST and PUTs). It does come from accepts_nested_attributes. I've since been trying to avoid using that in favor of builder/form objects but it is rails' default way of handling this.

[opsb]

@fivetanley yes, exactly as @jurre describes, this is for using accepts_nested_attibutes which is the out of the box rails solution for working with nested attributes. While the asymmetry with active record serialisers is a little irksome it is an effective solution and is the path of least resistance for rails users.

[jurre]

@opsb FYI ROAR looks like a really nice solution that handles both ways, haven't been able to use it yet though.

[opsb]

@jurre I'm personally moving in the direction of ember-orbit with jsonapi-resources(on the rails side). Ember-orbit has the advantage of supporting offline use-cases and while it's stil in development i think will ultimately provide a stronger general solution than ember-data. Look out for an example project integrating the two at orbitjs/orbit#30 (comment).

[tomdale]

Rather than making big breaking changes to fit the Rails case exclusively, let's leave this up to addons to experiment with. Ideally, people can use JSON API easily on both ends and won't need to worry about this case.

[igorT]

This should be much easier to customize now due to #2790