diff --git a/lib/dao.js b/lib/dao.js index 62e658fe..0edc2adc 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -2666,6 +2666,13 @@ DataAccessObject.replaceById = function(id, data, options, cb) { Model.notifyObserversOf('before save', context, function(err, ctx) { if (err) return cb(err); + if (ctx.instance[pkName] !== id && !Model._warned.cannotOverwritePKInBeforeSaveHook) { + Model._warned.cannotOverwritePKInBeforeSaveHook = true; + console.warn('WARNING: id property cannot be changed from ' + + id + ' to ' + inst[pkName] + ' for model:' + Model.modelName + + ' in \'before save\' operation hook'); + } + data = inst.toObject(false); if (strict) { @@ -2717,7 +2724,15 @@ DataAccessObject.replaceById = function(id, data, options, cb) { Model.notifyObserversOf('loaded', ctx, function(err) { if (err) return cb(err); + if (ctx.data[pkName] !== id && !Model._warned.cannotOverwritePKInLoadedHook) { + Model._warned.cannotOverwritePKInLoadedHook = true; + console.warn('WARNING: id property cannot be changed from ' + + id + ' to ' + ctx.data[pkName] + ' for model:' + Model.modelName + + ' in \'loaded\' operation hook'); + } + inst.__persisted = true; + ctx.data[pkName] = id; inst.setAttributes(ctx.data); var context = { diff --git a/lib/model-builder.js b/lib/model-builder.js index 3be47b66..6d13d4a9 100644 --- a/lib/model-builder.js +++ b/lib/model-builder.js @@ -231,6 +231,7 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett hiddenProperty(ModelClass, 'http', { path: pathName }); hiddenProperty(ModelClass, 'base', ModelBaseClass); hiddenProperty(ModelClass, '_observers', {}); + hiddenProperty(ModelClass, '_warned', {}); // inherit ModelBaseClass static methods for (var i in ModelBaseClass) { diff --git a/test/manipulation.test.js b/test/manipulation.test.js index f0baa94d..29afab09 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -980,6 +980,58 @@ describe('manipulation', function() { }); }); + it('should ignore PK if it is set for `instance`' + + 'in `before save` operation hook', function(done) { + Post.findById(postInstance.id, function(err, p) { + if (err) return done(err); + changePostIdInHook('before save'); + p.replaceAttributes({ title: 'b' }, function(err, data) { + data.id.should.eql(postInstance.id); + if (err) return done(err); + Post.find(function(err, p) { + if (err) return done(err); + p[0].id.should.eql(postInstance.id); + done(); + }); + }); + }); + }); + + it('should set cannotOverwritePKInBeforeSaveHook flag, if `instance` in' + + '`before save` operation hook is set, so we report a warning just once', + function(done) { + Post.findById(postInstance.id, function(err, p) { + if (err) return done(err); + changePostIdInHook('before save'); + p.replaceAttributes({ title: 'b' }, function(err, data) { + if (err) return done(err); + Post._warned.cannotOverwritePKInBeforeSaveHook.should.equal(true); + data.id.should.equal(postInstance.id); + done(); + }); + }); + }); + + it('should ignore PK if it is set for `data`' + + 'in `loaded` operation hook', function(done) { + Post.findById(postInstance.id, function(err, p) { + if (err) return done(err); + changePostIdInHook('loaded'); + p.replaceAttributes({ title: 'b' }, function(err, data) { + data.id.should.eql(postInstance.id); + if (err) return done(err); + // clear observers to make sure `loaded` + // hook does not affect `find()` method + Post.clearObservers('loaded'); + Post.find(function(err, p) { + if (err) return done(err); + p[0].id.should.eql(postInstance.id); + done(); + }); + }); + }); + }); + it('works without options(promise variant)', function(done) { Post.findById(postInstance.id) .then(function(p) { @@ -1055,6 +1107,13 @@ describe('manipulation', function() { }); }); }); + + function changePostIdInHook(operationHook) { + Post.observe(operationHook, function(ctx, next) { + (ctx.data || ctx.instance).id = 99; + next(); + }); + } }); }