Merge pull request #1084 from strongloop/strict-mode-cleanup

[SEMVER-MAJOR] Strict mode cleanup

Author: davidcheung

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.

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);
     }
   }

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);
       }
     }

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

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);
 

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,
-        },
-      });
-    });
-  });
 });

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();
             });
           });