RFC: Add IFileUploadValidator to mirror the pattern of the new validator classes

[ivarne]

Some service owners has asked for validation of uploaded files. Since we decided to use a new approach for the new validation classes, I think it would make sense to provide a new interface IFileUploadValidator to provide a way that follows the same patterns as the new validation interfaces to validate file uploads.

Description

IFileValidator and IFileAnalyser are registered in custom fields on the DataType object in applicationmetadata.json. All the other new validation interfaces uses a different pattern where the validator has a DataType/TaskId field to configure when it should be executed.

This PR:

• Adds a new IFileUploadValidator interface to match the new validation interfaces (including language parameter)
• Adds default implementation LegacyFileAnalyzerValidator that ensures IFileValidator/IFileAnalyser continue to work in a backwards compatible way.
• Consolidates validation functionality into ValidationService/ValidationFactory.

/// <summary>
/// Interface for handling validation of files when uploaded.
/// A validation failiure will cause the upload to be rejected.
/// </summary>
public interface IFileUploadValidator
{
    /// <summary>
    /// The data type that this validator should run for. This is the id of the data type from applicationmetadata.json
    /// The string "*" is a special value that means that the validator should run for all data types that don't have app-logic.
    /// </summary>
    string DataType { get; }

    /// <summary>
    /// Returns the group id of the validator.
    /// The default is based on the FullName and DataType fields, and should not need customization
    /// </summary>
    string ValidationSource => $"{this.GetType().FullName}-{DataType}";

    /// <summary>
    /// Validating a an uploaded file to possibly prevent the upload.
    /// The file is considered valid if the list of issues has no entries where <see cref="ValidationIssue.Severity"/> is Error.
    /// </summary>
    Task<List<ValidationIssue>> Validate(Instance instance, DataType dataType, byte[] fileContent, string? filename, string? mimeType, string? language);
}

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)

[martinothamar]

Pretty limited understanding of the whole context here, but approach looks good to me. I'm uncertain about removing public types and such though. Since it seemed like we agreed to follow semver pretty strictly?

[martinothamar]

I think this should be internal and sealed? This was meant to bridge the migration to the the new interfaces right, so we have some expectations from the behavior here?

[ivarne]

I just followed previous practice. Having this internal would make it more awkward for users to unregister this implementation from the DI container, and they would have to do it in a way that does not cause compiler warnings if this implementation is removed. I don't have a strong opinion either way.

[martinothamar]

Should users be able to unregister this? I thought it was needed to still support the file analyzer abstraction

[ivarne]

I don't see any reason why we should actively prevent users from unregistering anything and replace it with their own interface implementation.

I also can't find any realistic scenario when you'd actually need to unregister this service, but a contrived example might be if you had a badly behaved IFileAnalyser in a package that should not be constructed for uploads to all DataTypes, but only the ones where it is actually used. A quick fix would be to replace this implementation that inject IEnumerable<IFileAnalysr> with one that fetches from IServiceProvider only when it is actually required.

[martinothamar]

It doesn't prevent unregistering though. My impression from previous discussions was that we would be more intentional about type accessibility going forward, and that any modification to public types is a breaking change. That's what I was considering here when reviewing, and so if I consider the tradeoffs between the ergonomics of unregistering and including this as a public API I personally am more worried about adding another public type

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
86.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
86.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud