Skip to content

Commit

Permalink
Fix tests and cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
ivarne committed Aug 19, 2024
1 parent a688536 commit f7b91e0
Show file tree
Hide file tree
Showing 29 changed files with 313 additions and 178 deletions.
4 changes: 2 additions & 2 deletions src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,12 @@ private static void AddValidationServices(IServiceCollection services, IConfigur
services.AddScoped<IValidationService, ValidationService>();
if (configuration.GetSection("AppSettings").Get<AppSettings>()?.RequiredValidation == true)
{
services.AddTransient<IFormDataValidator, RequiredLayoutValidator>();
services.AddTransient<IValidator, RequiredLayoutValidator>();
}

if (configuration.GetSection("AppSettings").Get<AppSettings>()?.ExpressionValidation == true)
{
services.AddTransient<IFormDataValidator, ExpressionValidator>();
services.AddTransient<IValidator, ExpressionValidator>();
}
services.AddTransient<IFormDataValidator, DataAnnotationValidator>();
services.AddTransient<IDataElementValidator, DefaultDataElementValidator>();
Expand Down
2 changes: 2 additions & 0 deletions src/Altinn.App.Core/Features/IProcessExclusiveGateway.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@ public interface IProcessExclusiveGateway
/// </summary>
/// <param name="outgoingFlows">Complete list of defined flows out of gateway</param>
/// <param name="instance">Instance where process is about to move next</param>
/// <param name="dataAccessor">Cached accessor for instance.Data</param>
/// <param name="processGatewayInformation">Information connected with the current gateway under evaluation</param>
/// <returns>List of possible SequenceFlows to choose out of the gateway</returns>
public Task<List<SequenceFlow>> FilterAsync(
List<SequenceFlow> outgoingFlows,
Instance instance,
IInstanceDataAccessor dataAccessor,
ProcessGatewayInformation processGatewayInformation
);
}
11 changes: 8 additions & 3 deletions src/Altinn.App.Core/Features/IValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ public interface IValidator
///
/// </summary>
/// <param name="instance">The instance to validate</param>
/// <param name="instanceDataAccessor">Use this to access data from other data elements</param>
/// <param name="taskId">The current task. </param>
/// <param name="language">Language for messages, if the messages are too dynamic for the translation system</param>
/// <param name="instanceDataAccessor">Use this to access data from other data elements</param>
/// <returns></returns>
public Task<List<ValidationIssue>> Validate(
Instance instance,
IInstanceDataAccessor instanceDataAccessor,
string taskId,
string? language,
IInstanceDataAccessor instanceDataAccessor
string? language
);

