From 88a235de3a688d80457a2f18a0557f7ee9541d8f Mon Sep 17 00:00:00 2001 From: Sonu Kapoor Date: Fri, 20 Mar 2020 07:49:25 -0400 Subject: [PATCH] fix(forms): handle numeric values properly in the validator (#36157) Previously, the behavior of the `minLength` and `maxLength` validators caused confusion, as they appeared to work with numeric values but did not in fact produce consistent results. This commit fixes the issue by skipping validation altogether when a numeric value is used. BREAKING CHANGES: * The `minLength` and `maxLength` validators now verify that a value has numeric `length` property and invoke validation only if that's the case. Previously, falsey values without the length property (such as `0` or `false` values) were triggering validation errors. If your code relies on the old behavior, you can include other validators such as [min][1] or [requiredTrue][2] to the list of validators for a particular field. [1]: https://angular.io/api/forms/Validators#min [2]: https://angular.io/api/forms/Validators#requiredTrue Closes #35591 PR Close #36157 --- packages/forms/src/validators.ts | 24 ++++++++++------ packages/forms/test/validators_spec.ts | 40 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/packages/forms/src/validators.ts b/packages/forms/src/validators.ts index a1e8f8a157..ab09fa6f08 100644 --- a/packages/forms/src/validators.ts +++ b/packages/forms/src/validators.ts @@ -11,13 +11,18 @@ import {forkJoin, from, Observable} from 'rxjs'; import {map} from 'rxjs/operators'; import {AsyncValidatorFn, ValidationErrors, Validator, ValidatorFn} from './directives/validators'; -import {AbstractControl, FormControl} from './model'; +import {AbstractControl} from './model'; function isEmptyInputValue(value: any): boolean { // we don't check for string here so it also works with arrays return value == null || value.length === 0; } +function hasValidLength(value: any): boolean { + // non-strict comparison is intentional, to check for both `null` and `undefined` values + return value != null && typeof value.length === 'number'; +} + /** * @description * An `InjectionToken` for registering additional synchronous validators used with @@ -295,12 +300,14 @@ export class Validators { */ static minLength(minLength: number): ValidatorFn { return (control: AbstractControl): ValidationErrors|null => { - if (isEmptyInputValue(control.value)) { - return null; // don't validate empty values to allow optional controls + if (isEmptyInputValue(control.value) || !hasValidLength(control.value)) { + // don't validate empty values to allow optional controls + // don't validate values without `length` property + return null; } - const length: number = control.value ? control.value.length : 0; - return length < minLength ? - {'minlength': {'requiredLength': minLength, 'actualLength': length}} : + + return control.value.length < minLength ? + {'minlength': {'requiredLength': minLength, 'actualLength': control.value.length}} : null; }; } @@ -334,9 +341,8 @@ export class Validators { */ static maxLength(maxLength: number): ValidatorFn { return (control: AbstractControl): ValidationErrors|null => { - const length: number = control.value ? control.value.length : 0; - return length > maxLength ? - {'maxlength': {'requiredLength': maxLength, 'actualLength': length}} : + return hasValidLength(control.value) && control.value.length > maxLength ? + {'maxlength': {'requiredLength': maxLength, 'actualLength': control.value.length}} : null; }; } diff --git a/packages/forms/test/validators_spec.ts b/packages/forms/test/validators_spec.ts index 0a6719421b..8cbbf223bf 100644 --- a/packages/forms/test/validators_spec.ts +++ b/packages/forms/test/validators_spec.ts @@ -225,6 +225,26 @@ describe('Validators', () => { 'minlength': {'requiredLength': 2, 'actualLength': 1} }); }); + + it('should always return null with numeric values', () => { + expect(Validators.minLength(1)(new FormControl(0))).toBeNull(); + expect(Validators.minLength(1)(new FormControl(1))).toBeNull(); + expect(Validators.minLength(1)(new FormControl(-1))).toBeNull(); + expect(Validators.minLength(1)(new FormControl(+1))).toBeNull(); + }); + + it('should trigger validation for an object that contains numeric length property', () => { + const value = {length: 5, someValue: [1, 2, 3, 4, 5]}; + expect(Validators.minLength(1)(new FormControl(value))).toBeNull(); + expect(Validators.minLength(10)(new FormControl(value))).toEqual({ + 'minlength': {'requiredLength': 10, 'actualLength': 5} + }); + }); + + it('should return null when passing a boolean', () => { + expect(Validators.minLength(1)(new FormControl(true))).toBeNull(); + expect(Validators.minLength(1)(new FormControl(false))).toBeNull(); + }); }); describe('maxLength', () => { @@ -261,6 +281,26 @@ describe('Validators', () => { 'maxlength': {'requiredLength': 1, 'actualLength': 2} }); }); + + it('should always return null with numeric values', () => { + expect(Validators.maxLength(1)(new FormControl(0))).toBeNull(); + expect(Validators.maxLength(1)(new FormControl(1))).toBeNull(); + expect(Validators.maxLength(1)(new FormControl(-1))).toBeNull(); + expect(Validators.maxLength(1)(new FormControl(+1))).toBeNull(); + }); + + it('should trigger validation for an object that contains numeric length property', () => { + const value = {length: 5, someValue: [1, 2, 3, 4, 5]}; + expect(Validators.maxLength(10)(new FormControl(value))).toBeNull(); + expect(Validators.maxLength(1)(new FormControl(value))).toEqual({ + 'maxlength': {'requiredLength': 1, 'actualLength': 5} + }); + }); + + it('should return null when passing a boolean', () => { + expect(Validators.maxLength(1)(new FormControl(true))).toBeNull(); + expect(Validators.maxLength(1)(new FormControl(false))).toBeNull(); + }); }); describe('pattern', () => {