Flag id as updateOnly when forceId is in effect (#1453)

* updateOnly, forceId changes

* support getUpdateOnlyProperties

* fixup! fix updateOrCreate in forceId mode

The contract of `updateOrCreate` is expecting a full object instance
to be passed to the callback.

The current implementation was creating an empty instance and
calling updateAttributes under the hood. As a result, the callback
was called with the attributes being updated only.

In order to preserve existing behaviour, we have to always build
a full initial instance by calling `findById`.

See the following discussion for more context:
#966

* fixup! fix tests of upsert validation

* move forceId to model-builder

* remove TODO comment

* revert refactoring and test fixes

* Remove duplicate test

* change testcase names

* change to ModeClass.settingse

* forceId setup from datasource to model-builder

* fix inheritance of auto-generated forceId

* Fixed failing tests for auto change

* fixed a comment

Author: rashmihunt

lib/dao.js

@@ -537,16 +537,11 @@ DataAccessObject.upsert = function(data, options, cb) {
   if (forceId) {
     options = Object.create(options);
     options.validate = !!doValidate;
-    if (doValidate) {
-      Model.findById(id, options, function(err, model) {
-        if (err) return cb(err);
-        if (!model) return cb(errorModelNotFound(id));
-        model.updateAttributes(data, options, cb);
-      });
-    } else {
-      const model = new Model({id: id}, {persisted: true});
+    Model.findById(id, options, function(err, model) {
+      if (err) return cb(err);
+      if (!model) return cb(errorModelNotFound(id));
       model.updateAttributes(data, options, cb);
-    }
+    });
     return cb.promise;
   }
 

lib/datasource.js

@@ -628,13 +628,6 @@ DataSource.prototype.setupDataAccess = function(modelClass, settings) {
       idProp.type = idType;
       modelClass.definition.rawProperties[idName].type = idType;
       modelClass.definition.properties[idName].type = idType;
-      var forceId = settings.forceId;
-      if (idProp.generated && forceId !== false) {
-        forceId = true;
-      }
-      if (forceId) {
-        modelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'});
-      }
     }
     if (this.connector.define) {
       // pass control to connector

lib/model-builder.js

@@ -342,6 +342,32 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett
     });
   }
 
+  // updateOnly property is added to indicate that this property will appear in
+  // the model for update/updateorcreate operations but and not for create operation.
+  var forceId = ModelClass.settings.forceId;
+  if (idNames.length > 0) {
+    var idName = modelDefinition.idName();
+    idProp = ModelClass.definition.rawProperties[idName];
+    if (idProp.generated && forceId !== false) {
+      forceId = 'auto';
+    } else if (!idProp.generated && forceId === 'auto') {
+      // One of our parent models has enabled forceId because
+      // it uses an auto-generated id property. However,
+      // this particular model does not use auto-generated id,
+      // therefore we need to disable `forceId`.
+      forceId = false;
+    }
+
+    if (forceId) {
+      ModelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'});
+    }
+
+    ModelClass.definition.properties[idName].updateOnly = !!forceId;
+    ModelClass.definition.rawProperties[idName].updateOnly = !!forceId;
+
+    ModelClass.settings.forceId = forceId;
+  }
+
   // A function to loop through the properties
   ModelClass.forEachProperty = function(cb) {
     var props = ModelClass.definition.properties;

lib/model.js

@@ -827,6 +827,18 @@ ModelBaseClass.getMergePolicy = function(options) {
   return mergePolicy;
 };
 
+/**
+ * Gets properties defined with 'updateOnly' flag set to true from the model. This flag is also set to true
+ * internally for the id property, if this property is generated and IdInjection is true.
+ * @returns {updateOnlyProps} List of properties with updateOnly set to true.
+ */
+
+ModelBaseClass.getUpdateOnlyProperties = function() {
+  var Model = this;
+  const props = this.definition.properties;
+  return Object.keys(props).filter(key => props[key].updateOnly);
+};
+
 // Mixin observer
 jutil.mixin(ModelBaseClass, require('./observer'));
 

test/loopback-dl.test.js

@@ -645,7 +645,7 @@ describe('DataSource define model', function() {
 
     var User = ds.define('User', {});
     assert.deepEqual(User.definition.properties.id,
-      {type: Number, id: 1, generated: true});
+      {type: Number, id: 1, generated: true, updateOnly: true});
 
     done();
   });
@@ -663,13 +663,13 @@ describe('DataSource define model', function() {
 
     var User = builder.define('User', {id: {type: String, generated: true, id: true}});
     assert.deepEqual(User.definition.properties.id,
-      {type: String, id: 1, generated: true});
+      {type: String, id: 1, generated: true, updateOnly: true});
 
     var ds = new DataSource('memory');// define models
     User.attachTo(ds);
 
     assert.deepEqual(User.definition.properties.id,
-      {type: Number, id: 1, generated: true});
+      {type: Number, id: 1, generated: true, updateOnly: true});
 
     done();
   });
@@ -1911,3 +1911,61 @@ describe('ModelBuilder options.models', function() {
     assert.deepEqual(codes.number, ['unknown-property']);
   });
 });
