-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added support for polymorphic associations. #750
Conversation
+1 |
var id = hash[key+'_id']; | ||
|
||
if (id) { | ||
return {id: id, type: hash[key+'_type']}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be a separate hook to get the sufixes naming. keyForPolymorphicId
/ keyForPolymorphicType
. And the underscore convention should be exposed in RESTSerializer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I'll fix that.
@tchak I have updated the code to reflect your comments |
yes dear lord I want this :) |
@wycats this is good for review. Let me know what you think. |
@cyril-sf I'm testing this branch out now as I need polymorphic associations ASAP. After building ember-data from your branch and updating my store revision to 12 I'm getting some funny behaviour with belongsTo associations. If the store does not have the ID of the model for the association in its store, it doesn't automatically fetch the record from the server. |
@seanrucker do you have a jsfiddle? |
@seanrucker does the behavior you describe for a monomorphic or a polymorphic association? In the network panel of your browser do you see a request being fired? A jsfiddle would actually be a great help to figure out what's going on, or at least the definition of your models. |
Its just a monomorphic association. In the network panel there is no request being fired. The only changes I made were updating the ember-data.js to one I built from your branch and updated the store revision to 12. I can try to put a jsfiddle together. What's the best way to get a jsfiddle environment setup with the proper libraries? |
I repeated the same steps using a fresh build of the latest ember-data on master and the problem did not exist. So it would appear that it is indeed related to this branch. |
@seanrucker I can reproduce it, I will debug and let you know. |
@seanrucker I've fixed that issue. This branch doesn't pass on Travis because I rebased without realizing that master is broken. |
Awesome! Thank you so much for your hard work and contributions. I'll be putting this feature through its paces. I'll let you know if I find anything else. Thanks again. |
@seanrucker Thanks for taking the time to report the issue! |
Added support for polymorphic associations.
@cyril-sf The serializer is always instantiating the base type. I must be missing something.
A call to the endpoint /conversations/1 returns something like this:
Which triggers a call to /messages?ids[]=1 and returns something like this:
The issue is that Ember is serializing the message as App.Message rather than App.DocumentMessage. What am I missing? |
@seanrucker The payload you get from your server isn't valid for polymorphic associations. The payload you receive from your server needs to be like this: {
"conversation": {
"id": 1,
"messages": [{"id": 1, "type": "document_message"}]
}
} If the server doesn't return the type with the id, the framework has no way to figure out which model to instantiate. I'll check if I can have a good error message for that case. Let me know if that fixes your problem. |
@cyril-sf I've got the payload returning as described but now I'm getting this error:
I've tried setting up the alias as described in the PR comments but it doesn't seem to be working:
|
@cyril-sf I've got it working now. I looked through your tests and changed the way I'm creating the aliases:
|
@seanrucker Good to hear that things are working now. Maybe you configure your adapter too late. There should be no reason to configure the serializer instead of the adapter. Check this code: DS.RESTAdapter.configure('App.Post', {
alias: 'post'
});
DS.RESTAdapter.configure('App.Comment', {
alias: 'comment'
});
App.Store = DS.Store.extend({
revision: 12,
adapter: DS.RESTAdapter.create()
}); https://github.com/Cyril-sf/ember_data_example/blob/polymorphism/app/assets/javascripts/store.js I believe you need to configure the adapter before the store instantiates it. |
I need this feature. What's the status of it? |
Awaiting this as well 👍 |
Tried this pull-request. Works like a charm, with some minor issues: Does not work for root collectionsI can't do following: Does require backend to deliver types along with IDsIt became apparent that type information is required before actual object will be loaded, hence Parent need to specify IDs. In sample above:
Note that type is on the message_ids which is a significant problem - type should be defined not on the parent but on child nodes, parent should't be aware about the types of children. Need additional tweaking based on loading polymorphic childrenIMO polymorphic children should be loaded from one resource (collection). For example in sample above polymorphic Post and Comments should be loaded from /api/1/messages as well as created/updated into the /api/1/messages too and not into /api/1/posts and /api/1/comments. |
@zubairov what you are describing seems to be single table inheritance, where you actually have a unique ID for a message and you can deduce the type from it. The problem I can see with it is that you can't get a promise back because you don't know the type of your object at that moment. I'll try to think about it and see how this can be solved. |
@cyril-sf exactly, there are different approaches on how to map inheritance into the JSON structures, some of them like in your sample are using two different resources (comments & posts) and some are served from the same resource (single resource inheritance ;) ) but with a distinct identifier. BTW we really need this feature in our product and we ready to invest time in it, so I would be happy to help developing it and contributing back to your repository and/or ember-data. |
@zubairov there are a couple of issues with identifying by id only. Non-shared Id-spacesPolymorphic associations may not share an id-space. If your server happens to be backing the association using single table inheritance on a relational database, you'll get a unique id-space but it's just a side effect of the implementation. In principle we could make it possible to have a custom adapter that retrieved from PromisesThe bigger issue is the one @cyril-sf mentioned. In something like rails, it's fine to say Root TypeGetting I hope I've made it clear why we pair ids with types. Do you still think it's a significant problem for association references to need to be id, type tuples rather than just ids? Any other thoughts? |
This is a really useful PR. +1 on working out the remaining niggles. I'd also add that this strategy is appropriate for not only STI but also Multiple Table Inheritance (Class Table Inheritance). I think the above concerns are valid but probably don't describe the way that the majority of people would architect a web app. As such it might be a good idea to try and progress this as first implementation and then iterate for the more advanced use cases? FYI - using a unique ID for an object type is the way of handling this in Doctrine (http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/inheritance-mapping.html) and Hibernate (http://docs.jboss.org/hibernate/orm/3.3/reference/en/html/inheritance.html) - although hibernate offers insane customisation that could be used to have one type per table, leading to the above issue. Never seen it in IRL though. Edit: Having re-read the thread it seems like second guessing others architectural decisions is probably something I shouldn't do - looks to be enough variation in this thread alone to justify further discussion. FWIW I have the same schema and concerns as @zubairov (using class table inheritance). |
@hjdivad @cyril-sf @calumbrodie thanks for good discussion! Shaed/Non-shaed ID spacesAbsolutely agree with you @hjdivad it is indeed a huge problem in ORM as separate relational database primary keys are not consistently unique across all database. In the REST world situation is different. As I wrote in my initial comment approach and expectations on which ember-data is based are more dictated by relational (ActiveRecord-style) backend then REST backend. I believe both approaches have their place however once ember-data no longer talking to database we should target more REST-like or to be more precise Hypermedia-driven states. PromisesGot your point here. It is obviously not a simple problem, however I think Ember-backed infrastructure is there to help (especially in JavaScript-land). Root typesI believe in the @hjdivad sample it should return a object of Account's subtype. |
@heartsentwined Because you call
|
Thanks for the clarification @cyril-sf, I shall wait on your example app for more implementation guidance. |
|
||
aliases.forEach(function(key, type) { | ||
plural = self.pluralize(key); | ||
Ember.assert("The '" + key + "' alias has already been defined", !aliases.get(plural)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this assertion needs to also check the aliased type -- same as what's being done on line 1092 below.
Ember.assert("The '" + key + "' alias has already been defined", !aliases.get(plural) || (aliases.get(plural) === type) );
@cyril-sf, the server payload key in the RESTAdapter for the polymorphic hasMany seems to have to be 'message_ids' and not 'messages' (e.g. below). Does this sound right?
|
@kylenathan correct. There is no specific code for the key of a polymorphic |
@cyril-sf I've run into an issue when a model has multiple polymorphic associations - a Post can be Commentable, Attachable, and Notifiable - what is the best way to handle that right now? I've partly worked around it by having each of my "polymorphic" models extend from the last but this means I have no control over which attributes appear on each model. |
@kevinansfield That's a good question. I would try to declare them as mixins. App.Notifiable = Ember.Mixin.create({
notifications: DS.hasMany('notifications')
});
App.Notification = DS.Model.extend({
notifiable: DS.belongsTo('App.Notifiable', {
polymorphic: true,
inverse: 'notifications'
})
});
App.Commentable = Ember.Mixin.create({
comments: DS.hasMany('comments')
});
App.Comment = DS.Model.extend({
message: DS.belongsTo('App.Commentable', {
polymorphic: true,
inverse: 'comments'
});
});
App.Post = DS.Model.extend(App.Commentable, App.Notifiable, {
}); I haven't tried this and I don't know if you would run into more problems. |
@cyril-sf Thanks. That does look like a nice way to handle it but unfortunately the app errors with the following: And this is the code I'm using: App.Commentable = Ember.Mixin.create
comments: DS.hasMany('comments')
App.Comment = App.Model.extend
commentable: DS.belongsTo 'App.Commentable',
polymorphic: true,
inverse: 'comments'
App.Notifiable = Ember.Mixin.create
notifications: DS.hasMany('notifications')
App.Notification = App.Model.extend
notifiable: DS.belongsTo 'App.Notifiable',
polymorphic: true
inverse: 'notifications'
App.Attachable = Ember.Mixin.create
attachments: DS.hasMany('attachments')
App.Attachment = App.Model.extend App.Commentable, App.Notifiable,
attachable: DS.belongsTo 'App.Attachable',
polymorphic: true,
inverse: 'attachments'
App.Post = App.Model.extend App.Attachable, App.Commentable, App.Notifiable, |
A jsfiddle might help. Have you also changed notifications: DS.hasMany('notifications')
attachments: DS.hasMany('attachments') to notifications: DS.hasMany('App.Notifications')
attachments: DS.hasMany('App.Attachments') ? |
Yes, I have also changed those. I'll see what I can do about sorting out a fiddle. |
@cyril-sf @kevinansfield you should probably get this working with Something like: DS.RESTAdapter.configure('App.Notification' {
alias: 'notification'
}); |
Does anyone has a working example for the reversed rails polymorphic models yet? /cc @heartsentwined @seanrucker @cyril-sf I'd be happy to help, but I can't get this working just yet. |
@ayrton I've started on updating the sample project I have to use this. It's not ready yet. |
@cyril-sf I'm trying to setup some polymorphic associations and am encountering the following error:
I tried to add the configurations to the adapter, and got this error:
When I dumped the aliases variable, it was a mapping with the following values under keys:
Shouldn't it be
? Have you ever encountered this issue before? |
@pzuraq There is a bug when detecting if an alias has already been defined. I have a fix and need to make a PR. How do you define your aliases? |
@cyril-sf I've been following the examples above, so like so:
However, it seems to me that the problem is in the creation of the aliases mapping. At least in my case, the aliases mapping is initially populated with only the plurals, not the singular forms of any of the aliases. Pluralize is redundant in this case, unless I'm missing something. I was trying to find out how the |
I've been struggling with this aswell, any news? |
@cyril-sf Still having this issue, I've been looking for days but I can't find the source. Can you or someone who knows explain how the |
@triptec I'm currently overriding DS.Serializer.reopen({
_completeAliases: function() {
this._singularizeAliases();
this._reifyAliases();
},
_singularizeAliases: function() {
if (this._didSingularizeAliases) { return; }
var aliases = this.aliases,
sideloadMapping = this.aliases.sideloadMapping,
singular,
self = this;
aliases.forEach(function(key, type) {
singular = self.singularize(key);
Ember.assert("The '" + key + "' alias has already been defined", !aliases.get(singular));
aliases.set(singular, type);
});
// This map is only for backward compatibility with the `sideloadAs` option.
if (sideloadMapping) {
sideloadMapping.forEach(function(key, type) {
Ember.assert("The '" + key + "' alias has already been defined", !aliases.get(key) || (aliases.get(key)===type) );
aliases.set(key, type);
});
delete this.aliases.sideloadMapping;
}
this._didSingularizeAliases = true;
}
}); |
@triptec @pzuraq @seanrucker I've been busy lately, things should get back to normal this week |
I never really understood why does polymorphism support is added on associations. Otherwise we need to have dirty hacks like these. Polymorphism in JSON, as I understand it, is basically a reserved object property (ideally configurable) which value defines what concrete instance to instantiate. Well known libraries like Jackson do this, and I think this covers more use cases. If we're defining polymorphism on associations we're never going to get polymorphism in root objects, without associations to them. A children hasMany polymorphic collection of models shouldn't be different from querying a base polymorphic collection of models. Am I missing something? |
Is this feature documented? I struggle to understand how it works, but i failed to find a single working JSBin demo. |
Okay, i got it working for a polymorphic hasMany: http://emberjs.jsbin.com/rivav/4/ And here's a working example with the DS.EmbeddedRecordsMixin: http://emberjs.jsbin.com/rivav/8/edit?html,js,output Thank you for the awesome functionality! ^_^ |
You need to configure the serializer to map to the correct type:
The expected payload for a polymorphic association with the REST adapter/serializer should contain the type:
Initializing an alias will automatically allow you to sideload this
type. The support for
sideloadAs
is only for backward compatibility.Two hooks exist for the serialization and materialization of an embedded
polymorphic association:
addType
that adds the record's type to the serialized dataextractEmbeddedType
that retrieves the record's type from thepayload.
Both hooks rely in the JSONSerializer on
keyForEmbeddedType
, which returns the string used torepresent the record's type in the payload. The default value is
'type'