Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
View file
Open in desktop
Expand Up @@ -632,12 +632,6 @@
{
"name": "cleanUpView"
},
{
Copy link
Member Author

@crisbeto crisbeto Oct 21, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"name": "coerceToAsyncValidator"
},
{
"name": "coerceToValidator"
},
{
"name": "collectNativeNodes"
},
Expand Down
6 changes: 0 additions & 6 deletions packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json
View file
Open in desktop
Expand Up @@ -608,15 +608,9 @@
{
"name": "cleanUpView"
},
{
"name": "coerceToAsyncValidator"
},
{
"name": "coerceToBoolean"
},
{
"name": "coerceToValidator"
},
{
"name": "collectNativeNodes"
},
Expand Down
40 changes: 28 additions & 12 deletions packages/forms/src/model/abstract_model.ts
View file
Open in desktop
Expand Up @@ -372,15 +372,15 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T
*
* @internal
*/
private _composedValidatorFn: ValidatorFn|null;
private _composedValidatorFn!: ValidatorFn|null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* 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:
Expand All @@ -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:
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the type be:

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

Always an array and always readonly

Copy link
Member Author

@crisbeto crisbeto Oct 21, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* The current value of the control.
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}
}
44 changes: 44 additions & 0 deletions packages/forms/test/form_control_spec.ts
View file
Open in desktop
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

it('should override validators by setting `control.asyncValidator` field value',
fakeAsync(() => {
const c = new FormControl('');
Expand Down