diff --git a/lib/dao.js b/lib/dao.js index 4432859c4..be4a0ed85 100644 --- a/lib/dao.js +++ b/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; } diff --git a/lib/datasource.js b/lib/datasource.js index 443b86adb..de68516da 100644 --- a/lib/datasource.js +++ b/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 diff --git a/lib/model-builder.js b/lib/model-builder.js index 10df41115..6faf2c1ea 100644 --- a/lib/model-builder.js +++ b/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; diff --git a/lib/model.js b/lib/model.js index 183ef53f4..ee30f1741 100644 --- a/lib/model.js +++ b/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')); diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index 1d343c31d..a59480d3f 100644 --- a/test/loopback-dl.test.js +++ b/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(); + }); +}); diff --git a/test/model-inheritance.test.js b/test/model-inheritance.test.js index 95931a30a..9663bfc2b 100644 --- a/test/model-inheritance.test.js +++ b/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: [ { diff --git a/test/validations.test.js b/test/validations.test.js index eb16a8302..279d98551 100644 --- a/test/validations.test.js +++ b/test/validations.test.js @@ -205,9 +205,12 @@ 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) { @@ -215,26 +218,33 @@ describe('validations', function() { 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(); + }); }); });