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

@@ -1976,13 +1976,40 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
    *
    * @description
    * 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 ngModelOptions.allowInvalid
+   * is `true`. If the validity changes to valid, it will set the model to the last available valid
+   * modelValue.
    */
   this.$validate = function() {
     // ignore $validate before model is initialized
     if (isNumber(ctrl.$modelValue) && isNaN(ctrl.$modelValue)) {
       return;
     }
-    this.$$parseAndValidate();
+
+    var viewValue = ctrl.$$lastCommittedViewValue;
+    var modelValue = ctrl.$$rawModelValue;
+
+    var prevValid = ctrl.$valid;
+    var prevModelValue = ctrl.$modelValue;
+
+    var allowInvalid = ctrl.$options && ctrl.$options.allowInvalid;
+
+    ctrl.$$runValidators(undefined, 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 +2157,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 +2282,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

@@ -934,6 +934,209 @@ describe('NgModelController', function() {
   });
 });
 
+describe('initialization', function() {
+  var formElm, inputElm, ctrl, scope, $compile, $sniffer, $compileProvider, changeInputValueTo;
+
+  function compileInput(inputHtml) {
+    inputElm = jqLite(inputHtml);
+    formElm = jqLite('<form name="form"></form>');
+    formElm.append(inputElm);
+    $compile(formElm)(scope);
+    ctrl = inputElm.controller('ngModel');
+    scope.$digest();
+  }
+
+  function addValidator(validity, shouldObserve) {
+    if (!isDefined(shouldObserve)) {
+      shouldObserve = true;
+    }
+
+    $compileProvider.directive('obs', function() {
+      return {
+        require: 'ngModel',
+        link: function(scope, element, attrs, ngModelCtrl) {
+
+          ngModelCtrl.$validators.obs = isFunction(validity) ? validity : function(value) {
+            return validity;
+          };
+
+          if (shouldObserve) {
+            attrs.$observe('obs', function() {
+              ngModelCtrl.$validate();
+            });
+          }
+        }
+      };
+
+    });
+  }
+
+  function addFormatter(formatFunction) {
+    $compileProvider.directive('format', function() {
+      return {
+        require: 'ngModel',
+        link: function(scope, element, attrs, ctrl) {
+
+          ctrl.$formatters.push(formatFunction);
+        }
+      };
+
+    });
+  }
+
+  function addParser(parseFunction) {
+    $compileProvider.directive('parse', function() {
+      return {
+        require: 'ngModel',
+        link: function(scope, element, attrs, ctrl) {
+
+          ctrl.$parsers.push(parseFunction);
+        }
+      };
+
+    });
+  }
+
+  beforeEach(module(function(_$compileProvider_) {
+    $compileProvider = _$compileProvider_;
+  }));
+
+  beforeEach(inject(function(_$compile_, _$rootScope_, _$sniffer_) {
+    $compile = _$compile_;
+    $sniffer = _$sniffer_;
+    scope = _$rootScope_;
+
+    changeInputValueTo = function(value) {
+      inputElm.val(value);
+      browserTrigger(inputElm, $sniffer.hasEvent('input') ? 'input' : 'change');
+    };
+  }));
+
+  afterEach(function() {
+    dealoc(formElm);
+  });
+
+  // https://github.com/angular/angular.js/issues/9959
+  it('should not change model of type number to string with validator using observer', function() {
+    addValidator(true);
+    scope.value = 12345;
+    scope.attr = 'mock';
+    scope.ngChangeSpy = jasmine.createSpy();
+
+    compileInput('<input type="text" name="input" ng-model="value"' +
+      'ng-change="ngChangeSpy()" obs="{{attr}}" />');
+
+    expect(scope.value).toBe(12345);
+    expect(scope.ngChangeSpy).not.toHaveBeenCalled();
+  });
+
+  //https://github.com/angular/angular.js/issues/9063
+  it('should not set a null model that is invalid to undefined', function() {
+    addValidator(false);
+    scope.value = null;
+    scope.required = true;
+    compileInput('<input type="text" name="textInput" ng-model="value"' +
+      'ng-required="required" obs="{{attr}}" />');
+
+    expect(inputElm).toBeInvalid();
+    expect(scope.value).toBe(null);
+    expect(scope.form.textInput.$error.obs).toBeTruthy();
+  });
+
+  //https://github.com/angular/angular.js/issues/9996
+  it('should not change an undefined model that uses ng-required and formatters and parsers', function() {
+    addParser(function(viewValue) {
+      return null;
+    });
+    addFormatter(function(modelValue) {
+      return '';
+    });
+
+    scope.ngChangeSpy = jasmine.createSpy();
+    compileInput('<input type="text" parse format name="textInput" ng-model="value"' +
+      'ng-required="undefinedProp" ng-change="ngChangeSpy()" />');
+
+    expect(inputElm).toBeValid();
+    expect(scope.value).toBeUndefined();
+    expect(scope.ngChangeSpy).not.toHaveBeenCalled();
+  });
+
+  // https://github.com/angular/angular.js/issues/10025
+  it('should not change a model that uses custom $formatters and $parsers', function() {
+    addValidator(true);
+    addFormatter(function(modelValue) {
+      return 'abc';
+    });
+    addParser(function(viewValue) {
+      return 'xyz';
+    });
+    scope.value = 'abc';
+    scope.attr = 'mock';
+    compileInput('<input type="text" parse format name="textInput" ng-model="value"' +
+      'obs="{{attr}}" />');
+
+    expect(inputElm).toBeValid();
+    expect(scope.value).toBe('abc');
+  });
+
+  describe('$validate', function() {
+
+    // Sanity test: since a parse error sets the modelValue to undefined, the
+    // $$rawModelValue will always be undefined, hence $validate does not have
+    // a 'good' value to update
+    it('should not update a model that has a parse error', function() {
+      scope.value = 'abc';
+      addParser(function() {
+        return undefined;
+      });
+
+      addValidator(true, false);
+
+      compileInput('<input type="text" name="textInput" obs parse ng-model="value"/>');
+      expect(inputElm).toBeValid();
+      expect(scope.value).toBe('abc');
+
+      changeInputValueTo('xyz');
+      expect(inputElm).toBeInvalid();
+      expect(scope.value).toBeUndefined();
+      expect(ctrl.$error.parse).toBe(true);
+
+      ctrl.$validate();
+      expect(inputElm).toBeInvalid();
+      expect(scope.value).toBeUndefined();
+    });
+
+    it('should restore the last valid modelValue when a validator becomes valid', function() {
+      scope.value = 'abc';
+      scope.count = 0;
+
+      addValidator(function() {
+        scope.count++;
+        dump('count', scope.count);
+        return scope.count === 1 ? true : scope.count === 2 ? false : true;
+      });
+
+      compileInput('<input type="text" name="textInput" obs ng-model="value"/>');
+      expect(inputElm).toBeValid();
+      expect(scope.value).toBe('abc');
+      expect(ctrl.$viewValue).toBe('abc');
+
+      ctrl.$validate();
+      scope.$digest();
+      expect(inputElm).toBeInvalid();
+      expect(scope.value).toBeUndefined();
+      expect(ctrl.$viewValue).toBe('abc');
+
+      ctrl.$validate();
+      scope.$digest();
+      expect(inputElm).toBeValid();
+      expect(scope.value).toBe('abc');
+    });
+
+
+  });
+});
+
 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;
 
@@ -1348,6 +1551,16 @@ describe('input', function() {
     expect(scope.form.$$renameControl).not.toHaveBeenCalled();
   });
 
+  it('should not invoke viewChangeListeners before input is touched', function() {
+    scope.value = 1;
+    var change = scope.change = jasmine.createSpy('change');
+    var element = $compile('<div><div ng-repeat="i in [1]">' +
+      '<input type="text" ng-model="value" maxlength="1" ng-change="change()" />' +
+      '</div></div>')(scope);
+    scope.$digest();
+    expect(change).not.toHaveBeenCalled();
+    dealoc(element);
+  });
 
   describe('compositionevents', function() {
     it('should not update the model between "compositionstart" and "compositionend" on non android', inject(function($sniffer) {
@@ -2311,6 +2524,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();
+    });
+
   });
 
 
@@ -2409,6 +2630,14 @@ describe('input', function() {
       expect(scope.value).toBe('12345');
     });
 
+    // This works both for string formatter and toString() in validator
+    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();
+    });
+
   });