Support objects in expressions#1777
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 |
5a4dacc to
5c9c571
Compare
2fb14e2 to
ee69ba5
Compare
| #pragma warning disable CA1720 | ||
| @object, | ||
| #pragma warning restore CA1720 |
There was a problem hiding this comment.
Sonar complained about this variable having a name that contains "object", but we need this for the object function to work.
4908de3 to
2a553d2
Compare
2a553d2 to
ab5d0b1
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…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
|
|
||
| namespace Altinn.App.Core.Internal.Expressions; | ||
|
|
||
| internal sealed class ObjectFunctionEvaluator |
There was a problem hiding this comment.
Du laget vell en mappe for disse klassene som vi diskuterte i den andre PRen?
There was a problem hiding this comment.
Ja, den blir opprettet i PR-en for Jmespath. Her har vi bare én slik klasse, så da tenker jeg uansett ikke at det er nødvendig med en egen mappe.
| JsonValueKind.False => "false", | ||
| JsonValueKind.String => String, | ||
| JsonValueKind.Number => Number.ToString(CultureInfo.InvariantCulture), | ||
| // JsonValueKind.Object => JsonSerializer.Serialize(Object), |
There was a problem hiding this comment.
Hva med denne, utenfor scope?
There was a problem hiding this comment.
Ja. Så vidt jeg forstår brukes den når man caster til streng, og det er ikke noe vi støtter for objekter (i hvert fall ikke foreløpig).
|
|
||
| /// <summary>Get the value as an object (or throw if it isn't an object ValueKind)</summary> | ||
| public Dictionary<string, ExpressionValue> Dictionary => | ||
| public JsonObject Dictionary => |
There was a problem hiding this comment.
Bør kanskje rename Dictionary, nå når det ikke er en Dictionary lenger?
There was a problem hiding this comment.
Kalte den det fordi Sonar klagde på "Object". Har du noen andre forslag?
| public ObjectFunctionEvaluator(ExpressionValue[] args) => _args = args; | ||
|
|
||
| public Dictionary<string, ExpressionValue> Evaluate() | ||
| public JsonObject Evaluate() |
There was a problem hiding this comment.
Claude skriver:
LINQ density (style nit): ObjectFunctionEvaluator.Evaluate makes several passes over _args (Where ×2, Distinct, Zip, ToDictionary). Fine for the tiny arg counts expected, but AGENTS.md notes "sometimes a for loop is just as good as LINQ" — a single loop could build the dict, detect odd-count, dup keys, and bad key types in one pass. Optional.
There was a problem hiding this comment.
Jeg kan ikke se hvordan en løkke vil gjøre dette mer oversiktlig. Løkker er vanskelige å lese. Verktøy som CodeQL foreslår jo ofte å bytte ut løkker med nettopp Linq. Jeg kan eventuelt dele opp i flere funksjoner, for eksempel ExtractEvenArguments og ExtractOddArguments for Where-biten.
There was a problem hiding this comment.
La til disse metodene nå. Det ble mer ryddig.
| { | ||
| throw new JsonException("Expected StartObject token."); | ||
| } | ||
| var values = |
There was a problem hiding this comment.
Småpirk, men values i flertallsform henger sikkert igjen fra dette var Dictionary?
| } | ||
| var values = | ||
| JsonSerializer.Deserialize<JsonObject>(ref reader, options) | ||
| ?? throw new JsonException("Expected EndObject token."); |
There was a problem hiding this comment.
Er det riktig å anta at dersom Deserialize returnerer null så mangler endObject token, finnes det ikke andre tilfeller der Deserialize kan returnere null?
There was a problem hiding this comment.
Nei, Deserialize feiler vel uansett hva som det er som gjør at verdien ikke kan omgjøres til JsonObject. Godt sett! Skal gjøre feilmeldingen mer riktig.
Co-authored-by: olavsorl <olav.sorlie@digdir.no>
Co-authored-by: olavsorl <olav.sorlie@digdir.no>
Co-authored-by: olavsorl <olav.sorlie@digdir.no>
|



Important
This pull request is stacked upon #1769, which should be merged first.
Description
This pull request adds support for objects in expressions, along with an
objectfunction for creating an object. Here is an overview of the changes:objectfunction inExpressionEvaluator. Because of the complexity of the function, I created a separate class namedObjectFunctionEvaluatorfor evaluating the function.ExpressionValue. Most of the code did already exist, hiding in comments.Here is the corresponding change in App frontend: Altinn/altinn-studio#18982
Related Issue(s)
Verification
Documentation