+
+describe('updateOnly', function() {
+  it('sets forceId to true when model id is generated', function(done) {
+    var ds = new DataSource('memory');
+    var Post = ds.define('Post', {
+      title: {type: String, length: 255},
+      date: {type: Date, default: function() {
+        return new Date();
+      }},
+    });
+    // check if forceId is added as true in ModelClass's settings[] explicitly,
+    // if id a generated (default) and forceId in from the model is
+    // true(unspecified is 'true' which is the default).
+    Post.settings.should.have.property('forceId').eql('auto');
+    done();
+  });
+
+  it('flags id as updateOnly when forceId is undefined', function(done) {
+    var ds = new DataSource('memory');
+    var Post = ds.define('Post', {
+      title: {type: String, length: 255},
+      date: {type: Date, default: function() {
+        return new Date();
+      }},
+    });
+    // check if method getUpdateOnlyProperties exist in ModelClass and check if
+    // the Post has 'id' in updateOnlyProperties list
+    Post.should.have.property('getUpdateOnlyProperties');
+    Post.getUpdateOnlyProperties().should.eql(['id']);
+    done();
+  });
+
+  it('does not flag id as updateOnly when forceId is false', function(done) {
+    var ds = new DataSource('memory');
+    var Person = ds.define('Person', {
+      name: String,
+      gender: String,
+    }, {forceId: false});
+    // id should not be there in updateOnly properties list if forceId is set
+    // to false
+    Person.should.have.property('getUpdateOnlyProperties');
+    Person.getUpdateOnlyProperties().should.eql([]);
+    done();
+  });
+
+  it('flags id as updateOnly when forceId is true', function(done) {
+    var ds = new DataSource('memory');
+    var Person = ds.define('Person', {
+      name: String,
+      gender: String,
+    }, {forceId: true});
+    // id should be there in updateOnly properties list if forceId is set
+    // to true
+    Person.should.have.property('getUpdateOnlyProperties');
+    Person.getUpdateOnlyProperties().should.eql(['id']);
+    done();
+  });
+});

test/model-inheritance.test.js

@@ -239,6 +239,8 @@ describe('Model class inheritance', function() {
         );
 
       assert.deepEqual(User.settings, {
+        // forceId is set to 'auto' in memory if idProp.generated && forceId !== false
+        forceId: 'auto',
         defaultPermission: 'ALLOW',
         acls: [
           {
@@ -257,6 +259,7 @@ describe('Model class inheritance', function() {
       });
 
       assert.deepEqual(Customer.settings, {
+        forceId: false,
         defaultPermission: 'DENY',
         acls: [
           {

test/validations.test.js

@@ -205,36 +205,46 @@ describe('validations', function() {
       it('should ignore errors on upsert by default', function(done) {
         delete User.validations;
         User.validatesPresenceOf('name');
-        // It's important to pass an id value, otherwise DAO falls back
-        // to regular create()
-        User.updateOrCreate({id: 999}, done);
+        // It's important to pass an existing id value to updateOrCreate,
+        // otherwise DAO falls back to regular create()
+        User.create({name: 'a-name'}, (err, u) => {
+          if (err) return done(err);
+          User.updateOrCreate({id: u.id}, done);
+        });
       });
 
       it('should be skipped by upsert when disabled via settings', function(done) {
         var Customer = User.extend('Customer');
         Customer.attachTo(db);
         db.autoupdate(function(err) {
           if (err) return done(err);
-          Customer.prototype.isValid = function() {
-            throw new Error('isValid() should not be called at all');
-          };
-          Customer.settings.validateUpsert = false;
-          // It's important to pass an id value, otherwise DAO falls back
-          // to regular create()
-          Customer.updateOrCreate({id: 999}, done);
+          // It's important to pass an existing id value,
+          // otherwise DAO falls back to regular create()
+          Customer.create({name: 'a-name'}, (err, u) => {
+            if (err) return done(err);
+
+            Customer.prototype.isValid = function() {
+              throw new Error('isValid() should not be called at all');
+            };
+            Customer.settings.validateUpsert = false;
+
+            Customer.updateOrCreate({id: u.id, name: ''}, done);
+          });
         });
       });
 
       it('should work on upsert when enabled via settings', function(done) {
-        delete User.validations;
         User.validatesPresenceOf('name');
         User.settings.validateUpsert = true;
-        // It's important to pass an id value, otherwise DAO falls back
-        // to regular create()
-        User.upsert({id: 999}, function(err, u) {
-          if (!err) return done(new Error('Validation should have failed.'));
-          err.should.be.instanceOf(ValidationError);
-          done();
+        // It's important to pass an existing id value,
+        // otherwise DAO falls back to regular create()
+        User.create({name: 'a-name'}, (err, u) => {
+          if (err) return done(err);
+          User.upsert({id: u.id, name: ''}, function(err, u) {
+            if (!err) return done(new Error('Validation should have failed.'));
+            err.should.be.instanceOf(ValidationError);
+            done();
+          });
         });
       });