From 805db78e1913f9f17be27eb48901368053c24ed2 Mon Sep 17 00:00:00 2001 From: David Cheung Date: Wed, 7 Sep 2016 14:24:48 -0400 Subject: [PATCH] Strict mode now always return validationError - Deprecation of strict:validate and strict:throw - When strict mode is enabled, it will now always return validation error (previous strict:validate) --- 3.0-RELEASE-NOTES.md | 52 ++++++++++++++++++++++++ lib/dao.js | 5 +-- lib/model.js | 12 +++--- lib/validations.js | 2 +- test/datatype.test.js | 5 ++- test/loopback-dl.test.js | 39 ------------------ test/manipulation.test.js | 84 +++++++++++++++++++++++++++++---------- 7 files changed, 127 insertions(+), 72 deletions(-) diff --git a/3.0-RELEASE-NOTES.md b/3.0-RELEASE-NOTES.md index 76a3f57ac..cbbef11f6 100644 --- a/3.0-RELEASE-NOTES.md +++ b/3.0-RELEASE-NOTES.md @@ -102,3 +102,55 @@ 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 +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'}); +``` +- #### strict: false + behavior will remain the same and unknown properties will be saved along with other properties + ```js + //johndoe would be created as + { + id: 1, + name: 'John doe', + age: 15, + geneder: 'm' + } + ``` + +- #### strict: `'validate'` and strict: `'throw'` + are removed in favor of `strict: true`, and because non-empty string + resolves as true, they will share the same behavior as `strict: true` + +- #### strict: `true`'s behavior change: + + **Previously:** (2.x) + + `strict:true` would silently removing unknown properties, and User instance will be created with unknown properties discarded + + ```js + johndoe.isValid(); //true + //johndoe would be created as + { + id: 1, + name: 'John doe', + age: 15 + } + ``` + + **New Behavior:** (3.x) + + ```js + johndoe.isValid(); //false + //johndoe would NOT be created and will result in the following error + //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..2585af109 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -93,10 +93,7 @@ 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) { inst.__unknownProperties.push(key); } } diff --git a/lib/model.js b/lib/model.js index 9df46a1e3..5e33c16d2 100644 --- a/lib/model.js +++ b/lib/model.js @@ -94,6 +94,10 @@ ModelBaseClass.prototype._initProperties = function(data, options) { if (strict === undefined) { strict = ctor.definition.settings.strict; + } else if (strict === 'throw') { + g.warn('Warning: Model %s, {{strict mode: `throw`}} has been removed, ' + + 'please use {{`strict: true`}} instead, which returns' + + '{{`Validation Error`}} for unknown properties,', ctor.modelName); } var persistUndefinedAsNull = ctor.definition.settings.persistUndefinedAsNull; @@ -141,7 +145,7 @@ ModelBaseClass.prototype._initProperties = function(data, options) { }, }); - if (strict === 'validate') { + if (strict) { Object.defineProperty(this, '__unknownProperties', { writable: true, enumerable: false, @@ -155,7 +159,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 +247,7 @@ 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 { 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..549736cc5 100644 --- a/test/datatype.test.js +++ b/test/datatype.test.js @@ -192,6 +192,7 @@ describe('datatypes', function() { TestModel = db.define( 'TestModel', { + name: {type: String, required: false}, desc: {type: String, required: false}, stars: {type: Number, required: false}, }, @@ -220,12 +221,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 a51b949b1..afd10d5a6 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -544,41 +544,83 @@ 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'); + p.should.not.have.property('unknownVar'); + done(); + }); + }); }); - }); + }); + + 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); + p.should.have.property('foo'); + done(); + }); + }); + }); - it('should throw error on unknown attributes when strict: throw', function(done) { + // Prior to version 3.0 `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) { + // 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.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'); + p.should.not.have.property('foo'); + Person.findById(p.id, function(e, p) { + if (e) return done(e); + p.should.not.have.property('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); + p.should.not.have.property('foo'); done(); }); }); }); }); - 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'}, @@ -588,7 +630,7 @@ describe('manipulation', function() { 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); + p.should.not.have.property('foo'); done(); }); });