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.

Sign up for GitHub

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

Jump to bottom

New validation interfaces to support incremental validation on multiple data models #701

Closed
wants to merge 3 commits into from
Closed

New validation interfaces to support incremental validation on multiple data models #701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6fd837
Save half finished work
ivarne Jun 24, 2024
caa7cb7
Continue work and fix a few more tests
ivarne Jun 29, 2024
657efd6
Make all tests actually work
ivarne Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 74 additions & 60 deletions src/Altinn.App.Api/Controllers/ActionsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Altinn.App.Core.Features.Action;
using Altinn.App.Core.Helpers;
using Altinn.App.Core.Internal.App;
using Altinn.App.Core.Internal.AppModel;
using Altinn.App.Core.Internal.Data;
using Altinn.App.Core.Internal.Instances;
using Altinn.App.Core.Internal.Validation;
Expand Down Expand Up @@ -33,23 +34,19 @@ public class ActionsController : ControllerBase
private readonly IValidationService _validationService;
private readonly IDataClient _dataClient;
private readonly IAppMetadata _appMetadata;
private readonly IAppModel _appModel;

/// <summary>
/// Create new instance of the <see cref="ActionsController"/> class
/// </summary>
/// <param name="authorization">The authorization service</param>
/// <param name="instanceClient">The instance client</param>
/// <param name="userActionService">The user action service</param>
/// <param name="validationService">Service for performing validations of user data</param>
/// <param name="dataClient">Client for accessing data in storage</param>
/// <param name="appMetadata">Service for getting application metadata</param>
public ActionsController(
IAuthorizationService authorization,
IInstanceClient instanceClient,
UserActionService userActionService,
IValidationService validationService,
IDataClient dataClient,
IAppMetadata appMetadata
IAppMetadata appMetadata,
IAppModel appModel
)
{
_authorization = authorization;
Expand All @@ -58,6 +55,7 @@ IAppMetadata appMetadata
_validationService = validationService;
_dataClient = dataClient;
_appMetadata = appMetadata;
_appModel = appModel;
}

/// <summary>
Expand Down Expand Up @@ -162,41 +160,53 @@ public async Task<ActionResult<UserActionResponse>> Perform(
);
}

var dataAccessor = new CachedInstanceDataAccessor(instance, _dataClient, _appMetadata, _appModel);
Dictionary<string, Dictionary<string, List<ValidationIssueWithSource>>>? validationIssues = null;

if (result.UpdatedDataModels is { Count: > 0 })
{
await SaveChangedModels(instance, result.UpdatedDataModels);
var changes = await SaveChangedModels(instance, dataAccessor, result.UpdatedDataModels);

validationIssues = await GetValidations(
instance,
dataAccessor,
changes,
actionRequest.IgnoredValidators,
language
);
}

return new OkObjectResult(
return Ok(
new UserActionResponse()
{
ClientActions = result.ClientActions,
UpdatedDataModels = result.UpdatedDataModels,
UpdatedValidationIssues = await GetValidations(
instance,
result.UpdatedDataModels,
actionRequest.IgnoredValidators,
language
),
UpdatedValidationIssues = validationIssues,
RedirectUrl = result.RedirectUrl,
}
);
}

private async Task SaveChangedModels(Instance instance, Dictionary<string, object> resultUpdatedDataModels)
private async Task<List<DataElementChange>> SaveChangedModels(
Instance instance,
CachedInstanceDataAccessor dataAccessor,
Dictionary<string, object> resultUpdatedDataModels
)
{
var changes = new List<DataElementChange>();
var instanceIdentifier = new InstanceIdentifier(instance);
foreach (var (elementId, newModel) in resultUpdatedDataModels)
{
if (newModel is null)
{
continue;
}
var dataElement = instance.Data.First(d => d.Id.Equals(elementId, StringComparison.OrdinalIgnoreCase));
var previousData = await dataAccessor.Get(dataElement);

ObjectUtils.InitializeAltinnRowId(newModel);
ObjectUtils.PrepareModelForXmlStorage(newModel);

var dataElement = instance.Data.First(d => d.Id.Equals(elementId, StringComparison.OrdinalIgnoreCase));
await _dataClient.UpdateData(
newModel,
instanceIdentifier.InstanceGuid,
Expand All @@ -206,61 +216,65 @@ await _dataClient.UpdateData(
instanceIdentifier.InstanceOwnerPartyId,
Guid.Parse(dataElement.Id)
);
// update dataAccessor to use the changed data
dataAccessor.Set(dataElement, newModel);
// add change to list
changes.Add(
new DataElementChange
{
DataElement = dataElement,
PreviousValue = previousData,
CurrentValue = newModel,
}
);
}
return changes;
}

private async Task<Dictionary<string, Dictionary<string, List<ValidationIssue>>>?> GetValidations(
private async Task<Dictionary<string, Dictionary<string, List<ValidationIssueWithSource>>>?> GetValidations(
Instance instance,
Dictionary<string, object>? resultUpdatedDataModels,
IInstanceDataAccessor dataAccessor,
List<DataElementChange> changes,
List<string>? ignoredValidators,
string? language
)
{
if (resultUpdatedDataModels is null || resultUpdatedDataModels.Count < 1)
{
return null;
}

var instanceIdentifier = new InstanceIdentifier(instance);
var application = await _appMetadata.GetApplicationMetadata();
var taskId = instance.Process.CurrentTask.ElementId;
var validationIssues = await _validationService.ValidateIncrementalFormData(
instance,
taskId,
changes,
dataAccessor,
ignoredValidators,
language
);

var updatedValidationIssues = new Dictionary<string, Dictionary<string, List<ValidationIssue>>>();
// For historical reasons the validation issues from actions controller is separated per data element
// The easiest way was to keep this behaviour to improve compatibility with older frontend versions
return PartitionValidationIssuesByDataElement(validationIssues);
}

// TODO: Consider validating models in parallel
foreach (var (elementId, newModel) in resultUpdatedDataModels)
private Dictionary<
string,
Dictionary<string, List<ValidationIssueWithSource>>
> PartitionValidationIssuesByDataElement(Dictionary<string, List<ValidationIssueWithSource>> validationIssues)
{
var updatedValidationIssues = new Dictionary<string, Dictionary<string, List<ValidationIssueWithSource>>>();
foreach (var (validationSource, issuesFromSource) in validationIssues)
{
if (newModel is null)
{
continue;
}

var dataElement = instance.Data.First(d => d.Id.Equals(elementId, StringComparison.OrdinalIgnoreCase));
var dataType = application.DataTypes.First(d =>
d.Id.Equals(dataElement.DataType, StringComparison.OrdinalIgnoreCase)
);

// TODO: Consider rewriting so that we get the original data the IUserAction have requested instead of fetching it again
var oldData = await _dataClient.GetFormData(
instanceIdentifier.InstanceGuid,
newModel.GetType(),
instance.Org,
instance.AppId.Split('/')[1],
instanceIdentifier.InstanceOwnerPartyId,
Guid.Parse(dataElement.Id)
);

var validationIssues = await _validationService.ValidateFormData(
instance,
dataElement,
dataType,
newModel,
oldData,
ignoredValidators,
language
);
if (validationIssues.Count > 0)
foreach (var issue in issuesFromSource)
{
updatedValidationIssues.Add(elementId, validationIssues);
if (!updatedValidationIssues.TryGetValue(issue.DataElementId ?? "", out var elementIssues))
{
elementIssues = new Dictionary<string, List<ValidationIssueWithSource>>();
updatedValidationIssues[issue.DataElementId ?? ""] = elementIssues;
}
if (!elementIssues.TryGetValue(validationSource, out var sourceIssues))
{
sourceIssues = new List<ValidationIssueWithSource>();
elementIssues[validationSource] = sourceIssues;
}
sourceIssues.Add(issue);
}
}

Expand Down
108 changes: 85 additions & 23 deletions src/Altinn.App.Api/Controllers/DataController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,53 @@
[FromBody] DataPatchRequest dataPatchRequest,
[FromQuery] string? language = null
)
{
var request = new DataPatchRequestMultiple()
{
Patches = new() { [dataGuid] = dataPatchRequest.Patch },
IgnoredValidators = dataPatchRequest.IgnoredValidators
};
var response = await PatchFormDataMultiple(org, app, instanceOwnerPartyId, instanceGuid, request, language);

if (response.Result is OkObjectResult { Value: DataPatchResponseMultiple newResponse })
{
// Map the new response to the old response
return Ok(
new DataPatchResponse()
{
ValidationIssues = newResponse.ValidationIssues,
NewDataModel = newResponse.NewDataModels[dataGuid],
}
);
}

// Return the error object unchanged
return response.Result ?? throw new InvalidOperationException("Response is null");
}

/// <summary>
/// Updates an existing form data element with a patch of changes.
/// </summary>
/// <param name="org">unique identfier of the organisation responsible for the app</param>
/// <param name="app">application identifier which is unique within an organisation</param>
/// <param name="instanceOwnerPartyId">unique id of the party that is the owner of the instance</param>
/// <param name="instanceGuid">unique id to identify the instance</param>
/// <param name="dataPatchRequest">Container object for the <see cref="JsonPatch" /> and list of ignored validators</param>
/// <param name="language">The language selected by the user.</param>
/// <returns>A response object with the new full model and validation issues from all the groups that run</returns>
[Authorize(Policy = AuthzConstants.POLICY_INSTANCE_WRITE)]
[HttpPatch("")]
[ProducesResponseType(typeof(DataPatchResponseMultiple), 200)]
[ProducesResponseType(typeof(ProblemDetails), 409)]
[ProducesResponseType(typeof(ProblemDetails), 422)]
public async Task<ActionResult<DataPatchResponseMultiple>> PatchFormDataMultiple(
[FromRoute] string org,
[FromRoute] string app,
[FromRoute] int instanceOwnerPartyId,
[FromRoute] Guid instanceGuid,
[FromBody] DataPatchRequestMultiple dataPatchRequest,
[FromQuery] string? language = null
)
{
try
{
Expand All @@ -464,44 +511,59 @@
);
}

var dataElement = instance.Data.First(m => m.Id.Equals(dataGuid.ToString(), StringComparison.Ordinal));
CachedInstanceDataAccessor dataAccessor = new CachedInstanceDataAccessor(
instance,
_dataClient,
_appMetadata,
_appModel
);
Comment on lines +514 to +519

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

This assignment to
dataAccessor
Loading
is useless, since its value is never read.

if (dataElement == null)
foreach (Guid dataGuid in dataPatchRequest.Patches.Keys)
{
return NotFound("Did not find data element");
}
var dataElement = instance.Data.First(m => m.Id.Equals(dataGuid.ToString(), StringComparison.Ordinal));

var dataType = await GetDataType(dataElement);
if (dataElement == null)
{
return NotFound("Did not find data element");
}

if (dataType?.AppLogic?.ClassRef is null)
{
_logger.LogError(
"Could not determine if {dataType} requires app logic for application {org}/{app}",
dataType,
org,
app
);
return BadRequest($"Could not determine if data type {dataType?.Id} requires application logic.");
var dataType = await GetDataType(dataElement);

if (dataType?.AppLogic?.ClassRef is null)
{
_logger.LogError(
"Could not determine if {dataType} requires app logic for application {org}/{app}",
dataType,
org,

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.
app

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.
);
return BadRequest($"Could not determine if data type {dataType?.Id} requires application logic.");
}
}

ServiceResult<DataPatchResult, DataPatchError> res = await _patchService.ApplyPatch(
ServiceResult<DataPatchResult, DataPatchError> res = await _patchService.ApplyPatches(
instance,
dataType,
dataElement,
dataPatchRequest.Patch,
dataPatchRequest.Patches,
language,
dataPatchRequest.IgnoredValidators
);

if (res.Success)
{
await UpdateDataValuesOnInstance(instance, dataType.Id, res.Ok.NewDataModel);
await UpdatePresentationTextsOnInstance(instance, dataType.Id, res.Ok.NewDataModel);
foreach (var dataGuid in dataPatchRequest.Patches.Keys)
{
await UpdateDataValuesOnInstance(instance, dataGuid.ToString(), res.Ok.NewDataModels[dataGuid]);
await UpdatePresentationTextsOnInstance(
instance,
dataGuid.ToString(),
res.Ok.NewDataModels[dataGuid]
);
}

return Ok(
new DataPatchResponse
new DataPatchResponseMultiple()
{
NewDataModel = res.Ok.NewDataModel,
NewDataModels = res.Ok.NewDataModels,
ValidationIssues = res.Ok.ValidationIssues
}
);
Expand All @@ -513,7 +575,7 @@
{
return HandlePlatformHttpException(
e,
$"Unable to update data element {dataGuid} for instance {instanceOwnerPartyId}/{instanceGuid}"
$"Unable to update data element {string.Join(", ", dataPatchRequest.Patches.Keys)} for instance {instanceOwnerPartyId}/{instanceGuid}"
);
}
}
Expand Down
Loading
Loading