Support backend validation for multiple data models #619

ivarne · 2024-04-27T19:47:45Z

New type record struct ModelBinding to replace the current use of string in a way that ensures a structured way of storing the optional reference to an extra data model.

public readonly record struct ModelBinding
{
    public ModelBinding()
    {
        DataType = "default";
    }

    public required string Field { get; init; }
    public string DataType { get; init; }

    // Consider removing the implicit operator. It makes the existing tests work with much less changes.
    public static implicit operator ModelBinding(string field)
    {
        return new ModelBinding { Field = field };
    }
}

I'm pretty happy about the minimal changes required for everything but the actual lookup of data to work.

Remaining work

Make the IDataModel implementations aware of multiple data models so that lookups don't just ignore them.
- Decide on a caching approach. Should we use a caching version IDataClient registered as scoped in DI? Or do we have other options?

Related Issue(s)

Layout & Expression changes due to supporting multiple data models #600

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 · 2024-04-29T08:26:49Z

Do you want me to review the whole thing or just the ModelBinding struct for now?

Would ModelBindingSelector be a better name? To me that sounds more specific and clear about what it does/is used for, in that it represents the selection/query into the datamodel. I think the structure is good. It doesn't really validate the inputs, so in terms of correctness I don't think it improves the "primitive obsession" situation that we have (I don't think there are any good ways to actually do that well atm anyway)

I think we have to have the implicit operator since we're doing string -> ModelBinding change on IDataModelAccessor? Re not having breaking changes

ivarne · 2024-04-29T08:31:53Z

Do you want me to review the whole thing or just the ModelBinding struct for now?

I want any feedback I can get, apart from "Hi! I made a test app, how do I test this new behavior", because it isn't complete yet.

src/Altinn.App.Core/Helpers/DataModel/DataModel.cs

src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs

src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs

src/Altinn.App.Core/Models/Layout/ModelBinding.cs

src/Altinn.App.Core/Models/Layout/PageComponentConverter.cs

src/Altinn.App.Core/Helpers/IDataModel.cs

src/Altinn.App.Core/Internal/Validation/ValidationService.cs

src/Altinn.App.Core/Models/Layout/LayoutModel.cs

src/Altinn.App.Api/Controllers/ResourceController.cs

-        [Route("{org}/{app}/api/validationconfig/{id}")]
-        public ActionResult GetValidationConfiguration(string org, string app, string id)
+        [Route("{org}/{app}/api/validationconfig/{dataTypeId}")]
+        public ActionResult GetValidationConfiguration(string org, string app, string dataTypeId)


…different models

Support the new interface for ValidationService Rewrite RequiredValidator to use the new interface to support muliple data models

src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs

src/Altinn.App.Core/Features/Validation/Default/RequiredValidator.cs

src/Altinn.App.Core/Models/Layout/LayoutModel.cs

test/Altinn.App.Core.Tests/LayoutExpressions/TestDataModel.cs

src/Altinn.App.Core/Implementation/AppResourcesSI.cs

ivarne · 2024-06-02T15:16:37Z

@martinothamar @bjosttveit

Da virker alle eksisterende tester og det er egentlig klart for review. Har ikke begynt å skrive tester for den nye funksjonaliteten ennå.

Har inkludert ["language"] expression funksjonen, fordi den krever mer context var naturlig å legge med når jeg uansett måtte endre LayoutEvaluatorStateInitializer.Init funksjonen for å støtte flere modeller.

…uest is valid

src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs

martinothamar

Made one pass, will make another once tests come in 😄

src/Altinn.App.Core/Internal/Expressions/ILayoutEvaluatorStateInitializer.cs

src/Altinn.App.Core/Models/Expressions/Expression.cs

src/Altinn.App.Core/Models/Layout/Components/GridComponent.cs

src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs

src/Altinn.App.Core/Helpers/DataModel/DataModel.cs

src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs

martinothamar · 2024-06-05T06:52:33Z

src/Altinn.App.Core/Helpers/DataModel/DataModel.cs

    }

+    /// <inheritdoc />
+    public DataElement DefaultDataElement { get; }


What's the usecase for this being public?

Nothing! (at least that I can think of)

I needed it to get the data element for ValidationIssue, but it did not solve the full issue, so I added LayoutEvaluatorState.GetDataElement(ModelBinding field) instead. I should probably just move that implementation into the data model and not rely on the _instanceContext

