Fix for passing fragment arrays to fragments

[patocallaghan]

Bringing this PR over from https://github.com/richgt/ember-data.model-fragments/pull/1 as I can't redirect the PR to a different repo.

@richgt @igorT This is a potential patch fix for the issue I mentioned I've been encountering with the RecordData refactor.

The use case

I previously mentioned:

Model Fragments doesn't provide an API to create Fragment Arrays (see issue #272) so a workaround we use in our app in cases where we want to set a fragmentArray (and possibly what Factory Guy also does) is to create a fragment using store.createFragment with a fragment array on it. We'll then take that fragment array property value and pass it to a store.createRecord call elsewhere. This is an extremely simplified case below.

let parentClone = store.createFragment('parent', {
 children: [{
   name: 'Pat'
 }]
});
let grandparent = store.createRecord('grandparent', {
 parent: {
   children: parentClone.children,
 }
});

This issue is also causing us a lot of problems in how we bootstrap our tests as we use Factory Guy quite heavily. We'll do something like the following a lot where make will create our models and model fragments for us. In the following example grandparent.parent.children is a fragment array.

test('grandparent works with factory guy', function(assert) {
    let grandparent = make('grandparent', {
      parent: make('parent'),
    });
    assert.equal(grandparent.parent.children.firstObject.name, 'Pat', 'Name is Pat');
  });

The problem

The specific problem appears to be that in current master when pass the children fragment array to the new model it doesn't try and normalize the fragment array a second time, but in the refactor it does. The error it throws is the following if you try to pass a fragmentArray to another model.

Error: Assertion Failed: You can only add 'child' fragments or object literals to this property
    at Object.assert (http://localhost:58354/assets/vendor.js:36148:15)
    at http://localhost:58354/assets/vendor.js:98132:120
    at Array.map (<anonymous>)
    at normalizeFragmentArray (http://localhost:58354/assets/vendor.js:98130:17)
    at Class._normalizeData (http://localhost:58354/assets/vendor.js:98192:14)
    at Class.setupData (http://localhost:58354/assets/vendor.js:98426:32)
    at FragmentRecordData.setupFragmentArray (http://localhost:58354/assets/vendor.js:99634:23)
    at FragmentRecordData.getFragmentArray (http://localhost:58354/assets/vendor.js:99608:30)
    at Parent.get (http://localhost:58354/assets/vendor.js:98722:48)
    at http://localhost:58354/assets/vendor.js:15522:32

It seems that when we are getting to this line

typeOf(data) === 'object' || isInstanceOfType(store.modelFor(type), data)

when trying to set up the FragmentArray, the instance of check in normalizeFragmentArray assertion is throwing. This is even though we are trying to add the correct type of fragment to the fragment array.

The fix

Digging in the actual problem is that rather than passing the Fragment Array directly to normalizeFragmentArray we are actually passing a Fragment Array wrapped in another array. This is due to the following line https://github.com/richgt/ember-data.model-fragments/blob/ab24b0fbd4870ac129ed634eb82181758b86524c/addon/array/stateful.js#L72. We aren't checking whether the passed data is already an array and are always wrapping it in one. My naive patch is to detect whether it's a fragment array and then copy it.

[patocallaghan]

@igorT added an instanceof check instead of using a specific property

[patocallaghan]

@richgt @igorT so it appears this approach this means we lose a reference to the fragment array when we pass it in and copy it. See this test where I modify the original array to have the value 'Wolfsbane' but the value of the product within the user model hasn't changed Tears of Lys. Only the original is changed.

Digging into this a bit more, the difference between this and the old master is that on these lines when we had a fragment array, we didn't call setupData instead we just assigned it directly to the property. This meant we didn't call normalizeFragmentArray.

https://github.com/lytics/ember-data-model-fragments/blob/f1c7060fb7560aaea26760e0270234823b9e20db/addon/attributes.js#L291-L304

With the recent refactor we are now always calling setupData which means we are now normalizing the fragment array, even when it's already set up

https://github.com/lytics/ember-data-model-fragments/blob/36b38129bd9e929089a2852d1c17cd404ad91984/addon/record-data.js#L235-L237

To tell you the truth I'm not entirely sure what is the right behavior here but possibly in the spirit of the original code it should just do a simple assignment instead of re-setting up the data? I guess the original code suffered from the problem where the wrong owner is assigned to the fragmentArray?

if (serverFragment instanceof StatefulArray) {
  serverFragment[name] = data.attributes[name];
} else {
  ...
}

[patocallaghan]

@igorT @richgt I'm just wondering what's the best way to proceed here.

Solution 1

This PR in its current form.

1. Does the copy but breaks backward compatibility
2. Loses the reference to the fragmentArray you passed in
3. The owner is set up correctly through setupData

Solution 2

1. Don't re-call serverFragment.setupData(data.attributes[name]) and instead just re-assign like we did pre-5.0.
2. Keeps the original reference to the fragment array
3. Unfortunately the owner is now incorrect. The fragment array is still set to the initial owner. I’ve actually verified that this is existing behaviour pre-5.0 which to me seems like a bug.

Thinking about it my preference would be Solution 2, not requiring the copy, and keeping the reference. Could we augment solution 2 with also resetting the owner to be the correct one? Should we even bother considering this “bug” already existed? Not having lots of context on the refactor what's the downside to not calling serverFragment.setupData(data.attributes[name]); again.