Open
Conversation
…port # Conflicts: # src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs
Contributor
|
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:
📝 WalkthroughWalkthroughExpressionEvaluator was extended to support arithmetic functions (plus, minus, multiply, divide), numeric parsing and arithmetic helpers updated to use double-based operations with overflow/divide-by-zero checks, ExpressionValue numeric handling adjusted, new enum members added, and comprehensive tests and test utilities were introduced. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
olavsorl
added
feature
![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: 2
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 947-956: In Average, materialize the enumerable returned by
PrepareMultipleNumericArgs (e.g., call ToArray() or ToList() on numericArgs) to
avoid double enumeration, then validate that the materialized collection has at
least one element and handle the empty case (throw an ArgumentException or
similar) before performing the sum and division; keep using
PrepareMultipleNumericArgs and CultureInfo.InvariantCulture so the method uses
the concrete collection count for division and avoids divide-by-zero.
- Around line 936-945: The Divide method currently uses
PrepareMultipleNumericArgs and divides the first value by subsequent args
without checking for zero; update Divide (and use numericArgs.First() and
numericArgs.Skip(1) locations) to detect any divisor equal to 0 before
performing the division and instead raise/return the same kind of user-friendly
expression error used elsewhere (e.g., an Expression evaluation
exception/message) indicating "Division by zero" so it fails gracefully rather
than throwing a raw DivideByZeroException.
🧹 Nitpick comments (6)
src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs (3)
18-19: Stale comment referencesdoubleinstead ofdecimal.The comment on line 18 still mentions "double" but the field is now
decimal. Consider updating for consistency.📝 Suggested comment update
- // double is a value type where nullable takes extra space, and we only read it when it should be set + // decimal is a value type where nullable takes extra space, and we only read it when it should be set private readonly decimal _numberValue = 0;
103-106: XML documentation referencesdoubleinstead ofdecimal.The summary comment should be updated to match the new parameter type.
📝 Suggested documentation update
/// <summary> - /// Convert a nullable double to ExpressionValue + /// Convert a nullable decimal to ExpressionValue /// </summary> public static implicit operator ExpressionValue(decimal? value) => new(value);
134-135: Misleading comments on float/double conversion.The comments state "expressions uses double" but the code is actually casting to
decimal. The comments should reflect the current behavior.📝 Suggested comment update
- float numberValue => (decimal?)numberValue, // expressions uses double which needs an explicit cast - double numberValue => (decimal?)numberValue, // expressions uses double which needs an explicit cast + float numberValue => (decimal?)numberValue, // expressions uses decimal which needs an explicit cast + double numberValue => (decimal?)numberValue, // expressions uses decimal which needs an explicit casttest/Altinn.App.Core.Tests/LayoutExpressions/ExpressionEvaluatorTests/ExpressionValueTests.cs (1)
58-71: Variable namedoubleValueis misleading now that it holds adecimal.Consider renaming to
decimalValueornumberValuefor clarity.📝 Suggested rename
[Fact] - public void TestDouble() + public void TestDecimal() { - decimal doubleValue = 123.456m; - ExpressionValue value = doubleValue; - Assert.Equal(doubleValue, value.ToObject()); - Assert.Equal(doubleValue, value.Number); + decimal decimalValue = 123.456m; + ExpressionValue value = decimalValue; + Assert.Equal(decimalValue, value.ToObject()); + Assert.Equal(decimalValue, value.Number); - value = ExpressionValue.FromObject(doubleValue); - Assert.Equal(doubleValue, value.ToObject()); + value = ExpressionValue.FromObject(decimalValue); + Assert.Equal(decimalValue, value.ToObject()); - Assert.Equal(doubleValue.ToString(CultureInfo.InvariantCulture), value.ToString()); + Assert.Equal(decimalValue.ToString(CultureInfo.InvariantCulture), value.ToString()); Assert.Throws<NotImplementedException>(() => value.GetHashCode()); - // Assert.Equal(doubleValue.GetHashCode(), value.GetHashCode()); + // Assert.Equal(decimalValue.GetHashCode(), value.GetHashCode()); }src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs (2)
829-837: Multiple enumeration issue withIEnumerable<decimal>return type.
PrepareMultipleNumericArgsreturnsIEnumerable<decimal>, but callers likeMinus,Multiply,Divide, andAverageenumerate it multiple times (e.g.,First(),Skip(1),Count()). This causes the LINQ chain to execute repeatedly.Consider materializing to an array or list:
♻️ Proposed fix to avoid multiple enumeration
- private static IEnumerable<decimal> PrepareMultipleNumericArgs(ExpressionValue[] args) + private static decimal[] PrepareMultipleNumericArgs(ExpressionValue[] args) { return args.Select(arg => PrepareNumericArg(arg)) .Select(x => !x.HasValue ? throw new ExpressionEvaluatorTypeErrorException("At least one of the arguments is not a number") : x.Value - ); + ) + .ToArray(); }
907-923: Missing minimum argument validation inPlusandMinus.
PlusandMinusdon't validate that at least one argument is provided. Ifargsis empty:
Plus:Sum()returns 0 (may be acceptable)Minus:First()throwsInvalidOperationExceptionConsider adding argument count validation for consistency with other functions like
AndandOr.🔧 Proposed fix for Minus
private static string Minus(ExpressionValue[] args) { + if (args.Length == 0) + { + throw new ExpressionEvaluatorTypeErrorException("Expected 1+ argument(s), got 0"); + } var numericArgs = PrepareMultipleNumericArgs(args); var sum = numericArgs.First();
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs
Outdated
Show resolved
Hide resolved
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply-multiple-decimals.json`:
- Around line 3-4: Update the expected string for the multiply expression
["multiply", 0.1, 0.2, 15, 1.50] from "0.4500" to "0.45" so it matches the
evaluator's decimal.ToString(CultureInfo.InvariantCulture) output and the
Assert.Equal comparison; locate the test entry with the expression and replace
the expects value accordingly.
...ayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply-multiple-decimals.json
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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)
134-145:⚠️ Potential issue | 🟠 MajorGuard float/double → decimal conversion for non-finite or out-of-range values.
Explicit casts from
float/doubletodecimalthrowOverflowExceptionon NaN, Infinity, or values outside the decimal range. This will surface as an unhandled exception in expression evaluation when user data contains such values. Wrap the conversion with a validation check and throwExpressionEvaluatorTypeErrorException(consistent with error handling patterns in this codebase) to keep failures controlled.Suggested fix
- float numberValue => (decimal?)numberValue, // expressions uses decimal which needs an explicit cast - double numberValue => (decimal?)numberValue, // expressions uses decimal which needs an explicit cast + float numberValue => ValidateAndCastFloatingPoint(numberValue), + double numberValue => ValidateAndCastFloatingPoint(numberValue),private static decimal? ValidateAndCastFloatingPoint(double value) { if (double.IsNaN(value) || double.IsInfinity(value) || value > (double)decimal.MaxValue || value < (double)decimal.MinValue) { throw new ExpressionEvaluatorTypeErrorException($"Cannot convert non-finite or out-of-range number to decimal: {value}"); } return (decimal?)value; }
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 907-974: The Multiply implementation can throw
InvalidOperationException when given zero arguments because numericArgs.First()
is called without a guard; add the same zero-argument check used in
Average/Divide so Multiply throws an ExpressionEvaluatorTypeErrorException with
a clear message (e.g., "At least one argument must be provided") before calling
PrepareMultipleNumericArgs, ensuring failures are consistent and testable;
update the Multiply method to check args.Length == 0 and throw
ExpressionEvaluatorTypeErrorException, then proceed with
PrepareMultipleNumericArgs and the existing multiplication logic.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide-one-argument.json`:
- Line 2: Update the test case title string "Should throw exception when less
then two arguments are provided" to correct the typo by replacing "less then"
with "less than" so it reads "Should throw exception when less than two
arguments are provided"; locate and modify the JSON property value for the test
name in the entry containing that exact string.
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs
Outdated
Show resolved
Hide resolved
...e.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide-one-argument.json
Outdated
Show resolved
Hide resolved
TomasEng
reviewed
Feb 4, 2026
TomasEng
left a comment
TomasEng
left a comment
There was a problem hiding this comment.
Er det slik å forstå at både plus, minus, multiply og divide kan ta imot et ubegrenset antall operander? Det er ikke noe problem i addisjon og multiplikasjon siden de er kommutative, men for subtraksjon og divisjon synes jeg ikke det gir mening med mer enn to operander (én minuend og én subtrahend for subtraksjon, og én dividend og én divisor for divisjon). Er det ytret noe ønske om at de skal ta imot flere?
Er det for øvrig gjort noen vurdering av navngivning på de nye funksjonene? For eksempel er jo både add og sum gode alternativer for plus, mens subtract og difference godt kan brukes i stedet for minus. Jeg har ikke noen umiddelbar formening om hva som er best, men tenker det er lurt å vurdere dette grundig siden det blir vanskelig å endre det senere.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 902-910: Remove the special-case zero-args guard from Minus so it
behaves like Plus/Multiply/Divide and always routes through
PrepareTwoNumericArgs; then update the error message in PrepareTwoNumericArgs
(which currently says "Invalid number of args for compare") to a neutral and
accurate message such as "Invalid number of args for numeric binary operation"
so a single-arg call to Minus yields a correct, non-misleading error.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/minus/subtracting-multiple-decimals.json`:
- Line 2: The test case name string "Should throw exception when more then two
arguments is present" contains a typo; update the test description used in the
JSON (the "name" field) to "Should throw exception when more than two arguments
is present" so it reads "than" instead of "then".
- Line 4: The hardcoded error string in PrepareTwoNumericArgs() wrongly
references "compare" and misleads arithmetic callers; update
PrepareTwoNumericArgs() to return a generic, function-agnostic message (e.g.,
"Invalid number of args" or "Expected 2 arguments") instead of "Invalid number
of args for compare" so callers like Minus, Plus, Multiply, Divide and
comparators (LessThan, GreaterThan) receive a correct message; change the error
text only (preserve validation logic) and run tests that exercise
PrepareTwoNumericArgs(), Minus, and comparison functions to confirm updated
message appears where appropriate.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/adding-multiple-values.json`:
- Around line 2-4: Fix the test metadata: correct the "name" string typo from
"more then two" to "more than two", and update the expectsFailure expectation to
match the corrected production error from PrepareTwoNumericArgs() for the plus
operator by changing the value to "Invalid number of args for plus"; ensure the
expression remains ["plus", 0.1, 0.2, 15, 1.50] so the test asserts the
plus-specific error message.
🧹 Nitpick comments (3)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs (3)
814-826: Error message is misleading for arithmetic callers.
PrepareTwoNumericArgsis now shared by comparison and arithmetic functions, but the error message still says"Invalid number of args for compare". This will confuse users when triggered fromplus,minus,multiply, ordivide.Proposed fix
- private static (decimal?, decimal?) PrepareTwoNumericArgs(ExpressionValue[] args) + private static (decimal?, decimal?) PrepareTwoNumericArgs(ExpressionValue[] args) { if (args.Length != 2) { - throw new ExpressionEvaluatorTypeErrorException("Invalid number of args for compare"); + throw new ExpressionEvaluatorTypeErrorException($"Expected 2 argument(s), got {args.Length}"); }
912-920: Guard partially overlaps withPrepareTwoNumericArgs.The
args.Length <= 1check provides a better error message for 0-1 args, butargs.Length > 2still falls through toPrepareTwoNumericArgswith its generic message. If the intent is exactly-two-args semantics (matching the commit message "changed functions to accept only two arguments"), the guard should check!= 2— or rely solely on the shared helper (especially after updating its error message).
869-869: Consider usingGeneratedRegexfor better performance.This
Regexis allocated once on a static field, but it's not compiled. Since this is on the expression evaluation hot path, using the .NET source generator ([GeneratedRegex]) would give compile-time optimized matching. This would require making the containing classpartial.
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs
Outdated
Show resolved
Hide resolved
...ayoutExpressions/CommonTests/shared-tests/functions/minus/subtracting-multiple-decimals.json
Outdated
Show resolved
Hide resolved
...ayoutExpressions/CommonTests/shared-tests/functions/minus/subtracting-multiple-decimals.json
Outdated
Show resolved
Hide resolved
....Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/adding-multiple-values.json
Outdated
Show resolved
Hide resolved
…d-object-support' into feature/expressions-with-list-and-object-support
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs (1)
843-859: Runtime type check in a generic method is a minor smell but acceptable here.The
typeof(T) != typeof(decimal)branch is documented as intentionally limited todouble/decimal. Consider adding aDebug.Assertor static constraint if the set of supported types ever grows, to prevent silent misuse with otherINumber<T>types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs` around lines 843 - 859, Add a runtime assertion in CastNumber<T> to guard the current assumption that T is only decimal or double: inside the method (before or at the top of the typeof(T) check) add a Debug.Assert or Contract.Requires that typeof(T) == typeof(decimal) || typeof(T) == typeof(double) so misuse with other INumber<T> implementations fails fast; keep the existing EnsureConvertibleToDecimal call and the T.CreateChecked paths unchanged (reference function CastNumber<T>, generic type parameter T, and EnsureConvertibleToDecimal).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.json`:
- Around line 139-143: The test "Should throw exception when any argument is
float min value" uses a float min literal that doesn't symmetrically match the
float max used elsewhere; update the expression's negative float from
-3.402825e+38 to the symmetric -3.4028235e+38 (matching the significand digits
used for float max) so the test uses consistent min/max float values in the
expression ["multiply", 0, -3.4028235E+38].
---
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 843-859: Add a runtime assertion in CastNumber<T> to guard the
current assumption that T is only decimal or double: inside the method (before
or at the top of the typeof(T) check) add a Debug.Assert or Contract.Requires
that typeof(T) == typeof(decimal) || typeof(T) == typeof(double) so misuse with
other INumber<T> implementations fails fast; keep the existing
EnsureConvertibleToDecimal call and the T.CreateChecked paths unchanged
(reference function CastNumber<T>, generic type parameter T, and
EnsureConvertibleToDecimal).
TomasEng
reviewed
Feb 18, 2026
TomasEng
left a comment
TomasEng
left a comment
There was a problem hiding this comment.
Nå var testdekningen mye bedre! Jeg har sett over testene spesielt med fokus på hvordan dette vil fungere i frontend. Jeg har kommentert på noen detaljer i divide-testene, men det samme gjelder stort sett for alle operasjonene.
...ltinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json
Show resolved
Hide resolved
...ltinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json
Show resolved
Hide resolved
...ltinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json
Outdated
Show resolved
Hide resolved
...ltinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json
Show resolved
Hide resolved
.../Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/minus/minus.json
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs (2)
814-819: Optional: include expected/actual counts in thePrepareNumericArgs<T>error message.
"Invalid number of args"omits the expected (2) and actual counts, unlike every other arity guard in this file (e.g.,$"Expected 2 argument(s), got {args.Length}"), making it harder to diagnose at runtime.♻️ Proposed fix
- throw new ExpressionEvaluatorTypeErrorException("Invalid number of args"); + throw new ExpressionEvaluatorTypeErrorException($"Expected 2 argument(s), got {args.Length}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs` around lines 814 - 819, The PrepareNumericArgs<T> method throws a generic "Invalid number of args" when args.Length != 2; update the ExpressionEvaluatorTypeErrorException message to match other guards by including expected and actual counts (e.g., $"Expected 2 argument(s), got {args.Length}"), changing the throw inside PrepareNumericArgs<T> to use that formatted message so callers can see both expected (2) and actual counts.
843-867: Optional: add aDebug.Assertto enforce the "decimal or double only" contract onCastNumber<T>.The non-decimal branch calls
T.CreateChecked(number.Value)without catchingOverflowException. Fordoublethis is safe (no overflow possible), but the constraint is only documented in a comment — nothing prevents a future caller from passing, say,T = float, whereCreateCheckedcan throw an unhandledOverflowException. A compile-time guard isn't possible here, but a debug assertion makes the violation visible early.♻️ Proposed fix
// Only intended to be used in cases where T is either decimal or double private static T? CastNumber<T>(double? number) where T : struct, INumber<T> { + Debug.Assert(typeof(T) == typeof(double) || typeof(T) == typeof(decimal), + "CastNumber<T> is only designed for T = double or T = decimal"); if (typeof(T) != typeof(decimal)) { return number.HasValue ? T.CreateChecked(number.Value) : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs` around lines 843 - 867, Add a Debug.Assert at the start of CastNumber<T> to assert that typeof(T) is either typeof(decimal) or typeof(double) (use System.Diagnostics.Debug.Assert) so callers violating the documented “decimal or double only” contract are caught in debug builds; keep the existing overflow handling for the decimal branch (the catch for OverflowException and throwing ExpressionEvaluatorTypeErrorException) and leave the non-decimal branch behavior unchanged other than the assert.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.json (2)
139-143: Typo in test name: "to" → "too".
"results in a to large number"→"results in a too large number"(same as divide.json line 135).✏️ Proposed fix
- "name": "Should throw exception when string scientific notation results in a to large number", + "name": "Should throw exception when string scientific notation results in a too large number",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.json` around lines 139 - 143, Update the test case "Should throw exception when string scientific notation results in a to large number" by correcting the typo in the "name" field to "Should throw exception when string scientific notation results in a too large number"; locate the JSON object with that exact name (the test whose "expression" is ["multiply", "8e28", 10] and "expectsFailure" is "Expected number, got value \"8e28\"") and change only the wording in the "name" property.
19-23: Missing null-second-operand test.Mirroring the gap in
divide.json:(null, X)→nullis covered but(X, null)is not. Consider adding["multiply", 2.1, null]expectingnullfor completeness.✏️ Suggested addition
{ "name": "Should return null when one of the arguments is null", "expression": ["multiply", null, 2.1], "expects": null }, + { + "name": "Should return null when second argument is null", + "expression": ["multiply", 2.1, null], + "expects": null + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.json` around lines 19 - 23, Add the missing test that asserts multiply returns null when the second operand is null: in the same test group that contains the case named "Should return null when one of the arguments is null", add an additional JSON test with expression ["multiply", 2.1, null] and expects null so both (null, X) and (X, null) cases are covered.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json (2)
134-138: Typo in test name: "to" → "too".
"results in a to large number"should be"results in a too large number".✏️ Proposed fix
- "name": "Should throw exception when string scientific notation results in a to large number", + "name": "Should throw exception when string scientific notation results in a too large number",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json` around lines 134 - 138, Update the test case "Should throw exception when string scientific notation results in a to large number" by fixing the typo in the JSON "name" field to "Should throw exception when string scientific notation results in a too large number"; locate the entry in the divide.json test array (the object with expression ["divide", "8e28", 10]) and edit its "name" value accordingly.
14-23: Missing null-divisor test (non-null dividend, null divisor).All three existing null cases use a
nulldividend:(null, null),(null, 2.1),(null, 0). There is no test for(non-null, null), e.g.,["divide", 2.1, null]. This leaves the divisor-null short-circuit path untested.✏️ Suggested addition
{ "name": "Should return null when one of the arguments is null", "expression": ["divide", null, 2.1], "expects": null }, + { + "name": "Should return null when divisor is null", + "expression": ["divide", 2.1, null], + "expects": null + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json` around lines 14 - 23, Add a test case covering the missing null-divisor scenario for the "divide" function by inserting an entry with expression ["divide", 2.1, null] and expects null so the short-circuit path where the divisor is null is exercised; place this alongside the other divide tests in the existing shared-tests/functions/divide/divide.json test array and follow the same object shape (name, expression, expects).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json`:
- Around line 89-93: The test "Should handle very large number formatted as
string" uses a string operand "9999999999999999" which exceeds JS
Number.MAX_SAFE_INTEGER and will parse differently in JS; update the
"expression" array in this test (the ["divide", ...] entry) to use a stringified
integer within the safe range (<= 9007199254740991) and adjust the "expects"
value to the correct integer division result for that new operand so both JS and
C# will agree (edit the expression and expects fields for this test case).
---
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 814-819: The PrepareNumericArgs<T> method throws a generic
"Invalid number of args" when args.Length != 2; update the
ExpressionEvaluatorTypeErrorException message to match other guards by including
expected and actual counts (e.g., $"Expected 2 argument(s), got {args.Length}"),
changing the throw inside PrepareNumericArgs<T> to use that formatted message so
callers can see both expected (2) and actual counts.
- Around line 843-867: Add a Debug.Assert at the start of CastNumber<T> to
assert that typeof(T) is either typeof(decimal) or typeof(double) (use
System.Diagnostics.Debug.Assert) so callers violating the documented “decimal or
double only” contract are caught in debug builds; keep the existing overflow
handling for the decimal branch (the catch for OverflowException and throwing
ExpressionEvaluatorTypeErrorException) and leave the non-decimal branch behavior
unchanged other than the assert.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json`:
- Around line 134-138: Update the test case "Should throw exception when string
scientific notation results in a to large number" by fixing the typo in the JSON
"name" field to "Should throw exception when string scientific notation results
in a too large number"; locate the entry in the divide.json test array (the
object with expression ["divide", "8e28", 10]) and edit its "name" value
accordingly.
- Around line 14-23: Add a test case covering the missing null-divisor scenario
for the "divide" function by inserting an entry with expression ["divide", 2.1,
null] and expects null so the short-circuit path where the divisor is null is
exercised; place this alongside the other divide tests in the existing
shared-tests/functions/divide/divide.json test array and follow the same object
shape (name, expression, expects).
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.json`:
- Around line 139-143: Update the test case "Should throw exception when string
scientific notation results in a to large number" by correcting the typo in the
"name" field to "Should throw exception when string scientific notation results
in a too large number"; locate the JSON object with that exact name (the test
whose "expression" is ["multiply", "8e28", 10] and "expectsFailure" is "Expected
number, got value \"8e28\"") and change only the wording in the "name" property.
- Around line 19-23: Add the missing test that asserts multiply returns null
when the second operand is null: in the same test group that contains the case
named "Should return null when one of the arguments is null", add an additional
JSON test with expression ["multiply", 2.1, null] and expects null so both
(null, X) and (X, null) cases are covered.
...ltinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs (1)
843-859:CastNumber<T>has a comment-only type restriction with no compile-time enforcement.The behaviour diverges meaningfully across concrete
Ttypes:
T = decimal→ callsEnsureConvertibleToDecimal(range-guarded)T = double→ bypasses the guard entirelyT = anything else(e.g.float,int) →T.CreateCheckedmay throwOverflowException, which is only swallowed insidePerformArithmetic'stry/catch; callers in comparison paths (LessThan, etc.) would surface a rawOverflowException.All current callers use only
doubleordecimal, so this is purely a guard-rails concern.♻️ Suggested refactor — explicit runtime guard
+ // Only intended to be used in cases where T is either decimal or double private static T? CastNumber<T>(double? number) where T : struct, INumber<T> { + if (typeof(T) != typeof(decimal) && typeof(T) != typeof(double)) + { + throw new UnreachableException($"CastNumber<T> is only valid for decimal or double, got {typeof(T)}"); + } if (typeof(T) != typeof(decimal)) { return number.HasValue ? T.CreateChecked(number.Value) : null; } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs` around lines 843 - 859, CastNumber<T> currently assumes T is decimal or double but silently permits any numeric T, causing inconsistent behavior and possible OverflowExceptions; add an explicit runtime guard at the start of CastNumber<T> that throws a clear ArgumentException/NotSupportedException if typeof(T) is not typeof(decimal) and not typeof(double), so callers (e.g., LessThan, PerformArithmetic) won’t unexpectedly get raw overflow errors; keep the existing decimal branch that calls EnsureConvertibleToDecimal and the double branch, but enforce the allowed types up-front with a descriptive error message referencing CastNumber<T>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 843-859: CastNumber<T> currently assumes T is decimal or double
but silently permits any numeric T, causing inconsistent behavior and possible
OverflowExceptions; add an explicit runtime guard at the start of CastNumber<T>
that throws a clear ArgumentException/NotSupportedException if typeof(T) is not
typeof(decimal) and not typeof(double), so callers (e.g., LessThan,
PerformArithmetic) won’t unexpectedly get raw overflow errors; keep the existing
decimal branch that calls EnsureConvertibleToDecimal and the double branch, but
enforce the allowed types up-front with a descriptive error message referencing
CastNumber<T>.
…l, doing arithmetic operation and then converting it back to double.
…d-object-support' into feature/expressions-with-list-and-object-support
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-coefficient.json (1)
1-5: Test name is misleading.Same issue as the greaterThanEq equivalent: the test name mentions "string scientific notation" but
"10"is a plain string number, and-5E2is a JSON number literal, not a string. Consider clarifying the test name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-coefficient.json` around lines 1 - 5, The test name "Should parse string scientific notation with negative sign in front of coefficient (true)" is misleading because the expression uses a plain string "10" and a numeric literal -5E2; update the "name" field to accurately describe the inputs (for example: "Should compare plain numeric string to negative scientific numeric literal (true)") and ensure the description references the expression array ["greaterThan", "10", -5E2] so it clearly reflects that only the second operand is a scientific-notation numeric literal and the first is a plain string.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.json (1)
1-5: Test name is misleading.The test name states "string scientific notation" but
"10"is just a plain string number, not scientific notation. The scientific notation value-5E2is actually a JSON number literal, not a string. Consider renaming to clarify what's being tested, e.g., "Should compare string number against negative scientific notation number (true)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.json` around lines 1 - 5, The test's "name" field is misleading; update the JSON "name" value in this test (the top-level "name" property adjacent to "expression" and "expects") to something clearer such as "Should compare string number against negative scientific notation number (true)" so it accurately describes that the left operand is a plain string "10" and the right operand is a negative scientific-notation numeric literal -5E2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 1012-1023: The overflow catch in ExpressionEvaluator.cs currently
throws ExpressionEvaluatorTypeErrorException with a grammatically awkward
message; update the exception message to a clear, user-facing sentence such as
"Arithmetic overflow: operation on {a.Value} and {b.Value} exceeds the supported
range." Replace the existing message string in the catch block (where
ExpressionEvaluatorTypeErrorException is thrown) with the improved wording,
keeping interpolation of a.Value and b.Value.
---
Nitpick comments:
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-coefficient.json`:
- Around line 1-5: The test name "Should parse string scientific notation with
negative sign in front of coefficient (true)" is misleading because the
expression uses a plain string "10" and a numeric literal -5E2; update the
"name" field to accurately describe the inputs (for example: "Should compare
plain numeric string to negative scientific numeric literal (true)") and ensure
the description references the expression array ["greaterThan", "10", -5E2] so
it clearly reflects that only the second operand is a scientific-notation
numeric literal and the first is a plain string.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.json`:
- Around line 1-5: The test's "name" field is misleading; update the JSON "name"
value in this test (the top-level "name" property adjacent to "expression" and
"expects") to something clearer such as "Should compare string number against
negative scientific notation number (true)" so it accurately describes that the
left operand is a plain string "10" and the right operand is a negative
scientific-notation numeric literal -5E2.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (63)
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/shared-tests/functions/divide/divide.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-negative-exponents.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-numbers.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-with-regular.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-basic.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-no-whole.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-plus-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-plus-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-zero.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-negative-exponents.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-numbers.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-with-regular.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-basic.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-no-whole.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-plus-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-plus-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-zero.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-notation-negative-exponents.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-notation-numbers.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-notation-with-regular.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-basic.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-negative-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-no-whole.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-plus-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-plus-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-zero.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-negative-exponents.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-numbers.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-with-regular.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-basic.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-negative-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-no-whole.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-plus-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-plus-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-zero.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/minus/minus.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/plus.json
✅ Files skipped from review due to trivial changes (1)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.json
…d-object-support' into feature/expressions-with-list-and-object-support
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs (1)
1021-1023: Ungrammatical overflow error message (previously flagged).
"causes supported range to be exceeds"is still grammatically incorrect. The suggested fix from the earlier review remains unapplied.✍️ Suggested wording fix
- $"Arithmetic overflow: {a.Value} and {b.Value} or operation on them causes supported range to be exceeds" + $"Arithmetic overflow: result of operation on {a.Value} and {b.Value} exceeds the supported range"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs` around lines 1021 - 1023, The exception message thrown by ExpressionEvaluatorTypeErrorException in ExpressionEvaluator.cs is ungrammatical; update the string to a clear, grammatical message such as "Arithmetic overflow: {a.Value} and {b.Value} or an operation on them causes the value to exceed the supported range." Locate the throw that constructs ExpressionEvaluatorTypeErrorException and replace the existing message text with the corrected wording (ensuring interpolation placeholders {a.Value} and {b.Value} are preserved).test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-exponent.json (1)
1-1:⚠️ Potential issue | 🟡 MinorSame UTF-8 BOM issue as
error-sci-not-string-format-2.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-exponent.json` at line 1, This JSON file contains a UTF-8 BOM at the start (same issue as error-sci-not-string-format-2.json); open the file referenced (error-sci-not-string-no-exponent.json) and remove the BOM so it starts with '{', then save it encoded as UTF-8 without BOM; ensure related test JSON fixtures in the shared-tests/functions/lessThan directory follow the same encoding to avoid duplicate BOM-related failures.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-coefficient.json (1)
1-1:⚠️ Potential issue | 🟡 MinorSame UTF-8 BOM issue as
error-sci-not-string-format-2.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-coefficient.json` at line 1, This JSON file begins with a UTF-8 BOM which breaks parsers; open error-sci-not-string-no-coefficient.json, remove the leading BOM character (the U+FEFF at the file start) so the file starts with '{' and save it as UTF-8 without BOM; validate the file parses as JSON after the change.
🧹 Nitpick comments (8)
test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-plus-exponent.json (1)
1-5: LGTM — test logic is correct.
"5E+2"parses to 500, and 500 > 400 istrue. The name is descriptive and the structure matches the existing test convention.One optional addition: a complementary
falsecase (e.g.,["greaterThan", "5E+2", 600]) in a paired file would make the coverage symmetric with what olavsorl described for theminussuite (covering both branches).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-plus-exponent.json` around lines 1 - 5, Add a complementary false-case test to mirror this true-case: create a test with the same structure but with expression ["greaterThan", "5E+2", 600] (or similar) and expects false so the suite covers both branches; match naming convention (e.g., "Should parse string scientific notation with plus sign in front of exponent (false)"), keep JSON structure and placement consistent with other paired tests in the greaterThan/minus suites.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-basic.json (1)
1-5: LGTM — but coverage gaps worth closing: missingfalsecase and negative-exponent variant.
"5E2"(500) > 400 →trueis correct. However, per the discussion thread, per-function scientific-notation coverage should include:
- A
falsecounterpart (e.g.,"5E2"vs 600), so both branches of the comparison are exercised.- A negative-exponent string (e.g.,
"5E-2"= 0.05), since the parsing path for negative exponents differs and can surface conversion bugs separately from positive ones.📝 Suggested companion files
sci-string-basic-false.json+{ + "name": "Should parse string scientific notation (false)", + "expression": ["greaterThan", "5E2", 600], + "expects": false +}
sci-string-negative-exp.json+{ + "name": "Should parse string negative scientific notation", + "expression": ["greaterThan", "5E-2", 0], + "expects": true +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-basic.json` around lines 1 - 5, Add two complementary tests to close coverage: create a false-case test mirroring the existing "Should parse string scientific notation (true)" but comparing ["greaterThan", "5E2", 600] with expects false (suggested filename sci-string-basic-false.json) and a negative-exponent test that asserts ["greaterThan", "5E-2", 0.01] (or ["greaterThan", "5E-2", 0.05] depending on intended comparison) with the appropriate expects value (suggested filename sci-string-negative-exp.json); ensure each JSON includes the same keys ("name", "expression", "expects") and that the "expression" strings exactly match the scientific notation strings used in the comment so the parser path for negative exponents is exercised.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-with-regular.json (1)
3-3: Consider adding a negative-exponent variant (e.g.,1e-2).The PR discussion explicitly calls out both positive (
3E2) and negative (3E-2) exponent notation as desired coverage. This file only exercises the positive-exponent path. A sibling file such assci-notation-negative-exp-with-regular.jsonusing["greaterThan", 1e-2, 0](expectstrue) would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-with-regular.json` at line 3, Add a sibling test exercising negative-exponent scientific notation for the "greaterThan" expression: create a file (e.g., sci-notation-negative-exp-with-regular.json) with an "expression" array using ["greaterThan", 1e-2, 0] and the appropriate expected result (true), so the negative-exponent path is covered alongside the existing ["greaterThan", 1e2, 50] case.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-negative-exponent.json (2)
1-5: Math and JSON are correct; also consider adding an uppercase-Esibling
"5e-2"→0.05 ≤ 10→true✓. The PR discussion specifically calls out3E-2(uppercaseE) as a format to cover. A sibling file testing"5E-2"would close that gap forlessThanEq.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-negative-exponent.json` around lines 1 - 5, Add a sibling test covering uppercase scientific notation: copy this test (name referencing "Should parse string scientific notation with negative sign in front of exponent (true)" and the "expression" using "lessThanEq") and change the numeric string from "5e-2" to "5E-2" so the test asserts ["lessThanEq", "5E-2", 10] expects true; ensure the new file name mirrors the original but indicates uppercase E to make it discoverable.
2-2: Test name slightly ambiguous — "negative sign in front of exponent" vs. negative exponent"negative sign in front of exponent" can be misread as a negative sign on the base (e.g.,
-5e2), whereas the intent is a negative exponent (5e-2). The file namesci-string-negative-exponentis already more precise. Consider aligning the test name:📝 Suggested rename
- "name": "Should parse string scientific notation with negative sign in front of exponent (true)", + "name": "Should parse string scientific notation with negative exponent (true)",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-negative-exponent.json` at line 2, Rename the ambiguous test "Should parse string scientific notation with negative sign in front of exponent (true)" to a clearer label that reflects a negative exponent, e.g., "Should parse string scientific notation with negative exponent (true)"; update the "name" field in the JSON (the value for the "name" key) to match the file name and intent (sci-string-negative-exponent) so the test description and filename are aligned.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-with-regular.json (1)
2-4: Consider adding a companion test with scientific notation in the first operand position.This test only validates sci notation in argument position 2 (
["lessThanEq", 50, 1e2]). A test such as["lessThanEq", 1e2, 200]→truewould confirm both operand positions parse correctly through the sameDecimalpath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-with-regular.json` around lines 2 - 4, Add a companion test to verify scientific notation in the first operand by duplicating the existing case "Should work with scientific notation and regular number (true)" but swap operands so the expression is ["lessThanEq", 1e2, 200] with expects true and an updated name like "Should work with scientific notation in first operand (true)"; this ensures the same Decimal parsing path is exercised for the first argument as is used for the second.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-numbers.json (1)
1-5: Consider adding an equality boundary test forgreaterThanEqwith scientific notation.This test and its
greaterThancounterpart are structurally identical (5e3 op 2e3 → true). The defining semantic ofgreaterThanEq— returningtruewhen both sides are equal — is not exercised by any of the new sci-notation files. A case like["greaterThanEq", 5e3, 5e3]expectingtruewould close this gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-numbers.json` around lines 1 - 5, Add an equality-boundary test for the greaterThanEq operator using scientific notation: create a test case (in the same test group/file pattern as sci-notation-numbers.json) where the expression is ["greaterThanEq", 5e3, 5e3] and expects true so the operator's equality behavior is covered; target the file containing the current sci-notation tests (shared-tests/functions/greaterThanEq/sci-notation-numbers.json) and follow the same JSON structure and naming conventions as the existing test.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-coefficient.json (1)
1-5: Test logic and error message are correct — LGTM.
"e2"correctly represents a missing-coefficient case, and the failure message matches expected evaluator output.Nit: This file (and several others in this PR:
error-sci-not-string-format-1.json,error-sci-not-string-format-2.json,error-sci-not-string-no-exponent.json, and thegreaterThanequivalents) begins with a UTF-8 BOM (\uFEFF), whilesci-string-negative-coefficient.jsonandsci-string-plus-coefficient.jsondo not. Consider stripping the BOM for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-coefficient.json` around lines 1 - 5, Remove the leading UTF-8 BOM from this JSON test file so its encoding matches the other test files; open error-sci-not-string-no-coefficient.json (and the sibling files error-sci-not-string-format-1.json, error-sci-not-string-format-2.json, error-sci-not-string-no-exponent.json, and the greaterThan equivalents) and strip the \uFEFF character at the start of the file, saving them as plain UTF-8 without BOM so the test suite treats them consistently with sci-string-negative-coefficient.json and sci-string-plus-coefficient.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 914-922: The current Divide method uses b == 0 to guard division
but tiny non-zero doubles can round to 0m inside PerformArithmetic and trigger
an unhandled DivideByZeroException; update PerformArithmetic to catch
DivideByZeroException (in addition to OverflowException) and rethrow a
consistent ExpressionEvaluatorTypeErrorException with a clear "cannot divide by
0" message so callers like Divide (which calls PrepareNumericArgs and
PerformArithmetic) get a handled, user-friendly error; you may also consider
tightening the Divide check by treating very small magnitude b as zero, but the
immediate fix is to handle DivideByZeroException inside PerformArithmetic.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-coefficient.json`:
- Around line 3-4: The test's second operand in the "expression" array is a bare
JSON number (-5E2) rather than the intended string scientific notation; update
the "expression" entry so the second element is the string "-5E2" (i.e., replace
-5E2 with "-5E2") so the string-parsing path for negative-coefficient scientific
notation is exercised.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.json`:
- Line 3: The test's "expression" currently uses a bare numeric literal (-5E2)
so the JSON parser produces a number and never exercises the string-parsing
path; update the "expression" array so the scientific-notation negative
coefficient is a string (e.g., "-5E2") instead of a bare number so the evaluator
triggers the string-to-number parsing code (locate the "expression" array in the
test file and replace the numeric -5E2 element with the string "-5E2").
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-2.json`:
- Line 1: Remove the leading UTF-8 BOM (U+FEFF) from the start of the JSON file
so it begins with '{'—specifically edit error-sci-not-string-format-2.json (and
similarly error-sci-not-string-no-exponent.json and
error-sci-not-string-no-coefficient.json if present) to remove the BOM character
that some editors inserted, ensuring the files start with a plain ASCII '{'
consistent with the other test JSON files.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-1.json`:
- Line 1: The JSON test files with names matching error-*.json in the
shared-tests/functions/lessThanEq folder contain a UTF-8 BOM; remove the BOM by
re-saving these files as UTF-8 without BOM (e.g., via your editor’s “Save as
UTF-8 (no BOM)” option or using a tool like iconv/recode) so the files become
plain UTF-8 and eliminate diff noise and tooling issues; update the three
error-*.json files accordingly and commit the changed files.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-2.json`:
- Line 1: The file starts with a UTF-8 BOM (U+FEFF) which must be removed; open
the JSON file named error-sci-not-string-format-2.json and save it without the
BOM (ensure the first character is '{' not the BOM), e.g., re-encode or strip
the BOM in your editor/CI so the JSON begins directly with '{' to comply with
RFC 8259 and avoid frontend parser issues.
---
Duplicate comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 1021-1023: The exception message thrown by
ExpressionEvaluatorTypeErrorException in ExpressionEvaluator.cs is
ungrammatical; update the string to a clear, grammatical message such as
"Arithmetic overflow: {a.Value} and {b.Value} or an operation on them causes the
value to exceed the supported range." Locate the throw that constructs
ExpressionEvaluatorTypeErrorException and replace the existing message text with
the corrected wording (ensuring interpolation placeholders {a.Value} and
{b.Value} are preserved).
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-coefficient.json`:
- Line 1: This JSON file begins with a UTF-8 BOM which breaks parsers; open
error-sci-not-string-no-coefficient.json, remove the leading BOM character (the
U+FEFF at the file start) so the file starts with '{' and save it as UTF-8
without BOM; validate the file parses as JSON after the change.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-exponent.json`:
- Line 1: This JSON file contains a UTF-8 BOM at the start (same issue as
error-sci-not-string-format-2.json); open the file referenced
(error-sci-not-string-no-exponent.json) and remove the BOM so it starts with
'{', then save it encoded as UTF-8 without BOM; ensure related test JSON
fixtures in the shared-tests/functions/lessThan directory follow the same
encoding to avoid duplicate BOM-related failures.
---
Nitpick comments:
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-with-regular.json`:
- Line 3: Add a sibling test exercising negative-exponent scientific notation
for the "greaterThan" expression: create a file (e.g.,
sci-notation-negative-exp-with-regular.json) with an "expression" array using
["greaterThan", 1e-2, 0] and the appropriate expected result (true), so the
negative-exponent path is covered alongside the existing ["greaterThan", 1e2,
50] case.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-basic.json`:
- Around line 1-5: Add two complementary tests to close coverage: create a
false-case test mirroring the existing "Should parse string scientific notation
(true)" but comparing ["greaterThan", "5E2", 600] with expects false (suggested
filename sci-string-basic-false.json) and a negative-exponent test that asserts
["greaterThan", "5E-2", 0.01] (or ["greaterThan", "5E-2", 0.05] depending on
intended comparison) with the appropriate expects value (suggested filename
sci-string-negative-exp.json); ensure each JSON includes the same keys ("name",
"expression", "expects") and that the "expression" strings exactly match the
scientific notation strings used in the comment so the parser path for negative
exponents is exercised.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-plus-exponent.json`:
- Around line 1-5: Add a complementary false-case test to mirror this true-case:
create a test with the same structure but with expression ["greaterThan",
"5E+2", 600] (or similar) and expects false so the suite covers both branches;
match naming convention (e.g., "Should parse string scientific notation with
plus sign in front of exponent (false)"), keep JSON structure and placement
consistent with other paired tests in the greaterThan/minus suites.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-coefficient.json`:
- Around line 1-5: Remove the leading UTF-8 BOM from this JSON test file so its
encoding matches the other test files; open
error-sci-not-string-no-coefficient.json (and the sibling files
error-sci-not-string-format-1.json, error-sci-not-string-format-2.json,
error-sci-not-string-no-exponent.json, and the greaterThan equivalents) and
strip the \uFEFF character at the start of the file, saving them as plain UTF-8
without BOM so the test suite treats them consistently with
sci-string-negative-coefficient.json and sci-string-plus-coefficient.json.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-numbers.json`:
- Around line 1-5: Add an equality-boundary test for the greaterThanEq operator
using scientific notation: create a test case (in the same test group/file
pattern as sci-notation-numbers.json) where the expression is ["greaterThanEq",
5e3, 5e3] and expects true so the operator's equality behavior is covered;
target the file containing the current sci-notation tests
(shared-tests/functions/greaterThanEq/sci-notation-numbers.json) and follow the
same JSON structure and naming conventions as the existing test.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-with-regular.json`:
- Around line 2-4: Add a companion test to verify scientific notation in the
first operand by duplicating the existing case "Should work with scientific
notation and regular number (true)" but swap operands so the expression is
["lessThanEq", 1e2, 200] with expects true and an updated name like "Should work
with scientific notation in first operand (true)"; this ensures the same Decimal
parsing path is exercised for the first argument as is used for the second.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-negative-exponent.json`:
- Around line 1-5: Add a sibling test covering uppercase scientific notation:
copy this test (name referencing "Should parse string scientific notation with
negative sign in front of exponent (true)" and the "expression" using
"lessThanEq") and change the numeric string from "5e-2" to "5E-2" so the test
asserts ["lessThanEq", "5E-2", 10] expects true; ensure the new file name
mirrors the original but indicates uppercase E to make it discoverable.
- Line 2: Rename the ambiguous test "Should parse string scientific notation
with negative sign in front of exponent (true)" to a clearer label that reflects
a negative exponent, e.g., "Should parse string scientific notation with
negative exponent (true)"; update the "name" field in the JSON (the value for
the "name" key) to match the file name and intent (sci-string-negative-exponent)
so the test description and filename are aligned.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (63)
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/shared-tests/functions/divide/divide.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-negative-exponents.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-numbers.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-notation-with-regular.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-basic.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-no-whole.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-plus-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-plus-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-zero.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-negative-exponents.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-numbers.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-notation-with-regular.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-basic.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-no-whole.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-plus-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-plus-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-zero.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-notation-negative-exponents.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-notation-numbers.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-notation-with-regular.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-basic.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-negative-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-no-whole.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-plus-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-plus-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/sci-string-zero.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-negative-exponents.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-numbers.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-with-regular.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-basic.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-negative-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-no-whole.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-plus-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-plus-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-string-zero.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/minus/minus.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/plus.json
✅ Files skipped from review due to trivial changes (4)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-format-2.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-coefficient.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-1.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/sci-notation-numbers.json
🚧 Files skipped from review as they are similar to previous changes (3)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json
- src/Altinn.App.Core/Models/Expressions/ExpressionFunction.cs
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/plus.json
...ressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-coefficient.json
Outdated
Show resolved
Hide resolved
...ssions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.json
Outdated
Show resolved
Hide resolved
...utExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-2.json
Outdated
Show resolved
Hide resolved
...Expressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-1.json
Outdated
Show resolved
Hide resolved
...Expressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-2.json
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs (1)
914-922:⚠️ Potential issue | 🟡 Minor
DivideByZeroExceptionfrom subnormal divisors is still unhandled.A very small but non-zero
double(magnitude below~1e-28) passes theb == 0guard on line 917 but rounds to0mwhen cast todecimalinsidePerformArithmetic, producing an uncaughtDivideByZeroException. Thecatchblock only handlesOverflowException.The simplest fix is to also catch
DivideByZeroExceptioninPerformArithmetic:Proposed fix
catch (OverflowException) { throw new ExpressionEvaluatorTypeErrorException( $"Arithmetic overflow: {a.Value} and {b.Value} or operation on them exceeds the supported range" ); } + catch (DivideByZeroException) + { + throw new ExpressionEvaluatorTypeErrorException( + "The second argument is 0, cannot divide by 0" + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs` around lines 914 - 922, The Divide implementation allows very small non-zero doubles that become zero when cast to decimal, causing an unhandled DivideByZeroException inside PerformArithmetic; update PerformArithmetic to also catch DivideByZeroException (in addition to OverflowException) and rethrow a suitable ExpressionEvaluatorTypeErrorException (or convert it to the existing error path) so division by effectively-zero operands is handled; reference the Divide, PrepareNumericArgs, and PerformArithmetic methods when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Around line 914-922: The Divide implementation allows very small non-zero
doubles that become zero when cast to decimal, causing an unhandled
DivideByZeroException inside PerformArithmetic; update PerformArithmetic to also
catch DivideByZeroException (in addition to OverflowException) and rethrow a
suitable ExpressionEvaluatorTypeErrorException (or convert it to the existing
error path) so division by effectively-zero operands is handled; reference the
Divide, PrepareNumericArgs, and PerformArithmetic methods when making this
change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/sci-string-negative-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/sci-string-negative-coefficient.json
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-exponent.json (1)
3-4: Consider adding a companion test file for the "e2" (no-mantissa) variant to improve scientific notation error coverage.The current test correctly covers the "no-exponent" case (
"5e"— mantissa present, exponent missing). A sibling test for"e2"(exponent present, mantissa missing) would provide complementary coverage of invalid scientific notation formats.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-exponent.json` around lines 3 - 4, Add a sibling test JSON that mirrors this one but uses the "e2" invalid scientific-notation input (i.e., set "expression": ["greaterThan", "e2", 10]) and update the expected failure message accordingly (e.g., "expectsFailure": "Expected number, got value \"e2\""); name the file consistently with the existing pattern so it runs alongside error-sci-not-string-no-exponent.json and verifies the missing-mantissa variant is covered.src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs (1)
1013-1013: Nitpick: prefer(double)resultover(double?)resultfor clarity.
resultis a non-nullabledecimal; casting it todouble?works via C#'s lifted explicit conversion, but returning(double)resultand letting thedouble?return type handle the implicit lift is more idiomatic and signals intent more clearly.✨ Suggested simplification
- return (double?)result; + return (double)result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs` at line 1013, The return casts a non-nullable decimal "result" to (double?) which relies on lifted conversion; change the cast to (double)result and let the method's double? return type implicitly lift it—update the return in the ExpressionEvaluator method that currently does "return (double?)result" to "return (double)result" to make the intent clearer and more idiomatic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs`:
- Line 1013: The return casts a non-nullable decimal "result" to (double?) which
relies on lifted conversion; change the cast to (double)result and let the
method's double? return type implicitly lift it—update the return in the
ExpressionEvaluator method that currently does "return (double?)result" to
"return (double)result" to make the intent clearer and more idiomatic.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-exponent.json`:
- Around line 3-4: Add a sibling test JSON that mirrors this one but uses the
"e2" invalid scientific-notation input (i.e., set "expression": ["greaterThan",
"e2", 10]) and update the expected failure message accordingly (e.g.,
"expectsFailure": "Expected number, got value \"e2\""); name the file
consistently with the existing pattern so it runs alongside
error-sci-not-string-no-exponent.json and verifies the missing-mantissa variant
is covered.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cstest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-exponent.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-1.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-2.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-no-coefficient.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-no-exponent.json
✅ Files skipped from review due to trivial changes (2)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-no-coefficient.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-no-exponent.json
🚧 Files skipped from review as they are similar to previous changes (10)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-format-1.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-exponent.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-no-exponent.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-no-coefficient.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThanEq/error-sci-not-string-format-2.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/greaterThan/error-sci-not-string-format-2.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-1.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-format-2.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThanEq/error-sci-not-string-no-coefficient.json
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/lessThan/error-sci-not-string-format-2.json
TomasEng
approved these changes
Feb 24, 2026
TomasEng
left a comment
TomasEng
left a comment
There was a problem hiding this comment.
Jeg går god for testene, men se gjerne på kommentaren jeg la til. Fint at du også har lagt til flere tester på sammenligningsoperasjonene, så vi får verifisert at vi støtter de samme tallformatene overalt.
Et lite innspill til slutt: Jeg ser at alle testfilene for de aritmestiske uttrykkene inneholder minst én operasjon som ville feilet om vi ikke brukte decimal (0.1 + 0.2, 50.8 - 20.3, 0.1 * 0.2, 0.00000001 / 0.0001), men kanskje vi i tillegg burde ha én test i hver fil som spesifiserer dette eksplisitt? Eksempel:
{
"name": "Returns expected value for inputs that would return different result when treated as 64-bit binary floats",
"expression": ["plus", 0.1, 0.2],
"expects": 0.3
}Da blir det veldig tydelig hvorfor vi har tatt i bruk decimal, og vi unngår at dekningen blir fjernet som følge av at noen endrer testdata i en test som tester noe annet.
Ellers regner jeg med at @ivarne også kan tenke seg å ta en ny kikk. Til info: Vi startet med å se på en løsning hvor vi konverterer tallet direkte til decimal dersom det er en streng, men dette har vi gått bort fra. I løsningen vi har nå behandler vi inndataen som double uavhengig av format, og konverterer til decimal kun for å utføre operasjonen. Begrunnelsen bak det er at vi uansett mister presisjonen vi får med decimal når vi returnerer en double. (Hadde vi returnert decimal, ville det vært en annen sak, men da ville vi fått problemer med serialisering i frontend). Da er det bedre å si at vi behandler inndataen likt uavhengig av format, slik det allerede er i de andre uttrykkene. Dessuten er jo grunnen til at vi har dratt inn decimal å gjøre utregningen riktig, ikke å støtte tall med større presisjon.
...ltinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json (1)
100-108: De-duplicate identical “small numbers” test inputs.Lines 100–103 and 105–108 are identical. Consider adjusting one case to keep both tests meaningful.
♻️ Proposed tweak (keep small-number coverage distinct)
{ "name": "Should handle very small numbers", - "expression": ["divide", 0.00000001, 0.0001], - "expects": 0.0001 + "expression": ["divide", 0.00000002, 0.0001], + "expects": 0.0002 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json` around lines 100 - 108, Two tests share identical input/expects ("Should handle very small numbers" and "Returns expected value for inputs that would return different result when treated as 64-bit binary floats") — remove the duplication by either deleting one of the entries or modifying the second test to use a different small-number pair (e.g., change the expression in the test named "Returns expected value for inputs that would return different result when treated as 64-bit binary floats" to another tiny numerator/denominator or edge-case values) so both tests remain meaningful while keeping the expected output consistent with the new inputs.test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/plus.json (1)
5-13: Avoid duplicate 0.1 + 0.2 coverage in two separate tests.Lines 5–8 and Lines 10–13 exercise the same inputs/outputs. Consider making the generic test use different values so the “binary-float” case stays distinct.
♻️ Proposed tweak (diversify the generic case)
{ "name": "Should add the two given numbers together", - "expression": ["plus", 0.1, 0.2], - "expects": 0.3 + "expression": ["plus", 1.5, 2.25], + "expects": 3.75 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/plus.json` around lines 5 - 13, The two tests for the plus function are duplicate (both use expression ["plus", 0.1, 0.2]); update the generic test ("Should add the two given numbers together") to use different numeric inputs so the binary-float-specific test ("Returns expected value for inputs that would return different result when treated as 64-bit binary floats") remains unique; locate the JSON object with "name": "Should add the two given numbers together" and change its "expression" and "expects" to a different pair (for example 1.2 and 3.4 with expected 4.6 or another sensible pair) ensuring the "Returns expected value..." test continues to use 0.1 and 0.2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.json`:
- Around line 100-108: Two tests share identical input/expects ("Should handle
very small numbers" and "Returns expected value for inputs that would return
different result when treated as 64-bit binary floats") — remove the duplication
by either deleting one of the entries or modifying the second test to use a
different small-number pair (e.g., change the expression in the test named
"Returns expected value for inputs that would return different result when
treated as 64-bit binary floats" to another tiny numerator/denominator or
edge-case values) so both tests remain meaningful while keeping the expected
output consistent with the new inputs.
In
`@test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/plus.json`:
- Around line 5-13: The two tests for the plus function are duplicate (both use
expression ["plus", 0.1, 0.2]); update the generic test ("Should add the two
given numbers together") to use different numeric inputs so the
binary-float-specific test ("Returns expected value for inputs that would return
different result when treated as 64-bit binary floats") remains unique; locate
the JSON object with "name": "Should add the two given numbers together" and
change its "expression" and "expects" to a different pair (for example 1.2 and
3.4 with expected 4.6 or another sensible pair) ensuring the "Returns expected
value..." test continues to use 0.1 and 0.2.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/divide/divide.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/minus/minus.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.jsontest/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/plus/plus.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Core.Tests/LayoutExpressions/CommonTests/shared-tests/functions/multiply/multiply.json
|
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
3 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
Added +-*/ and changed parsing of JsonTokenType.Number to Decimal instead of Double.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Tests