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

Simplify File analysis/validation #378

Closed
wants to merge 3 commits into from
Closed

Conversation

ivarne
Copy link
Member

@ivarne ivarne commented Dec 12, 2023

While working on rewriting the interfaces for validation to remove ModelStateDictionary and related infrastructure in #369 / #374, I took a look at File validation and think I found something to simplify there as well.

This PR has no functional changes, but introduces a new way to registrer file analysers and validators through keyed services, to match the new way of registrering validations.

services.AddKeyedTransient<IFileValidator, MyFileValidator>("dataTypeId")

instead of the more convoluted config where myValidator.Id => "MY_VALIDATOR" and datatype.EnabledValidators[] contains "MY_VALIDATOR".

I think we should strive to use only one concept to register any type of validator, and the double approach used here is probably not a very good idea.

Note: This PR includes #369, so for review only look at 91bfcd1

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 (there were no tests 👿 )

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@ivarne ivarne changed the base branch from ivarne/new-validation to v8 December 12, 2023 21:12
@ivarne ivarne marked this pull request as ready for review December 12, 2023 21:13
@ivarne ivarne force-pushed the ivarne/fileAnalyze-validation branch from 91bfcd1 to bf6ec82 Compare December 13, 2023 12:05
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.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

57.9% Coverage on New Code (required ≥ 65%)
60.69% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarCloud

@ivarne ivarne marked this pull request as draft January 26, 2024 12:01
@ivarne ivarne force-pushed the ivarne/fileAnalyze-validation branch from ee5c90f to 577ee8e Compare January 26, 2024 12:11
fileAnalysisResults = await Analyse(dataType, fileStream, fileName);
}

bool fileValidationSuccess = true;

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

This assignment to
fileValidationSuccess
is useless, since its value is never read.
List<ValidationIssue> validationIssues = new();
if (FileValidationEnabledForDataType(dataType))
{
(fileValidationSuccess, validationIssues) = await Validate(dataType, fileAnalysisResults);

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

This assignment to
fileValidationSuccess
is useless, since its value is never read.
Copy link

Quality Gate Passed Quality Gate passed

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

4 New issues
0 Security Hotspots
96.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@ivarne
Copy link
Member Author

ivarne commented Feb 16, 2024

We do not have any issue

@ivarne ivarne closed this Feb 16, 2024
@ivarne ivarne deleted the ivarne/fileAnalyze-validation branch October 10, 2024 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant