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.

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

Validation - Future direction and principles #180

Open
2 tasks done
RonnyB71 opened this issue Jun 7, 2023 · 7 comments
Open
2 tasks done

Validation - Future direction and principles #180

RonnyB71 opened this issue Jun 7, 2023 · 7 comments
Labels
Epic status/draft Status: When you create an issue before you have enough info to properly describe the issue.

Comments

@RonnyB71
Copy link
Member

RonnyB71 commented Jun 7, 2023

Description

This epic describes the future direction and principles which validation issues should be based upon in order to solve the major challenges we see with validation today.

Challenges today

Frontend and backend both validates input but don't always agree

Today frontend validates the data model against the Json Schema and backend validates against the strongly typed C# class with attributes based on the XSD. While all originate from the same source (Json Schema) there might end up being small differences like a large number that validates against the Json Schema, but fails against the strongly typed C# model. Also custom validations (in backend) will in the case of field triggering override the validation done in frontend, meaning you will have to reimplement any frontend validation logic.

Altinn/app-frontend-react#279

Datamodel restrictions vs component validation

We have two levels of validation. One is to adhere to the restrictions set on the data model and the second is validation rules defined on the component level. While both needs to be there and respected, we should have a clear definition of the order, the originating source of the error and which part is responsible for what kind of validation.

Dependent on custom validation in backend

If you need custom validation, the only way to add this is by resolving to C# and coding it in the backend. This makes the bar higher than necessary for application developers who should be able to express this as rules using the expression language in json configuration or in Altinn Studio.

Implementation goals

Only involve backend validation when strictly needed

This means that frontend should be able to validate as much as possible on it's own and that backend should only be involved in the last step when submitting the data as a final safe guard when submitting the data. This also means we to some level need to extend the support for custom validation to frontend.

Opt-in when you need special validation

If you need to resolve to custom validation in the backend this should be (as today) a choice for the developers. However the implementation should probably change a bit towards small validation interfaces with easy access to necessary information like what page triggered the validation, what component, what was the previous value, what is the new value etc.

Standardized validation messages

Validation messages should be based on the Problem Details for HTTP APIs. Validation problem messages should contain the originating source of the error (custom, data, serialization) to allow frontend to make a decision on how to handle them and other required information ref. todays ValidationIssue class.

Extend the current expression language with validation support

In order to allow more validation in the frontend we need to support the use of expressions when configuring components. These should run in the frontend by default, and in backend as part of the validation done when submitting data.

Altinn/app-frontend-react#726

Validate data model against the same Json Schema in frontend and backend

In order for frontend and backend to be consistent in the data model validation we should preferably validate against the same schema. We still need to address XSD validation as an option to enable as this is critical for some applications depending on XSDs.

Altinn/app-lib-dotnet#197

Validation of composed components

Some components consists of multiple fields. When we do component validation today we don't know which of these fields that has a problem. We should know which field and be able to provide different validation rules for different fields in components composed of multiple fields.

Scope

In scope

No response

Out of scope

Validation of application configuration/state

This issue addresses form validation and not validation of the state of an app ie. are all configuration files valid, are there missing text resources etc.

Additional Information

Tasks

No response

Tasks

Preview Give feedback
  1. kind/feature-request
    bjosttveit
  2. kind/feature-request
    bjosttveit
@RonnyB71 RonnyB71 added status/draft Status: When you create an issue before you have enough info to properly describe the issue. Epic labels Jun 7, 2023
@olemartinorg
Copy link
Contributor

olemartinorg commented Oct 17, 2023

I was about to write a new issue, when I found this one. Adding my thoughts here. I don't think we're quite done yet finding a solution that fits all use-cases we want to cover.

Background

Currently, validation is somewhat modeled on Altinn 2, with a few extensions (presumably, I'm not too familiar with Altinn 2). In Altinn 2, the validation model is based upon the intended app flow where the user fills inn relevant information into form fields, and when reaching the end of the form they click a button ("Kontroller skjema") to run the entire form through validation (meaning schema validation runs, and potential custom validation). In Altinn 3 we also run full valdation at the end of the form submission, but it happens implicitly when the user clicks the submit button, and we prevent the submission and present the user with validation messages in the bottom of the form, around the submit button, telling them what they have to fix before sending in. See screenshots in Altinn/app-frontend-react#100.

In addition to that, we have some extra possibilities:

  1. An app developer can trigger validations on any form field (aka single field validation)
  2. Every form component should be bound to the data model somehow, and if the data entered into the form field is not valid according to the schema, we attempt to provide the user with a meaningful validation message letting them know how to address the problem (i.e. "this should be no higher than 15", etc)
  3. A form field can be marked as required: true. Before submitting the form we check that all (non-hidden) fields marked as required are filled out, and if they aren't, we show a validation message about it.

Problems

1a. Single field validation is hard to get right

References:

In short, it's very difficult for app developers to write backend code that works the way they probably expected it to work. Since we re-use the same endpoint for both regular instance/data model validation and custom single field validation, it is very easy to write validation logic that either:

  • Runs all validation logic when you thought you ran single-field validation logic
  • Or only runs the validations you wanted to run, but leaves it open for the user to ignore those validations and submit the form with validation errors anyway

This is related to 1b, because a solution to this could be to provide better interfaces to work with on the backend, and make validations declarative in a way that makes it possible to know which validations ran and were OK for each validation trigger/run.

1b. Having to use *FIXED* validation messages is a code smell

References:

In essence, our validations are currently not made in a way that makes them easy to handle in a declarative way, so this hack is used to indicate that a validation ran and did not produce an error. Instead of making the app developers implement that, our abstractions should do it for them.

1c. Validation messages are lost when refreshing the page

References:

What it says on the tin. If you have a costly validation routine, and/or a validation rule that is only triggered when changing a single field (and not upon validating the whole process step), there's no guarantee the error will show up or run again should you choose to refresh the page after it showed up. At that point the data is stored the way it was, you can choose not to change the data again, and thus any single-field validation will trigger on it.

If we're happy with leaving this door (partly) open for app developers to fall into (and users to exploit?), we should at least make it a very conscious choice by the app developer. And if we're not, we should construct our APIs and app-frontend in a way that makes sure we always trigger any single-field validation when they exist, and that we can simply trigger a full form validation on the backend when the user loads an existing instance. It should possibly be possible to configure when a validation should appear and separate that from when we triggered the request to the backend.

2. Duplicated error messages in frontend

References:

Solved now, by tagging validation messages with a code/type/source?

3a. Validation messages show up on all pages, even pages they're not relevant for

References:

We need to figure out:

  • Do we want app developers to control where validation messages show up? Great defaults vs customization
  • Does this indicate that something in our current model is broken? Maybe it only makes sense to show all messages on all pages if they're showing up because you tried to submit the form?

3b. It should be possible to declare when the error message should show up, different from when the trigger caused the validation code to run

References:

Similar to 3a, deep in the referenced issue an app is mentioned where a custom backend-triggered validation checks that the user has filled out at least one of 6 different form field. Filling out one is required, and should produce a validation error, but none of the fields can be marked with required: true as none of them are really required if any of them have been filled out.

The problem here is that required-validations behave a little differently in app-frontend, but we haven't exposed a way for app developers to mimic that behavior using custom validations. In app-frontend we postpone required validations and show them to the user as late as possible, because we don't want to nag the user when they haven't filled out anything (possibly because they haven't gotten to it yet). It should be entirely possible to use the tab button to jump through the form without causing a lot of red to show up, because focusing and then blurring a field does not mean you did something wrong. Not filling out a required field and trying to submit the form is an error, however, but our current validation triggers are called when blurring a field with a validation trigger (and rightly so).

A way to think about this is; there are lots of required-validations present in all forms when you open them up, but they gradually disappear when you start filling out the form. Other validations (such as "the value you typed is not a valid phone number") appear when the user types an invalid value. The required validations are shown as late as possible (when we cannot hide the fact that you haven't filled out a part of the form yet) and other validations are shown as early as possible (as soon as we can reliably assume you've finished typing the invalid value).

EDIT: The second reference (the Slack thread) is a good example of where this goes wrong. Error messages have been 'triggered' to show up, but they're not automatically updated when the form inputs change from the outside.

4. Costly and/or time consuming validations should only run once per unique form data input

References:

Some validation messages are produced as a result of an external API call, possibly using a very strictly rate-limited API (such as SSN lookups), and these are very much in conflict with the usual light-weigh validations we want to run as often as possible.

We need to have some infrastructure in place to make it possible to write these kinds of validations, and cache their results in a way so that we can simplify the development process for application developers and still preserve the validate-as-soon-as-possible model as described in the last parts of 3b.

@olemartinorg
Copy link
Contributor

With all that said, me and @tjololo had a discussion about a few of these, and I think we've landed on a better path going forward. Key takeaways from that discussion:

  • We should go for a declarative approach. Meaning, when we 'trigger validation' the frontend send a request to the backend and the backend replies with a list of validation messages and crucially also a list of the unique validator functions that ran. That way we can store that state in the frontend, and always know what's the latest result for a validation procedure, and importantly we can know immediately if the response from request B told us the validation we previously got in the response from request A now got fixed.
    • This fixes 1b
  • The new interfaces the app developer gets to work with on the backend should be written in a way that makes it clear to app developers that the code they now write will be called when validating the whole data model (before the process step is submitted) and possibly also immediately after changing some data (as configured, with certain paths in the data model indicating which field changes should 'trigger' a call to the custom code). This then replaces the current way of running single-field validation, as it provides a way backend code to hook into certain changes and run their custom validation logic.
    • This fixes 1a
    • This alleviates 4, but does not entirely fix it. With hooks to certain data model paths in the backend code, how often certain validator functions run will be limited to how often these data model paths change. With some caching infrastructure around these interfaces we can most likely use this methodology to fix 4 entirely.
    • If the backend already has a list of data model paths known to trigger validation logic, this list could be requested and returned to app-frontend. That way app-frontend could know which fields ought to trigger validation without them having a validation trigger configured.
  • Repeating groups makes this a lot more difficult. Right now we support validation triggers on repeating groups, and they work such that clicking the 'save and close' button in an open group row will run validation before letting the user close the row for editing. A triggers: ['validation'] on a Group will cause all rows to be validated, but the newer triggers: ['validateRow'] will only trigger validation for that group row. How does this overlap with, or complicate the newer backend interfaces? Supporting single field validation triggers inside repeating groups is easy enough, but putting the validation trigger on the Group means we postpone the validation message until the user tries to leave the group row.

In addition to this discussion with @tjololo, we had a meeting about the future of validation as well. Some thought that came up there includes:

  • We could just return validation messages as a part of the form saving operations. If validations are always triggered according to configuration (especially when that configuration is on the backend), the backend always knows that the data model should be validated after saving certain fields.
  • A fully declarative approach. What does triggering validation mean? It really only means the app developer wants to control when to show the message, not necessarily when we should run the code to check fields and values. If the app developer gets to control when validation messages show up in a way, we can make sure to run the code as soon as possible and hide the messages until they are supposed to be displayed.

@bjosttveit
Copy link
Member

bjosttveit commented Nov 2, 2023

I would like to highlight a few issues related to how we store validations in the frontend.

  1. We currently only store information about the component the validation is for, not the actual dataModel field/binding. This leads to the limitation of not being able to link errors to specific input fields in complex components like the Address component. Error summary links are wrong when used with the address component app-frontend-react#366

    There are a few ways to solve this:

    • We could store the bindingKey (simpleBinding, streetAddress, postPlace, etc.) along with the validation message.
    • With a few exceptions (like fileupload, or required at the component level), validation messages are usually connected to fields in the datamodel rather than the components themselves, and we could store validations per datamodel field instead of per component, and then each node is responsible for getting the validation messages for the fields it is bound to. This would also avoid storing validation messages multiple times for the same field if two or more components are bound to it. This would of course make it more difficult to link messages in the error report to components; if there are multiple components bound to a single field, which component should the error report then link to?
  2. Validation messages are stored using the "resolved" text resources, meaning that langAsString() is called before storing the message. This leads to the limitation where existing validation messages are not translated when switching language using the language selector, this is not a huge problem, but if we are already gonna change how we handle validations we could take this into account. I could not find an issue in app-frontend-react for this particular problem, but I have been aware of it for some time.

