From aebf5e9e6bef0b66964dfe1c216c7c31e609ef06 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 5 Sep 2014 16:35:01 +0200 Subject: [PATCH 1/4] Enforce id (prevent user-set value), fix isNewRecord --- lib/dao.js | 14 +++++++++----- lib/datasource.js | 3 +++ lib/model.js | 5 ++++- test/manipulation.test.js | 13 ++++++++++++- 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/lib/dao.js b/lib/dao.js index 6806209a..fc9da99b 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -137,14 +137,16 @@ DataAccessObject.create = function (data, callback) { } var obj; + var idValue = getIdValue(this, data); + // if we come from save - if (data instanceof Model && !getIdValue(this, data)) { + if (data instanceof Model && !idValue) { obj = data; } else { obj = new Model(data); } data = obj.toObject(true); - + // validation required obj.isValid(function (valid) { if (valid) { @@ -315,7 +317,7 @@ DataAccessObject.findById = function find(id, cb) { if (!getIdValue(this, data)) { setIdValue(this, data, id); } - obj = new this(data, {applySetters: false}); + obj = new this(data, {applySetters: false, persisted: true}); } cb(err, obj); }.bind(this)); @@ -732,7 +734,7 @@ DataAccessObject.find = function find(query, cb) { this.getDataSource().connector.all(this.modelName, query, function (err, data) { if (data && data.forEach) { data.forEach(function (d, i) { - var obj = new self(d, {fields: query.fields, applySetters: false}); + var obj = new self(d, {fields: query.fields, applySetters: false, persisted: true}); if (query && query.include) { if (query.collect) { @@ -960,6 +962,7 @@ DataAccessObject.prototype.save = function (options, callback) { return callback(err, inst); } inst._initProperties(data); + inst.__persisted = true; updateDone.call(inst, function () { saveDone.call(inst, function () { callback(err, inst); @@ -1027,7 +1030,7 @@ DataAccessObject.updateAll = function (where, data, cb) { }; DataAccessObject.prototype.isNewRecord = function () { - return !getIdValue(this.constructor, this); + return !this.__persisted; }; /** @@ -1168,6 +1171,7 @@ DataAccessObject.prototype.updateAttributes = function updateAttributes(data, cb inst._adapter().updateAttributes(model, getIdValue(inst.constructor, inst), inst.constructor._forDB(typedData), function (err) { + if (!err) inst.__persisted = true; done.call(inst, function () { saveDone.call(inst, function () { if(cb) cb(err, inst); diff --git a/lib/datasource.js b/lib/datasource.js index 19522e29..edb1d380 100644 --- a/lib/datasource.js +++ b/lib/datasource.js @@ -489,6 +489,9 @@ DataSource.prototype.setupDataAccess = function (modelClass, settings) { var idType = this.connector.getDefaultIdType() || String; idProp.type = idType; modelClass.definition.properties[idName].type = idType; + if (settings.forceId) { + modelClass.validatesAbsenceOf(idName, { if: 'isNewRecord' }); + } } if (this.connector.define) { // pass control to connector diff --git a/lib/model.js b/lib/model.js index 5d29a5d8..16bddae6 100644 --- a/lib/model.js +++ b/lib/model.js @@ -47,6 +47,7 @@ function ModelBaseClass(data, options) { * @param {Object} options An object to control the instantiation * @property {Boolean} applySetters Controls if the setters will be applied * @property {Boolean} strict Set the instance level strict mode + * @property {Boolean} persisted Whether the instance has been persisted * @private */ ModelBaseClass.prototype._initProperties = function (data, options) { @@ -67,7 +68,9 @@ ModelBaseClass.prototype._initProperties = function (data, options) { if(strict === undefined) { strict = ctor.definition.settings.strict; } - + + this.__persisted = options.persisted === true; + if (ctor.hideInternalProperties) { // Object.defineProperty() is expensive. We only try to make the internal // properties hidden (non-enumerable) if the model class has the diff --git a/test/manipulation.test.js b/test/manipulation.test.js index 2d609063..83fce335 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -16,7 +16,7 @@ describe('manipulation', function () { age: {type: Number, index: true}, dob: Date, createdAt: {type: Number, default: Date.now} - }); + }, { forceId: true }); db.automigrate(done); @@ -50,6 +50,17 @@ describe('manipulation', function () { person.should.be.an.instanceOf(Person); should.not.exist(person.id); }); + + it('should not allow user-defined value for the id of object', function (done) { + Person.create({ id: 123456 }, function (err, p) { + err.should.be.instanceof(ValidationError); + err.message.should.equal('The `Person` instance is not valid. Details: `id` can\'t be set.'); + err.statusCode.should.equal(422); + p.should.be.instanceof(Person); + p.id.should.equal(123456); + done(); + }); + }); it('should work when called without callback', function (done) { Person.afterCreate = function (next) { From fafe51833b916dc230982d2f6d6139626463527b Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 5 Sep 2014 17:09:23 +0200 Subject: [PATCH 2/4] More fixes/tests --- lib/dao.js | 4 ++-- lib/model.js | 4 +++- test/manipulation.test.js | 31 +++++++++++++++++++++++++++++-- 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/lib/dao.js b/lib/dao.js index fc9da99b..f8e4431b 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -172,6 +172,7 @@ DataAccessObject.create = function (data, callback) { if (err) { return callback(err, obj); } + obj.__persisted = true; saveDone.call(obj, function () { createDone.call(obj, function () { callback(err, obj); @@ -961,8 +962,7 @@ DataAccessObject.prototype.save = function (options, callback) { if (err) { return callback(err, inst); } - inst._initProperties(data); - inst.__persisted = true; + inst._initProperties(data, { persisted: true }); updateDone.call(inst, function () { saveDone.call(inst, function () { callback(err, inst); diff --git a/lib/model.js b/lib/model.js index 16bddae6..e12b9b4b 100644 --- a/lib/model.js +++ b/lib/model.js @@ -69,7 +69,9 @@ ModelBaseClass.prototype._initProperties = function (data, options) { strict = ctor.definition.settings.strict; } - this.__persisted = options.persisted === true; + if (options.persisted !== undefined) { + this.__persisted = options.persisted === true; + } if (ctor.hideInternalProperties) { // Object.defineProperty() is expensive. We only try to make the internal diff --git a/test/manipulation.test.js b/test/manipulation.test.js index 83fce335..56e06725 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -40,6 +40,18 @@ describe('manipulation', function () { }); }); }); + + it('should instantiate an object', function (done) { + var p = new Person({name: 'Anatoliy'}); + p.name.should.equal('Anatoliy'); + p.isNewRecord().should.be.true; + p.save(function(err, inst) { + should.not.exist(err); + inst.isNewRecord().should.be.false; + inst.should.equal(p); + done(); + }); + }); it('should return instance of object', function (done) { var person = Person.create(function (err, p) { @@ -51,13 +63,28 @@ describe('manipulation', function () { should.not.exist(person.id); }); - it('should not allow user-defined value for the id of object', function (done) { - Person.create({ id: 123456 }, function (err, p) { + it('should not allow user-defined value for the id of object - create', function (done) { + Person.create({id: 123456}, function (err, p) { err.should.be.instanceof(ValidationError); err.message.should.equal('The `Person` instance is not valid. Details: `id` can\'t be set.'); err.statusCode.should.equal(422); p.should.be.instanceof(Person); p.id.should.equal(123456); + p.isNewRecord().should.be.true; + done(); + }); + }); + + it('should not allow user-defined value for the id of object - save', function (done) { + var p = new Person({id: 123456}); + p.isNewRecord().should.be.true; + p.save(function(err, inst) { + err.should.be.instanceof(ValidationError); + err.message.should.equal('The `Person` instance is not valid. Details: `id` can\'t be set.'); + err.statusCode.should.equal(422); + inst.isNewRecord().should.be.true; + inst.id.should.equal(123456); + inst.isNewRecord().should.be.true; done(); }); }); From b4144598bfc69d6853e390de9ca9c78b91dbd4de Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 5 Sep 2014 17:22:14 +0200 Subject: [PATCH 3/4] DAO save() now uses isNewRecord() --- lib/dao.js | 2 +- lib/relation-definition.js | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/dao.js b/lib/dao.js index f8e4431b..c4236a91 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -931,7 +931,7 @@ DataAccessObject.prototype.save = function (options, callback) { var data = inst.toObject(true); var modelName = Model.modelName; - if (!getIdValue(Model, this)) { + if (this.isNewRecord()) { return Model.create(this, callback); } diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 3a5b7371..143e381b 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1968,6 +1968,7 @@ EmbedsMany.prototype.prepareEmbeddedInstance = function(inst) { var self = this; var propertyName = this.definition.keyFrom; var modelInstance = this.modelInstance; + inst.__persisted = true; inst.triggerParent = function(actionName, callback) { if (actionName === 'save' || actionName === 'destroy') { var embeddedList = self.embeddedList(); From d8cafd0c844d9c0600bec43e36e5088f9d0d074d Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 5 Sep 2014 17:28:50 +0200 Subject: [PATCH 4/4] Tiny fixes --- lib/relation-definition.js | 2 ++ test/manipulation.test.js | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/relation-definition.js b/lib/relation-definition.js index 143e381b..232e63fe 100644 --- a/lib/relation-definition.js +++ b/lib/relation-definition.js @@ -1749,6 +1749,8 @@ EmbedsOne.prototype.related = function (refresh, params) { } var embeddedInstance = modelInstance[propertyName]; + embeddedInstance.__persisted = true; + if (typeof params === 'function') { // acts as async getter var cb = params; process.nextTick(function() { diff --git a/test/manipulation.test.js b/test/manipulation.test.js index 56e06725..0f93ae6c 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -82,7 +82,6 @@ describe('manipulation', function () { err.should.be.instanceof(ValidationError); err.message.should.equal('The `Person` instance is not valid. Details: `id` can\'t be set.'); err.statusCode.should.equal(422); - inst.isNewRecord().should.be.true; inst.id.should.equal(123456); inst.isNewRecord().should.be.true; done();