Relationships within a fragment

dev-inigmas commented 10 years ago

Great work on the fragments library so far; I've found it very helpful in upgrading from the old Ember Data to the new 1.0 Beta series.

However, I am wondering if we could re-enable relationships inside of fragments. There are several instances in my data-model where my fragments "point to" other models. Before, they would use a "belongsTo" relationship; but now that doesn't seem to work. I've found workarounds, but they haven't been seamless for all use-cases.

This is the scenario where I'm most frequently running into this issue:

A many-to-many relationship between models A and B where A has a list of fragments, C. C has the "belongsTo" relationship with B along with other fields that "describe" the relationship. B, meanwhile, doesn't know anything about A or C.

My current work-around looks like this:

App.Transaction = DS.Model.extend
  purchases: DS.hasManyFragments 'purchase'
  ...

App.Purchase = DS.ModelFragment.extend
  product_id: DS.attr 'string'
  cost: DS.attr 'number'
  quantity: DS.attr 'number'
  product: (() ->
    @store.find('product', @get('product_id'))
  ).property('product_id')

App.Product = DS.Model.extend
  name: DS.attr 'string'
  ...

However, if Transaction needs to compute a property based on the Products' name (to list all products purchased, for instance), then I'm suddenly forced to implement a much more obscure work-around to get the Transaction's computed property to update in templates. I'm pretty sure that this is due to the fact that store.find returns a Promise.

If someone has a much better work-around to this issue, please let me know. Right now, it seems to me that allowing Fragments to have "belongsTo" relationships is the easiest way to correct the problem while keeping DS.ModelFragment's usage similar to DS.Model's current usage.

Glavin001 commented 10 years ago

+1 I have yet to personally have this issue, however I know I will soon be using Relationships (hasMany) inside of fragments, so if this is in fact an issue I hope it can be resolved soon!

slindberg commented 10 years ago

@dlpatri Fragments don't currently have any of the model plumbing that allows them to have relationships, so I'm surprised that a DS.belongsTo property on a fragment ever worked for you.

I can definitely see the argument for relationships to models within fragments. Since fragments are conceptually portions of a DS.Model record, these relationships would in reality be relationships to the fragment's owner record. Because of this, much of the relation logic from DS.Model isn't reusable, unlike a great deal of fragment behavior. This means a lot more custom code, likely having to override core ED methods completely, (as opposed to extending, which this plugin does for the most part), and in turn creating a greater maintenance cost and much higher risk of core regressions.

Unfortunately I'm not going to be able to put effort into this anytime soon partly because I just finished a sprint devoted to this plugin and have other duties, but mostly because the work will just have to be re-done when the SSOT branch gets merged and other plans completely change how relationships work. That, and of course my feeling that there's a whole host of edge cases that would make this a difficult task to implement correctly. I'm always open to fully-tested feature pull requests though!

In regards to your current issue/workaround:

Firstly, are 'purchases' really not first-class models in your API? If they are, then I think the DS.EmbeddedRecordsMixin is really the solution you are looking for.

If 'purchases' are indeed serialized solely through your 'transaction' endpoint and have no unique identifiers, you are stuck with workarounds for a while. However, if you can guarantee that the related products are loaded into the store before you access any product property, you can simply replace find with getById, which returns a full record object that you can use with other computed properties. Also keep in mind that with the upcoming changes to ED, all relationships will become async and the computed property issue will be solved using DataBoundPromises.

slindberg commented 10 years ago

I'm also labeling this issue with the 'discuss' tag in order to encourage more input from users of the plugin.

Glavin001 commented 10 years ago

Firstly, are 'purchases' really not first-class models in your API? If they are, then I think the DS.EmbeddedRecordsMixin is really the solution you are looking for.

I was unsuccessful in getting DS.EmbeddedRecordsMixin working. Like @dlpatri's usage requirements, my ModelFragments do not have IDs, which I believe was the requirement for using EmbeddedRecordsMixin.

I really hope that we can create a fix for this! I'd be up for submitting a Pull Request although I am not as familiar with Ember Data's source code, nor ember-data.model-fragments. If someone could point me in the right direction, I will try and implement it ASAP this week. Thank you!

My Goal

May it be nested hasMany or hasOne or belongsTo in a model-fragment. My goal is to be able to model any MongoDB/other schema structure comprised of documents (Model), embedded documents (Model Fragment), and referenced documents (relationship).

App.Transaction = DS.Model.extend({
  purchases: DS.hasManyFragments('purchase')
});

App.Purchase = DS.ModelFragment.extend({
  cost: DS.attr 'number'
  quantity: DS.attr 'number'
  products: DS.hasMany('product')
});

App.Product = DS.Model.extend({
  name: DS.attr 'string'
});

dev-inigmas commented 10 years ago

