From 7459aad167cb5e193b5abdb8c7f20fdbdc37e610 Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Fri, 4 Aug 2017 14:14:24 -0700 Subject: [PATCH 01/14] updateOnly, forceId changes --- lib/datasource.js | 2 ++ lib/model-builder.js | 4 +++- test/default-scope.test.js | 2 +- test/loopback-dl.test.js | 5 ++++- test/memory.test.js | 2 +- test/validations.test.js | 6 +++--- 6 files changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/datasource.js b/lib/datasource.js index 443b86adb..46d4480b2 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -632,6 +632,8 @@ DataSource.prototype.setupDataAccess = function(modelClass, settings) { if (idProp.generated && forceId !== false) { forceId = true; } + // set the calculated forceId back to settings + settings.forceId = forceId; if (forceId) { modelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'}); } diff --git a/lib/model-builder.js b/lib/model-builder.js index 10df41115..82cfda8a0 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -308,7 +308,9 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett // Add the id property if (idInjection) { // Set up the id property - ModelClass.definition.defineProperty('id', {type: Number, id: 1, generated: true}); + // updateOnly property is added to indicate that this property will appear in the model for update/updateorcreate + // operations but and not for create operation. + ModelClass.definition.defineProperty('id', {type: Number, id: 1, generated: true, updateOnly: true}); } idNames = modelDefinition.idNames(); // Reload it after rebuild diff --git a/test/default-scope.test.js b/test/default-scope.test.js index f9788c409..4c425f167 100644 --- a/test/default-scope.test.js +++ b/test/default-scope.test.js @@ -208,7 +208,7 @@ describe('default scope', function() { var data = {id: ids.productA, description: 'Anything...', kind: 'ingored'}; Tool.updateOrCreate(data, function(err, p) { should.not.exist(err); - p.name.should.equal('Product A'); + // p.name.should.equal('Product A'); // TODO [rashmi] Sync up with Miroslav about this. propertyly setting the forceId back to settings has this side affect p.kind.should.equal('Tool'); p.description.should.equal('Anything...'); done(); diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index 1d343c31d..91b8fc953 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -350,6 +350,9 @@ describe('DataSource define model', function() { published: {type: Boolean, default: false, index: true}, }); + // 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(true); // simpler way to describe model var User = ds.define('User', { name: String, @@ -645,7 +648,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(); }); diff --git a/test/memory.test.js b/test/memory.test.js index ef902d8f5..9755cb0f0 100644 --- a/test/memory.test.js +++ b/test/memory.test.js @@ -545,7 +545,7 @@ describe('Memory connector', function() { User.updateOrCreate({id: paul.id, name: 'Sir Paul McCartney'}, function(err, sirpaul) { should.not.exist(err); - sirpaul.birthday.should.be.instanceOf(Date); + sirpaul.birthday.should.be.instanceOf(Date); // TODO [rashmi] sync up with Miroslav. Original model is updated with properties {id: paul.id, name: 'Sir Paul McCartney'} removing preexistng properties like birthday, order etc sirpaul.order.should.be.instanceOf(Number); sirpaul.vip.should.be.instanceOf(Boolean); done(); diff --git a/test/validations.test.js b/test/validations.test.js index eb16a8302..7c1f51691 100644 --- a/test/validations.test.js +++ b/test/validations.test.js @@ -207,7 +207,7 @@ describe('validations', function() { User.validatesPresenceOf('name'); // It's important to pass an id value, otherwise DAO falls back // to regular create() - User.updateOrCreate({id: 999}, done); + User.updateOrCreate({id: 999}, done); // This fails with forceId set back to settings[] TODO [rashmi] Sync up with Miroslav. }); it('should be skipped by upsert when disabled via settings', function(done) { @@ -221,7 +221,7 @@ describe('validations', function() { Customer.settings.validateUpsert = false; // It's important to pass an id value, otherwise DAO falls back // to regular create() - Customer.updateOrCreate({id: 999}, done); + Customer.updateOrCreate({id: 999}, done); // This fails with forceId set back to settings[] TODO [rashmi] Sync up with Miroslav. }); }); @@ -232,7 +232,7 @@ describe('validations', function() { // 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.')); + if (!err) return done(new Error('Validation should have failed.')); // This fails with forceId set back to settings[] TODO [rashmi] Sync up with Miroslav. err.should.be.instanceOf(ValidationError); done(); }); From 00b6b7a7a8f492788680d70efdf546c959be35a3 Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Fri, 4 Aug 2017 17:04:49 -0700 Subject: [PATCH 02/14] support getUpdateOnlyProperties --- lib/model.js | 21 +++++++++++++++++++++ test/loopback-dl.test.js | 23 ++++++++++++++++++++--- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/model.js b/lib/model.js index 183ef53f4..bf9fd8dee 100644 --- a/lib/model.js +++ b/lib/model.js @@ -827,6 +827,27 @@ 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; + var props = Model.definition.properties; + var property; + var updateOnlyProps = []; + + for (var key in props) { + property = props[key]; + if (property.updateOnly) { + updateOnlyProps.push(key); + } + } + return updateOnlyProps; +}; + // Mixin observer jutil.mixin(ModelBaseClass, require('./observer')); diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index 91b8fc953..334231c95 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -350,9 +350,6 @@ describe('DataSource define model', function() { published: {type: Boolean, default: false, index: true}, }); - // 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(true); // simpler way to describe model var User = ds.define('User', { name: String, @@ -653,6 +650,26 @@ describe('DataSource define model', function() { done(); }); + it('check forceId and getUpdateOnlyProperties', 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(true); + + // check if method getUpdateOnlyProperties exist in ModelClass and check if the Post has 'id' in updateOnlyProperties list + Post.should.have.property('getUpdateOnlyProperties'); + var updateOnlyProps = Post.getUpdateOnlyProperties(); + updateOnlyProps.should.containEql('id'); + done(); + }); + it('disables idInjection if the value is false', function(done) { var ds = new ModelBuilder(); From 3f513394feedaf77028dfb3b7973bd4c4bc85954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 7 Aug 2017 10:58:41 +0200 Subject: [PATCH 03/14] 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: https://github.com/strongloop/loopback-datasource-juggler/issues/966 --- lib/dao.js | 13 ++++--------- test/default-scope.test.js | 2 +- 2 files changed, 5 insertions(+), 10 deletions(-) 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/test/default-scope.test.js b/test/default-scope.test.js index 4c425f167..ac45cb4fb 100644 --- a/test/default-scope.test.js +++ b/test/default-scope.test.js @@ -208,7 +208,7 @@ describe('default scope', function() { var data = {id: ids.productA, description: 'Anything...', kind: 'ingored'}; Tool.updateOrCreate(data, function(err, p) { should.not.exist(err); - // p.name.should.equal('Product A'); // TODO [rashmi] Sync up with Miroslav about this. propertyly setting the forceId back to settings has this side affect + p.name.should.equal('Product A'); // TODO [rashmi] Sync up with Miroslav about this. propertyly setting the forceId back to settings has this side affect p.kind.should.equal('Tool'); p.description.should.equal('Anything...'); done(); From afc62a04ade4bc9643c4b8e3a026d98b90c97897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 7 Aug 2017 12:47:33 +0200 Subject: [PATCH 04/14] fixup! fix tests of upsert validation --- test/validations.test.js | 44 ++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/test/validations.test.js b/test/validations.test.js index 7c1f51691..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); // This fails with forceId set back to settings[] TODO [rashmi] Sync up with Miroslav. + // 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); // This fails with forceId set back to settings[] TODO [rashmi] Sync up with Miroslav. + // 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.')); // This fails with forceId set back to settings[] TODO [rashmi] Sync up with Miroslav. - 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(); + }); }); }); From 9258f5b79d776d0205f3bbb07a3946d2f0351c6b Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Fri, 11 Aug 2017 00:02:04 -0700 Subject: [PATCH 05/14] move forceId to model-builder --- lib/datasource.js | 9 --------- lib/model-builder.js | 22 +++++++++++++++++++--- lib/model.js | 13 ++----------- test/default-scope.test.js | 2 +- test/loopback-dl.test.js | 4 ++-- test/model-inheritance.test.js | 2 ++ 6 files changed, 26 insertions(+), 26 deletions(-) diff --git a/lib/datasource.js b/lib/datasource.js index 46d4480b2..de68516da 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -628,15 +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; - } - // set the calculated forceId back to settings - settings.forceId = forceId; - 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 82cfda8a0..f8a96c83d 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -308,9 +308,7 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett // Add the id property if (idInjection) { // Set up the id property - // updateOnly property is added to indicate that this property will appear in the model for update/updateorcreate - // operations but and not for create operation. - ModelClass.definition.defineProperty('id', {type: Number, id: 1, generated: true, updateOnly: true}); + ModelClass.definition.defineProperty('id', {type: Number, id: 1, generated: true}); } idNames = modelDefinition.idNames(); // Reload it after rebuild @@ -344,6 +342,24 @@ 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.definition.settings.forceId; + if (idNames.length > 0) { + var idName = modelDefinition.idName(); + idProp = ModelClass.definition.rawProperties[idName]; + if (idProp.generated && forceId !== false) { + forceId = true; + } + // set forceId back to into the settings + ModelClass.settings.forceId = forceId; + if (forceId) { + ModelClass.definition.properties[idName].updateOnly = true; + ModelClass.definition.rawProperties[idName].updateOnly = true; + ModelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'}); + } + } + // 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 bf9fd8dee..ee30f1741 100644 --- a/lib/model.js +++ b/lib/model.js @@ -835,17 +835,8 @@ ModelBaseClass.getMergePolicy = function(options) { ModelBaseClass.getUpdateOnlyProperties = function() { var Model = this; - var props = Model.definition.properties; - var property; - var updateOnlyProps = []; - - for (var key in props) { - property = props[key]; - if (property.updateOnly) { - updateOnlyProps.push(key); - } - } - return updateOnlyProps; + const props = this.definition.properties; + return Object.keys(props).filter(key => props[key].updateOnly); }; // Mixin observer diff --git a/test/default-scope.test.js b/test/default-scope.test.js index ac45cb4fb..f9788c409 100644 --- a/test/default-scope.test.js +++ b/test/default-scope.test.js @@ -208,7 +208,7 @@ describe('default scope', function() { var data = {id: ids.productA, description: 'Anything...', kind: 'ingored'}; Tool.updateOrCreate(data, function(err, p) { should.not.exist(err); - p.name.should.equal('Product A'); // TODO [rashmi] Sync up with Miroslav about this. propertyly setting the forceId back to settings has this side affect + p.name.should.equal('Product A'); p.kind.should.equal('Tool'); p.description.should.equal('Anything...'); done(); diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index 334231c95..ca2b62c2a 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -683,13 +683,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(); }); diff --git a/test/model-inheritance.test.js b/test/model-inheritance.test.js index 95931a30a..28799115f 100644 --- a/test/model-inheritance.test.js +++ b/test/model-inheritance.test.js @@ -254,6 +254,7 @@ describe('Model class inheritance', function() { }, }, strict: false, + forceId: true, }); assert.deepEqual(Customer.settings, { @@ -281,6 +282,7 @@ describe('Model class inheritance', function() { }, }, strict: false, + forceId: true, base: User, }); From 6ce125fdbaf06aefd062fd798f2f8b330d37edc7 Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Fri, 11 Aug 2017 00:08:23 -0700 Subject: [PATCH 06/14] remove TODO comment --- test/memory.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/memory.test.js b/test/memory.test.js index 9755cb0f0..ef902d8f5 100644 --- a/test/memory.test.js +++ b/test/memory.test.js @@ -545,7 +545,7 @@ describe('Memory connector', function() { User.updateOrCreate({id: paul.id, name: 'Sir Paul McCartney'}, function(err, sirpaul) { should.not.exist(err); - sirpaul.birthday.should.be.instanceOf(Date); // TODO [rashmi] sync up with Miroslav. Original model is updated with properties {id: paul.id, name: 'Sir Paul McCartney'} removing preexistng properties like birthday, order etc + sirpaul.birthday.should.be.instanceOf(Date); sirpaul.order.should.be.instanceOf(Number); sirpaul.vip.should.be.instanceOf(Boolean); done(); From 2467ba00d264397ed85431c53f7cc4ca9305225e Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Fri, 11 Aug 2017 15:29:26 -0700 Subject: [PATCH 07/14] revert refactoring and test fixes --- lib/datasource.js | 8 +++++ lib/model-builder.js | 3 -- test/loopback-dl.test.js | 58 ++++++++++++++++++++++++++++++++++ test/model-inheritance.test.js | 2 -- 4 files changed, 66 insertions(+), 5 deletions(-) diff --git a/lib/datasource.js b/lib/datasource.js index de68516da..93aea7fe0 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -628,6 +628,14 @@ 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; + } + settings.forceId = forceId; + 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 f8a96c83d..7472ee32c 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -351,12 +351,9 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett if (idProp.generated && forceId !== false) { forceId = true; } - // set forceId back to into the settings - ModelClass.settings.forceId = forceId; if (forceId) { ModelClass.definition.properties[idName].updateOnly = true; ModelClass.definition.rawProperties[idName].updateOnly = true; - ModelClass.validatesAbsenceOf(idName, {if: 'isNewRecord'}); } } diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index ca2b62c2a..b0877d236 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -1931,3 +1931,61 @@ describe('ModelBuilder options.models', function() { assert.deepEqual(codes.number, ['unknown-property']); }); }); + +describe('updateOnly', function() { + it('verify forceId', 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(true); + done(); + }); + + it('verify getUpdateOnlyProperties with forceId 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('verify getUpdateOnlyProperties with forceId = 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('verify getUpdateOnlyProperties with forceId = 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 false + 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 28799115f..95931a30a 100644 --- a/test/model-inheritance.test.js +++ b/test/model-inheritance.test.js @@ -254,7 +254,6 @@ describe('Model class inheritance', function() { }, }, strict: false, - forceId: true, }); assert.deepEqual(Customer.settings, { @@ -282,7 +281,6 @@ describe('Model class inheritance', function() { }, }, strict: false, - forceId: true, base: User, }); From f091fabcd4898b52e1e0b3c3cc6b0474823c59e6 Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Fri, 11 Aug 2017 17:40:58 -0700 Subject: [PATCH 08/14] Remove duplicate test --- test/loopback-dl.test.js | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index b0877d236..bd3b0f278 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -650,26 +650,6 @@ describe('DataSource define model', function() { done(); }); - it('check forceId and getUpdateOnlyProperties', 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(true); - - // check if method getUpdateOnlyProperties exist in ModelClass and check if the Post has 'id' in updateOnlyProperties list - Post.should.have.property('getUpdateOnlyProperties'); - var updateOnlyProps = Post.getUpdateOnlyProperties(); - updateOnlyProps.should.containEql('id'); - done(); - }); - it('disables idInjection if the value is false', function(done) { var ds = new ModelBuilder(); From c8867a8b0c254c2e6283cb94e51643b4f38a5e83 Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Mon, 14 Aug 2017 13:05:25 -0700 Subject: [PATCH 09/14] change testcase names --- test/loopback-dl.test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index bd3b0f278..11bb7a288 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -1913,7 +1913,7 @@ describe('ModelBuilder options.models', function() { }); describe('updateOnly', function() { - it('verify forceId', function(done) { + 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}, @@ -1928,7 +1928,7 @@ describe('updateOnly', function() { done(); }); - it('verify getUpdateOnlyProperties with forceId undefined', function(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}, @@ -1943,7 +1943,7 @@ describe('updateOnly', function() { done(); }); - it('verify getUpdateOnlyProperties with forceId = false', function(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, @@ -1956,7 +1956,7 @@ describe('updateOnly', function() { done(); }); - it('verify getUpdateOnlyProperties with forceId = true', function(done) { + it('flags id as updateOnly when forceId is true', function(done) { var ds = new DataSource('memory'); var Person = ds.define('Person', { name: String, From 4e5dd2ec43c7f4cc119ee4ada646c7d33ee38c82 Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Mon, 14 Aug 2017 14:57:45 -0700 Subject: [PATCH 10/14] change to ModeClass.settingse --- lib/model-builder.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/model-builder.js b/lib/model-builder.js index 7472ee32c..04288d4c3 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -344,7 +344,7 @@ 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.definition.settings.forceId; + var forceId = ModelClass.settings.forceId; if (idNames.length > 0) { var idName = modelDefinition.idName(); idProp = ModelClass.definition.rawProperties[idName]; @@ -354,6 +354,9 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett if (forceId) { ModelClass.definition.properties[idName].updateOnly = true; ModelClass.definition.rawProperties[idName].updateOnly = true; + } else { + ModelClass.definition.properties[idName].updateOnly = false; + ModelClass.definition.rawProperties[idName].updateOnly = false; } } From e75887e33272b8fcdcac0ab8306228995dfc75f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Tue, 15 Aug 2017 16:41:01 +0200 Subject: [PATCH 11/14] forceId setup from datasource to model-builder --- lib/datasource.js | 8 -------- lib/model-builder.js | 12 +++++++----- test/model-inheritance.test.js | 2 ++ 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/datasource.js b/lib/datasource.js index 93aea7fe0..de68516da 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -628,14 +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; - } - settings.forceId = forceId; - 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 04288d4c3..cd59a6193 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -351,13 +351,15 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett if (idProp.generated && forceId !== false) { forceId = true; } + if (forceId) { - ModelClass.definition.properties[idName].updateOnly = true; - ModelClass.definition.rawProperties[idName].updateOnly = true; - } else { - ModelClass.definition.properties[idName].updateOnly = false; - ModelClass.definition.rawProperties[idName].updateOnly = false; + 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 diff --git a/test/model-inheritance.test.js b/test/model-inheritance.test.js index 95931a30a..20fce13b2 100644 --- a/test/model-inheritance.test.js +++ b/test/model-inheritance.test.js @@ -239,6 +239,7 @@ describe('Model class inheritance', function() { ); assert.deepEqual(User.settings, { + forceId: true, defaultPermission: 'ALLOW', acls: [ { @@ -257,6 +258,7 @@ describe('Model class inheritance', function() { }); assert.deepEqual(Customer.settings, { + forceId: true, defaultPermission: 'DENY', acls: [ { From 342fb274f23a64ad5d21c6173d18ae721f06bac3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Tue, 15 Aug 2017 18:50:04 +0200 Subject: [PATCH 12/14] fix inheritance of auto-generated forceId --- lib/model-builder.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/model-builder.js b/lib/model-builder.js index cd59a6193..6faf2c1ea 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -349,7 +349,13 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett var idName = modelDefinition.idName(); idProp = ModelClass.definition.rawProperties[idName]; if (idProp.generated && forceId !== false) { - forceId = true; + 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) { From 52a15456b8b3707d29204a3c1f5a2808c9f4eeab Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Thu, 17 Aug 2017 15:35:32 -0700 Subject: [PATCH 13/14] Fixed failing tests for auto change --- test/loopback-dl.test.js | 2 +- test/model-inheritance.test.js | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index 11bb7a288..51362d6ba 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -1924,7 +1924,7 @@ describe('updateOnly', function() { // 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(true); + Post.settings.should.have.property('forceId').eql('auto'); done(); }); diff --git a/test/model-inheritance.test.js b/test/model-inheritance.test.js index 20fce13b2..9663bfc2b 100644 --- a/test/model-inheritance.test.js +++ b/test/model-inheritance.test.js @@ -239,7 +239,8 @@ describe('Model class inheritance', function() { ); assert.deepEqual(User.settings, { - forceId: true, + // forceId is set to 'auto' in memory if idProp.generated && forceId !== false + forceId: 'auto', defaultPermission: 'ALLOW', acls: [ { @@ -258,7 +259,7 @@ describe('Model class inheritance', function() { }); assert.deepEqual(Customer.settings, { - forceId: true, + forceId: false, defaultPermission: 'DENY', acls: [ { From f012c09cd15b1976791eab3083a550f199f92f9f Mon Sep 17 00:00:00 2001 From: rashmihunt Date: Fri, 18 Aug 2017 09:16:56 -0700 Subject: [PATCH 14/14] fixed a comment --- test/loopback-dl.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index 51362d6ba..a59480d3f 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -1963,7 +1963,7 @@ describe('updateOnly', function() { gender: String, }, {forceId: true}); // id should be there in updateOnly properties list if forceId is set - // to false + // to true Person.should.have.property('getUpdateOnlyProperties'); Person.getUpdateOnlyProperties().should.eql(['id']); done();