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

Start work on new validation #374

Closed
wants to merge 7 commits into from
Closed

Start work on new validation #374

wants to merge 7 commits into from

Conversation

ivarne
Copy link
Member

@ivarne ivarne commented Dec 7, 2023

New service

IValidationService:
A ServiceLocator for validation that does the different kinds of validation. Some logic in data element validation to download the actual data if the element specifies AppLogic and can do that kind of deep validation.

New interfaces:

  • IValidationService: The rest of the systems does all its validation needs through this service. It does not mutate any state in instance, process or data elements. Old extension points are accessed through this. Gets services from a SerivceProvider with a service locator pattern.
    • ValidateTask: Validates that a task is ready for process next. Finds all relevant data elements and other validations to run.
    • ValidateDataElement: Used for partial or full validation of a single data element.
  • ITaskValidator: Interface for App developers to add functionality for whole task validation.
    • Registered as a keyed service that specifies TaskId so that we know what tasks to trigger the validation for.
    • Does not support incremental validation.
  • IDataElementValidator: Interface for validation of data elements without loading the actual content of the data element.
  • IFormDataValidator: Interface for App developers to add incremental validation to form data elements.
    • Keyed service DataType to detect what Data elements to apply the validation to.
    • Methods ShouldRun and a Code that is automatically added to all validation issues allows many classes to be registered, but only run when relevant fields are changed. This means that the whole validation implementation needs to either run, or not run to ensure that we can tell front-end what validations has been updated and possibly fixed.

New functionality

  • GenericFormDataValidator: abstract class that app developers can use to simplify the implementation of IFormDataValidator adding a few convenience methods
    • ValidateFormData that gets the typed model, instead of object data
    • CreateValidationIssue: Very convenient way of adding errors.
      • Allows inputing an expression m=>m.field.children[1].name instead of a json path to bind the error to a field.
      • Uses AsyncLocal to not have to pass or return a list of issues. Just causes less clutter, and works even if the service is registered as a singleton.
  • LinqExpressionHelpers: static methods to allow parsing of expressions instead of strings to create json paths.

Validator registration

Validators can be registered in 3 ways

  • DataType or TaskId is set as "*" and it is registered as a service in dependency injection
  • DataType/TaskId, set as a specific DataType or task to run (Typically custom validator in a specific app)
  • (As a keyed service, so that users override the DataType/TaskId and run on a general validator on a specific type)

Compatibility

  • IValidation, together with ValidationAppSI has been removed and all standard shared validation has been moved to default validators
    • DataAnnotationValidation: Runs attribute validation from System.ComponentModel.DataAnnotations (eg: [RegEx()])
    • DefaultDataElementValidation: Validates maxSize, contentType, and fileScan
    • DefaultTaskValidator: Validates min and max count for data elements
    • ExpressionValidator: Validates data elements with app logic and ensures that expression validation has run.
    • LegacyIInstanceValidatorFormDataValidator: Runs existing IInstanceValidator.ValidateData class registered in dependency injection for backwards compatibility
    • LegacyIInstanceValidatorTaskValidator: Runs existing IInstanceValidator.ValidateTask class registered in dependency injection for backwards compatibility
    • RequiredLayoutValidator: Runs expressions in layout to ensure that every visible component with "required" actually has data.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@ivarne ivarne added the area/validation Custom validations/validation messages label Dec 7, 2023
@ivarne ivarne added this to the 8.0.0 milestone Dec 7, 2023
@ivarne ivarne force-pushed the ivarne/new-validation branch from 07726d6 to c2995db Compare December 7, 2023 21:37
Comment on lines +47 to +53
foreach (var changedField in changedFields)
{
if (IsMatch(changedField, prefix))
{
return true;
}
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Where Note

This foreach loop
implicitly filters its target sequence
- consider filtering the sequence explicitly using '.Where(...)'.
@tjololo tjololo linked an issue Dec 11, 2023 that may be closed by this pull request
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@ivarne ivarne force-pushed the ivarne/new-validation branch from a4a3bf4 to 159f0e0 Compare December 12, 2023 20:32
@ivarne ivarne mentioned this pull request Dec 12, 2023
5 tasks
Improve GetJsonPath(Expression) and fix some sonarcloud issues

Improve test coverage

Full validation rewrite complete and existing tests work

Probably still some issues and test coverage was spoty so more tests needs to be written.

Make IInstanceValidator obsolete

Add DataType as input to validators

Fix a few issues to prepare for tests of expressions

Add more tests
@ivarne ivarne force-pushed the ivarne/new-validation branch from 2657832 to de315c7 Compare January 3, 2024 11:17
}
catch (Exception e)
{
_logger.LogError(e, "Error while running validator {validatorName} on task {taskId} in instance {instanceId}", tv.GetType().Name, taskId, instance.Id);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
}
catch (Exception e)
{
_logger.LogError(e, "Error while running validator {validatorName} on task {taskId} in instance {instanceId}", tv.GetType().Name, taskId, instance.Id);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
{
try
{
_logger.LogDebug("Start running validator {validatorName} on task {taskId} in instance {instanceId}", tv.GetType().Name, taskId, instance.Id);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
{
try
{
_logger.LogDebug("Start running validator {validatorName} on task {taskId} in instance {instanceId}", tv.GetType().Name, taskId, instance.Id);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
}
catch (Exception e)
{
_logger.LogError(e, "Error while running validator {validatorName} on {dataType} for data element {dataElementId} in instance {instanceId}", v.GetType().Name, dataElement.DataType, dataElement.Id, instance.Id);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
{
try
{
_logger.LogDebug("Start running validator {validatorName} on {dataType} for data element {dataElementId} in instance {instanceId}", v.GetType().Name, dataElement.DataType, dataElement.Id, instance.Id);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a user-provided value.
This log entry depends on a user-provided value.
{
try
{
_logger.LogDebug("Start running validator {validatorName} on {dataType} for data element {dataElementId} in instance {instanceId}", v.GetType().Name, dataElement.DataType, dataElement.Id, instance.Id);

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a user-provided value.
This log entry depends on a user-provided value.
src/Altinn.App.Core/Features/Validation/ValidationService.cs Dismissed Show dismissed Hide dismissed
Comment on lines +66 to +69
catch (Exception e)
{
return Task.FromException<List<ValidationIssue>>(e);
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
Comment on lines +85 to +96
foreach (var file in files)
{
var data = File.ReadAllText(file);
ExpressionValidationTestModel testCase = JsonSerializer.Deserialize<ExpressionValidationTestModel>(
data,
new JsonSerializerOptions
{
ReadCommentHandling = JsonCommentHandling.Skip,
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
})!;
yield return new object[] { testCase };
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Select Note test

This foreach loop immediately
maps its iteration variable to another variable
- consider mapping the sequence explicitly using '.Select(...)'.
@ivarne ivarne force-pushed the ivarne/new-validation branch from d34acd6 to 088c125 Compare January 4, 2024 07:36
Copy link

sonarcloud bot commented Jan 4, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

11 New issues
0 Security Hotspots
66.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@ivarne ivarne closed this Jan 23, 2024
@ivarne ivarne deleted the ivarne/new-validation branch October 10, 2024 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/validation Custom validations/validation messages
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

New validation extension
1 participant