fix(forms): don't mutate validators array (#47830)

crisbeto · dylhunn · commit 779a76fa5a00 · 2022-11-17T09:36:14.000-08:00
Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes #47827. PR Close #47830
diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
@@ -632,12 +632,6 @@
   {
     "name": "cleanUpView"
   },
-  {
-    "name": "coerceToAsyncValidator"
-  },
-  {
-    "name": "coerceToValidator"
-  },
   {
     "name": "collectNativeNodes"
   },
diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json
@@ -608,15 +608,9 @@
   {
     "name": "cleanUpView"
   },
-  {
-    "name": "coerceToAsyncValidator"
-  },
   {
     "name": "coerceToBoolean"
   },
-  {
-    "name": "coerceToValidator"
-  },
   {
     "name": "collectNativeNodes"
   },
diff --git a/packages/forms/src/model/abstract_model.ts b/packages/forms/src/model/abstract_model.ts
@@ -372,15 +372,15 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T
    *
    * @internal
    */
-  private _composedValidatorFn: ValidatorFn|null;
+  private _composedValidatorFn!: ValidatorFn|null;
 
   /**
    * Contains the result of merging asynchronous validators into a single validator function
    * (combined using `Validators.composeAsync`).
    *
    * @internal
    */
-  private _composedAsyncValidatorFn: AsyncValidatorFn|null;
+  private _composedAsyncValidatorFn!: AsyncValidatorFn|null;
 
   /**
    * Synchronous validators as they were provided:
@@ -390,7 +390,7 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T
    *
    * @internal
    */
-  private _rawValidators: ValidatorFn|ValidatorFn[]|null;
+  private _rawValidators!: ValidatorFn|ValidatorFn[]|null;
 
   /**
    * Asynchronous validators as they were provided:
@@ -401,7 +401,7 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T
    *
    * @internal
    */
-  private _rawAsyncValidators: AsyncValidatorFn|AsyncValidatorFn[]|null;
+  private _rawAsyncValidators!: AsyncValidatorFn|AsyncValidatorFn[]|null;
 
   /**
    * The current value of the control.
@@ -427,10 +427,8 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T
   constructor(
       validators: ValidatorFn|ValidatorFn[]|null,
       asyncValidators: AsyncValidatorFn|AsyncValidatorFn[]|null) {
-    this._rawValidators = validators;
-    this._rawAsyncValidators = asyncValidators;
-    this._composedValidatorFn = coerceToValidator(this._rawValidators);
-    this._composedAsyncValidatorFn = coerceToAsyncValidator(this._rawAsyncValidators);
+    this._assignValidators(validators);
+    this._assignAsyncValidators(asyncValidators);
   }
 
   /**
@@ -620,8 +618,7 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T
    * using `addValidators()` method instead.
    */
   setValidators(validators: ValidatorFn|ValidatorFn[]|null): void {
-    this._rawValidators = validators;
-    this._composedValidatorFn = coerceToValidator(validators);
+    this._assignValidators(validators);
   }
 
   /**
@@ -635,8 +632,7 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T
    * using `addAsyncValidators()` method instead.
    */
   setAsyncValidators(validators: AsyncValidatorFn|AsyncValidatorFn[]|null): void {
-    this._rawAsyncValidators = validators;
-    this._composedAsyncValidatorFn = coerceToAsyncValidator(validators);
+    this._assignAsyncValidators(validators);
   }
 
   /**
@@ -1377,4 +1373,24 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T
   _find(name: string|number): AbstractControl|null {
     return null;
   }
+
+  /**
+   * Internal implementation of the `setValidators` method. Needs to be separated out into a
+   * different method, because it is called in the constructor and it can break cases where
+   * a control is extended.
+   */
+  private _assignValidators(validators: ValidatorFn|ValidatorFn[]|null): void {
+    this._rawValidators = Array.isArray(validators) ? validators.slice() : validators;
+    this._composedValidatorFn = coerceToValidator(this._rawValidators);
+  }
+
+  /**
+   * Internal implementation of the `setAsyncValidators` method. Needs to be separated out into a
+   * different method, because it is called in the constructor and it can break cases where
+   * a control is extended.
+   */
+  private _assignAsyncValidators(validators: AsyncValidatorFn|AsyncValidatorFn[]|null): void {
+    this._rawAsyncValidators = Array.isArray(validators) ? validators.slice() : validators;
+    this._composedAsyncValidatorFn = coerceToAsyncValidator(this._rawAsyncValidators);
+  }
 }
