Support lists in expressions#1769
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 labels
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 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 |
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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Fixed. |
|
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! |
|
The Sonar errors on this pull request seem to be related to existing code only. |
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
| await ExpressionEvaluator.EvaluateExpression( | ||
| state, | ||
| test.Expression, | ||
| (Expression)test.Expression!, |
There was a problem hiding this comment.
I think these should be test.Expression.Value
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 (2)
src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs (2)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove unused
using System.Collections;directive — this is causing the pipeline failure.The pipeline reports
IDE0005: Using directive is unnecessaryin this file. After refactoring array storage fromExpressionValue[]toJsonArray, the non-genericSystem.Collectionsnamespace is no longer used.🔧 Proposed fix
-using System.Collections; using System.Diagnostics;🤖 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` at line 1, The IDE0005 pipeline failure is caused by the now-unused using directive "using System.Collections;" in ExpressionValue.cs; remove that using line from the top of the file so only required namespaces remain (leave references for JsonArray/ExpressionValue types intact) and rebuild to confirm IDE0005 is resolved.
286-298:⚠️ Potential issue | 🟠 MajorHandle
JsonValueKind.ArrayinExpressionValuestring conversions (or block it earlier)
ExpressionValue.ToString(),ToStringForText(), andExpressionValue.ToStringForEquals()intentionally have the Object/Array serialization commented out and currently throw forJsonValueKind.Array(InvalidOperationException/NotImplementedException) instead (seeExpressionValue.csaround 286-320 and 329-348).This conflicts with existing expression support:
ExpressionEvaluator’sExpressionFunction.listreturns aJsonArray, while functions likeconcat/containscallToStringForEquals()on their arguments, so a list result used in those contexts will fail. Similarly,TranslationServicerenders text resources viafieldValue.ToStringForText(), so list-valued data model fields will fail during text rendering.Is this deferral intentional until aggregate/list-to-text support is implemented, or should array serialization be added now (e.g., JSON serialization with the existing serializer options) and/or replaced with proper expression type errors instead of throwing?
🤖 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 286 - 298, ExpressionValue.ToString()/ToStringForText()/ToStringForEquals() currently throw on JsonValueKind.Array/Object causing list-valued expressions (e.g., ExpressionEvaluator.ExpressionFunction.list) to break; implement proper serialization instead of throwing: add cases for JsonValueKind.Array and JsonValueKind.Object in ExpressionValue.ToString(), ToStringForText(), and ToStringForEquals() that call JsonSerializer.Serialize(Array/Object, _unsafeSerializerOptionsForSerializingDates) (mirror the String handling) so arrays and objects are converted to stable JSON for text/equals comparisons; keep existing serializer options and error branch for unknown kinds and add unit tests covering list return values used with concat/contains and TranslationService rendering.
🤖 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`:
- Line 1: The IDE0005 pipeline failure is caused by the now-unused using
directive "using System.Collections;" in ExpressionValue.cs; remove that using
line from the top of the file so only required namespaces remain (leave
references for JsonArray/ExpressionValue types intact) and rebuild to confirm
IDE0005 is resolved.
- Around line 286-298:
ExpressionValue.ToString()/ToStringForText()/ToStringForEquals() currently throw
on JsonValueKind.Array/Object causing list-valued expressions (e.g.,
ExpressionEvaluator.ExpressionFunction.list) to break; implement proper
serialization instead of throwing: add cases for JsonValueKind.Array and
JsonValueKind.Object in ExpressionValue.ToString(), ToStringForText(), and
ToStringForEquals() that call JsonSerializer.Serialize(Array/Object,
_unsafeSerializerOptionsForSerializingDates) (mirror the String handling) so
arrays and objects are converted to stable JSON for text/equals comparisons;
keep existing serializer options and error branch for unknown kinds and add unit
tests covering list return values used with concat/contains and
TranslationService rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afe81d00-257b-4270-b229-4b3fe8c515f0
📒 Files selected for processing (2)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs
|
2 participants
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