@slindberg, Thanks for the quick and thorough response. The belongsTo worked before I switched over to Fragments; both during the old ED version and while I was coercing DS.EmbeddedRecordsMixin to function with my embedded document "fragments". 80-90% of the time, "model-fragments" seems to be a better solution. I was just hoping for an easy way to close that gap.

Purchases are indeed fragments, I have no reason to store them apart from Transactions (in a relational database, I'd use a separate join table; but I'm not using a relational database). Also, this was just one of the many examples my app has for needing features like this. I figured that it would make things easier to think about than continuing with the A, B, C terminology.

I'll have to research more about the SSOT branch, as well as the ED road map. Until then, I'll try out the getById function you mentioned. The Products are being side-loaded as a part of this request, so I think it ought to work.

Thanks again.

slindberg commented 10 years ago

@Glavin001 Here's the things that would have to happen off the top of my head.

Minimum necessary:

Fragments have to have their relations initialized, which is difficult since the store currently does this in the normalizeRelationships function (not to be confused with the DS.RestSerializer method of the same name). That function is called from both DS.Store#push and DS.Store#didSaveRecord, and is inaccessible and therefore can't be overridden or even reused.
Relationship methods will need to be added to CoreModel and adjusted if necessary.
DS.Model#suspendRelationshipObservers will need to be overridden to support fragments.

Support for inverse relationships:

Semantics for specifying inverse relationships must be defined, e.g. DS.belongsTo('purchase', { inverse: 'purchase.product' }). This is also important for relationship metadata which makes serializing inverse relationships possible.
Setting a fragment property or adding/removing fragments in a fragment array must update inverse relationships. Here be dragons. Right now this is handled through change objects which are highly specific and complex, not to mention that SSOT is completely replacing them. Currently relationship order is not important to ED, but I can see the need for defining ordering semantics when it comes to relationships on fragments. I'll update this comment as I think of more things that need to happen.
Defining behavior when adding a record to an inverse relationship, or making those relationships read-only.

dev-inigmas commented 10 years ago

Quick question.... Shouldn't the default case for a fragment's relationship be such that there can't be an inverse?

For instance.... why should the Product care about which Purchases it may, or may not, be involved in. And without an ID on the Purchase, how would you point it at the correct Purchase anyway?

Personally, I don't think I need anything more complicated than the work-around I provided above. Just a short-cut that allows you to get/set (a) Product(s) into a fragment. And a reasonable implementation for looking up that Product as we "deserialize" the product_id(s). Preferably one that doesn't break computed properties in Transaction.

Thoughts?

slindberg commented 10 years ago

Quick question.... Shouldn't the default case for a fragment's relationship be such that there can't be an inverse?

My experience on the subject tells me that the minute after you make that concession, you will regret it. However I am open to an initial implementation that does not include inverse relationships.

For instance.... why should the Product care about which Purchases it may, or may not, be involved in. And without an ID on the Purchase, how would you point it at the correct Purchase anyway?

