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

[BUGFIX] DS.EmbeddedRecordsMixin support for belongsTo relationships #1637

Merged
merged 1 commit into from May 6, 2014

Conversation

pixelhandler
Copy link
Contributor

See proposal on discuss.emberjs.com and PR #1516, #1633

DS.EmbeddedRecordsMixin handles embedded hasMany relationships, but not embedded belongsTo relationships.

This PR adds in the missing support for embedded objects in a payload that should be extracted/serialized for records with a belongsTo relationship.

  • ActiveModel::Serializers can embed objects in a JSON payload using has_one :xxxx, embed: :objects
  • DS.ActiveModelSerializer only supports embedded hasMany objects and is missing support for embedded belongsTo objects.
  • Supports multiply-nested belongsTo objects as proposed in #1633

@pixelhandler
Copy link
Contributor Author

@bradleypriest Let me know if you have any questions re this PR and your integration test app ( https://github.com/bradleypriest/ams_integration_app )

@bradleypriest
Copy link
Member

Hey @pixelhandler sorry, I meant to get to this in the weekend, but my birthday got in the way, I'll try to take a look tonight, thanks for the work

@pixelhandler
Copy link
Contributor Author

@bradleypriest Happy Birthday!

@pixelhandler
Copy link
Contributor Author

@bradleypriest any update yet?

I've been adding new features for dirty/saved state tracking between embedded and parent records over on my fork. I don't see much traction on the embedded records support within the Ember Data project; so figured I'd just maintain a separate library at: https://github.com/pixelhandler/ember-data-extensions.

Originally when you reviewed some changes I made to support belongsTo relationships, you asked me to refactor the work into the embedded records mixin. From then I've been continuing to add support for embedded records based on applications I contribute to at my day job.

Let me know if I should continue to merge that support (for belongsTo and other dirty/saved state tracking between embedded and parent records) into this PR.

I'd like to contribute to the activemodel-adapter project; so I am curious what the direction is for completing support for the active_model_serializers gem.

@bradleypriest
Copy link
Member

Hey @pixelhandler, sorry about the delay, life tends to get in the way.
I personally am not using embedded records in any projects at the moment so it's hard to get my head around.
Me and @igorT keep trying to organize a time to sit down and flesh it out.
He's done a lot of work getting embedded records working at his current job, probably very similar to your stuff.

@igorT do you mind taking a look at this and seeing how it compares to what you're doing?

@pixelhandler
Copy link
Contributor Author

@igorT do you have a fork w/ support for using belongsTo with embedded records?

@pixelhandler
Copy link
Contributor Author

Can someone provide some direction on this topic?

I'm confused...

  1. On the Ember Data roadmap is: "Out-of-the-box support for Rails apps that follow the active_model_serializers gem's conventions." -> https://github.com/emberjs/data/blob/master/README.md#roadmap
  2. The gem active_model_serializers embeddeds by default. "By default, associations will be embedded inside the serialized object." https://github.com/rails-api/active_model_serializers#embedding-associations
  3. EmbeddedRecordsMixin supports embedded hasMany, but has no support for belongsTo: https://github.com/emberjs/data/blob/master/packages/activemodel-adapter/lib/system/embedded_records_mixin.js
  4. The transition guide: "Explicit support for embedded records is gone for now." -> https://github.com/emberjs/data/blob/master/TRANSITION.md#embedded-records

Right now it stands that there is partial support for (embedded) hasMany relationships, but no support for (embedded) belongsTo relationships. In my opinion, it seems like the support for embedded records needs to be all or nothing. It's confusing to have partial support.

I use the active_model_serializers gem which can use both hasMany and embed an array of objects and also can use belongsTo and embed an object.

@wycats, @tomdale, @igorT - would you mind providing some clarity regarding support for active_model_serializers gem? I would be happy to contribute to that goal listed on the roadmap.

@bradleypriest
Copy link
Member

@pixelhandler I completely understand your frustration.

The original support for active_model_serializers was based on having an ApplicationSerializer with embed :ids, include: true. I thought it was generated by ember-rails but I can only find it here now.

Embedded relation support was dropped from 1.0 betas because of the level of complexity and edge-cases it was running into.

It has had some very basic functionality implemented, but I split it out into the EmbeddedRecordsMixin because there were so many things that it was falling short on, and people were wondering why it didn't just work/

I agree full support is the best option, but adding embedded belongsTo is still the very first step.
Dirty states, ID-less records, sideloading and polymorphism will need solving as well.

I guess what I'm saying it's a hard problem to solve, and as horrible as it sounds, none of us have had the time lately.

If @igorT can find some time to have a look over this PR maybe we can start moving forward again.

At the very least a face-to-face at EmberConf should help kick things off again.

/my 2cents

@pixelhandler
Copy link
Contributor Author

@bradleypriest this PR is only addressing the missing support for embedding belongsTo as for

  • Dirty states, ID-less records, Side-loading (compound documents), Polymorphism

^ these also can be addressed with a library of mixins, like the embedded records mixin.

For the projects I work on we use mixins for dirty tracking / notification of embedded records, basically any embedded record is dirty when parent is dirty and vise-versa. For polymorphism, our db layer is smart we only use a _type attr in the document. For side-loading, we only ever side-load records that will never be embedded. And for now, won't support ID-less records, basically it's just easier to give embedded records an ID.

So starting with the missing support for belongsTo in the DS.EmbeddedRecords mixin my question is...

What is the direction for the activemodel-adapter package, going forward?

I understand that the current iteration only supports embedded records using embed :ids, include: true that works great. And with using the mixin, has_many: :model_name embed :objects can be used too. If you'd like to add support for has_one :model_name, embed: :objects then this PR should be considered, reviewed and merged. Then, I'd be happy to add more to the project for the default behavior of the active_model_serializers gem; perhaps some of the mixins we use for dirty tracking and documentation on how to support various model relationships using embedded records.

I already support embedded records for the daily work I do and we've shared that in a repo for others to use as well. The main reason I opened PRs for the DS.EmbeddedRecordsMixin is that like others I'm confused about the support for active_model_serializers gem using the activemodel-adapter package. I'd like to contribute to the project based on the work we've done for our ember apps. However if the activemodel-adapter package feature set is complete with only support for embed :ids, include: true then the package is done. I think that the docs should note exactly what 'out of the box support' using the ActiveModel::Serializers gem means. I'd be happy to add that info here: http://emberjs.com/api/data/classes/DS.ActiveModelAdapter.html as well.

So I'm not frustrated, I'm very happy using Ember Data and the Adapters provided, as well as using the adapter/serializer API to do more. I'm confused about the direction for the mixins that come with the activemodel-adapter package. I can help others using the active_model_serializers gem. For now I have a fork and do support embedded records there. I'd be happy to contribute that work to the Ember Data project. When I hear we don't have time for embedded records that translates to A) low priority, may never happen B) important, but we don't have enough people we trust using embedded records to make it a priority. I'm asking how I can help, either document clearly what the support is and continue with a separate project or iterate on the activemodel-adapter package and take some ownership of that work.

@bobbus
Copy link
Contributor

bobbus commented Feb 16, 2014

I would love to see remark from @pixelhandler considered, we truly need embedded records in our application and current state of embedded records in master is one reason wich prevent us from upgrading ( we are on an hacked version of 0.13 ).

It's been long time without a post about ember-data in the Ember.js Blog, an update about roadmap and goal would be really interesting.

@pixelhandler
Copy link
Contributor Author

@bobbus perhaps make the upgrade for your Ember Data use by using the mixins here: https://github.com/pixelhandler/ember-data-extensions

@bobbus
Copy link
Contributor

bobbus commented Feb 16, 2014

@pixelhandler , thanks a lot for sharing, your code looks LOT more cleaner than hacks we added in 0.13.

I'm not surprised as I know code on master is full of improvments and it's great that it's going further each week :-)

That said, added to the embedded objects feature, we are still hanging our app upgrade because of issue #1308 and #1288 .
We have custom (weird) hacks in 0.13 to support this and, as we can't know how much time it will take to get the same behavior on master, we are delaying the update until this 3 issues are resolved.

Hopefully we will upgrade soonish, thanks to your work and maybe single-source-of-truth branch :-) !

