Add Jmespath expression function - #1790
Merged
Merged
Conversation
…to support-objects-in-expressions # Conflicts: # src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs # test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds JMESPath query evaluation as a new expression function backed by the JmesPath.Net NuGet package. Refactors ExpressionValue to store array/object values as raw JSON text, introduces JSON-backed constructors and implicit conversions, and replaces ToObject() calls with explicit ValueKind switches. Introduces JmespathFunctionEvaluator and ObjectFunctionEvaluator, wires JMESPath into the expression evaluator dispatcher, and adds comprehensive test coverage. ChangesJMESPath Expression Function and JSON Representation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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
added
feature
TomasEng
force-pushed
the
add-jmespath-function
branch
2 times, most recently
from
June 2, 2026 12:44
323dac5 to
85a7a1a
Compare
TomasEng
force-pushed
the
add-jmespath-function
branch
from
June 2, 2026 12:53
85a7a1a to
1ed1350
Compare
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 (2)
src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs (1)
206-209: 💤 Low valueFallback expressions throw instead of returning a null value.
Null.String,Null.Dictionary, andNull.Arrayinvoke the.String/.Dictionary/.Arrayaccessors on aNull-kind value, all of which throwInvalidCastException. These branches are currently unreachable (GetString()/Deserializewon't return null for aString/Object/Arraykind), but if they ever were hit they'd throw a misleading cast error rather than yielding a null value. Consider returningExpressionValue.Nullinstead.♻️ Suggested change
- JsonValueKind.String => element.GetString() ?? Null.String, + JsonValueKind.String => element.GetString() ?? Null, JsonValueKind.Number => element.GetDouble(), - JsonValueKind.Object => element.Deserialize<JsonObject>() ?? Null.Dictionary, - JsonValueKind.Array => element.Deserialize<JsonArray>() ?? Null.Array, + JsonValueKind.Object => element.Deserialize<JsonObject>() is { } o ? new ExpressionValue(o) : Null, + JsonValueKind.Array => element.Deserialize<JsonArray>() is { } a ? new ExpressionValue(a) : Null,🤖 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 206 - 209, The switch branches that fall back to Null.String/Null.Dictionary/Null.Array should return the ExpressionValue.Null sentinel instead to avoid invoking the .String/.Dictionary/.Array accessors (which throw InvalidCastException); update the JsonValueKind.String, JsonValueKind.Object and JsonValueKind.Array cases in the ExpressionValue conversion logic to use ExpressionValue.Null as the fallback (instead of Null.String/Null.Dictionary/Null.Array) so a true null-expression value is returned if those fallbacks are ever hit.src/Altinn.App.Core/Internal/Expressions/JmespathFunctionEvaluator.cs (1)
6-6: 💤 Low valueMark the evaluator
sealed.As per coding guidelines, "Use sealed for classes unless inheritance is a valid use-case". This class isn't designed for inheritance.
♻️ Suggested change
-internal class JmespathFunctionEvaluator +internal sealed class JmespathFunctionEvaluator🤖 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/JmespathFunctionEvaluator.cs` at line 6, The class JmespathFunctionEvaluator should be declared sealed to prevent inheritance; update the class declaration for JmespathFunctionEvaluator by adding the sealed modifier (i.e., change the class declaration from "internal class JmespathFunctionEvaluator" to "internal sealed class JmespathFunctionEvaluator") and run the build to ensure no code relies on inheriting from this class.
🤖 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/JmespathFunctionEvaluator.cs`:
- Around line 18-21: The error message in JmespathFunctionEvaluator's argument
validation throws ExpressionEvaluatorTypeErrorException for _args[1] but
mistakenly interpolates _args[0]; update the exception message to reference the
offending argument (_args[1]) and include helpful info (e.g., its ValueKind or
ToString()) so the diagnostic reports the correct query argument when
_args[1].ValueKind != JsonValueKind.String.
---
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs`:
- Around line 206-209: The switch branches that fall back to
Null.String/Null.Dictionary/Null.Array should return the ExpressionValue.Null
sentinel instead to avoid invoking the .String/.Dictionary/.Array accessors
(which throw InvalidCastException); update the JsonValueKind.String,
JsonValueKind.Object and JsonValueKind.Array cases in the ExpressionValue
conversion logic to use ExpressionValue.Null as the fallback (instead of
Null.String/Null.Dictionary/Null.Array) so a true null-expression value is
returned if those fallbacks are ever hit.
In `@src/Altinn.App.Core/Internal/Expressions/JmespathFunctionEvaluator.cs`:
- Line 6: The class JmespathFunctionEvaluator should be declared sealed to
prevent inheritance; update the class declaration for JmespathFunctionEvaluator
by adding the sealed modifier (i.e., change the class declaration from "internal
class JmespathFunctionEvaluator" to "internal sealed class
JmespathFunctionEvaluator") and run the build to ensure no code relies on
inheriting from this class.
🪄 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: c7a28c66-4241-4568-b5dd-f57e42d5908f
📒 Files selected for processing (9)
Directory.Packages.propssrc/Altinn.App.Core/Altinn.App.Core.csprojsrc/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionValue.cssrc/Altinn.App.Core/Internal/Expressions/JmespathFunctionEvaluator.cssrc/Altinn.App.Core/Models/Expressions/ExpressionFunction.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestFunctions.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/jmespath/jmespath.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
TomasEng
marked this pull request as ready for review
June 3, 2026 12:00
ivarne
reviewed
Jun 16, 2026
ivarne
approved these changes
Jun 16, 2026
Co-authored-by: Ivar Nesje <ivarne@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs (1)
739-739: ⚡ Quick winSeal
ExpressionTypeUnionConverterif subclassing is not intended.This converter has no visible inheritance use-case; sealing it aligns with project defaults and avoids accidental extension.
As per coding guidelines: "Use sealed for classes unless inheritance is a valid use-case."
🤖 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 739, The ExpressionTypeUnionConverter class is currently not sealed, which allows it to be subclassed despite having no intended inheritance use-case. Add the sealed keyword to the ExpressionTypeUnionConverter class declaration to prevent accidental extension and align with project coding guidelines that require sealing classes unless inheritance is explicitly needed.Source: Coding guidelines
src/Altinn.App.Core/Internal/Expressions/FunctionEvaluators/ObjectFunctionEvaluator.cs (1)
11-16: ⚡ Quick winBuild the object in a single pass to cut allocations and repeated traversals.
Current flow materializes multiple arrays (
keys,values, even/odd argument arrays) and runs several LINQ passes. You can validate and construct directly in oneforloop.💡 Refactor sketch
public static JsonObject Evaluate(ExpressionValue[] args) { AssertEvenNumberOfArguments(args); - string[] keys = ExtractKeys(args); - AssertKeysAreUnique(keys, args); - JsonNode?[] values = ExtractValues(args); - Dictionary<string, JsonNode?> keyValuePairs = DictionaryFromKeysAndValues(keys, values); - return new JsonObject(keyValuePairs); + JsonObject result = []; + for (var i = 0; i < args.Length; i += 2) + { + if (args[i].ValueKind != JsonValueKind.String) + { + throw new ExpressionEvaluatorTypeErrorException( + "Object keys must be strings.", + ExpressionFunction.@object, + args + ); + } + + var key = args[i].String; + var value = JsonSerializer.SerializeToNode(args[i + 1]); + if (!result.TryAdd(key, value)) + { + throw new ExpressionEvaluatorTypeErrorException( + "Object keys must be unique.", + ExpressionFunction.@object, + args + ); + } + } + + return result; }As per coding guidelines: "Write efficient code - don't allocate unnecessarily (e.g., prefer for loops over LINQ when appropriate)."
Also applies to: 31-69
🤖 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/FunctionEvaluators/ObjectFunctionEvaluator.cs` around lines 11 - 16, Refactor the current multi-pass approach in the object construction logic to use a single for loop iteration instead of separate calls to AssertEvenNumberOfArguments, ExtractKeys, AssertKeysAreUnique, ExtractValues, and DictionaryFromKeysAndValues. Create the Dictionary<string, JsonNode?> directly within one for loop that processes argument pairs (even indices as keys, odd indices as values), performing validation for even count and key uniqueness during the iteration rather than in separate passes. This eliminates unnecessary intermediate array allocations and repeated traversals, replacing the LINQ-based extraction approach with efficient imperative code as per coding guidelines.Source: Coding guidelines
🤖 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 263-265: The ThrowIfNotOfKind method currently hardcodes a
reference to `.JsonObject property` in its error message, but it is called from
multiple accessors including .String, .Number, and .JsonArray. Modify the
ThrowIfNotOfKind method to accept an additional parameter indicating which
property accessor is being accessed (such as a string name or the JsonValueKind
being expected), then update all calls to ThrowIfNotOfKind from the .String,
.Number, and .JsonArray accessors to pass the appropriate context so the error
message correctly identifies which accessor failed and what type was expected.
- Around line 324-342: In the JsonElement property getter, when handling the
JsonValueKind.Object or JsonValueKind.Array case, the JsonDocument created by
JsonDocument.Parse() is never disposed, which retains pooled memory buffers.
Modify the code to clone the RootElement using the JsonElement.Clone() method
instead of directly returning the RootElement, then ensure the JsonDocument is
disposed after cloning. This way the returned JsonElement will be independent
and the underlying document and its pooled memory can be properly released.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/ExpressionEvaluatorTests/ExpressionValueTests.cs`:
- Around line 209-210: The test method contains a duplicate assertion checking
Assert.Equal(JsonValueKind.Null, nullValue.JsonElement.ValueKind) that appears
twice consecutively. Remove the second occurrence of this duplicate assertion to
eliminate test noise and improve code clarity. The first assertion is sufficient
to verify that the nullValue's JsonElement has a ValueKind of Null.
---
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs`:
- Line 739: The ExpressionTypeUnionConverter class is currently not sealed,
which allows it to be subclassed despite having no intended inheritance
use-case. Add the sealed keyword to the ExpressionTypeUnionConverter class
declaration to prevent accidental extension and align with project coding
guidelines that require sealing classes unless inheritance is explicitly needed.
In
`@src/Altinn.App.Core/Internal/Expressions/FunctionEvaluators/ObjectFunctionEvaluator.cs`:
- Around line 11-16: Refactor the current multi-pass approach in the object
construction logic to use a single for loop iteration instead of separate calls
to AssertEvenNumberOfArguments, ExtractKeys, AssertKeysAreUnique, ExtractValues,
and DictionaryFromKeysAndValues. Create the Dictionary<string, JsonNode?>
directly within one for loop that processes argument pairs (even indices as
keys, odd indices as values), performing validation for even count and key
uniqueness during the iteration rather than in separate passes. This eliminates
unnecessary intermediate array allocations and repeated traversals, replacing
the LINQ-based extraction approach with efficient imperative code as per coding
guidelines.
🪄 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: b18ff828-6e2a-40dd-a742-9eb3b585e982
📒 Files selected for processing (14)
Directory.Packages.propssrc/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionValue.cssrc/Altinn.App.Core/Internal/Expressions/FunctionEvaluators/JmespathFunctionEvaluator.cssrc/Altinn.App.Core/Internal/Expressions/FunctionEvaluators/ObjectFunctionEvaluator.cssrc/Altinn.App.Core/Models/Expressions/Expression.cssrc/Altinn.App.Core/Models/Expressions/ExpressionConverter.cssrc/Altinn.App.Core/Models/Expressions/ExpressionFunction.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/TestFunctions.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/jmespath/simple-use-cases.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/jmespath/with-data-model-input.jsontest/Altinn.App.Core.Tests/LayoutExpressions/ExpressionEvaluatorTests/EqualsTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/ExpressionEvaluatorTests/ExpressionValueTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
💤 Files with no reviewable changes (1)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/jmespath/simple-use-cases.json
✅ Files skipped from review due to trivial changes (1)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/jmespath/with-data-model-input.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.App.Core/Models/Expressions/ExpressionFunction.cs
Contributor
Author
|
@ivarne, jeg ser på testdekningen og det er mye av koden her som ikke ser ut til å være i bruk, for eksempel |
ivarne
enabled auto-merge (squash)
June 18, 2026 10:02
github-project-automation
Bot
moved this from 🔎 In review
to ✅ Done
in Team Altinn Studio
|
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
4 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.
Important
This pull request is stacked upon #1777, which should be merged first.
Description
This pull request adds support for Jmespath queries in our expression language. This inludes a new Nuget dependency, JmesPath.Net.
Here is the corresponding implementation in frontend: Altinn/altinn-studio#19028
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
jmespathlayout expression function for querying and transforming JSON data.Breaking Changes
ExpressionValuepublic API:Array/Objecthave been replaced byJsonArray/JsonObject, with related JSON-backed accessors and conversions.Bug Fixes