Feat/set data by expression suggestions#1801
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refines expression-based validation and data-field calculation by simplifying evaluator state initialization, unifying resolved-key behavior, and ensuring expression validation skips null field values.
Changes:
- Removed
ILayoutEvaluatorStateInitializerusage fromExpressionValidatorandDataModelFieldCalculator, relying onIInstanceDataAccessor.GetLayoutEvaluatorState()instead. - Unified resolved-key resolution by removing the
isCalculatingvariants and making key resolution always include resolved keys (even when leaf values are null). - Updated expression evaluation API (
EvaluateExpressionToExpressionValue) to acceptIInstanceDataAccessorandExpressionValue[]positional arguments; added a shared test case for “ignore when value is null”.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs | Removes state initializer dependency; adds explicit null short-circuit before evaluating validation expressions. |
| src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs | Removes state initializer + hidden-field filtering; resolves keys via accessor state and evaluates expressions via new API. |
| src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs | Changes EvaluateExpressionToExpressionValue to take IInstanceDataAccessor and ExpressionValue[]. |
| src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs | Removes GetResolvedKeys(..., isCalculating) overload. |
| src/Altinn.App.Core/Internal/Data/IFormDataWrapper.cs | Removes GetResolvedKeys(..., isCalculating) extension and routes to unified resolution. |
| src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs | Removes isCalculating behavior toggle and adjusts recursion to always resolve leaf keys. |
| test/Altinn.App.Core.Tests/Features/Validators/Default/ExpressionValidatorTests.cs | Updates tests to no longer mock/init layout evaluator state initializer. |
| test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs | Updates calculator tests for new constructor/signature and accessor setup. |
| test/Altinn.App.Core.Tests/Features/Validators/expression-validation-tests/shared/ignore-value-null.json | Adds shared regression test fixture ensuring validations are ignored when value is null. |
| test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt | Updates public API snapshot for removed/changed overloads and new evaluator signature. |
| test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_PdfFails_DataIsUnlocked.verified.txt | Updates verified telemetry output after internal call graph changes. |
| test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_FailingValidator_ReturnsValidationErrors.verified.txt | Updates verified telemetry output after internal call graph changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ExpressionValidation now checks for null by it self, not relying on isCalculating = false. (added a shared test for this)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
ivarne
force-pushed
the
feat/set-data-by-expression-suggestions
branch
from
6144424 to
d536d60
Compare
ivarne
force-pushed
the
feat/set-data-by-expression-suggestions
branch
from
d536d60 to
2054d48
Compare
olavsorl
left a comment
olavsorl
left a comment
There was a problem hiding this comment.
Syns det ser bra ut! Fint at du la til tester for GetResolvedKeys også. Har lagt inn noen kommentarer, fint om du tar en titt. Når det kommer til kommentarene på GetResolvedKeys så var det ting Claude påpekte da jeg ba den om å ta en nærmere titt på metoden. Claude ga også et implementasjonsforslag, som er en sammenstilling av de andre kommentarene på metoden. Jeg la inn implementasjonsforslaget som en egen kommentar, det kan hende du vil se på den først, før du eventuelt begynner å implementere forslagene jeg har lagt inn ellers i metoden.
| currentIndex + 1, | ||
| JoinFieldKeyParts(currentKey, $"{key}[{groupIndex}]"), | ||
| isCalculating | ||
| JoinFieldKeyParts(currentKey, $"{key}[{groupIndex}]") |
There was a problem hiding this comment.
Vi vet vell at groupIndex ikke er null her
| JoinFieldKeyParts(currentKey, $"{key}[{groupIndex}]") | |
| JoinFieldKeyParts(currentKey, $"{key}[{groupIndex.Value}]") |
When resolving keys that end in an enumerable, there are two interpetations that makes sense You either want the key to the list, or the key for each row in the list model.group => ["model.group"] or model.group => ["model.group[0]", "model.group[1]"] To resolve this we disambiguate by having you specify `model.group[]` if you want the latter.
|
ivarne
merged commit 3888591
into
feature/set-data-field-by-expression
github-project-automation
Bot
moved this from 🔎 In review
to ✅ Done
in Team Altinn Studio
Suggested changes to #1683
commit 9b1e757
Date: Mon Jun 8 23:21:16 2026 +0200
commit be58d5d
Date: Mon Jun 8 23:07:20 2026 +0200
commit b8f15ac
Date: Mon Jun 8 15:47:43 2026 +0200
commit 89840fa
Date: Mon Jun 8 15:42:09 2026 +0200