Merge pull request #4578 from runspired/fix/null-id-assertions

Improved null id assertions

addon/-private/system/store.js

@@ -68,16 +68,19 @@ function promiseRecord(internalModel, label) {
   return promiseObject(toReturn, label);
 }
 
-var get = Ember.get;
-var set = Ember.set;
 var once = Ember.run.once;
-var isNone = Ember.isNone;
-var isPresent = Ember.isPresent;
 var Promise = Ember.RSVP.Promise;
-var copy = Ember.copy;
 var Store;
 
-const { Service } = Ember;
+const {
+  copy,
+  get,
+  GUID_KEY,
+  isNone,
+  isPresent,
+  set,
+  Service
+} = Ember;
 
 // Implementors Note:
 //
@@ -1755,6 +1758,8 @@ Store = Service.extend({
       // normalize relationship IDs into records
       this._backburner.schedule('normalizeRelationships', this, '_setupRelationships', internalModel, data);
       this.updateId(internalModel, data);
+    } else {
+      assert(`Your ${internalModel.type.modelName} record was saved to the server, but the response does not have an id and no id has been set client side. Records must have ids. Please update the server response to provide an id in the response or generate the id on the client side either before saving the record or while normalizing the response.`, internalModel.id);
     }
 
     //We first make sure the primary data has been updated
@@ -1804,7 +1809,18 @@ Store = Service.extend({
     var oldId = internalModel.id;
     var id = coerceId(data.id);
 
-    assert("An adapter cannot assign a new id to a record that already has an id. " + internalModel + " had id: " + oldId + " and you tried to update it with " + id + ". This likely happened because your server returned data in response to a find or update that had a different id than the one you sent.", oldId === null || id === oldId);
+    // ID absolutely can't be missing if the oldID is empty (missing Id in response for a new record)
+    assert(`'${internalModel.type.modelName}:${internalModel[GUID_KEY]}' was saved to the server, but the response does not have an id and your record does not either.`, !(id === null && oldId === null));
+
+    // ID absolutely can't be different than oldID if oldID is not null
+    assert(`'${internalModel.type.modelName}:${oldId}' was saved to the server, but the response returned the new id '${id}'. The store cannot assign a new id to a record that already has an id.`, !(oldId !== null && id !== oldId));
+
+    // ID can be null if oldID is not null (altered ID in response for a record)
+    // however, this is more than likely a developer error.
+    if (oldId !== null && id === null) {
+      warn(`Your ${internalModel.type.modelName} record was saved to the server, but the response does not have an id.`, !(oldId !== null && id === null));
+      return;
+    }
 
     this.typeMapFor(internalModel.type).idToRecord[id] = internalModel;
 

tests/integration/adapter/rest-adapter-test.js

@@ -328,14 +328,14 @@ test("createRecord - a serializer's primary key and attributes are consulted whe
 test("createRecord - a serializer's attributes are consulted when building the payload if no id is pre-defined", function(assert) {
   var post;
   env.registry.register('serializer:post', DS.RESTSerializer.extend({
-    primarykey: '_id_',
-
     attrs: {
       name: '_name_'
     }
   }));
 
-  ajaxResponse();
+  ajaxResponse({
+    post: { '_name_': "The Parley Letter", id: '1' }
+  });
 
   run(function() {
     post = store.createRecord('post', { name: "The Parley Letter" });

tests/integration/adapter/store-adapter-test.js

@@ -1057,7 +1057,7 @@ test("createRecord receives a snapshot", function(assert) {
   var person;
 
   run(function() {
-    person = store.createRecord('person', { name: "Tom Dale" });
+    person = store.createRecord('person', { name: "Tom Dale", id: 1 });
     person.save();
   });
 });

tests/integration/client-id-generation-test.js

@@ -86,7 +86,7 @@ test("empty string and undefined ids should coerce to null", function(assert) {
 
   env.adapter.createRecord = function(store, type, record) {
     assert.equal(typeof get(record, 'id'), 'object', 'correct type');
-    return Ember.RSVP.resolve();
+    return Ember.RSVP.resolve({ id: 1 });
   };
 
   run(function() {

tests/integration/relationships/has-many-test.js

@@ -342,6 +342,7 @@ test("A hasMany updated link should not remove new children", function(assert) {
 
   env.adapter.createRecord = function(store, snapshot, link, relationship) {
     return Ember.RSVP.resolve({
+      id: 1,
       links: {
         comments: '/some/link'
       }
@@ -382,6 +383,7 @@ test("A hasMany updated link should not remove new children when the parent reco
 
   env.adapter.createRecord = function(store, snapshot, link, relationship) {
     return Ember.RSVP.resolve({
+      id: 1,
       links: {
         comments: '/some/link'
       }
@@ -2207,6 +2209,7 @@ test("adding and removing records from hasMany relationship #2666", function(ass
     })
   });
 
+  var commentId = 4;
   env.registry.register('adapter:comment', DS.RESTAdapter.extend({
     deleteRecord(record) {
       return Ember.RSVP.resolve();
@@ -2215,7 +2218,7 @@ test("adding and removing records from hasMany relationship #2666", function(ass
       return Ember.RSVP.resolve();
     },
     createRecord() {
-      return Ember.RSVP.resolve();
+      return Ember.RSVP.resolve({ comments: { id: commentId++ }});
     }
   }));
 

tests/integration/store-test.js

@@ -873,6 +873,19 @@ testInDebug('store#findRecord that returns an array should assert', assert => {
   }, /expected the primary data returned from a `findRecord` response to be an object but instead it found an array/);
 });
 
+testInDebug('store#didSaveRecord should assert when the response to a save does not include the id', function(assert) {
+  env.adapter.createRecord = function() {
+    return {};
+  };
+
+  assert.expectAssertion(function() {
+    run(function() {
+      var car = store.createRecord('car');
+      car.save();
+    });
+  }, /Your car record was saved to the server, but the response does not have an id and no id has been set client side. Records must have ids. Please update the server response to provide an id in the response or generate the id on the client side either before saving the record or while normalizing the response./);
+});
+
 module("integration/store - queryRecord", {
   beforeEach() {
     initializeStore(DS.Adapter.extend());
@@ -898,3 +911,5 @@ testInDebug('store#queryRecord should assert when normalized payload of adapter
     });
   }, /Expected the primary data returned by the serializer for a `queryRecord` response to be a single object or null but instead it was an array./);
 });
+
+

tests/unit/model-test.js

@@ -370,7 +370,7 @@ test("changedAttributes() works while the record is being saved", function(asser
       assert.deepEqual(toObj(cat.changedAttributes()), {
         name: [undefined, 'Argon'],
         likes: [undefined, 'Cheese'] });
-      return {};
+      return { id: 1 };
     }
   });
   var Mascot = DS.Model.extend({