diff --git a/packages/forms/test/form_control_spec.ts b/packages/forms/test/form_control_spec.ts
@@ -266,6 +266,16 @@ describe('FormControl', () => {
       expect(c.valid).toEqual(true);
     });
 
+    it('should not mutate the validators array when overriding using setValidators', () => {
+      const control = new FormControl('');
+      const originalValidators = [Validators.required];
+
+      control.setValidators(originalValidators);
+      control.addValidators(Validators.minLength(10));
+
+      expect(originalValidators.length).toBe(1);
+    });
+
     it('should override validators by setting `control.validator` field value', () => {
       const c = new FormControl('');
       expect(c.valid).toEqual(true);
@@ -357,6 +367,30 @@ describe('FormControl', () => {
       expect(c.hasValidator(Validators.required)).toEqual(false);
     });
 
+    it('should not mutate the validators array when adding/removing sync validators', () => {
+      const originalValidators = [Validators.required];
+      const control = new FormControl('', originalValidators);
+
+      control.addValidators(Validators.min(10));
+      expect(originalValidators.length).toBe(1);
+
+      control.removeValidators(Validators.required);
+      expect(originalValidators.length).toBe(1);
+    });
+
+    it('should not mutate the validators array when adding/removing async validators', () => {
+      const firstValidator = asyncValidator('one');
+      const secondValidator = asyncValidator('two');
+      const originalValidators = [firstValidator];
+      const control = new FormControl('', null, originalValidators);
+
+      control.addAsyncValidators(secondValidator);
+      expect(originalValidators.length).toBe(1);
+
+      control.removeAsyncValidators(firstValidator);
+      expect(originalValidators.length).toBe(1);
+    });
+
     it('should return false when checking presence of a validator not identical by reference',
        () => {
          const minValidator = Validators.min(5);
@@ -518,6 +552,16 @@ describe('FormControl', () => {
          expect(c.valid).toEqual(true);
        }));
 
+    it('should not mutate the validators array when overriding using setValidators', () => {
+      const control = new FormControl('');
+      const originalValidators = [asyncValidator('one')];
+
+      control.setAsyncValidators(originalValidators);
+      control.addAsyncValidators(asyncValidator('two'));
+
+      expect(originalValidators.length).toBe(1);
+    });
+
     it('should override validators by setting `control.asyncValidator` field value',
        fakeAsync(() => {
          const c = new FormControl('');

Original file line number	Diff line number	Diff line change
`@@ -632,12 +632,6 @@`
`632`	`632`	`{`
`633`	`633`	`"name": "cleanUpView"`
`634`	`634`	`},`
`635`		`- {`
`636`		`- "name": "coerceToAsyncValidator"`
`637`		`- },`
`638`		`- {`
`639`		`- "name": "coerceToValidator"`
`640`		`- },`
`641`	`635`	`{`
`642`	`636`	`"name": "collectNativeNodes"`
`643`	`637`	`},`
Original file line number	Diff line number	Diff line change
`@@ -608,15 +608,9 @@`
`608`	`608`	`{`
`609`	`609`	`"name": "cleanUpView"`
`610`	`610`	`},`
`611`		`- {`
`612`		`- "name": "coerceToAsyncValidator"`
`613`		`- },`
`614`	`611`	`{`
`615`	`612`	`"name": "coerceToBoolean"`
`616`	`613`	`},`
`617`		`- {`
`618`		`- "name": "coerceToValidator"`
`619`		`- },`
`620`	`614`	`{`
`621`	`615`	`"name": "collectNativeNodes"`
`622`	`616`	`},`