Add Jmespath expression function#1790
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:
📝 WalkthroughWalkthroughThis pull request adds JMESPath query evaluation as a new expression function type. The implementation introduces a NuGet dependency, defines a new enum member, creates a JMESPath evaluator class, adds JSON-to-ExpressionValue conversion, integrates the evaluator into the expression pipeline, and provides comprehensive test coverage with both positive and negative test cases. ChangesJMESPath Expression Function
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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
323dac5 to
85a7a1a
Compare
TomasEng
force-pushed
the
add-jmespath-function
branch
from
85a7a1a to
1ed1350
Compare
|
@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 (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
olavsorl
left a comment
olavsorl
left a comment
There was a problem hiding this comment.
Syns det ser bra ut! Supert at du har lagt til bra med tester også 💯 La inn noen spørsmål og forslag
| }; | ||
| } | ||
|
|
||
| /// <summary>Convert a JsonElement to ExpressionValue.</summary> |
There was a problem hiding this comment.
Ikke så farlig, men lurer på om det er standard at man har linjeskift før og etter taggene. Mao:
| /// <summary>Convert a JsonElement to ExpressionValue.</summary> | |
| /// <summary> | |
| /// Convert a JsonElement to ExpressionValue. | |
| /// </summary> |
There was a problem hiding this comment.
Det har du nok rett i. Jeg må si at jeg ikke er noen tilhenger av disse kommentarene i det hele tatt (navnet og signaturen til funksjonen burde være dokumenterende nok i seg selv), og disse XML-taggene gjør det bare enda mer rotete. Men jeg skjønner at dette er en konvensjon for metoder som er public. Ved å sette alt på én linje tenkte jeg i hvert fall at vi kunne bruke litt mindre vertikal plass, men jeg ser at det heller ikke er etablert praksis.
| return Implementation(_args[0], _args[1].String); | ||
| } | ||
|
|
||
| private static ExpressionValue Implementation(ExpressionValue data, string query) |
There was a problem hiding this comment.
Implementation funker, men du kunne vell også brukt overloading her og kalt den det samme som metoden som kaller den, Evaluate. Syns kanskje det blir hakket bedre, men er jo litt smak og behag dette med navngivning, du får vurdere det selv.
There was a problem hiding this comment.
Her valgte jeg samme format som i Equals, som har en EqualsImplementation etter at parametrene er sjekket, men jeg er enig i at vi bør jobbe mer med navngivningen her.
There was a problem hiding this comment.
Fikset. Valgte å kalle den EvaluateWithValidArguments, så det kommer frem at denne må ha verifiserte parametre.
|
|
||
| namespace Altinn.App.Core.Internal.Expressions; | ||
|
|
||
| internal sealed class JmespathFunctionEvaluator |
There was a problem hiding this comment.
Ønsker vi å begynne å trekke funksjonslogikken ut i egne klasser? Syns ikke det ser ut som vi har gjort det tidligere. Kanskje @ivarne har noen formeninger her.
There was a problem hiding this comment.
Dette er i henhold til enkeltansvarsprinsippet. ExpressionEvaluator er allerede en fryktelig stor klasse, så jeg ville unngå å legge til mer der enn nødvendig. Jeg har gjort det samme med objektfunksjonen. De interne funksjonene i disse nye klassene brukes ikke av noe annet, så det gir heller ikke mening å eksponere dem for hele ExpressionEvaluator-klassen.
There was a problem hiding this comment.
Er enig i at den er litt stor den klassen. Bør vi kanskje lage en egen mappe i Internal/Expression i så fall, sånn at vi ikke blander disse klassene med LayoutEvaluator osv? Kunne kanskje kalt den ExpressionEvaluation feks
There was a problem hiding this comment.
Ja, det er nok ikke dumt. Jeg har faktisk gjort akkurat det i frontend-PR-ene.
There was a problem hiding this comment.
Ja, den er stor, men alternativet er veldig mange veldig små filer som stort sett alltid har fordel av contexten fra de andre metodene.
There was a problem hiding this comment.
Fikset. Kalte mappen FunctionEvaluators.
There was a problem hiding this comment.
Ja, den er stor, men alternativet er veldig mange veldig små filer som stort sett alltid har fordel av contexten fra de andre metodene.
Så ikke denne kommentaren før nå. Ja, når funksjonene har mange felles avhengigheter, er det ikke sikkert at en slik fremgangsmåte er noe særlig til hjelp, men det er jo ikke tilfellet med de klassene jeg har laget nå. De består stort sett bare av private metoder og er bare avhengige av parametrene til uttrykket de skal evaluere. Det er bedre at disse metodene er abstrahert ut i egne klasser enn at de tilfører støy i en klasse som allerede har over 1000 linjer.
|
3 participants
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
New Features
Tests