Async validator not triggered on every change to input field value

[nsamarak]

Expected behaviour

I expected the async validator to be fired for every change to the value of an input field.

Actual behaviour

The async validator is fired for every change to the value of the input field when the value is passing validation. Once the value of the field no longer passes validation, the async validator looks like it's only firing for every other change to the input field.

Gist/Plunker/Demo

https://plnkr.co/edit/hdr3OFLNOXwuFuWKoQD1

[Anthropic]

@nsamarak you are correct, the way it works currently is stopping on the first error hit from tv4 and not processing further. I've added as an enhancement but it could be quite some time before it can be looked at in all honesty given limited resources.

[nsamarak]

Thanks for the response, @Anthropic.

From the documentation, it looks like $asyncValidators is built on top of ngModelController. What does tv4 have to do with $asyncValidators?

This issue seems more like a bug than an enhancement. Until this issue is fixed, someone that hits an error from an async validator can't submit the form. For example, there's an async validator on the title field in the plunker. The async validator ensures the word "test" does not appear in the title field. Type in "chickenstest". This will trigger the async validator. Then delete the last "t" so you end up with "chickenstes". Now there is valid input in that field. The error message disappears. However, the inline feedback icon for an error is still visible, there is still a red border around the input field, and the form can not be submitted. What should happen is the inline feedback icon for valid input should appear, the field should have a green border, and the field should also allow the form to be submitted.

Does this explain the bug better?

What do you think is causing the issue and are there any workarounds for it?

[nsamarak]

@Anthropic @davidlgj I might be able to help fix the issue if you guys can confirm that it is an issue, that there are no workarounds, and help me figure out a potential solution since I'm somewhat new to angular.

[Anthropic]

@nsamarak I missed this before I left for holidays, there is work being done in the webpack babel branch so that branch would be the ideal place to make changes.

It is hard to explain and I haven't finished the new version and don't have much time to update the documentation yet. But if you do think it isn't too late for you to take a look I'd be happy to try to take you through it at some point.

The main thing we need now is actually coded test cases, so that any fixes aren't broken again in future.

[bruderol]

Hi, I seem to have the same problem.

The funny thing is, that the validation message somehow disappears when the user pressed one additional key, but the revalidation ($asynchValidator function) does not get triggered and the field stays red (=ng-invalid).

I also not understand why you think that this has to do with TV4 validation? Because I think the $asynchValidator is set on the schema form form element and not on the json schema, therefore I do not see, why this should be a bug on TV4. In my case the schema validation stays allways valid (simple string field) it is just the asynchValidator that gets not triggered on one key press after it reported invalid once.

Let me know, if I can help somehow. We need a workaround or fix in my current project and I could help to fix it, if I get some guidance, how to do so.

Thanks a lot.

[shinnkondo]

A workaround is to use a tv4 key when you register your validator.

For the plunker example, if you substitute noTest with "tv4-1000" or something in script.js, the form should behave as expected.

This has a downside that this is probably not a proper usage of the tv4 error code.

[bruderol]

not sure if this tv4 workaround would help. Maybe I check tomorrow.

I at least isolated the problem a little bit more.
Following piece of code seems to be the source of the problem:

        // But we do use one custom validator in the case of Angular 1.3 to stop the model from
        // updating if we've found an error.
        if (ngModel.$validators) {
          ngModel.$validators.schemaForm = function() {
            //console.log('validators called.')
            // Any error and we're out of here!
            return !Object.keys(ngModel.$error).some(function(e) { return e !== 'schemaForm';});
          };
        }

Problem: this code prevents the $asynchValidators to get retriggered, because it decides, that the result is invalid allready, if there is allready another error, to prevent further validation pipeline being run. But I think that this makes only sense, if this other error occured in this same validation pipeline run, which definitely is not true in case this is an error which is validated asynchronously (and might also be wrong in other circumstances, e.g. if there are other custom validators that have not been executed at this point in time yet). Therefore I think this special kind of short circuit is very bad, and when I just removed this part, everything seems to work fine and I didn't see any downsides of it so far when testing (and I have quite some complex schema forms here, that all run very well without this code). Really not understanding why one would want to stop the whole validation pipeline at this point, when not even knowing whether this error at this point, is from last run through the validation pipeline or not. Therefore I recommend to remove this code or to somehow change it to not block the asynchValidators from doing their jobs.

[bruderol]

Aha, now I see the connection to what @shinkondo mentioned above, because there is in deed a little bit above the mentioned code another piece of code, which seems to clear any previously triggered "tv4-"-errors and then doing the tv4 validation as part of the parsers pipeline. So this kind of "tv4-"-errors are guaranteed to happen before this special piece of code (mentioned above) gets called. So I think this code should be changed to only stop validating further, in case such a tv4 error (but no other validation errors) allready occured before, which seems to be the purpose of this code? What you think about this? And this would be okay from my point of view and would not stop $asynchValidators from being called anymore.

If you agree, I could make a PR for this change.

And another question: Why is this issue only rated as "enhancement" and not as a "bug"?
As you can see in following part of the documentation, this is an explicitly mentioned feature that seems not really to work (even the mentioned example would not validate when typing "bob" and then making "bo" with backspace out of it): https://github.com/json-schema-form/angular-schema-form/blob/development/docs/index.md#asyncvalidators

[Anthropic]

@shinkondo san domo arigatou gozaimasu, I hadn't thought of that, interesting.
@bruderol please take a look at the code in the webpack branch as it may have already had some changes applied, I can't remember off the top of my head, I can check tonight when I get home.

[bruderol]

@Anthropic had a look at the webpack branch (you mean feature/webpack-babel), right? Same code is still there (in schemaValidate directive, line 96-104). Should I make a PR? Should this go to this branch (when is this planned to be released?) or shouldn't this better be done on a hotfix branch to release it isolated and then merge it to the mentioned feature branch?

[bruderol]

By the way, the workaround with "tv4-" prefix as proposed by @shinkondo seems to work. Thanks a lot!
Funny thing is that for the error message the message has to be mapped on error code without the tv4- prefix, because schemaform strips this special prefix from error codes. All in all I think that this behaviour is weird and should be fixed. Let me know if I should make a PR.

[Anthropic]

@bruderol if the same code is there (yes, in feature/webpack-babel) then a merge to develop should be fine and I can copy it across to the new branch, just wanted to check that it wasn't different to save you (and me) time! :)

The new branch is hopefully nearing readiness, just more cleanup to do and some more testing, but more importantly I need to make it easier to work with so good people like yourself can make changes to it and submit PR without getting lost. Unfortunately I am rather time-short which is why I need as much help as anyone is willing to afford on currently known issues :) The main thing I need help with is more test cases, but again much easier if they are done in the new branch to save re-work.

I am aware of the prefix issue, there is another ticket somewhere regarding textarea validations not working because of the way the code strips out the tv4 validations and doesn't replace the content appropriately. Basically if you have a text area with a max length of 20 the error only shows at 22 and only removes on deletions at 19 due to issues with angular errors vs tv4 errors.

[Anthropic]

I think the tv4 issue I mentioned was this regression #633