Merge pull request #3817 from BridgeAR/master

Fix two minor issues. Closes #3812 and #3770

Merge pull request #3817 from BridgeAR/master
Fix two minor issues. Closes #3812 and #3770
Mick Hansen
Commit 31133a67 authored May 28, 2015 by Mick Hansen
Showing with 42 additions and 30 deletions
changelog.md
lib/instance-validator.js
lib/instance.js
lib/sequelize.js
lib/sql-string.js
test/unit/instance/changed.test.js
test/unit/model/validation.test.js
--- a/changelog.md
+++ b/changelog.md
@@ -2,6 +2,8 @@
 - [FEATURE] Add support for keyword `after` in options of a field (useful for migrations), only for MySQL. [#3166](https://github.com/sequelize/sequelize/pull/3166)
 - [FIXED] Fix a case where `type` in `sequelize.query` was not being set to raw. [#3800](https://github.com/sequelize/sequelize/pull/3800)
 - [FIXED] Fix an issue where include all was not being properly expanded for self-references [#3804](https://github.com/sequelize/sequelize/issues/3804)
+- [FIXED] Fix instance.changed regression to not return false negatives for not changed null values [#3812](https://github.com/sequelize/sequelize/issues/3812)
+- [FIXED] Fix isEmail validator to allow args: true [#3770](https://github.com/sequelize/sequelize/issues/3770)
 # 3.1.1
 - [FIXED] Always quote aliases, even when quoteIdentifiers is false [#1589](https://github.com/sequelize/sequelize/issues/1589)

--- a/lib/instance-validator.js
+++ b/lib/instance-validator.js
@@ -256,11 +256,16 @@ InstanceValidator.prototype._builtinAttrValidate = function(value, field) {
  Utils._.forIn(this.modelInstance.validators[field], function(test,
    validatorType) {
-    if (['isUrl', 'isURL', 'isEmail'].indexOf(validatorType) !== -1 && test === true) {
+    if (['isUrl', 'isURL', 'isEmail'].indexOf(validatorType) !== -1) {
      // Preserve backwards compat. Validator.js now expects the second param to isURL and isEmail to be an object
+      if (typeof test === 'object' && test !== null && test.msg) {
+        test = {
+          msg: test.msg
+        };
+      } else if (test === true) {
        test = {};
      }
+    }
    // Check for custom validator.
    if (typeof test === 'function') {
@@ -273,8 +278,7 @@ InstanceValidator.prototype._builtinAttrValidate = function(value, field) {
    validators.push(validatorPromise);
  });
-  return Promise.settle(validators)
+  return Promise.settle(validators).then(this._handleSettledResult.bind(this, field));
-    .then(this._handleSettledResult.bind(this, field));
 };
 /**
@@ -311,11 +315,9 @@ InstanceValidator.prototype._invokeCustomValidator = Promise.method(function(val
    } else {
      validatorFunction = Promise.promisify(validator.bind(this.modelInstance));
    }
-    return validatorFunction()
+    return validatorFunction().catch(this._pushError.bind(this, false, errorKey));
-      .catch(this._pushError.bind(this, false, errorKey));
  } else {
-    return Promise.try(validator.bind(this.modelInstance, invokeArgs))
+    return Promise.try(validator.bind(this.modelInstance, invokeArgs)).catch(this._pushError.bind(this, false, errorKey));
-      .catch(this._pushError.bind(this, false, errorKey));
  }
 });
@@ -336,11 +338,7 @@ InstanceValidator.prototype._invokeBuiltinValidator = Promise.method(function(va
  }
  // extract extra arguments for the validator
-  var validatorArgs = test.hasOwnProperty('args') || test.hasOwnProperty('msg') ? test.args : test;
+  var validatorArgs = test.args || test;
-  // extract the error msg
-  var errorMessage = test.hasOwnProperty('msg') && test.msg ? test.msg :
-    'Validation ' + validatorType + ' failed';
  if (!Array.isArray(validatorArgs)) {
    if (validatorType === 'isImmutable') {
@@ -355,7 +353,8 @@ InstanceValidator.prototype._invokeBuiltinValidator = Promise.method(function(va
  }
  if (!Validator[validatorType].apply(Validator, [value].concat(validatorArgs))) {
-    throw errorMessage;
+  // extract the error msg
+    throw test.msg || 'Validation ' + validatorType + ' failed';
  }
 });

--- a/lib/instance.js
+++ b/lib/instance.js
@@ -321,8 +321,7 @@ module.exports = (function() {
              value = (value === 1) ? true : (value === 0) ? false : value;
            }
          }
+          if (!options.raw && ((primitives.indexOf(typeof value) === -1 && value !== null) || value !== originalValue)) {
-          if (!options.raw && (primitives.indexOf(typeof value) === -1 || value !== originalValue)) {
            this._previousDataValues[key] = originalValue;
            this.changed(key, true);
          }

--- a/lib/sequelize.js
+++ b/lib/sequelize.js
@@ -658,15 +658,15 @@ module.exports = (function() {
    }
    if (Utils._.isPlainObject(sql)) {
-      if (Object.prototype.hasOwnProperty.call(sql,'values')) {
+      if (sql.values !== undefined) {
-        if (Object.prototype.hasOwnProperty.call(options,'replacements')) {
+        if (options.replacements !== undefined) {
          throw new Error('Both `sql.values` and `options.replacements` cannot be set at the same time');
        }
        options.replacements = sql.values;
      }
-      if (Object.prototype.hasOwnProperty.call(sql,'query')) {
+      if (sql.query !== undefined) {
        sql = sql.query;
      }
    }

--- a/lib/sql-string.js
+++ b/lib/sql-string.js
@@ -156,7 +156,7 @@ SqlString.formatNamedParameters = function(sql, values, timeZone, dialect) {
      return value;
    }
-    if (Object.prototype.hasOwnProperty.call(values,key)) {
+    if (values[key] !== undefined) {
      return SqlString.escape(values[key], false, timeZone, dialect);
    } else {
      throw new Error('Named parameter "' + value + '" has no value in the given object.');

--- a/test/unit/instance/changed.test.js
+++ b/test/unit/instance/changed.test.js
@@ -25,20 +25,26 @@ describe(Support.getTestDialectTeaser('Instance'), function() {
        raw: true
      });
+      expect(user.changed('meta')).to.equal(false);
      user.set('name', 'b');
+      user.set('meta', null);
      expect(user.changed('name')).to.equal(true);
+      expect(user.changed('meta')).to.equal(true);
    });
    it('should return falsy for unchanged primitive', function () {
      var user = this.User.build({
-        name: 'a'
+        name: 'a',
+        meta: null
      }, {
        isNewRecord: false,
        raw: true
      });
      user.set('name', 'a');
+      user.set('meta', null);
      expect(user.changed('name')).to.equal(false);
+      expect(user.changed('meta')).to.equal(false);
    });
    it('should return true for multiple changed values', function () {

--- a/test/unit/model/validation.test.js
+++ b/test/unit/model/validation.test.js
@@ -179,7 +179,7 @@ describe(Support.getTestDialectTeaser('InstanceValidator'), function() {
          var validations = {}
            , message = validator + '(' + failingValue + ')';
-          if (validatorDetails.hasOwnProperty('spec')) {
+          if (validatorDetails.spec) {
            validations[validator] = validatorDetails.spec;
          } else {
            validations[validator] = {};
@@ -203,19 +203,24 @@ describe(Support.getTestDialectTeaser('InstanceValidator'), function() {
          });
        });
      }
-      , applyPassTest = function applyPassTest(validatorDetails, j, validator, msg) {
+      , applyPassTest = function applyPassTest(validatorDetails, j, validator, type) {
          var succeedingValue = validatorDetails.pass[j];
          it('correctly specifies an instance as valid using a value of "' + succeedingValue + '" for the validation "' + validator + '"', function() {
            var validations = {};
-            if (validatorDetails.hasOwnProperty('spec')) {
+            if (validatorDetails.spec !== undefined) {
              validations[validator] = validatorDetails.spec;
            } else {
              validations[validator] = {};
            }
-            if (msg) {
+            if (type === 'msg') {
              validations[validator].msg = validator + '(' + succeedingValue + ')';
+            } else if (type === 'args') {
+              validations[validator].args = validations[validator].args || true;
+              validations[validator].msg = validator + '(' + succeedingValue + ')';
+            } else if (type === 'true') {
+              validations[validator] = true;
            }
            var UserSuccess = this.sequelize.define('User' + config.rand(), {
@@ -238,7 +243,7 @@ describe(Support.getTestDialectTeaser('InstanceValidator'), function() {
          validator = validator.replace(/\$$/, '');
          var validatorDetails = checks[validator];
-          if (!validatorDetails.hasOwnProperty('raw')) {
+          if (!validatorDetails.raw) {
            validatorDetails.fail = Array.isArray(validatorDetails.fail) ? validatorDetails.fail : [validatorDetails.fail];
            validatorDetails.pass = Array.isArray(validatorDetails.pass) ? validatorDetails.pass : [validatorDetails.pass];
          }
@@ -248,11 +253,12 @@ describe(Support.getTestDialectTeaser('InstanceValidator'), function() {
          }
          for (i = 0; i < validatorDetails.pass.length; i++) {
-            applyPassTest(validatorDetails, i, validator, false);
+            applyPassTest(validatorDetails, i, validator);
+            applyPassTest(validatorDetails, i, validator, 'msg');
+            applyPassTest(validatorDetails, i, validator, 'args');
+            if (validatorDetails.spec === undefined) {
+              applyPassTest(validatorDetails, i, validator, 'true');
            }
-          for (i = 0; i < validatorDetails.pass.length; i++) {
-            applyPassTest(validatorDetails, i, validator, true);
          }
        }
      }