diff --git a/3.0-RELEASE-NOTES.md b/3.0-RELEASE-NOTES.md index 76a3f57ac..c56eb3506 100644 --- a/3.0-RELEASE-NOTES.md +++ b/3.0-RELEASE-NOTES.md @@ -102,3 +102,33 @@ bulk `updateOrCreate`, you could use `async.each` or `Promise.all` to repeatedly call `updateOfCreate` for each instance. See [related code change](https://github.com/strongloop/loopback-datasource-juggler/pull/889) here. + +## Removing strict: validate and strict: throw + +- `strict: 'validate'` and `strict: 'throw'` are removed in favor of `strict: true`. + +- `strict: true`'s behavior change: + Given a user model definition as follow: + ```js + ds.define('User', { + name: String, required: false, + age: Number, required: false + }) + + var johndoe = new User({ name: 'John doe', age: 15, gender: 'm'}); + ``` + + **Previously:** + + silently removing unknown properties + + ```js + User.isValid(); //true + ``` + + **New Behavior:** + + returning `ValidationError` (same behavior as previous `strict: 'validate'`), + >ValidationError: The `User` instance is not valid. Details: `gender` is not defined in the model (value: 'm'). + +See [related code change](https://github.com/strongloop/loopback-datasource-juggler/pull/1084) here. diff --git a/lib/dao.js b/lib/dao.js index edd939057..eea04a4c7 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -93,10 +93,11 @@ function applyStrictCheck(model, strict, data, inst, cb) { key = keys[i]; if (props[key]) { result[key] = data[key]; - } else if (strict === 'throw') { - cb(new Error(g.f('Unknown property: %s', key))); - return; - } else if (strict === 'validate') { + } else if (strict) { + if (strict === 'throw') { + g.warn('{{`strict: throw`}} has been removed, use {{`strict: true`}} instead ' + + 'which returns {{`Validation Error`}} for unknown properties'); + } inst.__unknownProperties.push(key); } } diff --git a/lib/model.js b/lib/model.js index 9df46a1e3..71fa4d6be 100644 --- a/lib/model.js +++ b/lib/model.js @@ -141,7 +141,7 @@ ModelBaseClass.prototype._initProperties = function(data, options) { }, }); - if (strict === 'validate') { + if (strict) { Object.defineProperty(this, '__unknownProperties', { writable: true, enumerable: false, @@ -155,7 +155,7 @@ ModelBaseClass.prototype._initProperties = function(data, options) { this.__dataSource = options.dataSource; this.__strict = strict; this.__persisted = false; - if (strict === 'validate') { + if (strict) { this.__unknownProperties = []; } } @@ -243,9 +243,10 @@ ModelBaseClass.prototype._initProperties = function(data, options) { this.constructor.modelName, p )); } - } else if (strict === 'throw') { - throw new Error(g.f('Unknown property: %s', p)); - } else if (strict === 'validate') { + } else { + if (strict === 'throw') + g.warn('{{`strict: throw`}} has been removed, use {{`strict: true`}} instead ' + + 'which returns {{`Validation Error`}} for unknown properties'); this.__unknownProperties.push(p); } } diff --git a/lib/validations.js b/lib/validations.js index 4397743b1..72c3916af 100644 --- a/lib/validations.js +++ b/lib/validations.js @@ -433,7 +433,7 @@ Validatable.prototype.isValid = function(callback, data, options) { var valid = true, inst = this, wait = 0, async = false; var validations = this.constructor.validations; - var reportDiscardedProperties = this.__strict === 'validate' && + var reportDiscardedProperties = this.__strict && this.__unknownProperties && this.__unknownProperties.length; // exit with success when no errors diff --git a/test/datatype.test.js b/test/datatype.test.js index bbc58aae8..b52be9009 100644 --- a/test/datatype.test.js +++ b/test/datatype.test.js @@ -206,7 +206,7 @@ describe('datatypes', function() { it('should set missing optional properties to null', function(done) { var EXPECTED = {desc: null, stars: null}; - TestModel.create({name: 'a-test-name'}, function(err, created) { + TestModel.create({}, function(err, created) { if (err) return done(err); created.should.have.properties(EXPECTED); @@ -220,12 +220,12 @@ describe('datatypes', function() { it('should convert property value undefined to null', function(done) { var EXPECTED = {desc: null, extra: null}; + var data = {desc: undefined, extra: undefined}; if (isStrict) { // SQL-based connectors don't support dynamic properties delete EXPECTED.extra; + delete data.extra; } - - var data = {desc: undefined, extra: undefined}; TestModel.create(data, function(err, created) { if (err) return done(err); diff --git a/test/loopback-dl.test.js b/test/loopback-dl.test.js index 07e59efe3..ff8d98398 100644 --- a/test/loopback-dl.test.js +++ b/test/loopback-dl.test.js @@ -1917,47 +1917,8 @@ describe('ModelBuilder options.models', function() { }, }); assert.equal(m1.address.number, undefined, 'm1 should not contain number property in address'); - }); - - it('should use strictEmbeddedModels setting (validate) when applied on modelBuilder', function() { - var builder = new ModelBuilder(); - builder.settings.strictEmbeddedModels = 'validate'; - var M1 = builder.define('testEmbedded', { - name: 'string', - address: { - street: 'string', - }, - }); - var m1 = new M1({ - name: 'Jim', - address: { - street: 'washington st', - number: 5512, - }, - }); - assert.equal(m1.address.number, undefined, 'm1 should not contain number property in address'); assert.equal(m1.address.isValid(), false, 'm1 address should not validate with extra property'); var codes = m1.address.errors && m1.address.errors.codes || {}; assert.deepEqual(codes.number, ['unknown-property']); }); - - it('should use the strictEmbeddedModels setting (throw) when applied on modelBuilder', function() { - var builder = new ModelBuilder(); - builder.settings.strictEmbeddedModels = 'throw'; - var M1 = builder.define('testEmbedded', { - name: 'string', - address: { - street: 'string', - }, - }); - assert.throws(function() { - var m1 = new M1({ - name: 'Jim', - address: { - street: 'washington st', - number: 5512, - }, - }); - }); - }); }); diff --git a/test/manipulation.test.js b/test/manipulation.test.js index d9fb7bd09..7d68c2465 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -544,31 +544,72 @@ describe('manipulation', function() { }); }); - it('should ignore unknown attributes when strict: true', function(done) { - // Using {foo: 'bar'} only causes dependent test failures due to the - // stripping of object properties when in strict mode (ie. {foo: 'bar'} - // changes to '{}' and breaks other tests - person.updateAttributes({name: 'John', foo: 'bar'}, - function(err, p) { - if (err) return done(err); - should.not.exist(p.foo); - Person.findById(p.id, function(e, p) { - if (e) return done(e); - should.not.exist(p.foo); - done(); - }); + it('should discard undefined values before strict validation', + function(done) { + Person.definition.settings.strict = true; + Person.findById(person.id, function(err, p) { + p.updateAttributes({name: 'John', unknownVar: undefined}, + function(err, p) { + // if uknownVar was defined, it would return validationError + if (err) return done(err); + Person.findById(p.id, function(e, p) { + if (e) return done(e); + p.name.should.equal('John'); + should.not.exist(p.unknownVar); + done(); + }); + }); }); - }); + }); - it('should throw error on unknown attributes when strict: throw', function(done) { + // `strict: true` used to silently remove unknown properties, + // now return validationError upon unknown properties, + // same as previous validate behavior + it('should allow unknown attributes when strict: false', + function(done) { + Person.definition.settings.strict = false; + Person.findById(person.id, function(err, p) { + p.updateAttributes({name: 'John', foo: 'bar'}, + function(err, p) { + if (err) return done(err); + should.not.exist(err); + should.exist(p.foo); + done(); + }); + }); + }); + + // `strict: true` used to silently remove unknown properties, + // now return validationError upon unknown properties + it('should return error on unknown attributes when strict: true', + function(done) { + Person.definition.settings.strict = true; + Person.findById(person.id, function(err, p) { + p.updateAttributes({name: 'John', foo: 'bar'}, + function(err, p) { + should.exist(err); + err.name.should.equal('ValidationError'); + err.message.should.containEql('`foo` is not defined in the model'); + should.not.exist(p.foo); + Person.findById(p.id, function(e, p) { + if (e) return done(e); + should.not.exist(p.foo); + done(); + }); + }); + }); + }); + + // strict: throw is deprecated, use strict: true instead + // which returns Validation Error for unknown properties + it('should fallback to strict:true when using strict: throw', function(done) { Person.definition.settings.strict = 'throw'; Person.findById(person.id, function(err, p) { p.updateAttributes({foo: 'bar'}, function(err, p) { should.exist(err); - err.name.should.equal('Error'); - err.message.should.equal('Unknown property: foo'); - should.not.exist(p); + err.name.should.equal('ValidationError'); + err.message.should.containEql('`foo` is not defined in the model'); Person.findById(person.id, function(e, p) { if (e) return done(e); should.not.exist(p.foo); @@ -578,7 +619,9 @@ describe('manipulation', function() { }); }); - it('should throw error on unknown attributes when strict: throw', function(done) { + // strict: validate is deprecated, use strict: true instead + // behavior remains the same as before, because validate is now default behavior + it('should fallback to strict:true when using strict:validate', function(done) { Person.definition.settings.strict = 'validate'; Person.findById(person.id, function(err, p) { p.updateAttributes({foo: 'bar'},