Proposal/Fix: Saving new embedded records creates duplicates


#1

Currently there is a problem saving new embedded records in a hasMany relationship. Ember has a hard time reconciling the returned embedded records with the ones already in memory and creates duplicates.

The problem:

You have a hasMany relationship:

User = DS.Model.extend({
  name: DS.attr(),
  posts: DS.hasMany('post')
});
  1. You create a new record within that array. this.get('posts').pushObject(newRecord)

  2. You call this.save() on the parent object and it embeds the new object, which has no server assigned id, in the JSON payload.

  3. Your server sees the embedded object and persists it, giving it an id.

  4. Server responds with the embedded object which now has an id.

  5. You normalize your payload so the embedded object is side-loaded (and now has an id)

    {
      "user": {
        "name": "Tim",
        "posts": ["1"] 
      },
      "posts": [
        {
          "id": "1"
          ...
        }
      ]
    }
    
  6. Ember sees this as a new record and pushes it into the store.

  7. The hasMany relationship looks up the reference to this id and appends the record, so it now references the original (no id) and the new record (with an id).

At this point you have duplicate records until you call reload() on the record. Ember has no way of knowing that these two records were supposed to be the same thing.

I have a working branch that solves this issue by sending a client specific identifier to the server (defaults to _clientId but is configurable) in any unsaved embedded records.

The server needs to be aware of this special key and pass it back unchanged with any embedded objects.

e.g. Request:

{
  "user": {
    "id": "1",
    "name": "Tim",
    "posts": [
      {
        "_clientId": "ember212",
        "body": "test"
      }
    ]
  }
}

Response:

{
  "user": {
    "id": "1",
    "name": "Tim",
    "posts": [
      {
        "id": "1",
        "_clientId": "ember212",
        "body": "test"
      }
    ]
  }
}

Using the fix I have implemented, Ember is able to reconcile these records and prevent duplicates in the resulting hasMany array.

I was hoping to get some feedback as to the actual implementation.

Ember Data pre 1.0 had an internal client id and a method for looking up a record by client id. It would have been perfect to use in this situation. 1.0 removed this.

Instead, I am using Ember.guidFor(record) as an alternative, but there is no global guid lookup to find an object by guid. Because of this, I have made my own clientId to record lookup in the EmbeddedRecordsMixin called clientIdMap. Once it reconciles a client side record it removes the entry from the map. This works - but has a small problem - Rails ActiveModelSerializer likes to send back a HTTP 204 with no content when you update a record. If this happens, the lookup entryis not removed and over time the clientIdMap will grow and grow.

I don’t really like the idea of the clientIdMap.

Is anyone aware of a better (and performant) method for looking up a record that has no id yet?

There are other less attractive options I’ve already pondered:

  • Store an expiration time on the clientIdMap entry and clean out old records when they expire (yuck)
  • Clear the entire map every time we finish extractSingle – this will probably fail to work if two responses arrive out of order.
  • Loop through the record’s hasMany relationship, looking for the guid (wont scale and the original record is not readily available inside the extractSingle method)

Any thoughts would be welcome.

Keep in mind, I am trying to keep this solution decoupled from the ActiveModelSerializer.


Saving object with children in 1 request
Ember to back-end: are there recommended conventions?
#2

I just came across this issue. Thanks for trying to develop a solution. This issue definitely needs to be addressed.


#3

@tstirrat perhaps we can resolve this in a separate repo from Ember Data.

Thanks for your feedback on activemodel-adapter and PR 1516; (FYI, see PR 1615).

I haven’t noticed duplication records after saving a payload w/ embedded records, as I have been using reload() between route changes anyway. (For the app I’ve been working on, state can be changed by other users so I need the reload.)

I hope the ActiveModelAdapter/Serializer and EmbeddedRecordsMixin can be improved or extended, especially from use within working applications.


#4

here’s a workaround for this issue, :

save: function() {
  var post = this.get('model'),
      store = this.get('store');

  var comment = Blogger.Comment._create({ store: store });
  comment.set('_data', {user: "user", content: 'this is a comment'});
  

  post.get('comments').pushObject(comment);
  
  comment.destroy();
  
  var onSuccess = function(post) {
    //console.log(post);
  };

  var onFail = function(post) {
    //console.log(post);
  };

  post.save().then(onSuccess, onFail);
  
}

#5

