New validation and partial validation via PATCH endpoint (#393)

* Revert Multipart form data #329 on PUT endpoint

* Add JsonPatch and create a new PATCH endpoint for updating form data

* Fix a few SonarCloud warnings

* Start work on new validation

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

* Fix sonar cloud issues

* Rename to get more consistent naming

* Clanup DataType/TaskId to allow *

Also comment out KeyedService fetching

* Fix more sonarcloud issues

* Registrer new validators in DI container

* Remove dead code

* Adjust interface and add a few tests

* Add object? previousData to IDataProcessor

* Cleanup and more tests

* OptionsSource doesn't really need a label to work. In recent fronted versions, this label can also be an expression, so parsing fails when this is expected to be a string.

* Removing the requirement for a 'pageRef' property on Summary components. Frontend doesn't use this anymore, and disallows rendering Summary components with it.

* Allowing attachments to be deleted (attachments don't have AppLogic)

* ValidationIssue.Severity is an int in json

Improve handeling of errors in PATCH endpoint
code style fixes

* Initialize lists for concistency in frontend after patch

also add tests

* Fix issues with list initialization and prettify tests

* Add test to verify that PATCH initializes arrays (as xml serialization does)

* Read json configuration file for api tests

* Set empty strings to null in patch endpoint

xml serializing does always preserve empty string when using attributes and [XmlText].

* Apply suggestions from code review

Co-authored-by: Vemund Gaukstad <[email protected]>

* Remove caching logic for CanTaskBeEnded

* Improve description on validationsource

* Rename ShouldRun to HasRelevantChanges

fix style issues

* Fix dataProcessorRewriter

* Add genericDataProcessor

* Fix a few codestyle issues

* Making some validator sources match those used on the frontend

* Fix tests after validator source name change

* Adding support for explicit repeating group components (new in frontend v4)

---------

Co-authored-by: Ole Martin Handeland <[email protected]>
Co-authored-by: Vemund Gaukstad <[email protected]>

cli-tools/altinn-app-cli/v7Tov8/CodeRewriters/IDataProcessorRewriter.cs

@@ -62,9 +62,9 @@ private MethodDeclarationSyntax Update_DataProcessWrite(MethodDeclarationSyntax
         private MethodDeclarationSyntax AddParameter_ChangedFields(MethodDeclarationSyntax method)
         {
             return method.ReplaceNode(method.ParameterList,
-                method.ParameterList.AddParameters(SyntaxFactory.Parameter(SyntaxFactory.Identifier("changedFields"))
+                method.ParameterList.AddParameters(SyntaxFactory.Parameter(SyntaxFactory.Identifier("previousData"))
                     .WithLeadingTrivia(SyntaxFactory.Space)
-                    .WithType(SyntaxFactory.ParseTypeName("System.Collections.Generic.Dictionary<string, string?>?"))
+                    .WithType(SyntaxFactory.ParseTypeName("object?"))
                     .WithLeadingTrivia(SyntaxFactory.Space)));
         }
 

src/Altinn.App.Api/Altinn.App.Api.csproj

@@ -48,4 +48,7 @@
     <NoWarn>$(NoWarn);1591</NoWarn>
   </PropertyGroup>
 
+  <ItemGroup>
+    <InternalsVisibleTo Include="$(AssemblyName).Tests" />
+  </ItemGroup>
 </Project>

src/Altinn.App.Api/Controllers/InstancesController.cs

@@ -971,15 +971,13 @@ private async Task StorePrefillParts(Instance instance, ApplicationMetadata appI
                     }
 
                     ModelDeserializer deserializer = new ModelDeserializer(_logger, type);
-                    ModelDeserializerResult deserializerResult = await deserializer.DeserializeAsync(part.Stream, part.ContentType);
+                    object? data = await deserializer.DeserializeAsync(part.Stream, part.ContentType);
 
-                    if (deserializerResult.HasError)
+                    if (!string.IsNullOrEmpty(deserializer.Error) || data is null)
                     {
-                        throw new InvalidOperationException(deserializerResult.Error);
+                        throw new InvalidOperationException(deserializer.Error);
                     }
 
-                    object data = deserializerResult.Model;
-
                     await _prefillService.PrefillDataModel(instance.InstanceOwner.PartyId, part.Name!, data);
 
                     await _instantiationProcessor.DataCreation(instance, data, null);

src/Altinn.App.Api/Controllers/ProcessController.cs

@@ -2,6 +2,7 @@
 using System.Net;
 using Altinn.App.Api.Infrastructure.Filters;
 using Altinn.App.Api.Models;
+using Altinn.App.Core.Features;
 using Altinn.App.Core.Features.Validation;
 using Altinn.App.Core.Helpers;
 using Altinn.App.Core.Internal.Instances;
@@ -34,7 +35,7 @@ public class ProcessController : ControllerBase
         private readonly ILogger<ProcessController> _logger;
         private readonly IInstanceClient _instanceClient;
         private readonly IProcessClient _processClient;
-        private readonly IValidation _validationService;
+        private readonly IValidationService _validationService;
         private readonly IAuthorizationService _authorization;
         private readonly IProcessEngine _processEngine;
         private readonly IProcessReader _processReader;
@@ -46,7 +47,7 @@ public ProcessController(
             ILogger<ProcessController> logger,
             IInstanceClient instanceClient,
             IProcessClient processClient,
-            IValidation validationService,
+            IValidationService validationService,
             IAuthorizationService authorization,
             IProcessReader processReader,
             IProcessEngine processEngine)
@@ -202,24 +203,11 @@ public async Task<ActionResult<List<string>>> GetNextElements(
             }
         }
 
-        private async Task<bool> CanTaskBeEnded(Instance instance, string currentElementId)
+        private async Task<bool> CanTaskBeEnded(Instance instance, string currentTaskId)
         {
-            List<ValidationIssue> validationIssues = new List<ValidationIssue>();
+            var validationIssues = await _validationService.ValidateInstanceAtTask(instance, currentTaskId);
 
-            bool canEndTask;
-
-            if (instance.Process?.CurrentTask?.Validated == null || !instance.Process.CurrentTask.Validated.CanCompleteTask)
-            {
-                validationIssues = await _validationService.ValidateAndUpdateProcess(instance, currentElementId);
-
-                canEndTask = await ProcessHelper.CanEndProcessTask(instance, validationIssues);
-            }
-            else
-            {
-                canEndTask = await ProcessHelper.CanEndProcessTask(instance, validationIssues);
-            }
-
-            return canEndTask;
+            return await ProcessHelper.CanEndProcessTask(instance, validationIssues);
         }
 
         /// <summary>

src/Altinn.App.Api/Controllers/StatelessDataController.cs

@@ -116,12 +116,12 @@ public async Task<ActionResult> Get(
             await _prefillService.PrefillDataModel(owner.PartyId, dataType, appModel);
 
             Instance virtualInstance = new Instance() { InstanceOwner = owner };
-            await ProcessAllDataWrite(virtualInstance, appModel);
+            await ProcessAllDataRead(virtualInstance, appModel);
 
             return Ok(appModel);
         }
 
-        private async Task ProcessAllDataWrite(Instance virtualInstance, object appModel)
+        private async Task ProcessAllDataRead(Instance virtualInstance, object appModel)
         {
             foreach (var dataProcessor in _dataProcessors)
             {
@@ -161,7 +161,7 @@ public async Task<ActionResult> GetAnonymous([FromQuery] string dataType)
 
             object appModel = _appModel.Create(classRef);
             var virtualInstance = new Instance();
-            await ProcessAllDataWrite(virtualInstance, appModel);
+            await ProcessAllDataRead(virtualInstance, appModel);
 
             return Ok(appModel);
         }
@@ -213,15 +213,13 @@ public async Task<ActionResult> Post(
             }
 
             ModelDeserializer deserializer = new ModelDeserializer(_logger, _appModel.GetModelType(classRef));
-            ModelDeserializerResult deserializerResult = await deserializer.DeserializeAsync(Request.Body, Request.ContentType);
+            object? appModel = await deserializer.DeserializeAsync(Request.Body, Request.ContentType);
 
-            if (deserializerResult.HasError)
+            if (!string.IsNullOrEmpty(deserializer.Error) || appModel is null)
             {
-                return BadRequest(deserializerResult.Error);
+                return BadRequest(deserializer.Error);
             }
 
-            object appModel = deserializerResult.Model;
-
             // runs prefill from repo configuration if config exists
             await _prefillService.PrefillDataModel(owner.PartyId, dataType, appModel);
 
@@ -262,22 +260,21 @@ public async Task<ActionResult> PostAnonymous([FromQuery] string dataType)
             }
 
             ModelDeserializer deserializer = new ModelDeserializer(_logger, _appModel.GetModelType(classRef));
-            ModelDeserializerResult deserializerResult = await deserializer.DeserializeAsync(Request.Body, Request.ContentType);
+            object? appModel = await deserializer.DeserializeAsync(Request.Body, Request.ContentType);
 
-            if (deserializerResult.HasError)
+            if (!string.IsNullOrEmpty(deserializer.Error) || appModel is null)
             {
-                return BadRequest(deserializerResult.Error);
+                return BadRequest(deserializer.Error);
             }
 
             Instance virtualInstance = new Instance();
-            var appModel = deserializerResult.Model;
             foreach (var dataProcessor in _dataProcessors)
             {
                 _logger.LogInformation("ProcessDataRead for {modelType} using {dataProcesor}", appModel.GetType().Name, dataProcessor.GetType().Name);
                 await dataProcessor.ProcessDataRead(virtualInstance, null, appModel);
             }
 
-            return Ok(deserializerResult.Model);
+            return Ok(appModel);
         }
 
         private async Task<InstanceOwner?> GetInstanceOwner(string? partyFromHeader)

src/Altinn.App.Api/Controllers/ValidateController.cs

@@ -1,8 +1,8 @@
 #nullable enable
 
+using Altinn.App.Core.Features;
 using Altinn.App.Core.Features.Validation;
 using Altinn.App.Core.Helpers;
-using Altinn.App.Core.Infrastructure.Clients;
 using Altinn.App.Core.Internal.App;
 using Altinn.App.Core.Internal.Instances;
 using Altinn.App.Core.Models.Validation;
@@ -21,14 +21,14 @@ public class ValidateController : ControllerBase
     {
         private readonly IInstanceClient _instanceClient;
         private readonly IAppMetadata _appMetadata;
-        private readonly IValidation _validationService;
+        private readonly IValidationService _validationService;
 
         /// <summary>
         /// Initialises a new instance of the <see cref="ValidateController"/> class
         /// </summary>
         public ValidateController(
             IInstanceClient instanceClient,
-            IValidation validationService,
+            IValidationService validationService,
             IAppMetadata appMetadata)
         {
             _instanceClient = instanceClient;
@@ -66,7 +66,7 @@ public async Task<IActionResult> ValidateInstance(
 
             try 
             {
-                List<ValidationIssue> messages = await _validationService.ValidateAndUpdateProcess(instance, taskId);
+                List<ValidationIssue> messages = await _validationService.ValidateInstanceAtTask(instance, taskId);
                 return Ok(messages);
             } 
             catch (PlatformHttpException exception) 
@@ -81,8 +81,7 @@ public async Task<IActionResult> ValidateInstance(
         }
 
         /// <summary>
-        /// Validate an app instance. This will validate all individual data elements, both the binary elements and the elements bound
-        /// to a model, and then finally the state of the instance.
+        /// Validate an app instance. This will validate a single data element
         /// </summary>
         /// <param name="org">Unique identifier of the organisation responsible for the app.</param>
         /// <param name="app">Application identifier which is unique within an organisation</param>
@@ -104,9 +103,6 @@ public async Task<IActionResult> ValidateData(
                 return NotFound();
             }
 
-            // Todo. Figure out where to get this from
-            Dictionary<string, Dictionary<string, string>> serviceText = new Dictionary<string, Dictionary<string, string>>();
-
             if (instance.Process?.CurrentTask?.ElementId == null)
             {
                 throw new ValidationException("Unable to validate instance without a started process.");
@@ -130,19 +126,21 @@ public async Task<IActionResult> ValidateData(
                 throw new ValidationException("Unknown element type.");
             }
 
-            messages.AddRange(await _validationService.ValidateDataElement(instance, dataType, element));
+            messages.AddRange(await _validationService.ValidateDataElement(instance, element, dataType));
 
             string taskId = instance.Process.CurrentTask.ElementId;
+
+            // Should this be a BadRequest instead?
             if (!dataType.TaskId.Equals(taskId, StringComparison.OrdinalIgnoreCase))
             {
                 ValidationIssue message = new ValidationIssue
                 {
                     Code = ValidationIssueCodes.DataElementCodes.DataElementValidatedAtWrongTask,
-                    InstanceId = instance.Id,
                     Severity = ValidationIssueSeverity.Warning,
                     DataElementId = element.Id,
-                    Description = AppTextHelper.GetAppText(
-                        ValidationIssueCodes.DataElementCodes.DataElementValidatedAtWrongTask, serviceText, null, "nb")
+                    Description = $"Data element for task {dataType.TaskId} validated while currentTask is {taskId}",
+                    CustomTextKey = ValidationIssueCodes.DataElementCodes.DataElementValidatedAtWrongTask,
+                    CustomTextParams = new List<string>() { dataType.TaskId, taskId },
                 };
                 messages.Add(message);
             }

src/Altinn.App.Api/Models/DataPatchRequest.cs

@@ -0,0 +1,25 @@
+#nullable enable
+using System.Text.Json.Serialization;
+using Altinn.App.Api.Controllers;
+using Json.Patch;
+
+namespace Altinn.App.Api.Models;
+
+/// <summary>
+/// Represents the request to patch data on the <see cref="DataController"/>.
+/// </summary>
+public class DataPatchRequest
+{
+    /// <summary>
+    /// The Patch operation to perform.
+    /// </summary>
+    [JsonPropertyName("patch")]
+    public required JsonPatch Patch { get; init; }
+
+    /// <summary>
+    /// List of validators to ignore during the patch operation.
+    /// Issues from these validators will not be run during the save operation, but the validator will run on process/next
+    /// </summary>
+    [JsonPropertyName("ignoredValidators")]
+    public required List<string>? IgnoredValidators { get; init; }
+}

src/Altinn.App.Api/Models/DataPatchResponse.cs

@@ -0,0 +1,20 @@
+using Altinn.App.Api.Controllers;
+using Altinn.App.Core.Models.Validation;
+
+namespace Altinn.App.Api.Models;
+
+/// <summary>
+/// Represents the response from a data patch operation on the <see cref="DataController"/>.
+/// </summary>
+public class DataPatchResponse
+{
+    /// <summary>
+    /// The validation issues that were found during the patch operation.
+    /// </summary>
+    public required Dictionary<string, List<ValidationIssue>> ValidationIssues { get; init; }
+
+    /// <summary>
+    /// The current data model after the patch operation.
+    /// </summary>
+    public required object NewDataModel { get; init; }
+}

src/Altinn.App.Core/Altinn.App.Core.csproj

@@ -15,6 +15,7 @@
     <PackageReference Include="Altinn.Platform.Models" Version="1.2.0" />
     <PackageReference Include="Altinn.Platform.Storage.Interface" Version="3.24.0" />
     <PackageReference Include="HtmlAgilityPack" Version="1.11.54" />
+    <PackageReference Include="JsonPatch.Net" Version="2.1.0" />
     <PackageReference Include="JWTCookieAuthentication" Version="3.0.1" />
     <PackageReference Include="Microsoft.ApplicationInsights.AspNetCore" Version="2.21.0" />
     <PackageReference Include="Microsoft.FeatureManagement.AspNetCore" Version="3.0.0" />

src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs

@@ -9,6 +9,7 @@
 using Altinn.App.Core.Features.PageOrder;
 using Altinn.App.Core.Features.Pdf;
 using Altinn.App.Core.Features.Validation;
+using Altinn.App.Core.Features.Validation.Default;
 using Altinn.App.Core.Implementation;
 using Altinn.App.Core.Infrastructure.Clients.Authentication;
 using Altinn.App.Core.Infrastructure.Clients.Authorization;
@@ -133,7 +134,7 @@ public static void AddAppServices(this IServiceCollection services, IConfigurati
         {
             // Services for Altinn App
             services.TryAddTransient<IPDP, PDPAppSI>();
-            services.TryAddTransient<IValidation, ValidationAppSI>();
+            AddValidationServices(services, configuration);
             services.TryAddTransient<IPrefill, PrefillSI>();
             services.TryAddTransient<ISigningCredentialsResolver, SigningCredentialsResolver>();
             services.TryAddSingleton<IAppResources, AppResourcesSI>();
@@ -146,7 +147,6 @@ public static void AddAppServices(this IServiceCollection services, IConfigurati
 #pragma warning restore CS0618, CS0612 // Type or member is obsolete
             services.TryAddTransient<IInstantiationProcessor, NullInstantiationProcessor>();
             services.TryAddTransient<IInstantiationValidator, NullInstantiationValidator>();
-            services.TryAddTransient<IInstanceValidator, NullInstanceValidator>();
             services.TryAddTransient<IAppModel, DefaultAppModel>();
             services.TryAddTransient<DataListsFactory>();
             services.TryAddTransient<InstanceDataListsFactory>();
@@ -178,6 +178,25 @@ public static void AddAppServices(this IServiceCollection services, IConfigurati
             }
         }
 
+        private static void AddValidationServices(IServiceCollection services, IConfiguration configuration)
+        {
+            services.TryAddTransient<IValidationService, ValidationService>();
+            if (configuration.GetSection("AppSettings").Get<AppSettings>()?.RequiredValidation == true)
+            {
+                services.TryAddTransient<IFormDataValidator, RequiredLayoutValidator>();
+            }
+
+            if (configuration.GetSection("AppSettings").Get<AppSettings>()?.ExpressionValidation == true)
+            {
+                services.TryAddTransient<IFormDataValidator, ExpressionValidator>();
+            }
+            services.TryAddTransient<IFormDataValidator, DataAnnotationValidator>();
+            services.TryAddTransient<IFormDataValidator, LegacyIInstanceValidatorFormDataValidator>();
+            services.TryAddTransient<IDataElementValidator, DefaultDataElementValidator>();
+            services.TryAddTransient<ITaskValidator, LegacyIInstanceValidatorTaskValidator>();
+            services.TryAddTransient<ITaskValidator, DefaultTaskValidator>();
+        }
+
         /// <summary>
         /// Checks if a service is already added to the collection.
         /// </summary>

src/Altinn.App.Core/Features/DataProcessing/GenericDataProcessor.cs

@@ -0,0 +1,39 @@
+using Altinn.Platform.Storage.Interface.Models;
+
+namespace Altinn.App.Core.Features.DataProcessing;
+
+/// <summary>
+/// Convenience class for implementing <see cref="IDataProcessor"/> for a specific model type.
+/// </summary>
+public abstract class GenericDataProcessor<TModel> : IDataProcessor where TModel : class
+{
+    /// <summary>
+    /// Do changes to the model after it has been read from storage, but before it is returned to the app.
+    /// this only executes on page load and not for subsequent updates.
+    /// </summary>
+    public abstract Task ProcessDataRead(Instance instance, Guid? dataId, TModel model);
+
+    /// <summary>
+    /// Do changes to the model before it is written to storage, and report back to frontend.
+    /// Tyipically used to add calculated values to the model.
+    /// </summary>
+    public abstract Task ProcessDataWrite(Instance instance, Guid? dataId, TModel model, TModel? previousModel);
+
+    /// <inheritdoc />
+    public async Task ProcessDataRead(Instance instance, Guid? dataId, object data)
+    {
+        if (data is TModel model)
+        {
+            await ProcessDataRead(instance, dataId, model);
+        }
+    }
+
+    /// <inheritdoc />
+    public async Task ProcessDataWrite(Instance instance, Guid? dataId, object data, object? previousData)
+    {
+        if (data is TModel model)
+        {
+            await ProcessDataWrite(instance, dataId, model, previousData as TModel);
+        }
+    }
+}