Support lists in expressions#1769
Open
TomasEng wants to merge 5 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds end-to-end array/list support: ExpressionValue gains array storage and conversions, JSON (de)serialization handles arrays, ExpressionFunction.list is added and evaluated by ExpressionEvaluator, and tests plus fixtures were added or updated for list behavior and lookups. ChangesArray Value Support in Expression Evaluator
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
3 tasks
TomasEng
force-pushed
the
support-lists-in-expressions
branch
2 times, most recently
from
51411b4 to
3b4b94b
Compare
TomasEng
force-pushed
the
support-lists-in-expressions
branch
from
3b4b94b to
e084500
Compare
TomasEng
added
backport-ignore
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 1
🧹 Nitpick comments (1)
src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs (1)
651-654: ⚡ Quick winDeserialize array directly to avoid an extra allocation.
Line 652deserializes toList<ExpressionValue>and immediately copies withToArray(). Deserializing directly toExpressionValue[]avoids one allocation/copy.Suggested refactor
- var values = - JsonSerializer.Deserialize<List<ExpressionValue>>(ref reader, options) - ?? throw new JsonException("Expected EndArray token."); - return new ExpressionValue(values.ToArray()); + var values = + JsonSerializer.Deserialize<ExpressionValue[]>(ref reader, options) + ?? throw new JsonException("Expected EndArray token."); + return new ExpressionValue(values);As per coding guidelines: "Write efficient code - don't allocate unnecessarily (e.g., avoid calling ToString twice, prefer for loops over LINQ when appropriate)".
🤖 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 `@src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs` around lines 651 - 654, The code in ExpressionValue.cs deserializes into List<ExpressionValue> then calls ToArray(), causing an extra allocation; change the JsonSerializer.Deserialize call to deserialize directly into ExpressionValue[] (i.e., JsonSerializer.Deserialize<ExpressionValue[]>(ref reader, options)) and keep the null-check/JsonException behavior and the return new ExpressionValue(array) using the deserialized array; update the local variable name (e.g., values) as needed.
🤖 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/Internal/Expressions/ExpressionValue.cs`:
- Around line 94-98: Constructor ExpressionValue(ExpressionValue[] value) stores
the caller's array reference into the private field _arrayValue and the array is
also exposed directly elsewhere (when ValueKind == JsonValueKind.Array),
allowing external mutation or a null value to leave the struct in an
inconsistent state; fix by null-guarding and defensively copying: in the
constructor set _arrayValue = (value == null) ? Array.Empty<ExpressionValue>() :
(ExpressionValue[])value.Clone() and ensure ValueKind is set to
JsonValueKind.Array only when the (non-null) copy is assigned, and in the public
accessor that currently exposes the internal array (the member that returns the
array when ValueKind == JsonValueKind.Array) return a copy (e.g.,
_arrayValue.Length == 0 ? Array.Empty<ExpressionValue>() :
(ExpressionValue[])_arrayValue.Clone()) instead of returning the internal
_arrayValue reference.
---
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs`:
- Around line 651-654: The code in ExpressionValue.cs deserializes into
List<ExpressionValue> then calls ToArray(), causing an extra allocation; change
the JsonSerializer.Deserialize call to deserialize directly into
ExpressionValue[] (i.e., JsonSerializer.Deserialize<ExpressionValue[]>(ref
reader, options)) and keep the null-check/JsonException behavior and the return
new ExpressionValue(array) using the deserialized array; update the local
variable name (e.g., values) as needed.
🪄 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: 08d275f1-888a-497c-9f83-05b7c9a6e7ee
📒 Files selected for processing (15)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionValue.cssrc/Altinn.App.Core/Models/Expressions/ExpressionFunction.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/ExpressionTestCaseRoot.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestBackendExclusiveFunctions.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestFunctions.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestInvalid.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/component/lookup-list.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/dataModel/array-is-null.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/dataModel/lookup-list.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/list/list.jsontest/Altinn.App.Core.Tests/LayoutExpressions/ExpressionEvaluatorTests/EqualsTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/ExpressionEvaluatorTests/ExpressionValueTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/TestUtilities/DynamicClassBuilder.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
💤 Files with no reviewable changes (2)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/dataModel/array-is-null.json
- test/Altinn.App.Core.Tests/LayoutExpressions/ExpressionEvaluatorTests/EqualsTests.cs
Contributor
Author
Fixed. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Contributor
Author
|
The Sonar errors on this pull request seem to be related to existing code only. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs (1)
199-210:⚠️ Potential issue | 🔴 CriticalFix/clarify
ExpressionValue.ToObject()array return type contract
ExpressionValue.ToObject()mapsJsonValueKind.Arrayto theArrayproperty, andArrayis typed asExpressionValue[], soToObject()returns the internal wrapper array (not anobject?[]of unwrapped CLR element values). Callers that need CLR-shaped arrays must explicitly unwrap elements (e.g., viaExpressionValue[]→ToObject()per element), or this behavior needs to be clearly documented as the intended API contract.🤖 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 `@src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs` around lines 199 - 210, The ToObject() method currently returns the internal ExpressionValue[] wrapper for JsonValueKind.Array via the Array property, which leaks internal wrappers; change the JsonValueKind.Array branch in ExpressionValue.ToObject() to return a CLR object?[] by unwrapping each element (call ExpressionValue.ToObject() on each item of the Array property and produce an object?[] result) so callers get a native array of element values rather than ExpressionValue instances; ensure you still return null/primitive CLR types for other kinds and keep the method signature as object?.
🤖 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.
Outside diff comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs`:
- Around line 199-210: The ToObject() method currently returns the internal
ExpressionValue[] wrapper for JsonValueKind.Array via the Array property, which
leaks internal wrappers; change the JsonValueKind.Array branch in
ExpressionValue.ToObject() to return a CLR object?[] by unwrapping each element
(call ExpressionValue.ToObject() on each item of the Array property and produce
an object?[] result) so callers get a native array of element values rather than
ExpressionValue instances; ensure you still return null/primitive CLR types for
other kinds and keep the method signature as object?.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df09f1b0-d9ab-443c-9b17-dc4762959fcf
📒 Files selected for processing (1)
src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs
|
ivarne
reviewed
May 29, 2026
| await ExpressionEvaluator.EvaluateExpression( | ||
| state, | ||
| test.Expression, | ||
| (Expression)test.Expression!, |
Member
There was a problem hiding this comment.
I think these should be test.Expression.Value
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
2 participants
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.
Description
This pull request adds support for arrays, along with a
listfunction, in expressions.listfunction is addedExpressionValue(most of the code was already there, hidden in comments)dataModel, of which the new filelookup-list.jsoncontain several test cases)Here is the corresponding change in App Frontend: Altinn/altinn-studio#18974
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Tests