Hey, I ran into a sort of similar problem, without embedded records though. http://stackoverflow.com/questions/20843893/subscribing-to-updates-via-faye-websockets-results-in-duplicate-records. Is this of any help?


#6

Cool, thanks for the link. Another interesting take on the same kind of problem.

I considered doing something like you’ve explained in the SO article where you delete the unsaved records in memory, however I didn’t like this idea because it will not play nice with the controllers/bindings (state) related to these objects.

I.e. lets say you have template listing all related records that needs to show the last item you had selected. To do this you may use an item controller that has an isSelected property on it that toggles on click.

{{#each children itemController="blah"}}
<li {{bindAttr class="isSelected:selected">{{name}}</li>
{{/each}}

If you delete the in-memory object (with no id), and replace it with a new almost identical object, it will lose its item controller and thus its selected state.

This may not seem like a big deal in a small test case, but on a larger scale it has caused me some grief.

Passing a client ID seemed to be the best way around this corner case that maintains the exact client side objects.


#7

Actually the opposite happens. Just before the in-memory object is assigned an id, it checks if any record (possibly inserted with pushPayload meanwhile) with that id already exist. If thats the case, it will remove that record from the store. So the in-memory object is left untouched.

But I agree passing along a client id is a more robust solution.


#8

Let me add to the discussion. I have the exactly same issue, except that both records have the same ID. Although I’m using it with an IndexedDB adapter, I concluded it’s a bug in the store. The test below might help you in your work.

test("#save doesn't duplicate relationships from the store", function() {
  stop();
  Em.run(function() {
    person = store.createRecord('person', {
      name: "Clint",
      cool: true
    });

    phone = store.createRecord('phone', {
      number: 1234
    });

    person.save().then(function(person) {
      equal(person.get('phones.length'), 0, "person has no phones initially");

      person.get('phones').pushObject(phone);
      equal(person.get('phones.length'), 1, "person has phones before it's saved");

      person.save().then(function(savedPerson) {
        // fails. Somehow, there are 2 phones.
        equal(savedPerson.get('phones.length'), 1, "person has phones after being saved");

        var phone1 = savedPerson.get('phones.firstObject'),
            phone2 = savedPerson.get('phones.lastObject');

        // For debugging only
        // fails! The two objects have the same id.
        ok(phone1.get('id') != phone1.get('id'), "ids are at least different");

        // these pass
        equal(phone1.get('number'), 1234, "phone1 number is correct");
        equal(phone2.get('number'), 1234, "phone2 number is correct");
        start();
      });
    });
  });
});

#9

Quick observation, the RESTAdapter#createRecord does return a promise that should be resolved after the POST is successful. Perhaps this adapter method should also add on a function using .then() to both push the new record into the store and also remove the record in the store that doesn’t have an ID.


#10

fyi, I pushed a PR somewhat related: https://github.com/emberjs/data/pull/1656


#11

@tstirrat thanks for your work. I have bumped on the same issue now. As we do not use reload I added:

  • delete support via ‘_destroy’
  • ‘embed: ids’ format from ActiveModel:Serializer on load
  • dirty state support
  • ‘_attributes’ support for ActiveModel:Serializer

All should work without ActiveModel:Serializer. And version of emder data used is beta 12. Hope it will be usefull for anyone. The repo is here on github.


#12

This problem still persists in latest ember data v1.0.0-beta.14.


#13

I have the same troubles in ember data v1.0.0-beta.14.1


#14

I use this, maybe dirty solution as workaround: if I’ve a model like

{
  "user": {
    "name": "Tim",
    "posts": [
      {
        "body": "test"
      }
    ]
  }
}

Every time I save I prune related record with null id in this way

this.get('model').save().then(function (returnItem) {
  returnItem.get('posts').filterBy('id', null).forEach(function(item) {
    item.deleteRecord();
  });
});

#15

You’re a lifesaver, thanks! Kinda crazy it doesn’t behave like this out of the box.


#16

A little shorter, but the same result:

.filterBy('id', null).invoke('deleteRecord');

#17

I am using ember-data 1.13.14 and this issue is still present, is it fixed in ember-data 2.x ?


#18

Using Ember Data 2.4.2 and this problem still exists. :frowning:

The deleteRecord doesn’t work with belongsTo relations though. You can ofcourse replace it by searching the store directly for any record with an id of null, but it’s still ugly.