@olemartinorg
Copy link
Contributor

Good points! I think solving both of these in Altinn/app-frontend-react#1506 should be at least looked into.

we could store validations per datamodel field instead of per component, and then each node is responsible for getting the validation messages for the fields it is bound to

Agree!

This would of course make it more difficult to link messages in the error report to components; if there are multiple components bound to a single field, which component should the error report then link to?

Good point. I guess that was "solved" by duplicating the validation in the state we have now, but I don't think that's the right solution. We should instead try to find the first available (non-hidden) component we can navigate you to, and if the user cannot fix the problem there, they're on their own. I guess that would be a limitation in our apps we would expose more clearly by storing the state this way, and if we want to at a later point we could add a new property to validation issues indicating which component we should focus/navigate to when the user clicks to fix the validation.

@olemartinorg
Copy link
Contributor

I'll add some notes about what I've been thinking lately about our future direction re: validations. Right now, as the configuration is often based on triggering validations, we've ended up with some of the problems I have described above. The solution I've envisioned, extends our current validation data types this way:

enum ValidationUrgency {
  // The validation message shows up immediately, even when the user is typing
  Immediate = 10,

  // Shows up when the user has stopped typing for a while (defaults to 400ms, but can be configured
  // using `saveWhileTyping` property on the component).
  AfterTyping = 15,

  // Shows up when the user moves focus out of the field
  OnBlur = 20,

  // Shows up when the user tries to 'save and close' a repeating group row. Only affects form fields inside
  // the repeating group row (validating all rows in the group at the same time was most likely a mistake in
  // when implementing `validateGroup`, so I think we're safe to ignore that case).
  OnGroupRowClose = 30,

  // Shows up when the user tries to navigate beyond the current page (finally regardless of the
  // component used to do it!). The user can still navigate to the previous page without getting the
  // validation message.
  OnPageNext = 35,

  // Shows up when the user tries to navigate away from the current page. It would not be possible to
  // navigate to the previous page without getting the validation message (and be blocked from navigating).
  OnPageNavigation = 40,

  // Shows up when the user tries to submit the form. At this point page navigation cannot be blocking, as
  // the user would not be able to go back and fix the validation issues.
  OnFormSubmit = 50,
}

interface NewValidationIssue extends BackendValidationIssue {
  urgency: ValidationUrgency;
}

interface ValidationGroups {
  [group:string]: BackendValidationIssue[];
}

Right now the validation endpoints returns BackendValidationIssue[], but they should now instead return ValidationGroups. That is, as I know @tjololo have started implementing, validations endpoints should say which validation groups ran, and how may BackendValidationIssue objects that run resulted in. This more declarative approach, as mentioned previously, fixes 1b in my list above.

The next part here is the extension of ValidationUrgency I'm proposing. Right now we have triggers for running validation as soon as the user clicks the button to navigate to the next page, but it should be irrelevant when that validation actually runs, the logic in app-frontend should instead be to not let the user navigate away from the page if any of the fields on the current page has validation errors with urgency <= 40. We already have this system in app-frontend today, in a sense, as required fields by default has an 'urgency' of 50 (or 40, if the navigation buttons are set to trigger validation), meaning the required validations will not show up unless you try to submit the form without them (or, if using navigation triggers, going to the next page without filling them out). I think we could get away with allowing a simple per-app (or per layout-set) configuration for how urgent the required validations should be.

