fix(ngModel): don't run parsers when executing $validate

Previously, $validate would execute the parsers to obtain a
modelValue for validation. This was necessary, because a validator
that is called outside of model / view update (e.g. from an observer)
otherwise might only an undefined modelValue, because a previous
view update has found a validation $error and set the model
to undefined (as is tradition in angular)

This is problematic as validators that are run immediately after
the ngModelController initializes would parse the modelValue
and replace the model, even though there had been no user input.

The solution is to go back to an older design: the ngModelController
will now internally record the $$rawModelValue. This means a model
or view update will store the set / parsed modelValue regardless
of validity, that is, it will never set it to undefined because of
validation errors.

When $validate is called, the $$rawModelValue will passed to the
validators. If the validity has changed, the usual behavior is kept:
if it became invalid, set the model to undefined, if valid,
restore the last available modelValue - the $$rawModelValue.

Additionally, $validate will only update the model when the validity
changed. This is to prevent setting initially invalid models other
than undefined to undefined (see angular#9063)

Fixes: angular#9063
Fixes: angular#9959
Fixes: angular#9996
Fixes: angular#10025

Closes: angular#9890
Closes: angular#9913
Closes: angular#9997
Closes: angular#10048

Author: Narretz

src/ng/directive/input.js

@@ -1721,6 +1721,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
     function($scope, $exceptionHandler, $attr, $element, $parse, $animate, $timeout, $rootScope, $q, $interpolate) {
   this.$viewValue = Number.NaN;
   this.$modelValue = Number.NaN;
+  this.$rawModelValue = undefined; // stores the parsed modelValue / model set from scope regardless of validity.
   this.$validators = {};
   this.$asyncValidators = {};
   this.$parsers = [];
@@ -1975,14 +1976,51 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
    * @name ngModel.NgModelController#$validate
    *
    * @description
-   * Runs each of the registered validators (first synchronous validators and then asynchronous validators).
+   * Runs each of the registered validators (first synchronous validators and then
+   * asynchronous validators).
+   * If the validity changes to invalid, the model will be set to `undefined`,
+   * unless {@link ngModelOptions `ngModelOptions.allowInvalid`} is `true`.
+   * If the validity changes to valid, it will set the model to the last available valid
+   * modelValue, i.e. either the last parsed value or the last value set from the scope.
    */
   this.$validate = function() {
     // ignore $validate before model is initialized
     if (isNumber(ctrl.$modelValue) && isNaN(ctrl.$modelValue)) {
       return;
     }
-    this.$$parseAndValidate();
+
+    var viewValue = ctrl.$$lastCommittedViewValue;
+    // Note: we use the $$rawModelValue as $modelValue might have been
+    // set to undefined during a view -> model update that found validation
+    // errors. We can't parse the view here, since that could change
+    // the model although neither viewValue nor the model on the scope changed
+    var modelValue = ctrl.$$rawModelValue;
+
+    // Check if the there's a parse error, so we don't unset it accidentially
+    var parserName = ctrl.$$parserName || 'parse';
+    var parserValid = ctrl.$error[parserName] ? false : undefined;
+
+    var prevValid = ctrl.$valid;
+    var prevModelValue = ctrl.$modelValue;
+
+    var allowInvalid = ctrl.$options && ctrl.$options.allowInvalid;
+
+    ctrl.$$runValidators(parserValid, modelValue, viewValue, function(allValid) {
+      // If there was no change in validity, don't update the model
+      // This prevents changing an invalid modelValue to undefined
+      if (!allowInvalid && prevValid !== allValid) {
+        // Note: Don't check ctrl.$valid here, as we could have
+        // external validators (e.g. calculated on the server),
+        // that just call $setValidity and need the model value
+        // to calculate their validity.
+        ctrl.$modelValue = allValid ? modelValue : undefined;
+
+        if (ctrl.$modelValue !== prevModelValue) {
+          ctrl.$$writeModelToScope();
+        }
+      }
+    });
+
   };
 
   this.$$runValidators = function(parseValid, modelValue, viewValue, doneCallback) {
@@ -2130,6 +2168,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
     }
     var prevModelValue = ctrl.$modelValue;
     var allowInvalid = ctrl.$options && ctrl.$options.allowInvalid;
+    ctrl.$$rawModelValue = modelValue;
     if (allowInvalid) {
       ctrl.$modelValue = modelValue;
       writeToModelIfNeeded();
@@ -2254,7 +2293,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
     // if scope model value and ngModel value are out of sync
     // TODO(perf): why not move this to the action fn?
     if (modelValue !== ctrl.$modelValue) {
-      ctrl.$modelValue = modelValue;
+      ctrl.$modelValue = ctrl.$$rawModelValue = modelValue;
 
       var formatters = ctrl.$formatters,
           idx = formatters.length;

test/ng/directive/inputSpec.js

@@ -505,6 +505,121 @@ describe('NgModelController', function() {
         expect(ctrl.$valid).toBe(true);
       });
 
+      it('should pass the last parsed modelValue to the validators', function() {
+        ctrl.$parsers.push(function(modelValue) {
+          return modelValue + 'def';
+        });
+
+        ctrl.$setViewValue('abc');
+
+        ctrl.$validators.test = function(modelValue, viewValue) {
+          return true;
+        };
+
+        spyOn(ctrl.$validators, 'test');
+
+        ctrl.$validate();
+
+        expect(ctrl.$validators.test).toHaveBeenCalledWith('abcdef', 'abc');
+      });
+
+      it('should set the model to undefined when it becomes invalid', function() {
+        var valid = true;
+        ctrl.$validators.test = function(modelValue, viewValue) {
+          return valid;
+        };
+
+        scope.$apply('value = "abc"');
+        expect(scope.value).toBe('abc');
+
+        valid = false;
+        ctrl.$validate();
+
+        expect(scope.value).toBeUndefined();
+      });
+
+      it('should update the model when it becomes valid', function() {
+        var valid = true;
+        ctrl.$validators.test = function(modelValue, viewValue) {
+          return valid;
+        };
+
+        scope.$apply('value = "abc"');
+        expect(scope.value).toBe('abc');
+
+        valid = false;
+        ctrl.$validate();
+        expect(scope.value).toBeUndefined();
+
+        valid = true;
+        ctrl.$validate();
+        expect(scope.value).toBe('abc');
+      });
+
+      it('should not update the model when it is valid, but there is a parse error', function() {
+        ctrl.$parsers.push(function(modelValue) {
+          return undefined;
+        });
+
+        ctrl.$setViewValue('abc');
+        expect(ctrl.$error.parse).toBe(true);
+        expect(scope.value).toBeUndefined();
+
+        ctrl.$validators.test = function(modelValue, viewValue) {
+          return true;
+        };
+
+        ctrl.$validate();
+        expect(ctrl.$error).toEqual({parse: true});
+        expect(scope.value).toBeUndefined();
+      });
+
+      it('should not set an invalid model to undefined when validity is the same', function() {
+        ctrl.$validators.test = function() {
+          return false;
+        };
+
+        scope.$apply('value = "invalid"');
+        expect(ctrl.$valid).toBe(false);
+        expect(scope.value).toBe('invalid');
+
+        ctrl.$validate();
+        expect(ctrl.$valid).toBe(false);
+        expect(scope.value).toBe('invalid');
+      });
+
+      it('should not change a model that has a formatter', function() {
+        ctrl.$validators.test = function() {
+          return true;
+        };
+
+        ctrl.$formatters.push(function(modelValue) {
+          return 'xyz';
+        });
+
+        scope.$apply('value = "abc"');
+        expect(ctrl.$viewValue).toBe('xyz');
+
+        ctrl.$validate();
+        expect(scope.value).toBe('abc');
+      });
+
+      it('should not change a model that has a parser', function() {
+        ctrl.$validators.test = function() {
+          return true;
+        };
+
+        ctrl.$parsers.push(function(modelValue) {
+          return 'xyz';
+        });
+
+        scope.$apply('value = "abc"');
+
+        ctrl.$validate();
+        expect(scope.value).toBe('abc');
+      });
+    });
+
     describe('view -> model update', function() {
       it('should always perform validations using the parsed model value', function() {
         var captures;
@@ -937,6 +1052,7 @@ describe('NgModelController', function() {
   });
 });
 
+
 describe('ngModel', function() {
   var EMAIL_REGEXP = /^[a-z0-9!#$%&'*+\/=?^_`{|}~.-]+@[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/i;
 
@@ -1351,7 +1467,6 @@ describe('input', function() {
     expect(scope.form.$$renameControl).not.toHaveBeenCalled();
   });
 
-
   describe('compositionevents', function() {
     it('should not update the model between "compositionstart" and "compositionend" on non android', inject(function($sniffer) {
       $sniffer.android = false;
@@ -2314,6 +2429,14 @@ describe('input', function() {
       expect(inputElm).toBeValid();
       expect(scope.form.input.$error.minlength).not.toBe(true);
     });
+
+    it('should validate when the model is initalized as a number', function() {
+      scope.value = 12345;
+      compileInput('<input type="text" name="input" ng-model="value" minlength="3" />');
+      expect(scope.value).toBe(12345);
+      expect(scope.form.input.$error.minlength).toBeUndefined();
+    });
+
   });
 
 
@@ -2412,6 +2535,13 @@ describe('input', function() {
       expect(scope.value).toBe('12345');
     });
 
+    it('should validate when the model is initalized as a number', function() {
+      scope.value = 12345;
+      compileInput('<input type="text" name="input" ng-model="value" maxlength="10" />');
+      expect(scope.value).toBe(12345);
+      expect(scope.form.input.$error.maxlength).toBeUndefined();
+    });
+
   });