Go a few steps further in unifying IInstanceDataAccessor and LayoutEvaluatorState#1747
Open
ivarne wants to merge 8 commits into
Open
Go a few steps further in unifying IInstanceDataAccessor and LayoutEvaluatorState#1747ivarne wants to merge 8 commits into
ivarne wants to merge 8 commits into
Conversation
Co-authored-by: Copilot <copilot@github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> WalkthroughThis PR refactors the layout evaluation APIs to be ChangesCore State Models and Public API Refactoring
State Initialization and Dependencies
Component Implementations
Service and Orchestration Updates
Test Infrastructure and Utilities
🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR continues the effort to phase out LayoutEvaluatorState in favor of IInstanceDataAccessor by making state/accessor conversion easier and shifting component/context APIs to primarily use the accessor.
Changes:
- Added
LayoutEvaluatorState.DataAccessorand updatedComponentContext/layout components to accept and carryIInstanceDataAccessorinstead ofLayoutEvaluatorState. - Updated layout context generation APIs (
LayoutModel.GenerateComponentContexts,BaseLayoutComponent.GetContext, etc.) to take an accessor. - Refactored test utilities and many layout-expression tests to initialize state via accessor and keep tests passing.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt | Updates public API snapshot to reflect new/changed members and obsolete overloads. |
| test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs | Extends fake accessor to be able to produce a LayoutEvaluatorState (and hold language/layout/text deps). |
| test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/DynamicClassBuilder.cs | Updates helpers to construct InstanceDataAccessorFake with new dependencies. |
| test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/SubForm/SubFormTests.cs | Updates full tests to provide layout/state-related dependencies to the accessor fake. |
| test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/RepeatingGroupsHidden/RepeatingGroupsHidden.cs | Simplifies test setup to use data object directly rather than accessor. |
| test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/RemoveHiddenData/RemoveHiddenDataTests.cs | Simplifies test setup to use data object directly rather than accessor. |
| test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/LikertHidden/LikertHiddenTests.cs | Updates helper invocation to new DataAccessorFromJsonDocument signature. |
| test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/LayoutTestUtils.cs | Updates shared test initialization to pass translation/layout/frontend settings into accessor fake. |
| test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestInvalid.cs | Updates expression test harness to create state via accessor. |
| test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestFunctions.cs | Updates expression test harness to construct accessor with translation/layout and use new context generation API. |
| test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestContextList.cs | Updates context-list test harness to create state via accessor. |
| test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestBackendExclusiveFunctions.cs | Updates backend-exclusive test harness to create state via accessor. |
| test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/ExpressionTestCaseRoot.cs | Adjusts context creation for new ComponentContext ctor signature (accessor-first). |
| test/Altinn.App.Core.Tests/Features/Validators/ValidationServiceTests.cs | Updates validator tests to construct accessor fake with additional constructor params. |
| test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceOldTests.cs | Updates legacy validator test to new accessor fake constructor signature. |
| test/Altinn.App.Core.Tests/Features/Validators/Default/LegacyIValidationFormDataTests.cs | Updates legacy IValidation tests to new accessor fake constructor signature. |
| test/Altinn.App.Core.Tests/Features/Validators/Default/ExpressionValidatorTests.cs | Updates expression validator tests to build evaluator state via accessor fake. |
| src/Altinn.App.Core/Models/Layout/LayoutModel.cs | Changes context generation entrypoint to accept IInstanceDataAccessor. |
| src/Altinn.App.Core/Models/Layout/Components/SubFormComponent.cs | Switches context-building from state to accessor and updates instance data access accordingly. |
| src/Altinn.App.Core/Models/Layout/Components/RepeatingGroupComponent.cs | Switches to accessor-based context building and uses accessor wrapper for row counting. |
| src/Altinn.App.Core/Models/Layout/Components/PageComponent.cs | Switches page context building to accept accessor. |
| src/Altinn.App.Core/Models/Layout/Components/OptionsComponent.cs | Switches options context building to accessor and uses accessor wrapper for row counting. |
| src/Altinn.App.Core/Models/Layout/Components/Base/ReferenceComponent.cs | Switches claimed-child context building to accessor-based flow. |
| src/Altinn.App.Core/Models/Layout/Components/Base/NoReferenceComponent.cs | Switches trivial context creation to accessor-based flow. |
| src/Altinn.App.Core/Models/Layout/Components/Base/BaseLayoutComponent.cs | Introduces new accessor-based abstract GetContext and adds an obsolete state-based overload. |
| src/Altinn.App.Core/Models/Expressions/ComponentContext.cs | Adds accessor-based ctor and stores accessor; makes State derived from accessor. |
| src/Altinn.App.Core/Internal/Texts/ITranslationService.cs | Obsoletes translation overload taking LayoutEvaluatorState. |
| src/Altinn.App.Core/Internal/Process/ExpressionsExclusiveGateway.cs | Constructs ComponentContext with accessor rather than state. |
| src/Altinn.App.Core/Internal/Pdf/PdfService.cs | Uses translation overload taking IInstanceDataAccessor and builds ComponentContext with accessor. |
| src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cs | Updates internal accessor used by initializer to be able to produce a LayoutEvaluatorState. |
| src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs | Adds DataAccessor property; routes context generation through accessor; adds WithDataAccessor. |
| src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs | Implements GetLayoutEvaluatorState() by reusing the underlying state with a new accessor. |
| src/Altinn.App.Core/Features/Validation/Default/RequiredLayoutValidator.cs | Stops using initializer and instead requires dataAccessor.GetLayoutEvaluatorState(). |
| src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs | Builds ComponentContext with accessor instead of state. |
| src/Altinn.App.Core/Features/IInstanceDataAccessor.cs | Adds helper to resolve IFormDataWrapper from a ModelBinding + default data element identifier. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ivarne
added
backport-ignore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceOldTests.cs (1)
180-190: ⚡ Quick winStrongly recommend using named parameters for clarity.
This constructor call has 9 positional parameters with 5 consecutive
nullvalues, making it very difficult to understand which parameter serves what purpose. The trailing string literals ("Task_1","test") are particularly opaque without documentation.♻️ Recommended refactor using named parameters
-var dataAccessorMock = new InstanceDataAccessorFake( - instance, - _applicationMetadata, - null, - null, - null, - null, - null, - "Task_1", - "test" -) +var dataAccessorMock = new InstanceDataAccessorFake( + instance: instance, + applicationMetadata: _applicationMetadata, + translationService: null, + layoutModel: null, + frontEndSettings: null, + gatewayAction: null, + language: null, + taskId: "Task_1", + dataType: "test" +)This significantly improves readability and reduces the risk of passing arguments in the wrong order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceOldTests.cs` around lines 180 - 190, The constructor call to InstanceDataAccessorFake with nine positional args (including five consecutive nulls and the trailing "Task_1", "test") is unclear; update the invocation to use named parameters for the InstanceDataAccessorFake constructor so each argument is explicit (e.g., name the parameters corresponding to instance, applicationMetadata, the optional values currently passed as null, taskId/"Task_1", and user/"test"), replacing the positional nulls with their parameter names to improve readability and prevent ordering mistakes.test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/LikertHidden/LikertHiddenTests.cs (1)
50-58: ⚡ Quick winConsider using named parameters for clarity.
The constructor call has 7 positional parameters, 6 of which are
null. This makes it difficult to understand which parameter is which and increases the risk of passing arguments in the wrong order.Consider using named parameters to improve readability:
IInstanceDataAccessor dataAccessor = DynamicClassBuilder.DataAccessorFromJsonDocument( instance: new Instance(), translationService: null, layoutModel: null, frontEndSettings: null, dataModel: jsonDoc.RootElement, gatewayAction: null, language: null );Alternatively, if this pattern is common across tests, consider providing overloads or a builder pattern in
DynamicClassBuilderto reduce the need for explicit nulls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/LikertHidden/LikertHiddenTests.cs` around lines 50 - 58, Replace the 7 positional arguments in the call to DynamicClassBuilder.DataAccessorFromJsonDocument with named parameters to make it clear which values are null and which is jsonDoc.RootElement; for example use parameter names like instance:, translationService:, layoutModel:, frontEndSettings:, dataModel:, gatewayAction:, language: so the call to DataAccessorFromJsonDocument clearly associates new Instance() and jsonDoc.RootElement with the correct parameters rather than relying on positional nulls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Altinn.App.Core/Features/Validation/Default/RequiredLayoutValidator.cs`:
- Line 18: The change removed the fallback that builds a LayoutEvaluatorState
from the current taskId and language, causing Validate to rely solely on
pre-attached accessor state; restore the fallback in RequiredLayoutValidator so
that if the ILayoutEvaluatorStateAccessor has no eager state the constructor or
Validate method will construct a new LayoutEvaluatorState using the provided
taskId and language parameters (use the existing LayoutEvaluatorState type and
the accessor methods to attach it), ensuring
RequiredLayoutValidator.Validate(taskId, language, ...) uses the
task/language-aware state when available or creates and attaches one when
missing.
In `@src/Altinn.App.Core/Models/Expressions/ComponentContext.cs`:
- Around line 19-27: The obsolete ComponentContext constructor discards the
incoming LayoutEvaluatorState causing State to call
DataAccessor.GetLayoutEvaluatorState() on every access and lose memoized caches;
modify the obsolete constructor overload in ComponentContext to store the
provided LayoutEvaluatorState (e.g., assign a private readonly field backing
State) and update the State property or accessor to return the stored instance
when present instead of always reconstructing via
DataAccessor.GetLayoutEvaluatorState(), ensuring the legacy path preserves and
reuses the original state and its caches (also apply the same change to the
other obsolete constructor at the other occurrence).
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/DynamicClassBuilder.cs`:
- Around line 168-186: The DataAccessorFromJsonDocument helper is ignoring its
gatewayAction and language parameters by passing null to the
InstanceDataAccessorFake constructor; update the call inside
DataAccessorFromJsonDocument to forward the gatewayAction and language arguments
(replace gatewayAction: null, language: null with gatewayAction: gatewayAction,
language: language) so tests exercise action- and language-dependent behavior in
InstanceDataAccessorFake.
In
`@test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt`:
- Around line 4766-4768: The Obsolete attribute on the legacy GetContext
overload (the method signature
GetContext(Altinn.App.Core.Internal.Expressions.LayoutEvaluatorState state,
Altinn.App.Core.Models.DataElementIdentifier defaultDataElementIdentifier,
int[]? rowIndexes, System.Collections.Generic.Dictionary<string,
Altinn.App.Core.Models.Layout.LayoutSetComponent> layoutsLookup)) currently uses
the 'true' flag which makes it a compilation error; change the attribute to mark
it obsolete but not an error (set the error flag to false or remove the true
argument) so consumers with custom BaseLayoutComponent implementations can
compile for one transition cycle.
- Line 4912: Add a deprecated bridge overload on LayoutModel: implement
GenerateComponentContexts(LayoutEvaluatorState) as a thin wrapper that is
annotated [Obsolete] (with a migration message) and forwards to the new
GenerateComponentContexts(IInstanceDataAccessor) implementation; inside the
wrapper obtain or adapt an IInstanceDataAccessor from the supplied
LayoutEvaluatorState (use the same adapter/conversion logic your codebase uses
for converting LayoutEvaluatorState to IInstanceDataAccessor) and return the
result of calling GenerateComponentContexts(IInstanceDataAccessor) so existing
callers keep working while steering them to the new API.
---
Nitpick comments:
In
`@test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceOldTests.cs`:
- Around line 180-190: The constructor call to InstanceDataAccessorFake with
nine positional args (including five consecutive nulls and the trailing
"Task_1", "test") is unclear; update the invocation to use named parameters for
the InstanceDataAccessorFake constructor so each argument is explicit (e.g.,
name the parameters corresponding to instance, applicationMetadata, the optional
values currently passed as null, taskId/"Task_1", and user/"test"), replacing
the positional nulls with their parameter names to improve readability and
prevent ordering mistakes.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/LikertHidden/LikertHiddenTests.cs`:
- Around line 50-58: Replace the 7 positional arguments in the call to
DynamicClassBuilder.DataAccessorFromJsonDocument with named parameters to make
it clear which values are null and which is jsonDoc.RootElement; for example use
parameter names like instance:, translationService:, layoutModel:,
frontEndSettings:, dataModel:, gatewayAction:, language: so the call to
DataAccessorFromJsonDocument clearly associates new Instance() and
jsonDoc.RootElement with the correct parameters rather than relying on
positional nulls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 174853e8-030e-408f-a75e-8d1020c24b63
📒 Files selected for processing (35)
src/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cssrc/Altinn.App.Core/Features/Validation/Default/RequiredLayoutValidator.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ExpressionsExclusiveGateway.cssrc/Altinn.App.Core/Internal/Texts/ITranslationService.cssrc/Altinn.App.Core/Models/Expressions/ComponentContext.cssrc/Altinn.App.Core/Models/Layout/Components/Base/BaseLayoutComponent.cssrc/Altinn.App.Core/Models/Layout/Components/Base/NoReferenceComponent.cssrc/Altinn.App.Core/Models/Layout/Components/Base/ReferenceComponent.cssrc/Altinn.App.Core/Models/Layout/Components/OptionsComponent.cssrc/Altinn.App.Core/Models/Layout/Components/PageComponent.cssrc/Altinn.App.Core/Models/Layout/Components/RepeatingGroupComponent.cssrc/Altinn.App.Core/Models/Layout/Components/SubFormComponent.cssrc/Altinn.App.Core/Models/Layout/LayoutModel.cstest/Altinn.App.Core.Tests/Features/Validators/Default/ExpressionValidatorTests.cstest/Altinn.App.Core.Tests/Features/Validators/Default/LegacyIValidationFormDataTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceOldTests.cstest/Altinn.App.Core.Tests/Features/Validators/ValidationServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/ExpressionTestCaseRoot.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestBackendExclusiveFunctions.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestContextList.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestFunctions.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestInvalid.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/LayoutTestUtils.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/LikertHidden/LikertHiddenTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/RemoveHiddenData/RemoveHiddenDataTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/RepeatingGroupsHidden/RepeatingGroupsHidden.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/SubForm/SubFormTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/DynamicClassBuilder.cstest/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Co-authored-by: Copilot <copilot@github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/SubForm/SubFormTests.cs (1)
417-421:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace finalizer-based mock verification with
IDisposable.Dispose().Using
~SubFormTests()for_appResourcesMock.Verify()and_appMetadataMock.Verify()is nondeterministic—finalizers run on arbitrary threads and can cause flaky or late test failures. xUnit callsDispose()onIDisposabletest classes after each test, providing predictable verification timing. A similar pattern exists intest/Altinn.App.Api.Tests/Controllers/DataController_PatchTests.cs:1115.Suggested fix for SubFormTests
-public class SubFormTests : IClassFixture<DataAnnotationsTestFixture> +public class SubFormTests : IClassFixture<DataAnnotationsTestFixture>, IDisposable { @@ - ~SubFormTests() + public void Dispose() { - _appResourcesMock?.Verify(); - _appMetadataMock?.Verify(); + _appResourcesMock.Verify(); + _appMetadataMock.Verify(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/SubForm/SubFormTests.cs` around lines 417 - 421, The test class currently verifies mocks in the finalizer (~SubFormTests()), which is nondeterministic; change the class to implement IDisposable and move the verification calls into a public void Dispose() method that calls _appResourcesMock?.Verify() and _appMetadataMock?.Verify(), and remove the finalizer. Ensure the class signature and any constructors are updated so xUnit will call Dispose() after each test (mirror the pattern used in DataController_PatchTests).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivConfigResolverTest.cs`:
- Line 237: The test is passing null-forgiving values into
FormDataWrapperFactory.Create which masks nullability and metadata-dependent
bugs; update the two calls that currently pass null! (including the call at the
highlighted location using .ReturnsAsync(FormDataWrapperFactory.Create(model,
null!, null))) to pass the actual test metadata variables modelDataType and
modelDataElement instead so the wrapper mock receives proper
DataType/DataElement values (ensure both occurrences—the one around line 237 and
the similar call around line 364—are changed to
FormDataWrapperFactory.Create(model, modelDataType, modelDataElement)).
In
`@test/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestFormDataWrapperFactory.cs`:
- Line 18: The test currently passes null! for the DataType argument to
FormDataWrapperFactory.Create (calls to FormDataWrapperFactory.Create(model,
null!, null)), which silences nullability but can break if wrapper metadata is
used; update both occurrences (the call at line ~18 and the similar call at line
~38) to supply a concrete DataType instance appropriate for the test (e.g., a
minimal/new DataType or a helper TestDataType object) instead of null!, so the
factory receives a valid DataType and the test remains robust.
In
`@test/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGeneratedGetter.cs`:
- Around line 53-54: Replace repeated null-suppressed metadata arguments with a
shared test DataType: add a single test field (e.g., private static readonly
DataType TestMetadata = new DataType(/* construct required fields */);) in the
TestGeneratedGetter test class and use TestMetadata instead of null! in all
wrapper constructor calls (the places using _skjema, null! for both generated
and reflection wrappers). Update every occurrence referenced in the comment
(lines around the constructor calls) to pass TestMetadata so null-forgiveness is
removed and both generated and reflection wrapper constructors receive the same
concrete metadata instance.
In
`@test/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGeneratedRemoval.cs`:
- Around line 66-67: The tests currently pass null! for the required
non-nullable DataType parameter to constructors like
ReflectionFormDataWrapper(...) and
Altinn_App_SourceGenerator_Integration_Tests_Models_SkjemaFormDataWrapper(...);
replace these null! usages with a minimal valid DataType created by a shared
test helper (e.g., TestDataTypeFactory.CreateDefault()) and update each affected
test file (TestGeneratedRemoval.cs, TestGeneratedCopy.cs, TestAltinnRowIds.cs,
TestGeneratedSetter.cs, TestDataModel.cs) to use that factory; alternatively, if
DataType is truly optional, change the constructor signatures and the DataType
type to DataType? and update the wrapper implementations to handle null safely,
but preferred is creating and passing a simple valid DataType instance via a
reusable test factory.
---
Outside diff comments:
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/SubForm/SubFormTests.cs`:
- Around line 417-421: The test class currently verifies mocks in the finalizer
(~SubFormTests()), which is nondeterministic; change the class to implement
IDisposable and move the verification calls into a public void Dispose() method
that calls _appResourcesMock?.Verify() and _appMetadataMock?.Verify(), and
remove the finalizer. Ensure the class signature and any constructors are
updated so xUnit will call Dispose() after each test (mirror the pattern used in
DataController_PatchTests).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74d569ef-90f4-49e4-bb4d-5e9b515f2c1f
⛔ Files ignored due to path filters (2)
test/Altinn.App.SourceGenerator.Integration.Tests/gen/Altinn.App.Analyzers/Altinn.App.Analyzers.FormDataWrapperGenerator/EmptyFormDataWrapper.g.csis excluded by!**/gen/**test/Altinn.App.SourceGenerator.Integration.Tests/gen/Altinn.App.Analyzers/Altinn.App.Analyzers.FormDataWrapperGenerator/SkjemaFormDataWrapper.g.csis excluded by!**/gen/**
📒 Files selected for processing (38)
src/Altinn.App.Analyzers/SourceTextGenerator/CopyGenerator.cssrc/Altinn.App.Analyzers/SourceTextGenerator/SourceTextGenerator.cssrc/Altinn.App.Api/Controllers/ActionsController.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Api/Helpers/Patch/InternalPatchService.cssrc/Altinn.App.Core/Features/IInstanceDataAccessor.cssrc/Altinn.App.Core/Internal/Data/FormDataWrapperFactory.cssrc/Altinn.App.Core/Internal/Data/IFormDataWrapper.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cssrc/Altinn.App.Core/Internal/Data/ReflectionFormDataWrapper.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorStateInitializer.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskFinalizer.cssrc/Altinn.App.Core/Models/Layout/Components/Base/ReferenceComponent.cstest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivConfigResolverTest.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/Features/Validators/ValidationServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanIncremental.verified.txttest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyIncremental.verified.txttest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/RequiredValidator/RequiredValidatorTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/SubForm/SubFormTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/Test2/FirstPage.jsontest/Altinn.App.Core.Tests/LayoutExpressions/TestDataModel.cstest/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestAddIndexToPath.cstest/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestAltinnRowIds.cstest/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestFormDataWrapperFactory.cstest/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGeneratedCopy.cstest/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGeneratedGetter.cstest/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGeneratedRemoval.cstest/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGeneratedSetter.cstest/Altinn.App.SourceGenerator.Tests/FullTests.Empty#SkjemaFormDataWrapper.g.verified.cstest/Altinn.App.SourceGenerator.Tests/FullTests.Run#SkjemaFormDataWrapper.g.verified.cstest/Altinn.App.SourceGenerator.Tests/FullTests.cstest/Altinn.App.SourceGenerator.Tests/SourceTextGeneratorTests.Generate.verified.cstest/Altinn.App.SourceGenerator.Tests/SourceTextGeneratorTests.GenerateEmpty.verified.cs
✅ Files skipped from review due to trivial changes (4)
- test/Altinn.App.SourceGenerator.Tests/FullTests.Run#SkjemaFormDataWrapper.g.verified.cs
- test/Altinn.App.SourceGenerator.Tests/SourceTextGeneratorTests.GenerateEmpty.verified.cs
- test/Altinn.App.SourceGenerator.Tests/SourceTextGeneratorTests.Generate.verified.cs
- test/Altinn.App.SourceGenerator.Tests/FullTests.Empty#SkjemaFormDataWrapper.g.verified.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs
- src/Altinn.App.Core/Features/IInstanceDataAccessor.cs
- src/Altinn.App.Core/Models/Layout/Components/Base/ReferenceComponent.cs
- test/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/InstanceDataAccessorFake.cs
- src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs
martinothamar
approved these changes
May 21, 2026
Contributor
martinothamar
left a comment
martinothamar
left a comment
There was a problem hiding this comment.
looks like a good improvement to me 👍
|
olavsorl
reviewed
May 22, 2026
Contributor
olavsorl
left a comment
olavsorl
left a comment
There was a problem hiding this comment.
Har bare lest gjennom koden uten å teste funksjonaliteten. La inn noen kommentarer blant annet på ting jeg tror vi kan kvitte oss med og et par kommentarer på dupliserte exception meldinger.
| Dictionary<string, LayoutSetComponent> layoutsLookup | ||
| ) | ||
| { | ||
| throw new NotImplementedException( |
Contributor
There was a problem hiding this comment.
Så det er en breaking change dette?
Contributor
There was a problem hiding this comment.
Hvis det bare skal kastes når den kalles. Så kan man kanskje likegodt bare kvitte seg med metoden?
| { | ||
| throw new ArgumentException( | ||
| $"Attempted to claim child with id {componentId} to component {Id}, but it has already been claimed by {claimedComponent}." | ||
| $"""Attempted to claim child with id "{componentId}" to component "{Id}", but it has already been claimed by "{claimedComponent}".""" |
Contributor
There was a problem hiding this comment.
Suggested change
| $"""Attempted to claim child with id "{componentId}" to component "{Id}", but it has already been claimed by "{claimedComponent}".""" | |
| $"Attempted to claim child with id {componentId} to component {Id}, but it has already been claimed by {claimedComponent}." |
| } | ||
| throw new ArgumentException( | ||
| $"Attempted to claim child with id {componentId} to component {Id}, but the componentId does not exist" | ||
| $"""Attempted to claim child with id "{componentId}" to component "{Id}", but the componentId does not exist""" |
Contributor
There was a problem hiding this comment.
Suggested change
| $"""Attempted to claim child with id "{componentId}" to component "{Id}", but the componentId does not exist""" | |
| $"Attempted to claim child with id {componentId} to component {Id}, but the componentId does not exist" |
| /// Provides access to the instance data accessor, enabling operations and interactions | ||
| /// with the underlying instance data used in layout evaluation and bindings. | ||
| /// </summary> | ||
| public IInstanceDataAccessor DataAccessor { get; } |
Contributor
There was a problem hiding this comment.
Blir litt tydeligere kanskje om man benytter hele navnet her InstanceDataAccessor?
Suggested change
| public IInstanceDataAccessor DataAccessor { get; } | |
| public IInstanceDataAccessor InstanceDataAccessor { get; } |
| ); | ||
| } | ||
| throw new InvalidOperationException($"Data element with type {key.DataType} not found on instance"); | ||
| return new DataReference() { Field = field, DataElementIdentifier = formDataWrapper.DataElement }; |
Contributor
There was a problem hiding this comment.
Utenfor scope av denne PRen, men DataElementIdentifier får jo flere egenskaper når man konstruerer den med DataElement, da får den også DataTypeId. Sikkert gode grunner til at det er gjort sånn, men det er vell ikke helt ideelt?
| /// Provides access to the current <see cref="IInstanceDataAccessor"/> . | ||
| /// </summary> | ||
| public LayoutEvaluatorState State { get; } | ||
| public IInstanceDataAccessor DataAccessor { get; } |
Contributor
There was a problem hiding this comment.
Samme her, litt tydeligere dersom man kaller det InstanceDataAccessor
| internal static IFormDataWrapper Create(object dataModel) | ||
| internal static IFormDataWrapper Create(object dataModel, DataType dataType, DataElement? dataElement) | ||
| { | ||
| if (_dataWrappers.TryGetValue(dataModel.GetType(), out var accessorType)) |
Contributor
There was a problem hiding this comment.
Register blir aldri kalt. Så det ser ut som vi egentlig kan kvitte oss med hele factoryen. Hva med å bare newe ReflectonFormDataWrapper direkte der hvor denne Create metoden blir kalt og fjerne denne klassen?
| public Type BackingDataType => _dataModel.GetType(); | ||
|
|
||
| /// <inheritdoc /> | ||
| public DataType DataType { get; } |
Contributor
There was a problem hiding this comment.
Denne blir vell aldri referert annet enn i konstruktøren og i Copy metoden, kan antagelig fjernes
| var formDataWrapper = await _dataAccessor.GetFormDataWrapper(dataElementId); | ||
| var formDataWrapper = | ||
| await DataAccessor.GetFormDataWrapper(binding, dataElementIdentifier) | ||
| ?? throw new InvalidOperationException($"No data element found for {binding.DataType} {binding.Field}"); |
Contributor
There was a problem hiding this comment.
Det var sikkert ikke meninga at disse exceptionmeldingene skulle være identiske, denne og den på linje 339.
| var formDataWrapper = await _dataAccessor.GetFormDataWrapper(dataElementId); | ||
| var formDataWrapper = | ||
| await DataAccessor.GetFormDataWrapper(binding, context.DataElementIdentifier) | ||
| ?? throw new NullReferenceException($"No data element found for {binding.DataType} {binding.Field}"); |
Contributor
There was a problem hiding this comment.
Det var sikkert ikke meninga at disse exceptionmeldingene skulle være identiske, denne og den på linje 308.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LayoutEvaluatorState is clunky and
IInstanceDataAccessorcan easily incorporate the same functionality.This PR solves the following two issues
DataAccessorproperty onLayoutEvaluatorStateObviously most of the work went into making the tests work again now that state and accessor needs to be initialised from each other.
Verification
Documentation
Summary by CodeRabbit
Refactor
Tests
Bug Fixes