fix(forms): ensure observable validators are properly canceled (#15132)

Observable subscriptions from previous validation runs should be canceled
before a new subscription is created for the next validation run.
Currently the subscription that sets the errors is canceled properly,
but the source observable created by the validator is not. While this
does not affect validation status or error setting, the source
observables will incorrectly continue through the pipeline until they
complete. This change ensures that the whole stream is canceled.

AsyncValidatorFn previously had an "any" return type, but now it more
explicitly requires a Promise or Observable return type. We don't
anticipate this causing problems given that any other return type
would have caused a runtime error already.

packages/forms/rollup.config.js

@@ -18,6 +18,7 @@ export default {
     'rxjs/Observable': 'Rx',
     'rxjs/Subject': 'Rx',
     'rxjs/observable/fromPromise': 'Rx.Observable',
-    'rxjs/operator/toPromise': 'Rx.Observable.prototype'
+    'rxjs/observable/forkJoin': 'Rx.Observable',
+    'rxjs/operator/map': 'Rx.Observable.prototype'
   }
 };

packages/forms/src/directives/validators.ts

@@ -166,7 +166,7 @@ export interface ValidatorFn { (c: AbstractControl): {[key: string]: any}; }
  * @stable
  */
 export interface AsyncValidatorFn {
-  (c: AbstractControl): any /*Promise<{[key: string]: any}>|Observable<{[key: string]: any}>*/;
+  (c: AbstractControl): Promise<{[key: string]: any}>|Observable<{[key: string]: any}>;
 }
 
 /**

packages/forms/src/model.ts

@@ -6,12 +6,12 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {EventEmitter, ɵisObservable as isObservable, ɵisPromise as isPromise} from '@angular/core';
+import {EventEmitter} from '@angular/core';
 import {Observable} from 'rxjs/Observable';
-import {fromPromise} from 'rxjs/observable/fromPromise';
 
 import {composeAsyncValidators, composeValidators} from './directives/shared';
 import {AsyncValidatorFn, ValidatorFn} from './directives/validators';
+import {toObservable} from './validators';
 
 
 
@@ -57,11 +57,6 @@ function _find(control: AbstractControl, path: Array<string|number>| string, del
     return null;
   }, control);
 }
-
-function toObservable(r: any): Observable<any> {
-  return isPromise(r) ? fromPromise(r) : r;
-}
-
 function coerceToValidator(validator: ValidatorFn | ValidatorFn[]): ValidatorFn {
   return Array.isArray(validator) ? composeValidators(validator) : validator;
 }
@@ -420,12 +415,8 @@ export abstract class AbstractControl {
     if (this.asyncValidator) {
       this._status = PENDING;
       const obs = toObservable(this.asyncValidator(this));
-      if (!(isObservable(obs))) {
-        throw new Error(
-            `expected the following validator to return Promise or Observable: ${this.asyncValidator}. If you are using FormBuilder; did you forget to brace your validators in an array?`);
-      }
       this._asyncValidationSubscription =
-          obs.subscribe({next: (res: {[key: string]: any}) => this.setErrors(res, {emitEvent})});
+          obs.subscribe((res: {[key: string]: any}) => this.setErrors(res, {emitEvent}));
     }
   }
 

packages/forms/src/validators.ts

@@ -6,8 +6,12 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {InjectionToken, ɵisPromise as isPromise, ɵmerge as merge} from '@angular/core';
+import {InjectionToken, ɵisObservable as isObservable, ɵisPromise as isPromise, ɵmerge as merge} from '@angular/core';
-import {toPromise} from 'rxjs/operator/toPromise';
+import {Observable} from 'rxjs/Observable';
+import {forkJoin} from 'rxjs/observable/forkJoin';
+import {fromPromise} from 'rxjs/observable/fromPromise';
+import {map} from 'rxjs/operator/map';
+
 import {AsyncValidatorFn, Validator, ValidatorFn} from './directives/validators';
 import {AbstractControl, FormControl, FormGroup} from './model';
 
@@ -156,8 +160,8 @@ export class Validators {
     if (presentValidators.length == 0) return null;
 
     return function(control: AbstractControl) {
-      const promises = _executeAsyncValidators(control, presentValidators).map(_convertToPromise);
+      const observables = _executeAsyncValidators(control, presentValidators).map(toObservable);
-      return Promise.all(promises).then(_mergeErrors);
+      return map.call(forkJoin(observables), _mergeErrors);
     };
   }
 }
@@ -166,8 +170,12 @@ function isPresent(o: any): boolean {
   return o != null;
 }
 
-function _convertToPromise(obj: any): Promise<any> {
+export function toObservable(r: any): Observable<any> {
-  return isPromise(obj) ? obj : toPromise.call(obj);
+  const obs = isPromise(r) ? fromPromise(r) : r;
+  if (!(isObservable(obs))) {
+    throw new Error(`Expected validator to return Promise or Observable.`);
+  }
+  return obs;
 }
 
 function _executeValidators(control: AbstractControl, validators: ValidatorFn[]): any[] {

packages/forms/test/form_control_spec.ts

@@ -983,8 +983,7 @@ export function main() {
           // test for the specific error since without the error check it would still throw an error
           // but
           // not a meaningful one
-          expect(fn).toThrowError(
+          expect(fn).toThrowError(`Expected validator to return Promise or Observable.`);
-              `expected the following validator to return Promise or Observable: ${syncValidator}. If you are using FormBuilder; did you forget to brace your validators in an array?`);
         });
 
       });

packages/forms/test/reactive_integration_spec.ts

@@ -8,10 +8,12 @@
 
 import {Component, Directive, EventEmitter, Input, Output, Type, forwardRef} from '@angular/core';
 import {ComponentFixture, TestBed, fakeAsync, tick} from '@angular/core/testing';
-import {AbstractControl, AsyncValidator, ControlValueAccessor, FormArray, FormControl, FormGroup, FormGroupDirective, FormsModule, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgControl, ReactiveFormsModule, Validators} from '@angular/forms';
+import {AbstractControl, AsyncValidator, AsyncValidatorFn, ControlValueAccessor, FormArray, FormControl, FormGroup, FormGroupDirective, FormsModule, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgControl, ReactiveFormsModule, Validators} from '@angular/forms';
 import {By} from '@angular/platform-browser/src/dom/debug/by';
 import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
 import {dispatchEvent} from '@angular/platform-browser/testing/src/browser_util';
+import {timer} from 'rxjs/observable/timer';
+import {_do} from 'rxjs/operator/do';
 
 export function main() {
   describe('reactive forms integration tests', () => {
@@ -1675,6 +1677,31 @@ export function main() {
            expect(control.valid).toEqual(false);
          }));
 
+      it('should cancel observable properly between validation runs', fakeAsync(() => {
+           const fixture = initTest(FormControlComp);
+           const resultArr: number[] = [];
+           fixture.componentInstance.control =
+               new FormControl('', null, observableValidator(resultArr));
+           fixture.detectChanges();
+           tick(100);
+
+           expect(resultArr.length).toEqual(1, `Expected source observable to emit once on init.`);
+
+           const input = fixture.debugElement.query(By.css('input'));
+           input.nativeElement.value = 'a';
+           dispatchEvent(input.nativeElement, 'input');
+           fixture.detectChanges();
+
+           input.nativeElement.value = 'aa';
+           dispatchEvent(input.nativeElement, 'input');
+           fixture.detectChanges();
+
+           tick(100);
+           expect(resultArr.length)
+               .toEqual(2, `Expected original observable to be canceled on the next value change.`)
+         }));
+
+
     });
 
     describe('errors', () => {
@@ -1932,6 +1959,12 @@ function uniqLoginAsyncValidator(expectedValue: string, timeout: number = 0) {
   };
 }
 
+function observableValidator(resultArr: number[]): AsyncValidatorFn {
+  return (c: AbstractControl) => {
+    return _do.call(timer(100), (resp: any) => resultArr.push(resp));
+  };
+}
+
 function loginIsEmptyGroupValidator(c: FormGroup) {
   return c.controls['login'].value == '' ? {'loginIsEmpty': true} : null;
 }

packages/forms/test/validators_spec.ts

@@ -6,11 +6,14 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {EventEmitter} from '@angular/core';
 import {fakeAsync, tick} from '@angular/core/testing';
 import {describe, expect, it} from '@angular/core/testing/src/testing_internal';
-import {AbstractControl, FormArray, FormControl, FormGroup, Validators} from '@angular/forms';
+import {AbstractControl, AsyncValidatorFn, FormArray, FormControl, FormGroup, Validators} from '@angular/forms';
 import {Observable} from 'rxjs/Observable';
+import {of } from 'rxjs/observable/of';
+import {timer} from 'rxjs/observable/timer';
+import {first} from 'rxjs/operator/first';
+import {map} from 'rxjs/operator/map';
 
 import {normalizeAsyncValidator} from '../src/directives/normalize_validator';
 import {AsyncValidator} from '../src/directives/validators';
@@ -210,19 +213,12 @@ export function main() {
     });
 
     describe('composeAsync', () => {
-      function asyncValidator(expected: any /** TODO #9100 */, response: any /** TODO #9100 */) {
-        return (c: any /** TODO #9100 */) => {
-          const emitter = new EventEmitter();
-          const res = c.value != expected ? response : null;
-          Promise.resolve(null).then(() => {
-            emitter.emit(res);
-            // this is required because of a bug in ObservableWrapper
-            // where callComplete can fire before callEmit
-            // remove this one the bug is fixed
-            setTimeout(() => { emitter.complete(); }, 0);
-          });
 
-          return emitter;
+      describe('promises', () => {
+        function promiseValidator(response: {[key: string]: any}): AsyncValidatorFn {
+          return (c: AbstractControl) => {
+            const res = c.value != 'expected' ? response : null;
+            return Promise.resolve(res);
           };
         }
 
@@ -230,49 +226,128 @@ export function main() {
            () => { expect(Validators.composeAsync(null)).toBeNull(); });
 
         it('should collect errors from all the validators', fakeAsync(() => {
-           const c = Validators.composeAsync([
+             const v = Validators.composeAsync(
-             asyncValidator('expected', {'one': true}), asyncValidator('expected', {'two': true})
+                 [promiseValidator({'one': true}), promiseValidator({'two': true})]);
-           ]);
 
-           let value: any /** TODO #9100 */ = null;
+             let errorMap: {[key: string]: any};
-           (<Promise<any>>c(new FormControl('invalid'))).then(v => value = v);
+             first.call(v(new FormControl('invalid')))
+                 .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+             tick();
 
-           tick(1);
+             expect(errorMap).toEqual({'one': true, 'two': true});
-
-           expect(value).toEqual({'one': true, 'two': true});
            }));
 
         it('should normalize and evaluate async validator-directives correctly', fakeAsync(() => {
-           const c = Validators.composeAsync(
+             const v = Validators.composeAsync(
                  [normalizeAsyncValidator(new AsyncValidatorDirective('expected', {'one': true}))]);
 
-           let value: any = null;
+             let errorMap: {[key: string]: any};
-           c(new FormControl()).then((v: any) => value = v);
+             first.call(v(new FormControl('invalid')))
-           tick(1);
+                 .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+             tick();
 
-           expect(value).toEqual({'one': true});
+             expect(errorMap).toEqual({'one': true});
            }));
 
         it('should return null when no errors', fakeAsync(() => {
-           const c = Validators.composeAsync([asyncValidator('expected', {'one': true})]);
+             const v = Validators.composeAsync([promiseValidator({'one': true})]);
 
-           let value: any /** TODO #9100 */ = null;
+             let errorMap: {[key: string]: any};
-           (<Promise<any>>c(new FormControl('expected'))).then(v => value = v);
+             first.call(v(new FormControl('expected')))
-           tick(1);
+                 .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+             tick();
 
-           expect(value).toBeNull();
+             expect(errorMap).toBeNull();
            }));
 
         it('should ignore nulls', fakeAsync(() => {
-           const c = Validators.composeAsync([asyncValidator('expected', {'one': true}), null]);
+             const v = Validators.composeAsync([promiseValidator({'one': true}), null]);
 
-           let value: any /** TODO #9100 */ = null;
+             let errorMap: {[key: string]: any};
-           (<Promise<any>>c(new FormControl('invalid'))).then(v => value = v);
+             first.call(v(new FormControl('invalid')))
+                 .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+             tick();
 
-           tick(1);
+             expect(errorMap).toEqual({'one': true});
-
-           expect(value).toEqual({'one': true});
            }));
       });
+
+      describe('observables', () => {
+        function observableValidator(response: {[key: string]: any}): AsyncValidatorFn {
+          return (c: AbstractControl) => {
+            const res = c.value != 'expected' ? response : null;
+            return of (res);
+          };
+        }
+
+        it('should return null when given null',
+           () => { expect(Validators.composeAsync(null)).toBeNull(); });
+
+        it('should collect errors from all the validators', () => {
+          const v = Validators.composeAsync(
+              [observableValidator({'one': true}), observableValidator({'two': true})]);
+
+          let errorMap: {[key: string]: any};
+          first.call(v(new FormControl('invalid')))
+              .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+
+          expect(errorMap).toEqual({'one': true, 'two': true});
+        });
+
+        it('should normalize and evaluate async validator-directives correctly', () => {
+          const v = Validators.composeAsync(
+              [normalizeAsyncValidator(new AsyncValidatorDirective('expected', {'one': true}))]);
+
+          let errorMap: {[key: string]: any};
+          first.call(v(new FormControl('invalid')))
+              .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+
+          expect(errorMap).toEqual({'one': true});
+        });
+
+        it('should return null when no errors', () => {
+          const v = Validators.composeAsync([observableValidator({'one': true})]);
+
+          let errorMap: {[key: string]: any};
+          first.call(v(new FormControl('expected')))
+              .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+
+          expect(errorMap).toBeNull();
+        });
+
+        it('should ignore nulls', () => {
+          const v = Validators.composeAsync([observableValidator({'one': true}), null]);
+
+          let errorMap: {[key: string]: any};
+          first.call(v(new FormControl('invalid')))
+              .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+
+          expect(errorMap).toEqual({'one': true});
+        });
+
+        it('should wait for all validators before setting errors', fakeAsync(() => {
+             function getTimerObs(time: number, errorMap: {[key: string]: any}): AsyncValidatorFn {
+               return (c: AbstractControl) => { return map.call(timer(time), () => errorMap); };
+             }
+
+             const v = Validators.composeAsync(
+                 [getTimerObs(100, {one: true}), getTimerObs(200, {two: true})]);
+
+             let errorMap: {[key: string]: any};
+             first.call(v(new FormControl('invalid')))
+                 .subscribe((errors: {[key: string]: any}) => errorMap = errors);
+
+             tick(100);
+             expect(errorMap).not.toBeDefined(
+                 `Expected errors not to be set until all validators came back.`);
+
+             tick(100);
+             expect(errorMap).toEqual(
+                 {one: true, two: true}, `Expected errors to merge once all validators resolved.`);
+           }));
+
+      });
+
+    });
   });
 }

tools/public_api_guard/forms/typings/forms.d.ts

@@ -112,7 +112,11 @@ export interface AsyncValidator extends Validator {
 
 /** @stable */
 export interface AsyncValidatorFn {
-    (c: AbstractControl): any;
+    (c: AbstractControl): Promise<{
+        [key: string]: any;
+    }> | Observable<{
+        [key: string]: any;
+    }>;
 }
 
 /** @stable */