@aortbals
Copy link

@pixelhandler I just wanted to thank you for working on this and releasing ember-data-extensions. I also wanted to validate that IMHO, embedded belongsTo associations are very common and this would be valuable included in Ember Data.

To give a little perspective on what I used this for:

I wanted to consume the Github API for events which includes embedded records like this:

[
  {
    "type": "Event",
    "public": true,
    "payload": {
    },
    "repo": {
      ...
    },
    "actor": {
      ...
    },
    "org": {
      ...
    },
    "created_at": "2011-09-06T17:26:27Z",
    "id": "12345"
  }
]

I first attempted using DS.EmbeddedRecordMixin when I quickly realized that it does not support belongsTo one-to-one associations (which isn't immediately obvious). I can validate that this worked well. I also value the mixin approach so that anyone using this functionality is willing opting in. It would be great to see belongsTo support merged into Ember Data.

@emk
Copy link

emk commented Mar 5, 2014

I had a local fork of DS.EmbeddedRecordMixin with support for belongsTo one-to-one associations, because it's absolutely necessary for one of my client's projects. But local forks are expensive to maintain, and I would love to see this PR go into DS.EmbeddedRecordMixin.

This PR by pixelhandler is actually a pretty simple fix: It adds support for ActiveModel::Serializer's has_one :model_name, embed: :objects that works exactly like the existing support for has_many: :model_name embed :objects. There's a nice test suite and some documentation. It doesn't get into the the more advanced features like ID-less child records, but that should probably be handled separately anyway: It's a very different use case, because ID records aren't normal DS.Model objects the way has_one models are.

We've gone through quite a few versions of this pull request: #1516 and #1633, for example. I know that nobody has any cycles to look at this. But there are quite a few of us who'd love to help out. Is there anything we can do? Answer questions? Write more unit tests? Write more docs? Testify "Yes, I use this code in production and this is the correct solution?"

Thank you for taking the time to look at this, and for such an excellent library!

@pixelhandler
Copy link
Contributor Author

I don't believe Ember Data will support embedding records over the long haul, closing this PR.

@wycats
Copy link
Member

wycats commented Mar 17, 2014

I think we probably will support embedded records in the long haul; I outlined the issue in #53 (comment), and we should discuss it more.

@pixelhandler
Copy link
Contributor Author

@igorT @rjackson per our chat in #emberjs-dev today I rebased this branch on master and reopened PR #1637 - Fixes DS.EmbeddedRecordsMixin (missing) support for belongsTo relationships.

@pixelhandler pixelhandler reopened this Apr 12, 2014
if (belongsTo) {
key = this.keyForAttribute(key);
json[key + "_type"] = Ember.String.capitalize(belongsTo.constructor.typeKey);
json[key + "_type"] = capitalize(camelize(belongsTo.constructor.typeKey));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase bug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradleypriest thanks, I missed that one, I fixed and pushed correction.

@pixelhandler
Copy link
Contributor Author

@igorT I added two commits per our pairing session today, I can squash but thought it would helpful to see the changes we made. We added removeInverseKey method to the EmbeddedRecordsMixin which removes the parent (foreign) key from the serialized (embedded) record. I checked the default behavior of active model serializers gem and this is consistent. So in the case where the server's serialization method uses embed_key: :external_id then the method for removeInverseKey may need to be set as a no-op (Em.K) method.

Link to your gist for open question to community re flags for embedded 'always' vs more specific serialize/deserialize flags. Proposed flags for serialize/deserialize: https://gist.github.com/igorT/ec4d5eadefe08fa90274

@pixelhandler pixelhandler changed the title DS.EmbeddedRecordsMixin support for records using a belongsTo relationship [BUGFIX] DS.EmbeddedRecordsMixin support for belongsTo relationships Apr 23, 2014
@bakura10
Copy link
Contributor

I'm using this in production too using my fork of Ember-Data, I'd love to have this merged. It's a pain to maintain my own copy of Ember-Data :).

@axelguiloff
Copy link

Our team has had to come up with some customizations to get this working also. Would love to see this PR merged.

@pixelhandler
Copy link
Contributor Author

@bakura10 @axelguiloff @igorT just need to finish off serializing options in the attrs property and this PR should be ready to merge.

@pixelhandler
Copy link
Contributor Author

@igorT I added the attrs option for serializing ids or records with the embedded: 'always' setting acting as shorthand for {serialize: 'records', deserialize: 'records'}. When using the mixin deserialize: 'records' is the default (for extracting records from an embedded payload). So an option to embed ids when serializing can be done with {serialize: 'ids'}

This PR is ready to merge when you give thumbs up. Thanks for the feedback yesterday.

@pixelhandler
Copy link
Contributor Author

@rjackson Do you know why the Travis build is failing? The command "grunt test:all" exited with 127. when I run grunt test:all locally it's all green.

@gabelimon
Copy link

Any status updates on this?

@pixelhandler
Copy link
Contributor Author

@gabelimon I chatted with @igorT last week, he needs to give this PR one more review then said he can merge it in.

@gabelimon
Copy link

Thanks!

}
if (includeIds) {
key = this.keyForRelationship(attr, relationship.kind);
json[key] = get(record, attr);//.get(this, 'primaryKey');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that should be deleted

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would want the commented out code, because we want to get the id, not the whole object?

- active_model_serializer gem `has_one` setting allows embedding objects
- Add attrs options for serialize/deserialize using records or ids
- Add attrs options for not serializing relationships using serialize:no
-Add a hook called removeEmbeddedForeignKey for optional removal of foreign key reference from the embedded record
@igorT
Copy link
Member

igorT commented May 6, 2014

We added way more lines of tests and comments than actual code. 👍 @pixelhandler

igorT added a commit that referenced this pull request May 6, 2014
…gs-to

[BUGFIX] DS.EmbeddedRecordsMixin support for belongsTo relationships
@igorT igorT merged commit ad3af80 into emberjs:master May 6, 2014
@jdjkelly
Copy link
Contributor

jdjkelly commented May 9, 2014

This is swell.

@gabelimon
Copy link

Maybe I'm just being an idiot here but how would I go about using some model for embedded data without an ID? I believe the data you're mocking for in your comments would be something like this.

{
  post: { 
    id:1,
    comments:[{ id:1, content: 'hello'}, {id:2, content:'goodbye'}],
    author: {name:'Gabe',email:'gabe.limon@tripit.com'}
  }
}

So is there currently some way to use a model that I've defined for author? Something like

App.Author = DS.Model.extend({
  name: attr('string'),
  author: attr('string')
});
``

@pixelhandler
Copy link
Contributor Author

@gabelimon models must have an ID! So, you can use a plain object with a raw transform or perhaps just using author: DS.attr() but this data would not be considered an embedded record. Perhaps post support questions on stack overflow and bring up any issues in the irc channel as well. This PR is closed and merged; so discussion should not continue on this thread for support topics.

@gabelimon
Copy link

Thanks! I'll be sure to follow up in more appropriate forums next time.
On May 16, 2014 5:10 PM, "Bill Heaton" notifications@github.com wrote:

@gabelimon https://github.com/gabelimon models must have an ID! So, you
can use a plain object with a raw transform or perhaps just using author:
DS.attr() but this data would not be considered an embedded record.
Perhaps post support questions on stack overflow and bring up any issues in
the irc channel as well. This PR is closed and merged; so discussion should
not continue on this thread for support topics. Best, Bill.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1637#issuecomment-43391527
.

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

Successfully merging this pull request may close these issues.

None yet