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

crisbeto · 2022-10-21T10:57:37Z

Fixes that the AbstractControl was mutating the validators arrays being passed into the constructor an helper methods like setValidators.

Fixes #47827.

crisbeto · 2022-10-21T11:15:23Z

packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json

@@ -632,12 +632,6 @@
  {
    "name": "cleanUpView"
  },
-  {


I'm not entirely sure why this was dropped since the functions are still used inside setValidators and setAsyncValidators. My guess is because the calls were removed from the constructor.

crisbeto · 2022-10-21T11:31:20Z

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;


I could also initialize these to null, but it seemed redundant since they're guaranteed to be defined in the constructor, TS just isn't able to figure it out statically.

alfaproject · 2022-10-21T12:43:25Z

packages/forms/src/model/abstract_model.ts

@@ -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;


Should the type be:

Suggested change

private _rawAsyncValidators!: AsyncValidatorFn|AsyncValidatorFn[]|null;

private _rawAsyncValidators: ReadonlyArray<AsyncValidatorFn> = [];

Always an array and always readonly

The array can be mutated later on by the addValidators and removeValidators methods. We just don't want to mutate the original array given to us by the author.

Always an array and always readonly

There is a code path where _rawValidators and _rawAsyncValidators may become a function:

https://github.com/angular/angular/blob/main/packages/forms/src/model/abstract_model.ts#L441-L446

However, we can do further refactoring (in a followup PR if needed) to replace the implementation of the validator and asyncValidator setters with setValidators and setAsyncValidators calls, in which case the _rawValidators and _rawAsyncValidators would always be arrays.

I don't think that this code path is relevant for the fix here though. We only need to clone the value when it's being set to an array and the validator getter/setter is a ValidatorFn|null.

AndrewKushnir

LGTM 👍

I've left a proposal on additional refactoring, but we can do that in a followup PR.

AndrewKushnir · 2022-10-21T17:26:08Z

packages/forms/src/model/abstract_model.ts

@@ -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;


Always an array and always readonly

There is a code path where _rawValidators and _rawAsyncValidators may become a function:

https://github.com/angular/angular/blob/main/packages/forms/src/model/abstract_model.ts#L441-L446

However, we can do further refactoring (in a followup PR if needed) to replace the implementation of the validator and asyncValidator setters with setValidators and setAsyncValidators calls, in which case the _rawValidators and _rawAsyncValidators would always be arrays.

pkozlowski-opensource · 2022-10-24T12:12:58Z

This PR was merged into the repository by commit 0329c13.

Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes #47827. PR Close #47830

This reverts commit 0329c13.

This reverts commit 0329c13. PR Close #47845

vakhramoff · 2022-10-29T03:02:38Z

packages/forms/test/form_control_spec.ts

+      control.setAsyncValidators(originalValidators);
+      control.addAsyncValidators(asyncValidator('two'));
+
+      expect(originalValidators.length).toBe(1);


It seems that better way is to check the deep equality of the array before and after the manipulations.

Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes angular#47827. PR Close angular#47830

…ngular#47845) This reverts commit 0329c13. PR Close angular#47845

Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes angular#47827.

crisbeto · 2022-11-16T12:37:52Z

I've fixed the issue that caused the PR to be reverted previously. I also have a passing TGP for it.

crisbeto · 2022-11-16T12:44:43Z

Caretaker Note: Please ignore the google-internal-tests status. TGP status isn't being picked up

dylhunn · 2022-11-17T17:36:17Z

This PR was merged into the repository by commit 779a76f.

Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes #47827. PR Close #47830

angular-automatic-lock-bot · 2022-12-18T16:07:42Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes angular#47827. PR Close angular#47830

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms target: patch This PR is targeted for the next patch release labels Oct 21, 2022

ngbot bot added this to the Backlog milestone Oct 21, 2022

crisbeto force-pushed the 47827/forms-validators-mutable branch from 7bce4ac to 88fd6ee Compare October 21, 2022 11:11

crisbeto commented Oct 21, 2022

View reviewed changes

crisbeto marked this pull request as ready for review October 21, 2022 11:28

pullapprove bot requested a review from AndrewKushnir October 21, 2022 11:28

crisbeto added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels Oct 21, 2022

crisbeto commented Oct 21, 2022

View reviewed changes

alfaproject reviewed Oct 21, 2022

View reviewed changes

AndrewKushnir approved these changes Oct 21, 2022

View reviewed changes

crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 24, 2022

pkozlowski-opensource closed this in 0329c13 Oct 24, 2022

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Oct 24, 2022

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

6310650

This reverts commit 0329c13.

This was referenced Oct 24, 2022

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

Closed

AbstractControl constructed with an array of validators mutates the original array on control.addValidators() #47827

Closed

crisbeto reopened this Oct 24, 2022

crisbeto removed the action: merge The PR is ready for merge by the caretaker label Oct 24, 2022

crisbeto marked this pull request as draft October 24, 2022 18:24

pkozlowski-opensource added a commit that referenced this pull request Oct 25, 2022

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

c0903ac

This reverts commit 0329c13. PR Close #47845

pkozlowski-opensource added a commit that referenced this pull request Oct 25, 2022

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

9bfedb1

This reverts commit 0329c13. PR Close #47845

vakhramoff reviewed Oct 29, 2022

View reviewed changes

nouraellm pushed a commit to nouraellm/angular that referenced this pull request Nov 6, 2022

Revert "fix(forms): don't mutate validators array (angular#47830)" (a…

c0d5db0

…ngular#47845) This reverts commit 0329c13. PR Close angular#47845

fix(forms): don't mutate validators array

93405c2

Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes angular#47827.

crisbeto force-pushed the 47827/forms-validators-mutable branch from 88fd6ee to 93405c2 Compare November 16, 2022 08:59

crisbeto added target: patch This PR is targeted for the next patch release and removed target: rc This PR is targeted for the next release-candidate labels Nov 16, 2022

crisbeto marked this pull request as ready for review November 16, 2022 12:38

crisbeto added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Nov 16, 2022

dylhunn closed this in 779a76f Nov 17, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

crisbeto commented Oct 21, 2022

crisbeto Oct 21, 2022 •

edited

crisbeto Oct 21, 2022

alfaproject Oct 21, 2022

crisbeto Oct 21, 2022 •

edited

AndrewKushnir Oct 21, 2022

crisbeto Oct 24, 2022

AndrewKushnir left a comment

AndrewKushnir Oct 21, 2022

pkozlowski-opensource commented Oct 24, 2022

vakhramoff Oct 29, 2022

crisbeto commented Nov 16, 2022

crisbeto commented Nov 16, 2022

dylhunn commented Nov 17, 2022

angular-automatic-lock-bot bot commented Dec 18, 2022

	private _rawAsyncValidators!: AsyncValidatorFn\|AsyncValidatorFn[]\|null;
	private _rawAsyncValidators: ReadonlyArray<AsyncValidatorFn> = [];

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

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

Conversation

crisbeto commented Oct 21, 2022

crisbeto Oct 21, 2022 • edited

Choose a reason for hiding this comment

crisbeto Oct 21, 2022

Choose a reason for hiding this comment

alfaproject Oct 21, 2022

Choose a reason for hiding this comment

crisbeto Oct 21, 2022 • edited

Choose a reason for hiding this comment

AndrewKushnir Oct 21, 2022

Choose a reason for hiding this comment

crisbeto Oct 24, 2022

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Oct 21, 2022

Choose a reason for hiding this comment

pkozlowski-opensource commented Oct 24, 2022

vakhramoff Oct 29, 2022

Choose a reason for hiding this comment

crisbeto commented Nov 16, 2022

crisbeto commented Nov 16, 2022

dylhunn commented Nov 17, 2022

angular-automatic-lock-bot bot commented Dec 18, 2022

crisbeto Oct 21, 2022 •

edited

crisbeto Oct 21, 2022 •

edited