Since the relationship is not Purchase → Product, but rather Transaction → Product, the product would get a Transaction ID. However, point taken: adding a Transaction to a Product would have undefined behavior, so the inverse relationship would have to be read-only anyway (which isn't a feature of record arrays currently).

It remains to be seen just how simple a 'get/set shortcut' would be to implement. I'll update my comment above to reflect inverse and no-inverse tasks.

dev-inigmas commented 10 years ago

Personally, I think that the real "concession" happened when people decided that inverse relationships were a good thing. Seems like a circular-dependency to me (hence the difficulty in managing it).

Is there a decent OO/Schema design where you have a many-to-many relationship and both sides really need to know about one another? I can't think of any off hand. Like I said before, in an RDBMS, you would typically solve the problem with a join table. Then neither entity in the many-to-many relationship "knows" about each other.

But this is just my opinion. It's been too many years since I've read up on this sort of theory. This is your project, so in the end it's your opinion that matters here. I am glad that you're open to a very simple initial implementation.

slindberg commented 10 years ago

I involuntarily cringe when I hear 'many-to-many' :stuck_out_tongue_closed_eyes:

So yes, I think in this specific Transaction/Purchase/Product case there's limited use for inverse relationships. However, many-to-one relationships are common, manageable, and very useful, and in those cases inverse relationships are difficult to do without (IMHO). So if we were to pretend that Transaction → Product was a many-to-one, the behavior of setting a product's transaction would still be undefined for fragments with relations.

Please, continue to share your opinions! One of the greatest things about Ember isn't Ember itself, but the community's inclusive attitude. I've never seen any other project use the community's collective experience to guide decisions quite as well as Ember has. I want to keep that spirit for this project as well.

Glavin001 commented 10 years ago

I am developing here: https://github.com/Glavin001/ember-data.model-fragments/tree/i18

Writing unit tests right now, then going to try and build a basic implementation for hasMany and belongsTo. As @slindberg mentioned, there are other relationship changes and the SSOT branch coming up, so hopefully this doesn't take too long to build a temporary fix.

Let me know if anyone is interested in helping me out! I've been reading through a lot of Ember and Ember Data source code recently, however there's still much to learn. Any help would be greatly appreciated.

Glavin001 commented 9 years ago

@slindberg it's been a while. I thought I'd post back here and see what your thoughts are on this issue? I was not able to implement this fix/feature at https://github.com/Glavin001/ember-data.model-fragments/tree/i18 however I did add some failing tests, hope that helps. Is there any chance this feature could be developed by someone more experienced with the inner workings of Ember / Ember Data? Thank you in advance.

slindberg commented 9 years ago

Although relationships have undergone a large refactor, and a few of my earlier bullet points are no longer issues, I think that adding support for fragment relations will still require a great deal of effort and require the use of many more private APIs. I haven't been following the refactor very closely, so I'm not as familiar as I used to be with how relationships are handled internally. I do know that many internal APIs have shifted recently as part of an ongoing effort to improve performance, which makes me even more nervous about relying on them. But aside from that, the sheer complexity it would introduce in keeping track of relationships to the primary record as fragments are added/removed/updated (and especially merged) is daunting.

This is not a blocker for me, and I don't have the bandwidth to attempt a solution (and am unlikely to in the future), but I will continue to encourage others to contribute. I know this was not the answer you were looking for, sorry man :confused:

joelalejandro commented 9 years ago

I'm not particularly fond of the idea of involving relationships here, as I believe fragments are supposed to be simple objects, strictly depending of a parent Model.

I've been using Model Fragments for a project at my workplace and immediately met the relationships' issue with fragments. Since I was using it to create a large object comprised of several directly-related subobjects, if any foreign keys were required, I declared those as common attributes and that was it. That was all the data I needed in order to persist data.

louy commented 9 years ago

Personally I believe that ember-data relationships are a mess. I have switched to using a computed property instead. So for each relationship I have two model attributes: relatedModelId and relatedModel, where the first is a string and the second is a computed property.

I don't think you should consider supporting relationships before they even decide what the want to do with them. The current implementation is broken and does not support dirtiness for example.

https://github.com/emberjs/rfcs/pull/21

christophersansone commented 9 years ago

+1. I use this library specifically to avoid the complexity of ED relationships for small simple models. The majority of use cases to reference a DS.Model within a fragment can be done with a simple computed property on the model fragment.

The OP is correct that it may be cumbersome if the linked model must be fetched. Most of the time this should be done in the route's afterModel, and the consensus in the Ember community is that this is where the data should be fetched anyway. Then the CP to select the records from the store becomes trivial because it is synchronous. In use cases where the relationships still need to be async (e.g. due to data size), it will require a bit more effort, but in my opinion it is still manageable. Baking in ED relationship support just for this use case is not worth the added complexity to the library.

At the very least, I don't think this should block v1.0.

Kilowhisky commented 5 years ago

Just to throw my 2c in here i think now that ember has recordData & the hasmany, and belongsTo reference classes this can now be achieved.

Here's just a simple example of a very complex object i have been trying to represent with fragments.

{
  "Id": "attribute",
  "Comment": "attribute",
  "Creator": "BelongsTo('user')",
  "ConditionGroups": [
    {
      "Id": "attribute",
      "Logic": "attribute",
      "Revision": "BelongsTo('parent model')",
      "Conditions": [
        {
          "Group": "BelongsTo('parent model')",
          "Id": "Identifier",
          "Logic": "attribute",
          "RelationalOperator": "property",
          "Type": "belongsTo('type of condition')",
          "TypeAttribute": "belongsTo('type of attribute')",
          "Value1": "attribute",
          "Value2": "attribute"
        }
      ]
    }
  ],
  "Outputs": [
    {
      "Id": "attribute",
      "Repeat": "attribute",
      "Revision": "belongsTo('main parent')",
      "Type": "belongsTo('type of output')",
      "Value1": "attribute",
      "Value2": "attribute"
    }
  ],
  "Priority": "attribute",
  "Rule": "belongsTo('main parent')"
}

So far I've been solving this using the embedded records mixin but that has its own problems. For example if i edit this structure and then try to undo the edits, ember literally cannot do it, i have to reload the entire page.

EDIT: also @louy 's strategy works too. I actually might investigate using that across the entirety of ember...

sdhull commented 5 years ago

I don't have specific knowledge around this but maybe someone more knowledgeable (@igorT?) could speak to this: now that ember-data has matured somewhat & with the introduction of recordData & the hasmany, and belongsTo reference classes (as pointed out by @Kilowhisky) is it more feasible to support having belongsTo & hasMany in Fragment classes?

adopted-ember-addons / ember-data-model-fragments

Relationships within a fragment #18

My Goal