From 26d4ce29e81d841270369dc2a870274e4e35149b Mon Sep 17 00:00:00 2001 From: Kara Date: Thu, 16 Mar 2017 10:15:17 -0700 Subject: [PATCH] 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 | 3 +- packages/forms/src/directives/validators.ts | 2 +- packages/forms/src/model.ts | 15 +- packages/forms/src/validators.ts | 20 +- packages/forms/test/form_control_spec.ts | 3 +- .../forms/test/reactive_integration_spec.ts | 35 +++- packages/forms/test/validators_spec.ts | 173 +++++++++++++----- .../public_api_guard/forms/typings/forms.d.ts | 6 +- 8 files changed, 184 insertions(+), 73 deletions(-) diff --git a/packages/forms/rollup.config.js b/packages/forms/rollup.config.js index fd38e88edf..c3daf2711b 100644 --- a/packages/forms/rollup.config.js +++ b/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' } }; diff --git a/packages/forms/src/directives/validators.ts b/packages/forms/src/directives/validators.ts index ab82af4275..7813e08eac 100644 --- a/packages/forms/src/directives/validators.ts +++ b/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}>; } /** diff --git a/packages/forms/src/model.ts b/packages/forms/src/model.ts index 1e28df9831..470bc16997 100644 --- a/packages/forms/src/model.ts +++ b/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, del return null; }, control); } - -function toObservable(r: any): Observable { - 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})); } } diff --git a/packages/forms/src/validators.ts b/packages/forms/src/validators.ts index 2db9118cd1..c0c5c235d5 100644 --- a/packages/forms/src/validators.ts +++ b/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 {toPromise} from 'rxjs/operator/toPromise'; +import {InjectionToken, ɵisObservable as isObservable, ɵisPromise as isPromise, ɵmerge as merge} from '@angular/core'; +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); - return Promise.all(promises).then(_mergeErrors); + const observables = _executeAsyncValidators(control, presentValidators).map(toObservable); + return map.call(forkJoin(observables), _mergeErrors); }; } } @@ -166,8 +170,12 @@ function isPresent(o: any): boolean { return o != null; } -function _convertToPromise(obj: any): Promise { - return isPromise(obj) ? obj : toPromise.call(obj); +export function toObservable(r: any): Observable { + 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[] { diff --git a/packages/forms/test/form_control_spec.ts b/packages/forms/test/form_control_spec.ts index 0543a25669..e52c23bbac 100644 --- a/packages/forms/test/form_control_spec.ts +++ b/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( - `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?`); + expect(fn).toThrowError(`Expected validator to return Promise or Observable.`); }); }); diff --git a/packages/forms/test/reactive_integration_spec.ts b/packages/forms/test/reactive_integration_spec.ts index 24cd6c574a..692f00e4d8 100644 --- a/packages/forms/test/reactive_integration_spec.ts +++ b/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; } diff --git a/packages/forms/test/validators_spec.ts b/packages/forms/test/validators_spec.ts index 8d2c5aad1f..08c57e8185 100644 --- a/packages/forms/test/validators_spec.ts +++ b/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,69 +213,141 @@ 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); + }; + } - it('should return null when given null', - () => { expect(Validators.composeAsync(null)).toBeNull(); }); + it('should return null when given null', + () => { expect(Validators.composeAsync(null)).toBeNull(); }); - it('should collect errors from all the validators', fakeAsync(() => { - const c = Validators.composeAsync([ - asyncValidator('expected', {'one': true}), asyncValidator('expected', {'two': true}) - ]); + it('should collect errors from all the validators', fakeAsync(() => { + const v = Validators.composeAsync( + [promiseValidator({'one': true}), promiseValidator({'two': true})]); - let value: any /** TODO #9100 */ = null; - (>c(new FormControl('invalid'))).then(v => value = v); + let errorMap: {[key: string]: any}; + 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 v = Validators.composeAsync( + [normalizeAsyncValidator(new AsyncValidatorDirective('expected', {'one': true}))]); - it('should normalize and evaluate async validator-directives correctly', fakeAsync(() => { - const c = 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); + tick(); - let value: any = null; - c(new FormControl()).then((v: any) => value = v); - tick(1); + expect(errorMap).toEqual({'one': true}); + })); - expect(value).toEqual({'one': true}); - })); + it('should return null when no errors', fakeAsync(() => { + const v = Validators.composeAsync([promiseValidator({'one': true})]); - it('should return null when no errors', fakeAsync(() => { - const c = Validators.composeAsync([asyncValidator('expected', {'one': true})]); + let errorMap: {[key: string]: any}; + first.call(v(new FormControl('expected'))) + .subscribe((errors: {[key: string]: any}) => errorMap = errors); + tick(); - let value: any /** TODO #9100 */ = null; - (>c(new FormControl('expected'))).then(v => value = v); - tick(1); + expect(errorMap).toBeNull(); + })); - expect(value).toBeNull(); - })); + it('should ignore nulls', fakeAsync(() => { + const v = Validators.composeAsync([promiseValidator({'one': true}), null]); - it('should ignore nulls', fakeAsync(() => { - const c = Validators.composeAsync([asyncValidator('expected', {'one': true}), null]); + let errorMap: {[key: string]: any}; + first.call(v(new FormControl('invalid'))) + .subscribe((errors: {[key: string]: any}) => errorMap = errors); + tick(); - let value: any /** TODO #9100 */ = null; - (>c(new FormControl('invalid'))).then(v => value = v); + expect(errorMap).toEqual({'one': true}); + })); + }); - tick(1); + 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.`); + })); + + }); - expect(value).toEqual({'one': true}); - })); }); }); } diff --git a/tools/public_api_guard/forms/typings/forms.d.ts b/tools/public_api_guard/forms/typings/forms.d.ts index 3b8461d601..bee02b8f1c 100644 --- a/tools/public_api_guard/forms/typings/forms.d.ts +++ b/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 */