src/Altinn.App.Core/Internal/Data/CachedFormDataAccessor.cs

src/Altinn.App.Core/Internal/Data/ICachedFormDataAccessor.cs

src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs

Also enforce that CachedFormDataAccessor verifies that it only runs in one request (to protect against any cross user leakage)

test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestBackendExclusiveFunctions.cs

+        catch (Exception e)
+        {
+            using var jsonDocument = JsonDocument.Parse(data);
+
+            testCase.Name = jsonDocument.RootElement.GetProperty("name").GetString();
+            testCase.ExpectsFailure = jsonDocument.RootElement.TryGetProperty("expectsFailure", out var expectsFailure)
+                ? expectsFailure.GetString()
+                : null;
+            testCase.ParsingException = e;
+        }


test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/DynamicClassBuilderChatGPTTests.cs

+        var deserializedObject = JsonSerializer.Deserialize(jsonString, dynamicType);
+        var numbersProperty = dynamicType.GetProperty("Numbers")!.GetValue(deserializedObject) as List<double?>;
+
+        numbersProperty.Should().NotBeNull();


test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/DynamicClassBuilderChatGPTTests.cs

test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/DynamicClassBuilderTests.cs

…dels

And ressurect docs on DataModelClass from removed IDataModelAccessor

…dels

martinothamar

Generally LGTM 👍

In terms of public vs internal - I don't think we should add more public members unless we have an agreed-upon use-case with consumers already. Didn't seem like that was the case, so according to previous discussions I think those should be made internal. If not, I think we should revisit the decision in the team, so we have shared understanding

…dels

src/Altinn.App.Core/Models/Layout/ModelBinding.cs

…dels

src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs

…sues in initialization

sonarcloud · 2024-06-19T09:42:24Z

Quality Gate passed

Issues
11 New issues
0 Accepted issues

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

See analysis details on SonarCloud

bjosttveit · 2024-06-19T11:41:34Z

src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs

+                                var validationIssue = new ValidationIssue
+                                {
+                                    Field = resolvedField.Field,
+                                    DataElementId = resolvedField.DataType,
+                                    Severity = validation.Severity ?? ValidationIssueSeverity.Error,
+                                    CustomTextKey = validation.Message,
+                                    Code = validation.Message,
+                                    Source = ValidationIssueSources.Expression,
+                                };
+                                validationIssues.Add(validationIssue);


The DataElementId should not be set to the data type id?

No, that would not be very helpful. Good catch!

…dels

sonarcloud · 2024-08-15T19:53:02Z

Quality Gate passed

Issues
14 New issues
0 Accepted issues

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

See analysis details on SonarCloud

ivarne requested a review from martinothamar April 27, 2024 19:47

martinothamar reviewed May 2, 2024

View reviewed changes

ivarne force-pushed the feat/multiple-data-models branch 2 times, most recently from 9b19779 to a02524d Compare May 25, 2024 18:02

github-advanced-security bot found potential problems May 25, 2024

View reviewed changes

ivarne added 4 commits June 1, 2024 09:26

Add ModelBinding type to encapsulate the possibility of referring to …

48a0d19

…different models

New validation interface IMultipleFormDataValidator

54d76a0

Support the new interface for ValidationService Rewrite RequiredValidator to use the new interface to support muliple data models

Remove IMultipleFormDataValidator

6be32e2

Make Expression a struct and other layout classes immutable records

edb533c

ivarne force-pushed the feat/multiple-data-models branch from 3c74bf2 to edb533c Compare June 1, 2024 11:51

github-advanced-security bot found potential problems Jun 1, 2024

View reviewed changes

src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs Fixed Show fixed Hide fixed

src/Altinn.App.Core/Features/Validation/Default/RequiredValidator.cs Fixed Show fixed Hide fixed

ivarne added 2 commits June 1, 2024 14:01

fix comment

a0524f7

Fix LayoutEvaluatorStateInitializer and add interface

7b81cf0

github-advanced-security bot found potential problems Jun 2, 2024

View reviewed changes

src/Altinn.App.Core/Models/Layout/LayoutModel.cs Fixed Show fixed Hide fixed

test/Altinn.App.Core.Tests/LayoutExpressions/TestDataModel.cs Fixed Show fixed Hide fixed

ivarne changed the title ~~WIP: Add ModelBinding type to encapsulate the possibility of referring to different models~~ Support backend validation for multiple data models Jun 2, 2024

Fix typos and code style warnings

e880d04

