New validation extension

[tjololo]

Description

Backend issue for EPIC Altinn/app-template-dotnet#180
Today custom validations performed in the backend is done by implementing the IInstanceValidator interface.
The code is triggered by frontend either just before the process/next call is performed or if singelfield validation is configured.
The same code is executed for both singel and full validation and is a place for mistakes and unclear logic.
Another problem is that if a previously error is fixed the code needs to generate a "FIXED" validation issue so that frontend can remove it.

The new implementation should support that one group of validations should be triggered if a field is changed, but all validation logic should be triggered when the process is tried to be moved to the next step.

The response from a group of validations should be the same either if it's ran as a singelfield validation or a full validation and the data is the same.

List of wanted functionality, this is a work in progress:

• A set of validations should be possible to group together as a set of fields as a whole could trigger/fix a issue.
• A validation group cannot clear or create issues for another validation group
• All validation should be executed and checks on process/next
• ValidationIssues are always group under their validation groups
• It should be clear to the developer when an implementation of the new validation logic is triggered (what fields, if any, triggers the validation. Remember all validation are always checked on process/next)
• One field in the datamodel can be checked in different validation groups
• When a multipart save is triggered the validators with explicit mentions of one or more of the fields should be triggered.
• Validators without any fields defined should only be executed on process/next
• Altinn.App.Core.Models.Validation.ValidationIssue should be extended with ValidationUrgency to give the developer a way to define when the issue should be shown.

Additional Information

Example of a repsone from a controller where one validation implementation is triggered:

{
  "validationGroupName": [
    { //obj of type Altinn.App.Core.Models.Validation.ValidationIssue },
    { //obj of type Altinn.App.Core.Models.Validation.ValidationIssue }
  ]
}

Full validation with issues from multiple validation groups:

{
  "validationGroupName1": [
    { //obj of type Altinn.App.Core.Models.Validation.ValidationIssue },
    { //obj of type Altinn.App.Core.Models.Validation.ValidationIssue }
  ],
  "validationGroupName2": [
    { //obj of type Altinn.App.Core.Models.Validation.ValidationIssue },
    { //obj of type Altinn.App.Core.Models.Validation.ValidationIssue }
  ]
}

[olemartinorg]

Validators without any fields defined should only be executed on process/next

Just to make sure, they would also trigger on a normal/full validate call, right? Or are we talking about the 'task validation' concept?

When a multipart save is triggered the validators with explicit mentions of one or more of the fields should be triggered.

And, to make it very clear, these validations should be returned from the save operation, for example in a new validations property in the response object.

The last thing I think we need is the new urgency property in the validation objects themselves, but the form and structure of that is up to @bjosttveit to decide.

[tjololo]

Validators without any fields defined should only be executed on process/next

Just to make sure, they would also trigger on a normal/full validate call, right? Or are we talking about the 'task validation' concept?

Yes, they would be triggered on normal/full validation. I think we should make a even more clear separation between task and data validation.

When a multipart save is triggered the validators with explicit mentions of one or more of the fields should be triggered.

And, to make it very clear, these validations should be returned from the save operation, for example in a new validations property in the response object.

Yes, I think that is the logical place to return them, removing the need to do an extra validation call after the data is saved. But I think we should still implement the validation endpoint there for the users consuming an application only through the API.

The last thing I think we need is the new urgency property in the validation objects themselves, but the form and structure of that is up to @bjosttveit to decide.

Yep:

• Altinn.App.Core.Models.Validation.ValidationIssue should be extended with ValidationUrgency to give the developer a way to define when the issue should be shown.
  Is meant to cover that, so we are adding the urgency as a new field on all validationissues

[olemartinorg]

Great! And sorry - i missed that last point!

[tjololo]

We should also keep in mind that simple validations like "field should not be empty" can easily be implemented using expression based validation: #311 as these validations can be executed by the frontend during formfilling and then again by the backend when data is submitted.

[bjosttveit]

Quick question regarding Severity, we should obviously get rid of Fixed. But can we also get rid of Unspecified? since we have no way to handle validations like that anyway, and we should require that the severity is specified. Having Error, Warning, Info and Success should be more than enough.

[bjosttveit]

One more thing, in v4, we store all validations even though they are not visible. This extends to validations for hidden components as well, so we don't need to "trigger" running validations whenever a component is unhidden. This means that the backend should no longer remove hidden data before running validations. We may need to add a featureflag so that it still works the same with v3.

[ivarne]

This means that the backend should no longer remove hidden data before running validations.

Pretty sure this is a bad idea. This will make it hard to have correct dependent validation on hidden fields (ie. the error shows up on an always visible feld but depends on a field that sometimes is hidden.

Showing fields will only trigger on a field change anyway, so frontend should just report the newly shown fields as changed.

[bjosttveit]

How would the backend know which fields to validate if the data has not actually changed? From a frontend perspective, the data is always there, regardless of its hidden status. So when saving it would not recognize a component becoming unhidden as fields being added to the data model. But I suppose if the backend does the diff and removes hidden data on the old and new formData it would see it that way.

[olemartinorg]

I'm aware that developers have re-used the code to remove hidden data when running validations, and I've seen bugs being introduced because the validation logic was too simple and ignored potential hidden fields.

As an example, let's say you have the classic form:

Contact me using:
[x] Phone  [ ] Email

Your phone number:
[                 ]

// Hidden: Your email:
// [                 ]

And the validation logic is:

if (model.phoneNumber && !isValidPhoneNumber(model.phoneNumber)) {
  addValidationError('model.phoneNumber', ...);
}

The result here is that if you type an invalid phone number, change your mind and switch to show the email field, the validation for an invalid phone number would still remain.

So, I think there are multiple strategies to solve this today:

• The simple one is to check the 'contact me using' field in the validation logic as well, and only add the validation if the field says 'phone'.
• For more complex scenarios, instead of duplicating hidden logic, you could create a full copy of the data model and remove the hidden fields, so that phoneNumber would be null in the cases where email is now the selected contact method.

But I think those two strategies have a crucial weakness; they require that the application developer fixes the problem for us. That's why I don't like them, and I think we need to do better.

This will make it hard to have correct dependent validation on hidden fields (ie. the error shows up on an always visible feld but depends on a field that sometimes is hidden.

Not sure what the use-case is for this, or if you have a good example for it, @ivarne, but I struggle to understand what's the desired outcome there. Do we want to remove the validation message for the always visible field when the source field is hidden? Or do we want the validation to stay as long as the data stays? As long as I don't have a good grasp on that use-case, I would think it depends - and that, of course, is awful. We don't want to make this configurable.

Showing fields will only trigger on a field change anyway, so frontend should just report the newly shown fields as changed.

Technically, expressions can also be used to hide/unhide fields on language changes - but I hope nobody is using that. We don't necessarily save the form data (and especially don't run validation again) when the language changes.

In the near future, when we'll support sub forms and multiple data models per process step, it will be very easy (and I guess, even recommended) to store prefill/shadow fields/metadata in a separate data model, and use that data model for expressions. So, if you'd like to go down that route of hiding/removing validation messages when they are hidden, it will quickly become a bottleneck when you have to fetch all kinds of different data models to evaluate expressions in order to figure it out. We still only call the request to save data (and validate) per data model.

And again, technically, we still need to support the old hide/unhide logic in frontend, and there you can even hide a field based on the positions of the sun and the moon - or the time of day, or some random number - but yes, it would only be possible to trigger it via a change in some data model.

[ivarne]

but I struggle to understand what's the desired outcome there.

I want the app developer to always see the real data that they intend to validate (and not see potential extra data that is hidden and will be removed).

The natural result that follows from that, is that for validation purposes there are two changes when you toggle the contactMeUsing radio button with to email, with an invalid phoneNumber.

1. contactMeUsing: "phone"=>"email"
2. phone: "+47113" => null
   and validator filter should run validations for both changes.

Hard to find concrete examples where there isn't really a better way to handle validation, but one somewhat contrived issue is that you at the end you declare that all contacts live in Norway, and you can add a validation error if you use a foreign country code in the phone number.

Do we want to remove the validation message for the always visible field when the source field is hidden?

No. Neither Frontend, nor backend knows the list of data model keys used to create a validation issue (with the current design). We only know the field where the issue should be presented. Frontend gets a list of the validators that executed for this partial validation, so it can clear all messages from that validator when it gets an update issues list.

What I want is for the validation implementation to know that the phone number must be revalidated when you click on the contact method radio button.

One issue that appears is what to do with validation issues pointing to a hidden field. I think that should be flagged as an problem in dev tools, but otherwise ignored until form submission where it is shown as a task issue that prevents processing of process/next

How would the backend know which fields to validate if the data has not actually changed?

My initial thought was that frontend would need to also report fields that changed from null (hidden) to the previous value.

Backend could possibly use the diff dictionary from frontend about the real data model changes, and recreate both the previous and new models, remove hidden data from both and create a new diff for validation. Writing this out indicates that it could work, but adds some load to the server that could be reduced if frontend does the work and provides the combined list of changed fields. Another drawback with this issue is that expressions might reference other data models and increase the time required to save/partially validate, and frontend has this info in memory anyway.

[olemartinorg]

Looks like we're coming to the same conclusion here.. 😅 I ended up talking with @RonnyB71 and @tjololo about this as well, spontaneously after a meeting we just had.

This takes me back a bit. I remember a discussion about what to do with form data for hidden components in april 2022, right after I had started working here - might also be the day we first met, @ivarne. Not sure if you remember it, but I believe the consensus then was that frontend should simply store all data server-side, regardless of the data being accessible in the form. The reason, of course, is that quickly changing between a radio (like the phone/email described above) should not make you lose your data (i.e. the phone number you started typing). I remember thinking back then that it seemed like an incredibly complex way of solving a seemingly minor problem. In my mind, a user probably won't expect that (hidden) phone number to re-appear in the form for very long, and especially not after a refresh (or even after continuing to work on the same instance on another device). Storing that stale (and sometimes schema-breaking) data client-side seems like a better solution. I think there's a potential privacy issue in storing hidden data and potentially submitting it to the service owner as well. Ideally I would like for the client-side to be the one to clean away hidden data - and, as I believe you've indicated as well, send the form data with hidden fields removed to the backend.

In that world, there would be little need for removing hidden fields on the backend, apart of course from shadow fields. I think it's unrealistic that API clients would send in data not needed, so I don't think the cleaning pass is needed there at all. Such a solution would solve a lot. There's no need for the application developer to implement any of my suggested workarounds from above, the backend gets a cleaner data model, and I think it's a small win for privacy.

I first thought it would be nice to store the data in localStorage on the client-side, so that it's still present when you refresh the page, but that's not necessarily encrypted - and it may open up for future bugs where users sharing a device suddenly also get other users hidden data back. During the meeting we concluded that it's perfectly fine to just keep the data in memory, and lose it at soon as you refresh the page or navigate somewhere else.

Even if we end up deprecating the hidden data removal stuff on the backend, expression support is alive and kicking, especially now because of the expression-based validation functionality. EDIT: What I didn't think of when writing this is that we still need to evaluate required on the backend, and to properly know if a field really is required, we still need to know if it is hidden. Sadly, that means we still have to know a lot about the layout on the backend.

[ivarne]

Key management for encryption is very easy, as the key can just be stored on the instance with the same access restrictions as the form content.

If you persist the full data model encrypted in the client and always send content with hidden data removal, that will definitely help simplify the backend without adding total complexity. The storage is required for offline capability anyway.