As @ivarne has mentioned, it's kind of weird that we have to run validation as a separate step after saving. I agree with him that validation issues (or, ValidationGroups) could just be returned as part of the response when having saved the form. That way we can maintain the internal state in app-frontend with a list of validations we currently know of - and of course use the urgency property to indicate when we should actually display them to the user.

I think this solution fixes most issues:

  • 1a will be fixed by the new interfaces for defining a ValidationGroup on the backend, which @tjololo is in the process of implementing. Along with validating fields that have changed, automatically after saving (as suggested by @ivarne) there should no longer be a need to actually declare "triggers": ["validation"] on components using single-field validation on the backend.
  • 1b would also be fixed by using ValidationGroups. There is no need to say that a validation is now fixed, as now-irrelevant validations would disappear from the group and app-frontend would know that fixed it.
  • 1c would be solvable using the urgency property. When loading an existing data model, app-frontend would have to run a validate call on it straight away to get the full current state of the validations belonging to that data model. Having an urgency of <= 30 means the validation message would then show up immediately upon loading the form. If the urgency is 30 and the form field is in a repeating group that is set to be open by default, we would of course wait to display that validation to the user until the user attempts to close the group row.
  • 2 is already solved
  • 3a is a design issue, I think. I'm not sure it is relevant to keep "dragging" along validation issues for unrelated pages as forms become deeper and more complex - but at the same time we need that big red box on the bottom to show the user why we couldn't just submit the form. I think we need to design some new interfaces or modes for how we want this to work in the future. It might be as easy as differentiating between 'you cannot submit this form yet, because...' and 'you cannot navigate to another page yet, because...'
  • 3b is by definition the problem we're solving with the urgency property and separating when app-frontend receives the validation message from when we should display it to the user.
  • 4 needs to be solved by caching on the backend

@olemartinorg
Copy link
Contributor

olemartinorg commented Nov 2, 2023

After having written this down, it became clear for me that some of these urgencies implicitly prevent page navigation (or navigating out of a repeating group row), while others do not.

Instead of tying when the validation message should appear to the user with which actions should the user now be allowed to do, we should consider decoupling these. But then again, maybe it never makes sense for a validation message to have an urgency of OnPageNext while also not preventing navigation to the next page, as the user could end up never seeing that validation message if it appears right before navigating to the next page.

EDIT: Maybe it could be made more clear to app developers in naming and documentation:

enum ValidationUrgency {
  // Does not block anything
  Immediate = 10,
  AfterTyping = 15,
  AfterBlur = 20,

  // Block some form of navigation
  BeforeGroupRowClose = 30,
  BeforePageNext = 35,
  BeforePageNavigation = 40,

  // Does not block any navigation
  OnFormSubmit = 50,
}

@bjosttveit
Copy link
Member

Some thoughts on custom validation in the backend, which I don't think have been covered here.

Currently, we use a ModelStateDictionary to add custom validations. The model state dictionary simply takes a key and message. Correct me if I'm wrong, but there is no way to add custom ValidationIssues in the class implementing IInstanceValidator. This works ok for ValidateData, since in that case you are mostly validating data model fields (which is what key specifies). However you lack the ability to use the customTextKey prop, so the validation message (code and description) will simply be a textResource key which is not necessarily great for API users.

More importantly, the same limitation exists for Task validation. However, in that case; you are not even validating form data and so using a ModelStateDictionary makes even less sense. It looks like the key is not even used (field gets set to null), so you can really just throw plain messages at the frontend. Which deals with this by using a special collection called unmapped to store these. It also looks like it does not get a Source field which is a little unfortunate, since at this point, all other messages have this property, and it is useful to know what type of validation we are dealing with. I would like to get rid of the unmapped collection, as I would rather be more explicit that this is a Task validation. I don't know whether there are more places that can produce unmapped errors, given that the concept is so vague.

tagging @ivarne and @olemartinorg in case you have some thoughts on this 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic status/draft Status: When you create an issue before you have enough info to properly describe the issue.
Projects
Status: 👷 In Progress
Development

No branches or pull requests

3 participants