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

Expression validation #311

Merged
merged 18 commits into from
Oct 4, 2023
Merged

Expression validation #311

merged 18 commits into from
Oct 4, 2023

Conversation

bjosttveit
Copy link
Member

@bjosttveit bjosttveit commented Sep 21, 2023

Description

  • API endpoint to serve expression validation config to frontend
  • Runs expression validations if ExpressionValidation is enabled in AppSettings.
  • Added argv expression for getting arguments passed to expressions. (used to get the dataModel field the validation is for)
  • RemoveHiddenDataPreview setting has been replaced with:
    • RemoveHiddenData: Removes hidden data on submit
    • RequiredValidation: Runs required validations (will run hidden expressions first regardless of the flag above, but does not affect submission)
    • Updated the upgrade-tool to fix this automatically when upgrading to v8
  • Changed dataModel expression to only return "legal" values and null if it is an object or array. This logic previously only existed in JsonDataModel (used for running shared tests) and not in the regular DataModel class. So the tests were passing while the implementation used in practice was wrong. This logic has been removed from JsonDataModel to match the actual DataModel implementation since other places in the code relies on getting objects and arrays from the datamodel.
  • Added shared tests
  • Refactored JsonDataModel to use JsonNode instead of JsonElement. JsonNode is mutable and was needed to be able to implement RemoveField for shared testing.

Related Issue(s)

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)

src/Altinn.App.Core/Implementation/AppResourcesSI.cs Dismissed Show resolved Hide resolved
src/Altinn.App.Core/Implementation/AppResourcesSI.cs Dismissed Show resolved Hide resolved
Comment on lines +53 to +67
foreach (var file in files)
{
var data = File.ReadAllText(file);
ExpressionValidationTestModel testCase = JsonSerializer.Deserialize<ExpressionValidationTestModel>(
data,
new JsonSerializerOptions
{
ReadCommentHandling = JsonCommentHandling.Skip,
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
})!;
yield return new object[] { testCase };
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Select Note test

This foreach loop immediately
maps its iteration variable to another variable
- consider mapping the sequence explicitly using '.Select(...)'.
@bjosttveit bjosttveit force-pushed the expression-validation branch from 0f837b9 to ee4527a Compare September 21, 2023 09:27
@bjosttveit bjosttveit added the breaking-change Label Pull requests with breaking changes. Used when generating releasenotes label Sep 21, 2023
@bjosttveit bjosttveit force-pushed the expression-validation branch from e829a24 to b84a621 Compare September 26, 2023 11:40
@bjosttveit bjosttveit marked this pull request as ready for review September 26, 2023 11:56
@bjosttveit bjosttveit requested a review from tjololo September 26, 2023 13:32
@bjosttveit bjosttveit self-assigned this Sep 26, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 2, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 26 Code Smells

53.6% 53.6% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

catch
{
logger.LogError($"Error while evaluating expression validation for {resolvedField}");
throw;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I think throwing is the best solution. Returning a validation error tied to the data would present the user with an error he cannot do anything about, tied to a field where he possibly have provided the data.

Copy link
Member

@tjololo tjololo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:godmode:

@bjosttveit bjosttveit merged commit 4da4527 into v8 Oct 4, 2023
9 of 10 checks passed
@bjosttveit bjosttveit deleted the expression-validation branch October 4, 2023 09:11
Comment on lines +70 to +73
catch
{
logger.LogError($"Error while evaluating expression validation for {resolvedField}");
throw;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Æsj, jeg burde sett dette før.
Det er lurt å sende med exception i loggen, og også gjøre meldingen parametrisk så man kan lage bedre queries i application insights.

Suggested change
catch
{
logger.LogError($"Error while evaluating expression validation for {resolvedField}");
throw;
catch(Exception e)
{
logger.LogError(e, "Error while evaluating expression validation for {resolvedField}", resolvedField);
throw;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice catch, I missed that one 🙈 fix in new PR: #319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Label Pull requests with breaking changes. Used when generating releasenotes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants