fix(forms): handle form groups/arrays own pending async validation - #22575

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Hi @benbraou, thanks for the reply and applying additional changes 👍

I had another look and I'm wondering if we can reuse existing _asyncValidationSubscription field as an indication that async validator is "in progress"? I.e. we can probably set it to undefined once async validator is completed (and may be have a function like _hasOwnPendingAsyncValidator that would return boolean based on the value of the _asyncValidationSubscription field). What do you think about this solution?

Also there are some failing tests on CI, could you please look into them? It looks like most of them are added as a part of this PR.

FYI, I also started tests in Google's codebase (blueprint and global, links are internal-only) to see how this change might affect existing apps.

Thank you.

Hi @AndrewKushnir. Thank you for the review. Sorry about the failing tests, I should've let you know that I haven't finished updating them yet. They should be fine now.
I am now using only _asyncValidationSubscription as you suggested.

      this._asyncValidationSubscription = obs.subscribe((errors: ValidationErrors|null) => {
        this._asyncValidationSubscription = undefined;
        this.setErrors(errors, {emitEvent});
      });

I just have one comment, the order is important. The subscription needs to be set to undefined before setting the errors. This is because this.setErrors will trigger a recalculation of the control status

  setErrors(errors: ValidationErrors|null, opts: {emitEvent?: boolean} = {}): void {
    (this as {errors: ValidationErrors | null}).errors = errors;
    this._updateControlsErrors(opts.emitEvent !== false);
  }

  _updateControlsErrors(emitEvent: boolean): void {
    (this as {status: string}).status = this._calculateStatus();
  ...

And _calculateStatus is now also based on whether this._asyncValidationSubscription is undefined.
Not sure, if this is error prone in the future and whether a comment is needed?
Thank you

@AndrewKushnir thank you for the continuous review and help. I have fixed the typos

Hi @benbraou, thanks for the fixes 👍

Tests in Google's codebase resulted in a couple failures. After further investigation it turned out that my proposal to reuse the _asyncValidationSubscription field was incorrect :( The thing is that some validators may return just of(null) (instead of more complicated setTimeout/Promise logic that we have in tests), which will synchronously emit values when we subscribe:

this._asyncValidationSubscription = obs.subscribe((errors: ValidationErrors|null) => {
  this._cancelExistingSubscription();
  this.setErrors(errors, {emitEvent});
});

So what'd happen in this case is that the callback function would be invoked synchronously and at that time the this._asyncValidationSubscription is not yet defined. As a part of the callback execution, we'll try to clear the subscription (which is not yet defined at this point) and apply the changes to the control and parent. Once callback function completes (and additional sync machinery in RxJS), the this._asyncValidationSubscription becomes defined. The problem is that the actual state is that validator already executed, but the this._asyncValidationSubscription field is still defined on a control, indicating that it's still pending.

So I think we should go back to your original approach of having a boolean flag (we can reuse the _hasOwnPendingAsyncValidator field and make it a boolean instead of the function) and revert other changes made to the _cancelExistingSubscription function. It'd be also great to add a few tests to illustrate the use case of a simple validator, i.e. when validator looks like this:

const validator = (c: AbstractControl) => of(null);

We can probably include this logic into the simpleAsyncValidator function that you developed and invoke that when timeout === 0. What do you think?

Sorry that my proposal turned out to be incorrect (thus we have to make this extra iteration) and thanks for all your help with this PR! Please let me know if any additional information is required.

Thank you.

@AndrewKushnir No problem, Thank you! I really appreciate the detailed explanations throughout the PR. That's very educational 👍
I am now using the boolean flag to handle async validation, reverted the changes for canceling the async subscription, updated the simpleAsyncValidator to return of(result) if timeout === 0.
Also, I added a couple of tests.
I have just one remark regarding this new test that I added:

    it('should handle failing async FormGroup and failing child control validators resolving synchronously',
       fakeAsync(() => {
         const c = new FormControl(
             'fcValue', null!, simpleAsyncValidator({timeout: 0, shouldFail: true}));
         const g =
             new FormGroup({'one': c}, null!, simpleAsyncValidator({timeout: 0, shouldFail: true}));

         // FormControl async validator has executed and failed synchronously with the deafult error
         // `{async: true}`. Next, the form group status is calculated. Since one of its children is
         // failing, the form group itself is marked `INVALID`. And its asynchronous validation is
         // not even triggered. Therefore, we end up with form group that is `INVALID` but whose
         // errors are null(child error do not propagate and own async validation not event
         // triggered)
         expect(currentStateOf([g, g.get('one')!])).toEqual([
           {errors: null, pending: false, status: 'INVALID'},           // Group
           {errors: {async: true}, pending: false, status: 'INVALID'},  // Control
         ]);
       }));

The comments should be clear, but basically, I was expecting the form group to have its own errors in its status.
However due to this logic, the async validator is not even run(the form group is INVALID because of its child)

  updateValueAndValidity(opts: {onlySelf?: boolean, emitEvent?: boolean} = {}): void {
     ....
      if (this.status === VALID || this.status === PENDING) {
        this._runAsyncValidator(opts.emitEvent);
      }
     ...
    }

Anyway, if this behaviour should be changed, I think this is not linked to the original issue that this PR is solving
Thank you

Hi @AndrewKushnir, I have addressed the remaining comments.
Thank you for the review and your time!

Thanks for addressing the feedback @benbraou.

FYI, I've rebased your PR on top of the current master, since CI was "red" (since PR was not rebased recently).

As I mentioned in my previous message, running tests in Google's codebase didn't indicate any issues (including a global run in Google's codebase). Also given the fact that this is a bug fix with low risk, I'm removing the "breaking changes" flag. In any case, since the behavior of the framework changes slightly (to become more accurate), we would merge it to master only and it will be released as a part of the next minor version (v10.1.0).

Thank you.

Great news. Thank you !

Hi @benbraou, just wanted to let you know that your PR is now merged to master and the changes should be available in v10.1.0 release of Angular 🎉 Thanks for your contribution to Angular!

Please let me know if you might be interested in doing some further refactoring around async validators in the tests (to reuse one async validator across multiple test cases). Thank you.

@AndrewKushnir thank you! Yes, I am interested to continuing the refactoring in tests. Can you please let me know if you want me to create a new issue for this?
Thank you

Hi @benbraou, thanks for the reply.

Yes, I think creating a new ticket would be great, thanks.

I was thinking if we can extract common validators code into a new packages/forms/test/util.ts file (we can store more functions there later). Some validators that are used across many tests (may be you can find more):

• function asyncValidator
• function asyncValidatorReturningObservable
• may be simpleAsyncValidator added in this test can use some common code from asyncValidator
• I also see a value of moving the currentStateOf function (added in this PR) into the util.ts file, so we can reuse it for other validators-related tests in other files

Please let me know if any additional information is required, I'll be glad to help.

Thank you.

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.}