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

New validation and partial validation via PATCH endpoint #393

Merged
merged 34 commits into from
Jan 22, 2024

Conversation

ivarne
Copy link
Member

@ivarne ivarne commented Jan 8, 2024

Combination of #374 and #384

Patch

New endpoint on the DataController that allows frontends to update single (or multiple) fields and get updated validation on those fields.

Implement a new PATCH endpoint using https://docs.json-everything.net/path/basics/

New validation

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)

  • #{issue number}

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)

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
Also comment out KeyedService fetching
@ivarne ivarne force-pushed the ivarne/path-validator branch from 709d48c to 8636355 Compare January 8, 2024 08:32
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 changed the title Ivarne/path validator Ivarne/patch and validator Jan 8, 2024
@ivarne ivarne force-pushed the ivarne/path-validator branch from 411bce5 to 88fc96d Compare January 10, 2024 22:50
Ole Martin Handeland and others added 4 commits January 12, 2024 17:32
…versions, this label can also be an expression, so parsing fails when this is expected to be a string.
…ts. Frontend doesn't use this anymore, and disallows rendering Summary components with it.
Improve handeling of errors in PATCH endpoint
code style fixes
@ivarne ivarne force-pushed the ivarne/path-validator branch from 9da5903 to ebcb89a Compare January 15, 2024 21:18
@ivarne ivarne force-pushed the ivarne/path-validator branch from 9b84ada to 9ebe588 Compare January 16, 2024 21:28
Copy link
Member

@tjololo tjololo left a comment

Choose a reason for hiding this comment

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

Some initial comments. Haven't looked at the test code yet, but wanted to get the first src comments out the door

src/Altinn.App.Api/Controllers/DataController.cs Outdated Show resolved Hide resolved
src/Altinn.App.Core/Features/IDataElementValidator.cs Outdated Show resolved Hide resolved
src/Altinn.App.Core/Features/ITaskValidator.cs Outdated Show resolved Hide resolved
src/Altinn.App.Core/Helpers/ObjectUtils.cs Outdated Show resolved Hide resolved
@ivarne ivarne force-pushed the ivarne/path-validator branch from 99ef9a3 to 00e3be8 Compare January 19, 2024 23:00
}
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
.
}
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
.
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
.
Comment on lines +64 to +67
catch (Exception e)
{
return Task.FromException<List<ValidationIssue>>(e);
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
Comment on lines +90 to +97
foreach (var file in files)
{
var data = File.ReadAllText(file);
ExpressionValidationTestModel testCase = JsonSerializer.Deserialize<ExpressionValidationTestModel>(
data,
JsonSerializerOptions)!;
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(...)'.
Copy link

sonarcloud bot commented Jan 22, 2024

Quality Gate Passed Quality Gate passed

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

14 New issues
0 Security Hotspots
77.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@ivarne ivarne changed the title Ivarne/patch and validator New validation and partial validation via PATCH endpoint Jan 22, 2024
Copy link
Member

@RonnyB71 RonnyB71 left a comment

Choose a reason for hiding this comment

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

Approving in order to merge, leaving the open conversations to be resolved later.

@ivarne ivarne merged commit 3bac97d into v8 Jan 22, 2024
10 of 11 checks passed
@ivarne ivarne deleted the ivarne/path-validator branch January 22, 2024 20:36
@tjololo tjololo added the feature Label Pull requests with new features. Used when generation releasenotes label Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Label Pull requests with new features. Used when generation releasenotes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants