From e3764e30a301ec6136c8e6b5493d39feb3cd1ecc Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Wed, 12 Nov 2014 21:08:39 +0100 Subject: [PATCH] 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 #9063) Fixes: #9063 Fixes: #9959 Fixes: #9996 Fixes: #10025 Closes: #9890 Closes: #9913 Closes: #9997 Closes: #10048 --- src/ng/directive/input.js | 45 ++++++++++- test/ng/directive/inputSpec.js | 132 ++++++++++++++++++++++++++++++++- 2 files changed, 173 insertions(+), 4 deletions(-) diff --git a/src/ng/directive/input.js b/src/ng/directive/input.js index 6b93d55e67cb..81f98dcc8515 100644 --- a/src/ng/directive/input.js +++ b/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; diff --git a/test/ng/directive/inputSpec.js b/test/ng/directive/inputSpec.js index 8b1fbe885608..b157ab9bbf58 100644 --- a/test/ng/directive/inputSpec.js +++ b/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(''); + 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(''); + expect(scope.value).toBe(12345); + expect(scope.form.input.$error.maxlength).toBeUndefined(); + }); + });