github-advanced-security bot found potential problems Jun 2, 2024

View reviewed changes

src/Altinn.App.Core/Implementation/AppResourcesSI.cs Dismissed Show dismissed Hide dismissed

src/Altinn.App.Core/Implementation/AppResourcesSI.cs Dismissed Show dismissed Hide dismissed

More sonarcloud issues

34863a2

ivarne force-pushed the feat/multiple-data-models branch from 93009a5 to 34863a2 Compare June 2, 2024 13:17

ivarne added 2 commits June 2, 2024 15:33

Fix double set in cache

ae8cddc

More sonar cloud issues

7fd0b18

ivarne force-pushed the feat/multiple-data-models branch from 0ba8554 to 7fd0b18 Compare June 2, 2024 14:49

Include tests for the new "language" function

d01c680

ivarne added 2 commits June 2, 2024 17:19

Add [ApiController] to make sonar cloud shut up about checking if req…

e3e7bef

…uest is valid

Remove unused var in ProcessTaskFinalizer

0161134

ivarne force-pushed the feat/multiple-data-models branch from f07f795 to 0161134 Compare June 2, 2024 15:34

bjosttveit reviewed Jun 3, 2024

View reviewed changes

src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs Show resolved Hide resolved

martinothamar reviewed Jun 5, 2024

View reviewed changes

Final touch to DataModel (remove IDataModelAccessor)

9c22bb4

Also enforce that CachedFormDataAccessor verifies that it only runs in one request (to protect against any cross user leakage)

github-advanced-security bot found potential problems Jun 5, 2024

View reviewed changes

ivarne added 4 commits June 5, 2024 10:07

Merge remote-tracking branch 'origin/main' into feat/multiple-data-mo…

f7f1687

…dels

Pull request feedback

7b77640

And ressurect docs on DataModelClass from removed IDataModelAccessor

Merge remote-tracking branch 'origin/main' into feat/multiple-data-mo…

c9c809a

…dels

Specify invariantCulture for int.Parse

ca9d61d

martinothamar approved these changes Jun 13, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into feat/multiple-data-mo…

06f0e26

…dels

bjosttveit reviewed Jun 13, 2024

View reviewed changes

src/Altinn.App.Core/Models/Layout/ModelBinding.cs Show resolved Hide resolved

ivarne added 4 commits June 18, 2024 12:18

Add shared tests with new format

c8d6dfc

Merge remote-tracking branch 'origin/main' into feat/multiple-data-mo…

81b8db1

…dels

Add missing files

3b3e1fe

Add more tests and fix edge cases

6311f93

bjosttveit reviewed Jun 19, 2024

View reviewed changes

src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs Outdated Show resolved Hide resolved

Ensure explicit references to the default data type does not cause is…

2142641

…sues in initialization

bjosttveit reviewed Jun 19, 2024

View reviewed changes

ivarne mentioned this pull request Jun 29, 2024

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

Closed

5 tasks

Merge remote-tracking branch 'origin/main' into feat/multiple-data-mo…

913056a

…dels

ivarne mentioned this pull request Aug 19, 2024

Support Extra data models and subforms in validation #730

Merged

5 tasks

ivarne closed this Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support backend validation for multiple data models #619

Support backend validation for multiple data models #619

ivarne commented Apr 27, 2024

martinothamar commented Apr 29, 2024

ivarne commented Apr 29, 2024

ivarne commented Jun 2, 2024

martinothamar left a comment

martinothamar Jun 5, 2024

ivarne Jun 6, 2024

martinothamar left a comment

sonarcloud bot commented Jun 19, 2024

bjosttveit Jun 19, 2024

ivarne Jun 19, 2024

sonarcloud bot commented Aug 15, 2024

Support backend validation for multiple data models #619

Support backend validation for multiple data models #619

Conversation

ivarne commented Apr 27, 2024

Remaining work

Related Issue(s)

Verification

Documentation

martinothamar commented Apr 29, 2024

ivarne commented Apr 29, 2024

ivarne commented Jun 2, 2024

martinothamar left a comment

Choose a reason for hiding this comment

martinothamar Jun 5, 2024

Choose a reason for hiding this comment

ivarne Jun 6, 2024

Choose a reason for hiding this comment

martinothamar left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 19, 2024

Quality Gate passed

bjosttveit Jun 19, 2024

Choose a reason for hiding this comment

ivarne Jun 19, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Aug 15, 2024

Quality Gate passed