/// <summary>
Expand Down Expand Up @@ -77,6 +77,11 @@ public class DataElementChange
/// </summary>
public interface IInstanceDataAccessor
{
/// <summary>
/// The instance that the accessor can access data for.
/// </summary>
Instance Instance { get; }

/// <summary>
/// Get the actual data represented in the data element.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Altinn.App.Core.Features.Validation.Default;
/// <summary>
/// Runs <see cref="System.ComponentModel.DataAnnotations"/> validation on the data object.
/// </summary>
public class DataAnnotationValidator : IFormDataValidator
public class DataAnnotationValidator : IFormDataValidator // TODO: This should be IValidator

Check warning on line 17 in src/Altinn.App.Core/Features/Validation/Default/DataAnnotationValidator.cs

View workflow job for this annotation

GitHub Actions / Static code analysis

Complete the task associated to this 'TODO' comment. (https://rules.sonarsource.com/csharp/RSPEC-1135)
{
private readonly IHttpContextAccessor _httpContextAccessor;
private readonly IObjectModelValidator _objectModelValidator;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Altinn.App.Core.Features.Validation.Default;
/// <summary>
/// Default validations that run on all data elements to validate metadata and file scan results.
/// </summary>
public class DefaultDataElementValidator : IDataElementValidator
public class DefaultDataElementValidator : IDataElementValidator //TODO: This should implemnt IValidator

Check warning on line 10 in src/Altinn.App.Core/Features/Validation/Default/DefaultDataElementValidator.cs

View workflow job for this annotation

GitHub Actions / Static code analysis

Complete the task associated to this 'TODO' comment. (https://rules.sonarsource.com/csharp/RSPEC-1135)
{
/// <summary>
/// Run validations on all data elements
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Altinn.App.Core.Features.Validation.Default;
/// <summary>
/// Implement the default validation of DataElements based on the metadata in appMetadata
/// </summary>
public class DefaultTaskValidator : ITaskValidator
public class DefaultTaskValidator : ITaskValidator //TODO: Implement IValidator

Check warning on line 10 in src/Altinn.App.Core/Features/Validation/Default/DefaultTaskValidator.cs

View workflow job for this annotation

GitHub Actions / Static code analysis

Complete the task associated to this 'TODO' comment. (https://rules.sonarsource.com/csharp/RSPEC-1135)
{
private readonly IAppMetadata _appMetadata;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,31 +13,34 @@ namespace Altinn.App.Core.Features.Validation.Default;
/// <summary>
/// Validates form data against expression validations
/// </summary>
public class ExpressionValidator : IFormDataValidator
public class ExpressionValidator : IValidator
{
private static readonly JsonSerializerOptions _jsonSerializerOptions =
new() { ReadCommentHandling = JsonCommentHandling.Skip, PropertyNamingPolicy = JsonNamingPolicy.CamelCase, };

private readonly ILogger<ExpressionValidator> _logger;
private readonly IAppResources _appResourceService;
private readonly ILayoutEvaluatorStateInitializer _layoutEvaluatorStateInitializer;
private readonly IAppMetadata _appMetadata;

/// <summary>
/// Constructor for the expression validator
/// </summary>
public ExpressionValidator(
ILogger<ExpressionValidator> logger,
IAppResources appResourceService,
ILayoutEvaluatorStateInitializer layoutEvaluatorStateInitializer
ILayoutEvaluatorStateInitializer layoutEvaluatorStateInitializer,
IAppMetadata appMetadata
)
{
_logger = logger;
_appResourceService = appResourceService;
_layoutEvaluatorStateInitializer = layoutEvaluatorStateInitializer;
_appMetadata = appMetadata;
}

/// <inheritdoc />
public string DataType => "*";
public string TaskId => "*";

/// <summary>
/// This validator has the code "Expression" and this is known by the frontend, who may request this validator to not run for incremental validation.
Expand All @@ -47,19 +50,48 @@ ILayoutEvaluatorStateInitializer layoutEvaluatorStateInitializer
/// <summary>
/// We don't have an efficient way to figure out if changes to the model results in different validations, and frontend ignores this anyway
/// </summary>
public bool HasRelevantChanges(object current, object previous) => true;
public Task<bool> HasRelevantChanges(
Instance instance,
string taskId,
List<DataElementChange> changes,
IInstanceDataAccessor instanceDataAccessor
) => Task.FromResult(true);

/// <inheritdoc />
public async Task<List<ValidationIssue>> ValidateFormData(
public async Task<List<ValidationIssue>> Validate(
Instance instance,
IInstanceDataAccessor instanceDataAccessor,
string taskId,
string? language
)
{
var dataTypes = (await _appMetadata.GetApplicationMetadata()).DataTypes;
var formDataElementsForTask = instance
.Data.Where(d =>
{
var dataType = dataTypes.Find(dt => dt.Id == d.DataType);
return dataType != null && dataType.TaskId == taskId;
})
.ToList();
var validationIssues = new List<ValidationIssue>();
foreach (var dataElement in formDataElementsForTask)
{
var data = instanceDataAccessor.Get(dataElement);

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

This assignment to
data
is useless, since its value is never read.
var issues = await ValidateFormData(instance, dataElement, instanceDataAccessor, taskId, language);
validationIssues.AddRange(issues);
}

return validationIssues;
}

internal async Task<List<ValidationIssue>> ValidateFormData(
Instance instance,
DataElement dataElement,
object data,
IInstanceDataAccessor dataAccessor,
string taskId,
string? language
)
{
// TODO: Consider not depending on the instance object to get the task
// to follow the same principle as the other validators
var taskId = instance.Process.CurrentTask.ElementId;
var rawValidationConfig = _appResourceService.GetValidationConfiguration(dataElement.DataType);
if (rawValidationConfig == null)
{
Expand All @@ -71,6 +103,7 @@ public async Task<List<ValidationIssue>> ValidateFormData(

var evaluatorState = await _layoutEvaluatorStateInitializer.Init(
instance,
dataAccessor,
taskId,
gatewayAction: null,
language
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ public string ValidationSource
/// <inheritdoc />
public async Task<List<ValidationIssue>> Validate(
Instance instance,
IInstanceDataAccessor instanceDataAccessor,
string taskId,
string? language,
IInstanceDataAccessor instanceDataAccessor
string? language
)
{
var issues = new List<ValidationIssue>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ public string ValidationSource
/// <inheritdoc />
public async Task<List<ValidationIssue>> Validate(
Instance instance,
IInstanceDataAccessor instanceDataAccessor,
string taskId,
string? language,
IInstanceDataAccessor instanceDataAccessor
string? language
)
{
var modelState = new ModelStateDictionary();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Altinn.App.Core.Features.Validation.Default;
/// <summary>
/// Validator that runs the required rules in the layout
/// </summary>
public class RequiredLayoutValidator : IFormDataValidator
public class RequiredLayoutValidator : IValidator
{
private readonly ILayoutEvaluatorStateInitializer _layoutEvaluatorStateInitializer;

Expand All @@ -20,37 +20,41 @@ public RequiredLayoutValidator(ILayoutEvaluatorStateInitializer layoutEvaluatorS
}

/// <summary>
/// Run for all data types
/// Run for all tasks
/// </summary>
public string DataType => "*";
public string TaskId => "*";

/// <summary>
/// This validator has the code "Required" and this is known by the frontend, who may request this validator to not run for incremental validation.
/// </summary>
public string ValidationSource => ValidationIssueSources.Required;

/// <summary>
/// We don't have an efficient way to figure out if changes to the model results in different validations, and frontend ignores this anyway
/// </summary>
public bool HasRelevantChanges(object current, object previous) => true;

/// <inheritdoc />
public async Task<List<ValidationIssue>> ValidateFormData(
public async Task<List<ValidationIssue>> Validate(
Instance instance,
DataElement dataElement,
object data,
IInstanceDataAccessor instanceDataAccessor,
string taskId,
string? language
)
{
var taskId = instance.Process.CurrentTask.ElementId;

var evaluationState = await _layoutEvaluatorStateInitializer.Init(
instance,
instanceDataAccessor,
taskId,
gatewayAction: null,
language
);

return LayoutEvaluator.RunLayoutValidationsForRequired(evaluationState);
}

/// <summary>
/// We don't have an efficient way to figure out if changes to the model results in different validations, and frontend ignores this anyway
/// </summary>
public Task<bool> HasRelevantChanges(
Instance instance,
string taskId,
List<DataElementChange> changes,
IInstanceDataAccessor instanceDataAccessor
) => Task.FromResult(true);
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ List<DataType> dataTypes
/// </summary>
public async Task<List<ValidationIssue>> Validate(
Instance instance,
IInstanceDataAccessor instanceDataAccessor,
string taskId,
string? language,
IInstanceDataAccessor instanceDataAccessor
string? language
)
{
var issues = new List<ValidationIssue>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ public FormDataValidatorWrapper(IFormDataValidator formDataValidator, string tas
/// </summary>
public async Task<List<ValidationIssue>> Validate(
Instance instance,
IInstanceDataAccessor instanceDataAccessor,
string taskId,
string? language,
IInstanceDataAccessor instanceDataAccessor
string? language
)
{
var issues = new List<ValidationIssue>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ public TaskValidatorWrapper(ITaskValidator taskValidator)
/// <inheritdoc />
public Task<List<ValidationIssue>> Validate(
Instance instance,
IInstanceDataAccessor instanceDataAccessor,
string taskId,
string? language,
IInstanceDataAccessor instanceDataAccessor
string? language
)
{
return _taskValidator.ValidateTask(instance, taskId, language);
Expand Down
13 changes: 9 additions & 4 deletions src/Altinn.App.Core/Helpers/ObjectUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,10 @@ private static void SetToDefaultIfShouldSerializeFalse(object model, PropertyInf
/// <summary>
/// Set all <see cref="Guid"/> properties named "AltinnRowId" to Guid.Empty
/// </summary>
public static void RemoveAltinnRowId(object model, int depth = 64)
/// <returns>true if any changes to the data has been performed</returns>
public static bool RemoveAltinnRowId(object model, int depth = 64)
{
var isModified = false;
ArgumentNullException.ThrowIfNull(model);
if (depth < 0)
{
Expand All @@ -192,14 +194,15 @@ public static void RemoveAltinnRowId(object model, int depth = 64)
var type = model.GetType();
if (type.Namespace?.StartsWith("System", StringComparison.Ordinal) == true)
{
return; // System.DateTime.Now causes infinite recursion, and we shuldn't recurse into system types anyway.
return isModified; // System.DateTime.Now causes infinite recursion, and we shuldn't recurse into system types anyway.
}

foreach (var prop in type.GetProperties())
{
// Handle guid fields named "AltinnRowId"
if (PropertyIsAltinRowGuid(prop))
{
isModified = true;
prop.SetValue(model, Guid.Empty);
}
// Recurse into lists
Expand All @@ -213,7 +216,7 @@ public static void RemoveAltinnRowId(object model, int depth = 64)
// Recurse into values of a list
if (item is not null)
{
RemoveAltinnRowId(item, depth - 1);
isModified |= RemoveAltinnRowId(item, depth - 1);
}
}
}
Expand All @@ -226,10 +229,12 @@ public static void RemoveAltinnRowId(object model, int depth = 64)
// continue recursion over all properties
if (value is not null)
{
RemoveAltinnRowId(value, depth - 1);
isModified |= RemoveAltinnRowId(value, depth - 1);
}
}
}

return isModified;
}

private static bool PropertyIsAltinRowGuid(PropertyInfo prop)
Expand Down
14 changes: 10 additions & 4 deletions src/Altinn.App.Core/Internal/Data/CachedFormDataAccessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ namespace Altinn.App.Core.Internal.Data;
/// </summary>
internal sealed class CachedInstanceDataAccessor : IInstanceDataAccessor
{
private readonly Instance _instance;
private readonly string _org;
private readonly string _app;
private readonly Guid _instanceGuid;
Expand All @@ -30,15 +31,20 @@ public CachedInstanceDataAccessor(
IAppModel appModel
)
{
_org = instance.Org;
_app = instance.AppId.Split("/")[1];
_instanceGuid = Guid.Parse(instance.Id.Split("/")[1]);
_instanceOwnerPartyId = int.Parse(instance.InstanceOwner.PartyId, CultureInfo.InvariantCulture);
var splitApp = instance.AppId.Split("/");
_org = splitApp[0];
_app = splitApp[1];
var splitId = instance.Id.Split("/");
_instanceOwnerPartyId = int.Parse(splitId[0], CultureInfo.InvariantCulture);
_instanceGuid = Guid.Parse(splitId[1]);
_instance = instance;
_dataClient = dataClient;
_appMetadata = appMetadata;
_appModel = appModel;
}

public Instance Instance => _instance;

/// <inheritdoc />
public async Task<object> Get(DataElement dataElement)
{
Expand Down
Loading

0 comments on commit f7b91e0

Please sign in to comment.