Open
Conversation
…ve method can be used during calculation of data fields using expressions. Added unit tests for the DataFieldValueCalculator
olavsorl
added
feature
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis pull request introduces data field value calculation functionality to Altinn App Core. It adds calculation configuration support, a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 8
🧹 Nitpick comments (6)
test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.json (1)
26-26: Inconsistent schema URL.This schema URL differs from other test files in the PR. Other files use
https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/layout/layout.schema.v1.json, but this one useshttps://altinncdn.no/schemas/json/layout/layout.schema.v1.json.Consider using a consistent schema URL across all test files for maintainability.
Proposed fix
- "$schema": "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json", + "$schema": "https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/layout/layout.schema.v1.json",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.json` at line 26, Update the inconsistent $schema value in the test JSON by replacing the existing "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json" with the canonical schema URL used elsewhere: "https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/layout/layout.schema.v1.json"; locate the "$schema" property (present in the JSON object) and update its string to the canonical toolkit path so all test files use the same schema URL.test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.json (1)
9-12: Consider using a calculation-specific schema URL.The
$schemareferencesvalidation.schema.v1.jsonbut this is a calculation configuration. If a dedicated calculation schema exists or is planned, consider using it for clarity and proper validation. This inconsistency appears in multiple test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.json` around lines 9 - 12, The test's calculationConfig uses a generic validation.schema.v1.json for "$schema" even though it contains calculation entries (calculations and keys like "form.name"); update the "$schema" value to point to the calculation-specific schema (or a dedicated stub schema for calculations) so tests validate against the correct schema; locate the JSON object named calculationConfig and replace the "$schema" string value currently set to "https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/validation/validation.schema.v1.json" with the proper calculation schema URL (or a local test calculation schema) ensuring all similar test files use the same calculation-specific schema.src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (2)
97-98: Avoid re-fetching form wrapper inside the inner loop.
dataElementis stable in this method, so fetch the wrapper once before iterating resolved fields.♻️ Proposed refactor
- foreach (var (baseField, calculations) in dataFieldCalculations) + var formDataWrapper = await dataAccessor.GetFormDataWrapper(dataElement); + foreach (var (baseField, calculations) in dataFieldCalculations) { @@ - var formDataWrapper = await dataAccessor.GetFormDataWrapper(dataElement); foreach (var calculation in calculations) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 97 - 98, The method in DataFieldValueCalculator repeatedly calls await dataAccessor.GetFormDataWrapper(dataElement) inside the inner loop over resolved fields/calculations; since dataElement is stable, call GetFormDataWrapper(dataElement) once before entering the loop (assign to formDataWrapper) and reuse that variable inside the loop (where calculations and resolved fields are processed), removing the duplicated await calls to avoid unnecessary I/O and improve performance.
14-38: Tighten class surface and remove service-locator dependency.This feature class can likely be
internal sealed, andIDataElementAccessCheckershould be injected directly instead of pulled fromIServiceProvider.♻️ Proposed refactor
-public class DataFieldValueCalculator +internal sealed class DataFieldValueCalculator { @@ - public DataFieldValueCalculator( + public DataFieldValueCalculator( ILogger<DataFieldValueCalculator> logger, ILayoutEvaluatorStateInitializer layoutEvaluatorStateInitializer, IAppResources appResourceService, - IServiceProvider serviceProvider + IDataElementAccessChecker dataElementAccessChecker ) { _logger = logger; _appResourceService = appResourceService; _layoutEvaluatorStateInitializer = layoutEvaluatorStateInitializer; - _dataElementAccessChecker = serviceProvider.GetRequiredService<IDataElementAccessChecker>(); + _dataElementAccessChecker = dataElementAccessChecker; }As per coding guidelines "Use internal accessibility on types by default", "Use sealed for classes unless inheritance is considered a valid use-case", and "register services in DI container properly".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 14 - 38, The DataFieldValueCalculator class should be narrowed and avoid the service-locator pattern: change the class declaration to internal sealed (DataFieldValueCalculator) and modify its constructor to accept an IDataElementAccessChecker parameter directly (instead of IServiceProvider), assign it to the _dataElementAccessChecker field, and remove the IServiceProvider parameter and its GetRequiredService call; update any call sites/DI registrations to register and pass IDataElementAccessChecker into the DataFieldValueCalculator constructor.src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cs (1)
9-20: Prefer direct constructor injection and a tighter type surface.
IServiceProviderhere introduces service-locator coupling. InjectDataFieldValueCalculatordirectly, and make this typeinternal sealedunless you intentionally expose/extensibility-enable it.♻️ Proposed refactor
-using Microsoft.Extensions.DependencyInjection; namespace Altinn.App.Core.Features.DataProcessing; -public class DataFieldValueCalculatorProcessor : IDataWriteProcessor +internal sealed class DataFieldValueCalculatorProcessor : IDataWriteProcessor { private readonly DataFieldValueCalculator _dataFieldValueCalculator; @@ - public DataFieldValueCalculatorProcessor(IServiceProvider serviceProvider) + public DataFieldValueCalculatorProcessor(DataFieldValueCalculator dataFieldValueCalculator) { - _dataFieldValueCalculator = serviceProvider.GetRequiredService<DataFieldValueCalculator>(); + _dataFieldValueCalculator = dataFieldValueCalculator; }As per coding guidelines "Use internal accessibility on types by default", "Use sealed for classes unless inheritance is considered a valid use-case", and "register services in DI container properly".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cs` around lines 9 - 20, The class DataFieldValueCalculatorProcessor currently uses IServiceProvider service-locator in its constructor; change it to use direct constructor injection by replacing the IServiceProvider parameter with a DataFieldValueCalculator parameter and assign it to the _dataFieldValueCalculator field inside the constructor, and mark the class as internal sealed (DataFieldValueCalculatorProcessor) to tighten accessibility and prevent inheritance; after this change ensure the DI registration for DataFieldValueCalculatorProcessor is updated to register the concrete type (or the interface it implements) so the container can resolve DataFieldValueCalculator directly.test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
14-15: Use xUnit assertions instead of FluentAssertions in this test project.Please replace
.Should()assertions withAssert.*to align with test standards.♻️ Proposed refactor
-using FluentAssertions; @@ - result.Get(expected.Field).Should().Be(expected.Result.ToObject()); + Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); @@ - result.Get(expected.Field).Should().Be(expected.Result.ToObject()); + Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); @@ - _logger.Collector.GetSnapshot().Select(x => x.Message).Should().Contain(expected.LogMessageWarning); + Assert.Contains(expected.LogMessageWarning, _logger.Collector.GetSnapshot().Select(x => x.Message));As per coding guidelines "
test/**/*.cs: Use xUnit asserts over FluentAssertions in test projects".Also applies to: 73-86, 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 14 - 15, In DataFieldValueCalculatorTests, remove the FluentAssertions dependency (delete the using FluentAssertions) and replace all `.Should()` style assertions in the test class (e.g., occurrences inside DataFieldValueCalculatorTests and the ranges noted around lines 73–86 and 137) with equivalent xUnit Assert calls (e.g., Assert.Equal(expected, actual), Assert.Null(value), Assert.True/False(conditions) or Assert.Throws for exceptions) ensuring each assertion maps to the appropriate Assert.* overload and preserves the original expected vs actual ordering and message semantics.
🤖 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/Extensions/ServiceCollectionExtensions.cs`:
- Around line 185-186: Replace the TryAddTransient registration for
IDataWriteProcessor with services.AddTransient<IDataWriteProcessor,
DataFieldValueCalculatorProcessor>() so the core
DataFieldValueCalculatorProcessor is always registered alongside any
app-provided implementations (the code that enumerates
GetAll<IDataWriteProcessor>() in InternalPatchService should therefore see
both). Also mark the DataFieldValueCalculatorProcessor class as sealed (unless
there is an intended inheritance use-case) to follow the coding guideline.
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 40-55: Wrap the Calculate lifecycle in OpenTelemetry
instrumentation: create or use an ActivitySource and a Meter and start a parent
Activity in Calculate (recording taskId and instance identifiers), then for each
loop iteration start a child Activity for the data element (use dataType.Id and
dataElement identifiers) and another nested Activity or span around the call to
CalculateFormData; record attributes/tags like taskId, dataType.Id,
calculationConfig and use Counter/Histogram instruments to emit metrics for
"calculations_started", "calculations_failed", "conversion_failures" and
duration; on any exception from _dataElementAccessChecker.CanRead,
_appResourceService.GetCalculationConfiguration or CalculateFormData catch/log
the exception, set the Activity status to error, increment the failures counter
and rethrow or handle accordingly, and ensure Activities are disposed in finally
blocks so tracing captures end times.
- Around line 81-84: The StartsWith check used in the hiddenFields.Exists
predicate is too broad and can match sibling fields; replace that logic with an
exact-or-descendant check (create a helper like IsSameOrDescendantField) that
returns true if candidate.Equals(hiddenField, StringComparison.Ordinal) or if
candidate.StartsWith(hiddenField, StringComparison.Ordinal) AND candidate.Length
> hiddenField.Length AND the next character is either '.' or '['; update the
predicate that currently uses resolvedField.Field.StartsWith(...) to call this
helper so only exact matches or true descendants are considered.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 124-133: GetResolvedKeys has a nullability/type mismatch with
GetResolvedKeysRecursive: change GetResolvedKeysRecursive's parameter and return
types from string?[] to string[] so callers (including GetResolvedKeys which
passes field.Split('.')) match non-nullable arrays; also replace the invalid
empty-array return syntax (return [];) with a proper empty string array (e.g.
use Array.Empty<string>()) and update any recursive call sites to use the
corrected signature (functions: GetResolvedKeys and GetResolvedKeysRecursive).
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionHelper.cs`:
- Around line 9-26: The stack-allocated buffer rowIndicesSpan (Span<int>
rowIndicesSpan = stackalloc int[200]) can be overflowed when count exceeds 200;
before writing into rowIndicesSpan[count] in the loop inside the method in
ExpressionHelper.cs, add a guard that checks if count >= rowIndicesSpan.Length
and if so throw an InvalidOperationException (or ArgumentOutOfRangeException)
with a clear message like "Too many indices in field: {field}" to fail fast;
ensure this check is performed immediately before assigning
rowIndicesSpan[count] and incrementing count so no out-of-range write occurs.
In `@src/Altinn.App.Core/Models/RawDataFieldValueCalculation.cs`:
- Around line 8-30: Change the two public config model classes to non-public
sealed implementation types: update the declarations of DataFieldCalculation and
RawDataFieldValueCalculation to use internal sealed instead of public so they
are not exposed as extension points; keep their existing members (Condition,
Ref) and nullability as-is and ensure no external code relies on the public
types (adjust any internal usages or tests to the new accessibility).
- Line 13: The non-nullable property Condition on class
RawDataFieldValueCalculation is declared without initialization; mark it as
required or initialize via the class constructor to satisfy
nullable-reference-type rules. Update RawDataFieldValueCalculation by adding the
required modifier to the Condition property (e.g., public required Expression
Condition { get; set; }) or add a constructor that accepts an Expression
parameter and assigns it to Condition so the property cannot remain null at
object creation.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.json`:
- Around line 5-7: The test currently sets the input and expected value for
"form.name" to the same string ("feil"), so the calculator may be skipped and
the test still passes; update the hidden-field.json test case so the input value
for "form.name" is different from the expected "feil" (e.g., set input to
"initial" or empty) to force the calculation to run and produce "feil", and make
the same change for the other identical case referenced (the case at the other
occurrence) so both cases validate actual calculation execution.
---
Nitpick comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 97-98: The method in DataFieldValueCalculator repeatedly calls
await dataAccessor.GetFormDataWrapper(dataElement) inside the inner loop over
resolved fields/calculations; since dataElement is stable, call
GetFormDataWrapper(dataElement) once before entering the loop (assign to
formDataWrapper) and reuse that variable inside the loop (where calculations and
resolved fields are processed), removing the duplicated await calls to avoid
unnecessary I/O and improve performance.
- Around line 14-38: The DataFieldValueCalculator class should be narrowed and
avoid the service-locator pattern: change the class declaration to internal
sealed (DataFieldValueCalculator) and modify its constructor to accept an
IDataElementAccessChecker parameter directly (instead of IServiceProvider),
assign it to the _dataElementAccessChecker field, and remove the
IServiceProvider parameter and its GetRequiredService call; update any call
sites/DI registrations to register and pass IDataElementAccessChecker into the
DataFieldValueCalculator constructor.
In
`@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cs`:
- Around line 9-20: The class DataFieldValueCalculatorProcessor currently uses
IServiceProvider service-locator in its constructor; change it to use direct
constructor injection by replacing the IServiceProvider parameter with a
DataFieldValueCalculator parameter and assign it to the
_dataFieldValueCalculator field inside the constructor, and mark the class as
internal sealed (DataFieldValueCalculatorProcessor) to tighten accessibility and
prevent inheritance; after this change ensure the DI registration for
DataFieldValueCalculatorProcessor is updated to register the concrete type (or
the interface it implements) so the container can resolve
DataFieldValueCalculator directly.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.json`:
- Around line 9-12: The test's calculationConfig uses a generic
validation.schema.v1.json for "$schema" even though it contains calculation
entries (calculations and keys like "form.name"); update the "$schema" value to
point to the calculation-specific schema (or a dedicated stub schema for
calculations) so tests validate against the correct schema; locate the JSON
object named calculationConfig and replace the "$schema" string value currently
set to
"https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/validation/validation.schema.v1.json"
with the proper calculation schema URL (or a local test calculation schema)
ensuring all similar test files use the same calculation-specific schema.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.json`:
- Line 26: Update the inconsistent $schema value in the test JSON by replacing
the existing "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json"
with the canonical schema URL used elsewhere:
"https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/layout/layout.schema.v1.json";
locate the "$schema" property (present in the JSON object) and update its string
to the canonical toolkit path so all test files use the same schema URL.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 14-15: In DataFieldValueCalculatorTests, remove the
FluentAssertions dependency (delete the using FluentAssertions) and replace all
`.Should()` style assertions in the test class (e.g., occurrences inside
DataFieldValueCalculatorTests and the ranges noted around lines 73–86 and 137)
with equivalent xUnit Assert calls (e.g., Assert.Equal(expected, actual),
Assert.Null(value), Assert.True/False(conditions) or Assert.Throws for
exceptions) ensuring each assertion maps to the appropriate Assert.* overload
and preserves the original expected vs actual ordering and message semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4d697be-2a03-453f-922f-325b22590c90
📒 Files selected for processing (23)
src/Altinn.App.Core/Configuration/AppSettings.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Features/DataLists/InstanceDataListsFactory.cssrc/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cssrc/Altinn.App.Core/Features/DataProcessing/GenericDataProcessor.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.ApplicationMetadata.Service.cssrc/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Implementation/AppResourcesSI.cssrc/Altinn.App.Core/Internal/App/IAppResources.cssrc/Altinn.App.Core/Internal/Data/IFormDataWrapper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionHelper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionValue.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cssrc/Altinn.App.Core/Models/RawDataFieldValueCalculation.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cstest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/single-expression-boolean.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/single-expression-number.json
19 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (1)
83-87:⚠️ Potential issue | 🟠 MajorUse an exact-or-descendant field check here.
This
StartsWithfilter is too broad: hidingform.namealso suppresses sibling targets likeform.nameResultBoolean. That can silently skip unrelated calculations.🐛 Proposed fix
- if ( - hiddenFields.Exists(d => - d.DataElementIdentifier == resolvedField.DataElementIdentifier - && resolvedField.Field.StartsWith(d.Field, StringComparison.InvariantCulture) - ) - ) + if ( + hiddenFields.Exists(d => + d.DataElementIdentifier == resolvedField.DataElementIdentifier + && IsSameOrDescendantField(resolvedField.Field, d.Field) + ) + ) { continue; }private static bool IsSameOrDescendantField(string candidate, string hiddenField) { return candidate.Equals(hiddenField, StringComparison.Ordinal) || ( candidate.StartsWith(hiddenField, StringComparison.Ordinal) && candidate.Length > hiddenField.Length && (candidate[hiddenField.Length] == '.' || candidate[hiddenField.Length] == '[') ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 83 - 87, The current StartsWith check in the hiddenFields.Exists lambda is too broad and hides siblings (e.g., form.name vs form.nameResultBoolean); replace it with an exact-or-descendant check by adding a helper like IsSameOrDescendantField(string candidate, string hiddenField) and use that in the Exists predicate for resolvedField.Field and d.Field; the helper should use StringComparison.Ordinal, return true if candidate.Equals(hiddenField), otherwise ensure candidate.StartsWith(hiddenField, StringComparison.Ordinal) && candidate.Length > hiddenField.Length && (candidate[hiddenField.Length] == '.' || candidate[hiddenField.Length] == '[') so only true for direct descendants.
🧹 Nitpick comments (1)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
181-186: Add at least one test throughCalculateor the processor.Calling
CalculateFormDatadirectly leaves the public feature path untested:GetCalculationConfiguration,CanRead, per-task data-element iteration, and the top-level activity startup are all bypassed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 181 - 186, The test currently calls CalculateFormData directly which bypasses the public execution path; update or add a test in DataFieldValueCalculatorTests that calls the public Calculate (or the processor entry-point) instead of CalculateFormData so GetCalculationConfiguration, CanRead, per-task data-element iteration and top-level activity startup are exercised; wire up the same test fixture inputs (dataAccessor, dataElement, CalculationConfig) or appropriate mocks for ICalculationProcessor/GetCalculationConfiguration and CanRead, invoke Calculate with the same Task id, and assert the expected side-effects/results to cover the public flow.
🤖 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/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 27-33: The constructor declaration for DataFieldValueCalculator
(the public DataFieldValueCalculator(...) initializer) is misformatted; run
CSharpier (or dotnet format if configured) on the file
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs to
reformat the constructor block so it matches the project's C# formatting rules
and resolves the "Verify dotnet format" failure—ensure the public
DataFieldValueCalculator(...) parameter list and opening brace adhere to
CSharpier's style and then re-run the format/verify step before committing.
- Around line 187-188: ResolveDataFieldCalculation can be made static because it
only uses its parameters and the static field _jsonSerializerOptions; update the
method signature of ResolveDataFieldCalculation to add the static modifier and
verify any callers still compile (no instance state is used), ensuring
references to ResolveDataFieldCalculation and the static _jsonSerializerOptions
remain valid.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 201-208: The recursion should stop when an explicit indexed row is
missing: in the GetResolvedKeysRecursive flow, after calling
GetElementAt(childModelList, groupIndex.Value) and before recursing via
GetResolvedKeysRecursive, check if groupIndex.HasValue and elementAt is null
and, in that case, bail out (return no resolved keys) instead of continuing in
calculation mode; update the branch around the GetElementAt →
GetResolvedKeysRecursive call (and preserve JoinFieldKeyParts usage for valid
elements) so missing indexed rows do not produce keys like "group[99].field".
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 40-51: The test fails to compile because the
DataFieldValueCalculator constructor signature changed to require a telemetry
dependency and the test creates accessCheckerMock but injects a different mock;
update the constructor call to pass a configured telemetry mock (e.g., a
Mock<ITelemetryClient> or whatever telemetry interface your production code
expects) and replace dataElementAccessChecker.Object with
accessCheckerMock.Object so the IDataElementAccessChecker you configure via
accessCheckerMock.Setup(...) is actually injected into DataFieldValueCalculator.
In
`@test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt`:
- Line 2287: Add backward-compatible forwarding overloads for the original
signatures so existing binaries don't break: implement
DataModelWrapper.GetResolvedKeys(string field) and
LayoutEvaluatorState.GetResolvedKeys(DataReference reference) as simple wrappers
that call the new parameterized overloads (the versions accepting the optional
bool isCalculating) passing isCalculating: false; ensure method names and
parameter types match the original public API exactly so they delegate to the
new methods and preserve binary compatibility.
- Around line 2914-2915: The new abstract method GetCalculationConfiguration on
IAppResources breaks external implementers; to fix, either make it a default
interface method on IAppResources that returns null (add a default
implementation for string? GetCalculationConfiguration(string dataTypeId) =>
null) so existing implementations continue to compile, or extract this member
into a new interface (e.g., IAppResourcesWithCalculation) and implement it only
where needed; update references to call the new interface where calculation
config is required. Ensure you modify the IAppResources declaration (or create
the new interface) and adjust usages accordingly.
---
Duplicate comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 83-87: The current StartsWith check in the hiddenFields.Exists
lambda is too broad and hides siblings (e.g., form.name vs
form.nameResultBoolean); replace it with an exact-or-descendant check by adding
a helper like IsSameOrDescendantField(string candidate, string hiddenField) and
use that in the Exists predicate for resolvedField.Field and d.Field; the helper
should use StringComparison.Ordinal, return true if
candidate.Equals(hiddenField), otherwise ensure
candidate.StartsWith(hiddenField, StringComparison.Ordinal) && candidate.Length
> hiddenField.Length && (candidate[hiddenField.Length] == '.' ||
candidate[hiddenField.Length] == '[') so only true for direct descendants.
---
Nitpick comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 181-186: The test currently calls CalculateFormData directly which
bypasses the public execution path; update or add a test in
DataFieldValueCalculatorTests that calls the public Calculate (or the processor
entry-point) instead of CalculateFormData so GetCalculationConfiguration,
CanRead, per-task data-element iteration and top-level activity startup are
exercised; wire up the same test fixture inputs (dataAccessor, dataElement,
CalculationConfig) or appropriate mocks for
ICalculationProcessor/GetCalculationConfiguration and CanRead, invoke Calculate
with the same Task id, and assert the expected side-effects/results to cover the
public flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1fc19bc-0504-491d-8c67-1c158176a694
📒 Files selected for processing (16)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataFieldValueCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionHelper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionValue.cssrc/Altinn.App.Core/Models/RawDataFieldValueCalculation.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cstest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/single-expression-boolean.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/single-expression-number.jsontest/Altinn.App.Core.Tests/Features/Validators/expression-validation-tests/shared/component-lookup-hidden.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.json
- src/Altinn.App.Core/Internal/Expressions/ExpressionHelper.cs
- src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
- src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cs
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.json
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.json
- src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
40-54:⚠️ Potential issue | 🟠 MajorWrong mock is injected—tests may pass spuriously or fail unexpectedly.
accessCheckerMockis configured to returntrueforCanRead, butdataElementAccessChecker(a separate, unconfigured mock) is injected into the calculator. Since the tests callCalculateFormDatadirectly (bypassingCalculate), the access checker isn't invoked and this bug is currently hidden. If tests are added that callCalculate, they will fail with aMoq.MockException.🐛 Proposed fix
public DataFieldValueCalculatorTests(ITestOutputHelper output) { - var accessCheckerMock = new Mock<IDataElementAccessChecker>(); - accessCheckerMock.Setup(x => x.CanRead(It.IsAny<Instance>(), It.IsAny<DataType>())).ReturnsAsync(true); - - var dataElementAccessChecker = new Mock<IDataElementAccessChecker>(); + var dataElementAccessChecker = new Mock<IDataElementAccessChecker>(); + dataElementAccessChecker + .Setup(x => x.CanRead(It.IsAny<Instance>(), It.IsAny<DataType>())) + .ReturnsAsync(true); var telemetry = new TelemetrySink();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 40 - 54, The tests configure accessCheckerMock to return true for CanRead but accidentally inject an unconfigured dataElementAccessChecker into the DataFieldValueCalculator; replace the injected mock with accessCheckerMock.Object (or remove the unused dataElementAccessChecker) when constructing DataFieldValueCalculator so the configured mock is used; ensure references to CalculateFormData and Calculate remain valid and add/adjust any tests that exercise Calculate to avoid Moq.MockException.src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (1)
188-200:⚠️ Potential issue | 🟠 Major
ResolveDataFieldCalculationshould bestatic(pipeline failure) and has dead code in the string branch.The pipeline is failing because this method doesn't access instance data. Additionally, when
definition.ValueKind == JsonValueKind.String, the parsedstringReferenceis validated but never used—the method proceeds with an emptyRawDataFieldValueCalculationthat will always fail the null-condition check at line 218.🐛 Proposed fix
- private DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger) + private static DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger) { var rawDataFieldValueCalculation = new RawDataFieldValueCalculation(); if (definition.ValueKind == JsonValueKind.String) { var stringReference = definition.GetString(); if (stringReference == null) { logger.LogError("Could not resolve null reference for calculation for field {Field}", field); return null; } + // TODO: Handle string-based calculation references (e.g., lookup or shorthand syntax) + logger.LogError("String-based calculation definitions are not yet supported for field {Field}", field); + return null; }The
staticmodifier issue was flagged in a previous review. The dead-code concern in the string branch is new.
🧹 Nitpick comments (2)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (1)
150-153: Redundantloggerparameter shadows instance field.
ParseDataFieldCalculationConfigaccepts anILogger<DataFieldValueCalculator>parameter but the caller always passes_logger(line 73). Since this is an instance method, consider removing the parameter and using_loggerdirectly for consistency.♻️ Proposed simplification
- private Dictionary<string, List<DataFieldCalculation>> ParseDataFieldCalculationConfig( - string rawCalculationConfig, - ILogger<DataFieldValueCalculator> logger - ) + private Dictionary<string, List<DataFieldCalculation>> ParseDataFieldCalculationConfig( + string rawCalculationConfig + ) { using var calculationConfigDocument = JsonDocument.Parse(rawCalculationConfig); var dataFieldCalculations = new Dictionary<string, List<DataFieldCalculation>>();And update the call site at line 73:
- var dataFieldCalculations = ParseDataFieldCalculationConfig(rawCalculationConfig, _logger); + var dataFieldCalculations = ParseDataFieldCalculationConfig(rawCalculationConfig);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 150 - 153, The ParseDataFieldCalculationConfig method currently takes an ILogger<DataFieldValueCalculator> parameter that always shadows the instance field _logger; remove the redundant logger parameter from the ParseDataFieldCalculationConfig signature and body, update all call sites (where the method is invoked) to stop passing _logger and rely on the instance field _logger inside the method, and run a quick compile to ensure no remaining references to the removed parameter remain.test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
64-86: Consider consolidating duplicate test methods.
RunDataFieldCalculationTestsForBackendandRunDataFieldCalculationTestsForSharedhave identical implementations—only the test data folder differs. This duplication can be reduced.♻️ Optional: Consolidate into a single parameterized test
+ public static IEnumerable<object[]> GetAllTestFolders() + { + yield return new object[] { "backend" }; + yield return new object[] { "shared" }; + } + [Theory] - [FileNamesInFolderData(["Features", "DataProcessing", "data-field-value-calculator-tests", "backend"])] - public async Task RunDataFieldCalculationTestsForBackend(string fileName, string folder) + [MemberData(nameof(GetAllTestFolders))] + [FileNamesInFolderData(["Features", "DataProcessing", "data-field-value-calculator-tests"])] + public async Task RunDataFieldCalculationTests(string fileName, string folder) { var (result, testCase) = await RunDataFieldCalculatorTest(fileName, folder); foreach (var expected in testCase.Expects) { Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); } } - - [Theory] - [FileNamesInFolderData(["Features", "DataProcessing", "data-field-value-calculator-tests", "shared"])] - public async Task RunDataFieldCalculationTestsForShared(string fileName, string folder) - { - var (result, testCase) = await RunDataFieldCalculatorTest(fileName, folder); - - foreach (var expected in testCase.Expects) - { - Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); - } - }Note: This depends on how
FileNamesInFolderDataworks—if it doesn't support dynamic folder injection, keeping them separate is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 64 - 86, Both test methods RunDataFieldCalculationTestsForBackend and RunDataFieldCalculationTestsForShared are identical except for the folder passed to FileNamesInFolderData; consolidate them into a single parameterized test that takes the folder as a parameter (or uses multiple FileNamesInFolderData attributes) and reuses RunDataFieldCalculatorTest and the same assertion loop, referencing RunDataFieldCalculationTestsForBackend/RunDataFieldCalculationTestsForShared, FileNamesInFolderData, and RunDataFieldCalculatorTest to locate and replace the duplicates.
🤖 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/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 215-222: The Expected record's properties Field, Result, and
LogMessageWarning are declared as non-nullable but may be omitted during JSON
deserialization, causing null refs; fix by making these properties nullable
(string? Field, ExpressionValue? Result, string? LogMessageWarning) or mark them
required, and then update the test assertion loops (places that access
expected.Result.ToObject() and similar) to null-check expected.Result and
expected.Field/LogMessageWarning before use or handle the null case explicitly
so assertions don't throw.
---
Duplicate comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 40-54: The tests configure accessCheckerMock to return true for
CanRead but accidentally inject an unconfigured dataElementAccessChecker into
the DataFieldValueCalculator; replace the injected mock with
accessCheckerMock.Object (or remove the unused dataElementAccessChecker) when
constructing DataFieldValueCalculator so the configured mock is used; ensure
references to CalculateFormData and Calculate remain valid and add/adjust any
tests that exercise Calculate to avoid Moq.MockException.
---
Nitpick comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 150-153: The ParseDataFieldCalculationConfig method currently
takes an ILogger<DataFieldValueCalculator> parameter that always shadows the
instance field _logger; remove the redundant logger parameter from the
ParseDataFieldCalculationConfig signature and body, update all call sites (where
the method is invoked) to stop passing _logger and rely on the instance field
_logger inside the method, and run a quick compile to ensure no remaining
references to the removed parameter remain.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 64-86: Both test methods RunDataFieldCalculationTestsForBackend
and RunDataFieldCalculationTestsForShared are identical except for the folder
passed to FileNamesInFolderData; consolidate them into a single parameterized
test that takes the folder as a parameter (or uses multiple
FileNamesInFolderData attributes) and reuses RunDataFieldCalculatorTest and the
same assertion loop, referencing
RunDataFieldCalculationTestsForBackend/RunDataFieldCalculationTestsForShared,
FileNamesInFolderData, and RunDataFieldCalculatorTest to locate and replace the
duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f033474-356e-4781-b2bf-4087d5bb89dd
📒 Files selected for processing (2)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (2)
84-88:⚠️ Potential issue | 🟠 MajorUse exact-or-descendant matching for hidden-field filtering.
Line 87 uses
StartsWith, which can match sibling fields (for exampleform.name2vs hiddenform.name) and skip unrelated calculations.🐛 Proposed fix
- if ( - hiddenFields.Exists(d => - d.DataElementIdentifier == resolvedField.DataElementIdentifier - && resolvedField.Field.StartsWith(d.Field, StringComparison.InvariantCulture) - ) - ) + if ( + hiddenFields.Exists(d => + d.DataElementIdentifier == resolvedField.DataElementIdentifier + && IsSameOrDescendantField(resolvedField.Field, d.Field) + ) + ) { continue; }+ private static bool IsSameOrDescendantField(string candidate, string hiddenField) + { + if (candidate.Equals(hiddenField, StringComparison.Ordinal)) + { + return true; + } + + return candidate.StartsWith(hiddenField, StringComparison.Ordinal) + && candidate.Length > hiddenField.Length + && (candidate[hiddenField.Length] == '.' || candidate[hiddenField.Length] == '['); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 84 - 88, In DataFieldValueCalculator, fix the hidden-field filter that currently uses resolvedField.Field.StartsWith(d.Field, ...) so it doesn't wrongly match siblings; change the logic in the hiddenFields.Exists predicate to allow either an exact match (resolvedField.Field == d.Field) or a descendant match (resolvedField.Field starts with d.Field + "." using the same StringComparison), preserving the DataElementIdentifier equality check; update the predicate used where hiddenFields.Exists(...) is called to implement this exact-or-descendant matching.
42-56:⚠️ Potential issue | 🟠 MajorTelemetry coverage is still partial for this feature flow.
Line 44 starts a parent activity, but the new calculation path still lacks per-data-element/per-calculation spans and failure metrics/status tagging. That makes operational troubleshooting much harder for this feature.
As per coding guidelines "
src/**/*.cs: Comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 42 - 56, Add per-data-element and per-calculation telemetry around the loop in Calculate: for each iteration create a child span using _telemetry (e.g., StartCalculateActivity or a new StartDataElementCalculationActivity) keyed by dataType.Id and taskId, set attributes for dataType.Id, dataElement.Id and calculationConfig, and record success/failure status and exception details if CalculateFormData throws; also emit a metric or counter for calculation attempts/failures. Instrument CalculateFormData entry/exit with its own span or use the child span to time the call, and ensure the span is ended in a finally block and errors are tagged using OpenTelemetry semantic conventions so per-data-element traces and failure metrics are available for observability.test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
213-220:⚠️ Potential issue | 🟡 MinorAlign
Expectednullability with actual JSON payloads.
expectsentries can omit properties (see Line 93-95), so non-nullableField,Result, andLogMessageWarningare unsafe and can lead to null dereferences in assertions.🛠️ Proposed fix
public record Expected { - public string Field { get; set; } + public string? Field { get; set; } - public ExpressionValue Result { get; set; } + public ExpressionValue? Result { get; set; } - public string LogMessageWarning { get; set; } + public string? LogMessageWarning { get; set; } }- foreach (var expected in testCase.Expects) - { - Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); - } + foreach (var expected in testCase.Expects) + { + if (expected.Field is not null && expected.Result is not null) + { + Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); + } + }As per coding guidelines "
**/*.cs: Use Nullable Reference Types in C# code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 213 - 220, The Expected record's properties (Field, Result, LogMessageWarning) are non-nullable but the test JSON can omit them; update the Expected record so each property is nullable (e.g., string? Field, ExpressionValue? Result, string? LogMessageWarning) and adjust any assertions in DataFieldValueCalculatorTests that access these properties to handle nulls (use null-safe checks or explicit assertions) to avoid potential null dereferences when deserializing missing fields.
🤖 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/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 84-88: In DataFieldValueCalculator, fix the hidden-field filter
that currently uses resolvedField.Field.StartsWith(d.Field, ...) so it doesn't
wrongly match siblings; change the logic in the hiddenFields.Exists predicate to
allow either an exact match (resolvedField.Field == d.Field) or a descendant
match (resolvedField.Field starts with d.Field + "." using the same
StringComparison), preserving the DataElementIdentifier equality check; update
the predicate used where hiddenFields.Exists(...) is called to implement this
exact-or-descendant matching.
- Around line 42-56: Add per-data-element and per-calculation telemetry around
the loop in Calculate: for each iteration create a child span using _telemetry
(e.g., StartCalculateActivity or a new StartDataElementCalculationActivity)
keyed by dataType.Id and taskId, set attributes for dataType.Id, dataElement.Id
and calculationConfig, and record success/failure status and exception details
if CalculateFormData throws; also emit a metric or counter for calculation
attempts/failures. Instrument CalculateFormData entry/exit with its own span or
use the child span to time the call, and ensure the span is ended in a finally
block and errors are tagged using OpenTelemetry semantic conventions so
per-data-element traces and failure metrics are available for observability.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 213-220: The Expected record's properties (Field, Result,
LogMessageWarning) are non-nullable but the test JSON can omit them; update the
Expected record so each property is nullable (e.g., string? Field,
ExpressionValue? Result, string? LogMessageWarning) and adjust any assertions in
DataFieldValueCalculatorTests that access these properties to handle nulls (use
null-safe checks or explicit assertions) to avoid potential null dereferences
when deserializing missing fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86bc4e8c-f0d4-4648-96a6-a3606589c61d
📒 Files selected for processing (2)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 150-153: Change the ParseDataFieldCalculationConfig method to a
static method since it only uses parameters and calls the static
ResolveDataFieldCalculation; update its signature to "private static
Dictionary<string, List<DataFieldCalculation>>
ParseDataFieldCalculationConfig(...)" and ensure any callers invoke it as a
static method (or via the class name) instead of relying on an instance; this
removes the CA1822 warning while keeping the call to ResolveDataFieldCalculation
intact.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 113-122: Add XML documentation for the public method
GetResolvedKeys(string field) to satisfy CS1591: provide a <summary> describing
that it returns resolved data model keys for the given dotted field path, a
<param name="field"> describing the expected dotted field string, and a
<returns> describing the returned string[]; match wording and style used by the
existing overload of GetResolvedKeys to keep consistency with other XML docs in
DataModelWrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e10ec47f-4ccd-49b9-a1aa-b70d738855e4
📒 Files selected for processing (3)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (2)
85-88:⚠️ Potential issue | 🟠 MajorUse exact-or-descendant path matching for hidden fields.
StartsWithon Line 87 is too broad and can hide sibling fields unintentionally. Match only exact field or true descendants (./[boundary), and useStringComparison.Ordinal.🐛 Proposed fix
- if ( - hiddenFields.Exists(d => - d.DataElementIdentifier == resolvedField.DataElementIdentifier - && resolvedField.Field.StartsWith(d.Field, StringComparison.InvariantCulture) - ) - ) + if ( + hiddenFields.Exists(d => + d.DataElementIdentifier == resolvedField.DataElementIdentifier + && IsSameOrDescendantField(resolvedField.Field, d.Field) + ) + ) { continue; }+ private static bool IsSameOrDescendantField(string candidate, string hiddenField) + { + if (candidate.Equals(hiddenField, StringComparison.Ordinal)) + { + return true; + } + + return candidate.StartsWith(hiddenField, StringComparison.Ordinal) + && candidate.Length > hiddenField.Length + && (candidate[hiddenField.Length] == '.' || candidate[hiddenField.Length] == '['); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 85 - 88, The hidden-field check in DataFieldValueCalculator (the lambda passed to hiddenFields.Exists comparing d.DataElementIdentifier and resolvedField.Field) currently uses StartsWith and should be tightened: replace the StartsWith(resolvedField.Field, StringComparison.InvariantCulture) logic with an exact match OR a descendant match that requires the next character after the prefix to be '.' or '[' and use StringComparison.Ordinal for comparisons; in other words, return true if d.Field == resolvedField.Field (ordinal) or if d.Field.Length > resolvedField.Field.Length && d.Field.StartsWith(resolvedField.Field, StringComparison.Ordinal) && (d.Field[resolvedField.Field.Length] == '.' || d.Field[resolvedField.Field.Length] == '[').
42-57:⚠️ Potential issue | 🟠 MajorTelemetry coverage is still too thin for this feature flow.
Only a top-level activity is started (Line 44). Per-data-element spans and failure metrics around access checks/config load/calculation execution are still missing.
As per coding guidelines "
src/**/*.cs: Comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 42 - 57, Add fine-grained telemetry inside Calculate: for each data element (loop over dataType/dataElement) start a per-data-element activity/span (use _telemetry.StartCalculateActivity or a new StartCalculateElementActivity) with attributes dataType.Id and taskId, then wrap the access check (_dataElementAccessChecker.CanRead), config load (_appResourceService.GetCalculationConfiguration), and the call to CalculateFormData in short child spans or timed metrics; on access denial, config missing, or CalculateFormData exceptions record failures via telemetry.RecordException or increment failure counters and set span status accordingly, and ensure the per-element activity is always ended in a finally block so metrics and statuses are emitted.
🧹 Nitpick comments (1)
src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs (1)
124-155: Consider consolidating the twoGetResolvedKeysoverloads.Both overloads duplicate null-check and split logic. Let
GetResolvedKeys(string field)delegate toGetResolvedKeys(field, isCalculating: false)to keep one path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs` around lines 124 - 155, Consolidate the duplicated logic by making the single-parameter GetResolvedKeys(string field) delegate to the two-parameter overload: have GetResolvedKeys(field) simply return GetResolvedKeys(field, isCalculating: false) (preserving the empty-array return when _dataModel is null inside the two-parameter overload), and remove the duplicated null-check and Split('.') logic from the single-parameter method so all logic flows through GetResolvedKeys(string field, bool isCalculating) which calls GetResolvedKeysRecursive(...).
🤖 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/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 189-197: The code in DataFieldValueCalculator reads a string JSON
definition into stringReference but never assigns it to
rawDataFieldValueCalculation.Condition, causing valid string-based calculations
to be treated as missing later; update the branch that checks
definition.ValueKind == JsonValueKind.String to set
rawDataFieldValueCalculation.Condition = stringReference (and keep the null
check/logging), and ensure the later validation that currently rejects missing
conditions (the check around rawDataFieldValueCalculation.Condition) accepts the
assigned string so string-form entries from calculation.json are executed.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 201-213: The current logic in GetResolvedKeysRecursive
(DataModelWrapper) sets elementType =
childType.GetGenericArguments().FirstOrDefault() ?? typeof(object), which fails
for arrays or non-generic enumerables; update the inference to handle arrays and
runtime instances: first check childType.IsArray and use
childType.GetElementType(), then check for non-generic IEnumerable and if still
null fall back to using child?.GetType() (the actual runtime type of the child
from childModelList) to derive the element type before recursing so the
recursive property lookup uses the real element type instead of object.
---
Duplicate comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 85-88: The hidden-field check in DataFieldValueCalculator (the
lambda passed to hiddenFields.Exists comparing d.DataElementIdentifier and
resolvedField.Field) currently uses StartsWith and should be tightened: replace
the StartsWith(resolvedField.Field, StringComparison.InvariantCulture) logic
with an exact match OR a descendant match that requires the next character after
the prefix to be '.' or '[' and use StringComparison.Ordinal for comparisons; in
other words, return true if d.Field == resolvedField.Field (ordinal) or if
d.Field.Length > resolvedField.Field.Length &&
d.Field.StartsWith(resolvedField.Field, StringComparison.Ordinal) &&
(d.Field[resolvedField.Field.Length] == '.' ||
d.Field[resolvedField.Field.Length] == '[').
- Around line 42-57: Add fine-grained telemetry inside Calculate: for each data
element (loop over dataType/dataElement) start a per-data-element activity/span
(use _telemetry.StartCalculateActivity or a new StartCalculateElementActivity)
with attributes dataType.Id and taskId, then wrap the access check
(_dataElementAccessChecker.CanRead), config load
(_appResourceService.GetCalculationConfiguration), and the call to
CalculateFormData in short child spans or timed metrics; on access denial,
config missing, or CalculateFormData exceptions record failures via
telemetry.RecordException or increment failure counters and set span status
accordingly, and ensure the per-element activity is always ended in a finally
block so metrics and statuses are emitted.
---
Nitpick comments:
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 124-155: Consolidate the duplicated logic by making the
single-parameter GetResolvedKeys(string field) delegate to the two-parameter
overload: have GetResolvedKeys(field) simply return GetResolvedKeys(field,
isCalculating: false) (preserving the empty-array return when _dataModel is null
inside the two-parameter overload), and remove the duplicated null-check and
Split('.') logic from the single-parameter method so all logic flows through
GetResolvedKeys(string field, bool isCalculating) which calls
GetResolvedKeysRecursive(...).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e33e47fa-3bee-44da-89cb-85f9e4c4f8c3
📒 Files selected for processing (5)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs (1)
194-196:⚠️ Potential issue | 🟡 MinorElement-type inference fails for array types.
GetGenericArguments()returns an empty array for non-generic arrays likestring[]orMyClass[], causing fallback totypeof(object). Property lookup againstobjectwill find nothing, breaking traversal.🐛 Proposed fix to handle arrays
// Get the element type of the collection -var elementType = childType.GetGenericArguments().FirstOrDefault() ?? typeof(object); +var elementType = childType.IsArray + ? childType.GetElementType() ?? typeof(object) + : childType.GetGenericArguments().FirstOrDefault() ?? typeof(object);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs` around lines 194 - 196, The element-type inference in DataModelWrapper (variable elementType computed from childType.GetGenericArguments().FirstOrDefault()) fails for array types because GetGenericArguments returns empty; update the logic that sets elementType to first check childType.GetGenericArguments(), and if that yields null/empty then if childType.IsArray use childType.GetElementType() ?? typeof(object), and additionally fall back to probing implemented generic IEnumerable<T> interfaces (e.g., find the first generic interface with GetGenericTypeDefinition()==typeof(IEnumerable<>)) before defaulting to typeof(object); apply this change where elementType is computed so property traversal uses the actual element type for arrays and enumerable types.
🤖 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/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 194-196: The element-type inference in DataModelWrapper (variable
elementType computed from childType.GetGenericArguments().FirstOrDefault())
fails for array types because GetGenericArguments returns empty; update the
logic that sets elementType to first check childType.GetGenericArguments(), and
if that yields null/empty then if childType.IsArray use
childType.GetElementType() ?? typeof(object), and additionally fall back to
probing implemented generic IEnumerable<T> interfaces (e.g., find the first
generic interface with GetGenericTypeDefinition()==typeof(IEnumerable<>)) before
defaulting to typeof(object); apply this change where elementType is computed so
property traversal uses the actual element type for arrays and enumerable types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9bb763d-af70-4f11-bd62-73e7c3dc088a
📒 Files selected for processing (3)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Models/RawDataFieldValueCalculation.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.App.Core/Models/RawDataFieldValueCalculation.cs
- src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs (1)
234-239: Minor: Comment could be more precise.The comment says "Collection is null/empty" but this branch only handles null collections. Empty collections are handled by the
foreachloop above (which iterates zero times and returns an empty array). Consider updating the comment.📝 Proposed comment clarification
- // Collection is null/empty + // Collection is null (currentModel was null or property value was null) if (isCalculating && currentIndex == keyParts.Length - 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs` around lines 234 - 239, Update the misleading comment in DataModelWrapper.cs: change "Collection is null/empty" to accurately reflect that this branch only handles null collections (e.g., "Collection is null") so readers don't confuse it with the empty-collection case handled by the preceding foreach; locate the branch in the method where isCalculating && currentIndex == keyParts.Length - 1 and the return [JoinFieldKeyParts(currentKey, key)] call to update the comment.
🤖 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/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 234-239: Update the misleading comment in DataModelWrapper.cs:
change "Collection is null/empty" to accurately reflect that this branch only
handles null collections (e.g., "Collection is null") so readers don't confuse
it with the empty-collection case handled by the preceding foreach; locate the
branch in the method where isCalculating && currentIndex == keyParts.Length - 1
and the return [JoinFieldKeyParts(currentKey, key)] call to update the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45792189-b3fe-4934-8365-df8a02c6a3a5
📒 Files selected for processing (1)
src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
2945-2946:⚠️ Potential issue | 🟠 Major
IAppResourcesinterface addition is still a breaking change for external implementers.Adding
GetCalculationConfiguration(string dataTypeId)to a public interface requires all third-party implementations to be updated. Consider a default interface implementation or a separate derived interface to keep compatibility in a non-major release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt` around lines 2945 - 2946, The addition of GetCalculationConfiguration(string dataTypeId) to the public IAppResources interface is a breaking change; either provide a default interface implementation on IAppResources (implement GetCalculationConfiguration returning null or a sensible default) or move the new member into a separate derived interface (e.g., IAppResourcesWithCalculationConfig) that consumers can opt into; locate the IAppResources interface declaration and implement one of these approaches to restore binary compatibility while allowing new behavior.test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs (1)
105-108:⚠️ Potential issue | 🟡 MinorAdd null check for
LogMessageWarningbefore assertion.
expected.LogMessageWarningis nullable (string?) but is used directly inAssert.Contains. If a future test case omits this property, the assertion will behave unexpectedly.🛡️ Proposed fix
foreach (var expected in testCase.Expects) { + if (expected.LogMessageWarning is null) + { + continue; + } Assert.Contains(expected.LogMessageWarning, _logger.Collector.GetSnapshot().Select(x => x.Message)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 105 - 108, The test iterates testCase.Expects and calls Assert.Contains with expected.LogMessageWarning which is nullable; update the loop in DataModelFieldCalculatorTests so you only call Assert.Contains when expected.LogMessageWarning is not null (e.g. if (expected.LogMessageWarning != null) { Assert.Contains(expected.LogMessageWarning, _logger.Collector.GetSnapshot().Select(x => x.Message)); }), leaving the behavior unchanged for non-null warnings and skipping the assertion when LogMessageWarning is null; reference the symbols testCase.Expects, expected.LogMessageWarning, _logger.Collector.GetSnapshot(), and Assert.Contains to locate the change.
🧹 Nitpick comments (1)
src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs (1)
42-58: Consider batching or documenting why sequential processing is required.The
foreachloop awaits async operations (CanReadandCalculateFormData) on each iteration. If calculations are independent, parallel processing with bounded concurrency could improve performance.However, if sequential processing is intentional (e.g., calculations depend on prior results), consider adding a comment documenting this design decision.
♻️ Example parallel approach (if independence is confirmed)
+ // If calculations are independent, consider: + var tasks = dataAccessor.GetDataElementsWithFormDataForTask(taskId) + .Select(async tuple => + { + var (dataType, dataElement) = tuple; + if (await _dataElementAccessChecker.CanRead(dataAccessor.Instance, dataType) is false) + return; + var calculationConfig = _appResourceService.GetCalculationConfiguration(dataType.Id); + if (!string.IsNullOrEmpty(calculationConfig)) + await CalculateFormData(dataAccessor, dataElement, taskId, calculationConfig); + }); + await Task.WhenAll(tasks);As per coding guidelines: "Do not await async operations in a loop; prefer batching with upper bounds on parallelism".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs` around lines 42 - 58, The Calculate method in DataModelFieldCalculator performs awaits inside a foreach (calls to _dataElementAccessChecker.CanRead and CalculateFormData) which forces sequential execution; either implement bounded parallelism if each dataElement is independent (e.g., collect the data elements from dataAccessor.GetDataElementsWithFormDataForTask(taskId), filter by awaiting CanRead with a limited concurrency pattern such as SemaphoreSlim and then run CalculateFormData tasks with a bounded Task.WhenAll), or, if ordering/side-effects require sequential processing, add a clear comment above Calculate explaining that sequential execution is intentional and why; reference the Calculate method, _dataElementAccessChecker.CanRead, CalculateFormData, and dataAccessor.GetDataElementsWithFormDataForTask when making the change.
🤖 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/Features/DataProcessing/DataModelFieldCalculator.cs`:
- Around line 188-192: The log message in DataModelFieldCalculator when checking
dataModelFieldCalculationDefinition.Expression is misleading; update the
_logger.LogError call inside the null check for
dataModelFieldCalculationDefinition.Expression to say "missing expression"
(reference: DataModelFieldCalculator,
dataModelFieldCalculationDefinition.Expression, _logger.LogError) so the message
accurately reflects the missing property.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 201-216: The loop in GetResolvedKeysRecursive iterates
childModelList and recurses into every child (including null), causing keys for
removed rows to be emitted; update the loop to skip null entries by checking the
child variable (from childModelList) before calling GetResolvedKeysRecursive and
only recurse when child != null, still incrementing the index i and using
JoinFieldKeyParts/currentKey as before so keys for null rows are not produced
during calculation mode.
- Around line 180-186: The property lookup currently uses
currentType.GetProperties(), which fails for polymorphic instances; change the
lookup to use the runtime type when available by resolving the search type as
currentModel?.GetType() ?? currentType before calling GetProperties(), then use
IsPropertyWithJsonName against that resolved type (affecting the lookup that
assigns prop and subsequent childType resolution) so derived JSON-mapped
properties are found while preserving null-safety.
---
Duplicate comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs`:
- Around line 105-108: The test iterates testCase.Expects and calls
Assert.Contains with expected.LogMessageWarning which is nullable; update the
loop in DataModelFieldCalculatorTests so you only call Assert.Contains when
expected.LogMessageWarning is not null (e.g. if (expected.LogMessageWarning !=
null) { Assert.Contains(expected.LogMessageWarning,
_logger.Collector.GetSnapshot().Select(x => x.Message)); }), leaving the
behavior unchanged for non-null warnings and skipping the assertion when
LogMessageWarning is null; reference the symbols testCase.Expects,
expected.LogMessageWarning, _logger.Collector.GetSnapshot(), and Assert.Contains
to locate the change.
In
`@test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt`:
- Around line 2945-2946: The addition of GetCalculationConfiguration(string
dataTypeId) to the public IAppResources interface is a breaking change; either
provide a default interface implementation on IAppResources (implement
GetCalculationConfiguration returning null or a sensible default) or move the
new member into a separate derived interface (e.g.,
IAppResourcesWithCalculationConfig) that consumers can opt into; locate the
IAppResources interface declaration and implement one of these approaches to
restore binary compatibility while allowing new behavior.
---
Nitpick comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs`:
- Around line 42-58: The Calculate method in DataModelFieldCalculator performs
awaits inside a foreach (calls to _dataElementAccessChecker.CanRead and
CalculateFormData) which forces sequential execution; either implement bounded
parallelism if each dataElement is independent (e.g., collect the data elements
from dataAccessor.GetDataElementsWithFormDataForTask(taskId), filter by awaiting
CanRead with a limited concurrency pattern such as SemaphoreSlim and then run
CalculateFormData tasks with a bounded Task.WhenAll), or, if
ordering/side-effects require sequential processing, add a clear comment above
Calculate explaining that sequential execution is intentional and why; reference
the Calculate method, _dataElementAccessChecker.CanRead, CalculateFormData, and
dataAccessor.GetDataElementsWithFormDataForTask when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22da5f6d-2d1e-44d8-889f-2d1df3e65472
📒 Files selected for processing (12)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cssrc/Altinn.App.Core/Models/RawDataModelFieldCalculation.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cstest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/component-lookup-hidden.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/hidden-field.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/hidden-page.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/single-expression-boolean.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/single-expression-number.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
✅ Files skipped from review due to trivial changes (4)
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/single-expression-number.json
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/hidden-page.json
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/single-expression-boolean.json
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/hidden-field.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.App.Core/Models/RawDataModelFieldCalculation.cs
- src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
…on value kind is number.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs (2)
134-144:⚠️ Potential issue | 🟡 MinorAdd null check for
expected.Fieldbefore using it.
expected.Fieldis nullable (string?) but is used directly at line 138 and in the failure message at line 142. This could cause aNullReferenceExceptionor misleading assertion failure message.🛡️ Proposed fix
foreach (var expected in testCase.Expects) { + if (expected.Field is null) + { + Assert.Fail("Test case 'expects' entry is missing required 'field' property"); + } if (expected.Result.HasValue) { Assert.Equal(expected.Result.Value.ToObject(), result.Get(expected.Field)); } else { Assert.Fail($"Expected result for field {expected.Field} not found"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 134 - 144, The loop uses expected.Field (nullable string?) directly—add a null check before using it: assert that expected.Field is not null (e.g., Assert.NotNull(expected.Field)) at the top of the foreach iteration for testCase.Expects, then use the non-null value when calling result.Get(expected.Field) (or expected.Field!); also update the failure message to handle the null case explicitly (e.g., fail with "Expected field is null for test case" if null) so no NullReferenceException or misleading interpolation occurs; references: testCase.Expects, expected.Field, result.Get.
122-125:⚠️ Potential issue | 🟡 MinorAdd null check for
LogMessagebefore assertion.
expected.LogMessageis nullable (string?) but is used directly inAssert.Contains. If a test case'sexpectsarray contains an entry withoutlogMessage, this will passnullto the assertion.🛡️ Proposed fix
foreach (var expected in testCase.Expects) { + if (expected.LogMessage is null) + { + continue; + } Assert.Contains(expected.LogMessage, _logger.Collector.GetSnapshot().Select(x => x.Message)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 122 - 125, The test iterates testCase.Expects and calls Assert.Contains with expected.LogMessage which is nullable; update the loop in DataModelFieldCalculatorTests so you only call Assert.Contains when expected.LogMessage is not null (e.g., if (!string.IsNullOrEmpty(expected.LogMessage)) { Assert.Contains(expected.LogMessage, _logger.Collector.GetSnapshot().Select(x => x.Message)); }) or alternatively assert that expected.LogMessage is not null before the Contains call; reference the variables expected.LogMessage, testCase.Expects, _logger.Collector.GetSnapshot() and Assert.Contains when making the change.
🧹 Nitpick comments (3)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs (1)
58-63: Consider handling deserialization failure explicitly.The null-forgiving operator (
!) at line 62 suppresses the null warning but could mask test data loading failures. Consider throwing a more descriptive exception.♻️ Proposed improvement
private async Task<DataModelFieldCalculatorTestModel> LoadData(string fileName, string folder) { var data = await File.ReadAllTextAsync(Path.Join(folder, fileName)); _output.WriteLine(data); - return JsonSerializer.Deserialize<DataModelFieldCalculatorTestModel>(data, _jsonSerializerOptions)!; + return JsonSerializer.Deserialize<DataModelFieldCalculatorTestModel>(data, _jsonSerializerOptions) + ?? throw new InvalidOperationException($"Failed to deserialize test data from {fileName}"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 58 - 63, The current LoadData method uses the null-forgiving operator after JsonSerializer.Deserialize, which can hide failures; change LoadData (the method) to check the result of JsonSerializer.Deserialize<DataModelFieldCalculatorTestModel>(data, _jsonSerializerOptions) and if it is null throw a clear exception (e.g. InvalidOperationException or FileLoadException) that includes the fileName and folder (and optionally a snippet or length of data) so test failures show descriptive diagnostics instead of a suppressed null reference.src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs (2)
148-175: Consider adding error handling for JSON parsing.
JsonDocument.Parseat line 152 will throw aJsonExceptionifrawCalculationConfigcontains invalid JSON. While the caller may handle this, defensive error handling with a descriptive log message would improve debuggability.♻️ Proposed improvement
private Dictionary<string, DataModelFieldCalculation> ParseDataModelFieldCalculationConfig( string rawCalculationConfig ) { - using var calculationConfigDocument = JsonDocument.Parse(rawCalculationConfig); + JsonDocument calculationConfigDocument; + try + { + calculationConfigDocument = JsonDocument.Parse(rawCalculationConfig); + } + catch (JsonException ex) + { + _logger.LogError(ex, "Failed to parse calculation configuration JSON"); + return new Dictionary<string, DataModelFieldCalculation>(); + } + using (calculationConfigDocument) + { var dataModelFieldCalculations = new Dictionary<string, DataModelFieldCalculation>(); // ... rest of the method + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs` around lines 148 - 175, Wrap the JsonDocument.Parse(rawCalculationConfig) call inside a try/catch that catches JsonException (and optionally other Exceptions), log a descriptive error via _logger.LogError including the rawCalculationConfig or a trimmed snippet and the exception (e.g., in ParseDataModelFieldCalculationConfig), and return an empty Dictionary<string, DataModelFieldCalculation> when parsing fails so callers get a safe fallback; ensure you do not rethrow so the method's contract remains the same.
42-58: Consider adding per-calculation telemetry spans.Basic telemetry is present via
StartCalculateActivity, but for comprehensive observability, consider adding child spans for each data element calculation and metrics for tracking calculation counts/failures.As per coding guidelines
src/**/*.cs: Comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs` around lines 42 - 58, Add per-calculation telemetry by creating a child span for each data element inside Calculate: after the access check and after obtaining calculationConfig (in Calculate method), start a child activity/span from _telemetry (or ActivitySource) for that specific data element (include dataType.Id, taskId, instance id), record start/stop and set attributes for dataType and calculationConfig, and wrap the call to CalculateFormData so exceptions are captured and a failure metric is incremented; also increment a success/failure counter metric for each calculation attempt (use the same telemetry abstraction used by _telemetry and reference CalculateFormData, _telemetry?.StartCalculateActivity, _appResourceService.GetCalculationConfiguration, and _dataElementAccessChecker.CanRead to locate insertion points).
🤖 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/Features/DataProcessing/DataModelFieldCalculator.cs`:
- Around line 84-92: The StartsWith check in DataModelFieldCalculator that skips
hidden fields is too broad and can mistakenly match sibling fields (e.g., "name"
matching "namePrefix"); replace the Exists predicate to use an
exact-or-descendant comparison: add a private helper
IsSameOrDescendantField(string candidate, string hiddenField) that returns true
when candidate equals hiddenField or when candidate starts with hiddenField and
the next character is a segment boundary ('.' or '['), then use
IsSameOrDescendantField(resolvedField.Field, d.Field) instead of
resolvedField.Field.StartsWith(...); update references in the skipping logic
accordingly.
---
Duplicate comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs`:
- Around line 134-144: The loop uses expected.Field (nullable string?)
directly—add a null check before using it: assert that expected.Field is not
null (e.g., Assert.NotNull(expected.Field)) at the top of the foreach iteration
for testCase.Expects, then use the non-null value when calling
result.Get(expected.Field) (or expected.Field!); also update the failure message
to handle the null case explicitly (e.g., fail with "Expected field is null for
test case" if null) so no NullReferenceException or misleading interpolation
occurs; references: testCase.Expects, expected.Field, result.Get.
- Around line 122-125: The test iterates testCase.Expects and calls
Assert.Contains with expected.LogMessage which is nullable; update the loop in
DataModelFieldCalculatorTests so you only call Assert.Contains when
expected.LogMessage is not null (e.g., if
(!string.IsNullOrEmpty(expected.LogMessage)) {
Assert.Contains(expected.LogMessage, _logger.Collector.GetSnapshot().Select(x =>
x.Message)); }) or alternatively assert that expected.LogMessage is not null
before the Contains call; reference the variables expected.LogMessage,
testCase.Expects, _logger.Collector.GetSnapshot() and Assert.Contains when
making the change.
---
Nitpick comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs`:
- Around line 148-175: Wrap the JsonDocument.Parse(rawCalculationConfig) call
inside a try/catch that catches JsonException (and optionally other Exceptions),
log a descriptive error via _logger.LogError including the rawCalculationConfig
or a trimmed snippet and the exception (e.g., in
ParseDataModelFieldCalculationConfig), and return an empty Dictionary<string,
DataModelFieldCalculation> when parsing fails so callers get a safe fallback;
ensure you do not rethrow so the method's contract remains the same.
- Around line 42-58: Add per-calculation telemetry by creating a child span for
each data element inside Calculate: after the access check and after obtaining
calculationConfig (in Calculate method), start a child activity/span from
_telemetry (or ActivitySource) for that specific data element (include
dataType.Id, taskId, instance id), record start/stop and set attributes for
dataType and calculationConfig, and wrap the call to CalculateFormData so
exceptions are captured and a failure metric is incremented; also increment a
success/failure counter metric for each calculation attempt (use the same
telemetry abstraction used by _telemetry and reference CalculateFormData,
_telemetry?.StartCalculateActivity,
_appResourceService.GetCalculationConfiguration, and
_dataElementAccessChecker.CanRead to locate insertion points).
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs`:
- Around line 58-63: The current LoadData method uses the null-forgiving
operator after JsonSerializer.Deserialize, which can hide failures; change
LoadData (the method) to check the result of
JsonSerializer.Deserialize<DataModelFieldCalculatorTestModel>(data,
_jsonSerializerOptions) and if it is null throw a clear exception (e.g.
InvalidOperationException or FileLoadException) that includes the fileName and
folder (and optionally a snippet or length of data) so test failures show
descriptive diagnostics instead of a suppressed null reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7578771d-cac8-4f67-bf41-9ffed51ca4d5
📒 Files selected for processing (6)
src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cstest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/assert-logger/parse-none-existing-definition.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/assert-logger/parse-none-existing-expression.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/assert-logger/unsupported-data-type.json
✅ Files skipped from review due to trivial changes (3)
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/assert-logger/parse-none-existing-expression.json
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/assert-logger/parse-none-existing-definition.json
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/assert-logger/unsupported-data-type.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs (1)
44-57:⚠️ Potential issue | 🟠 MajorAdd deeper telemetry for per-element/per-calculation execution.
Line 44 starts a parent activity, but there’s still no granular tracing/metrics around each data element and each
RunCalculationcall (including failure counters). This makes root-cause analysis and SLA tracking difficult for this feature path.As per coding guidelines "
src/**/*.cs: Comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs` around lines 44 - 57, The loop in DataModelFieldCalculator currently only starts a single parent telemetry activity; add granular telemetry by creating a child activity for each data element and a nested child (or measurement) around each RunCalculation call to record duration, success/failure counters and exception details; specifically, wrap the foreach body (iterating dataType/dataElement from dataAccessor.GetDataElementsWithFormDataForTask) with a per-element activity including tags taskId, dataType.Id and dataElement.Id, then inside CalculateFormData/RunCalculation record a per-calculation activity or timer and increment success/failure counters and log exceptions via _telemetry (use StartCalculateActivity or a new StartActivity variant and _telemetry.RecordException/_telemetry.IncrementCounter) so each element and each RunCalculation is traced and failures are counted.
🤖 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/Features/DataProcessing/DataModelFieldCalculator.cs`:
- Around line 15-19: In DataModelFieldCalculator.cs, update the code that calls
JsonDocument.Parse(...) (the parsing of the field calculator/config JSON) to use
JsonDocumentOptions with CommentHandling = JsonCommentHandling.Skip instead of
relying on _jsonSerializerOptions, and wrap the Parse call in a try-catch that
catches exceptions (e.g., JsonException/Exception), logs the error via the
existing logger, and returns an empty Dictionary<string, object> (or the same
empty result type currently expected) on failure; keep the existing
_jsonSerializerOptions for JsonSerializer.Deserialize calls but ensure parsing
uses JsonDocumentOptions as described.
---
Duplicate comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs`:
- Around line 44-57: The loop in DataModelFieldCalculator currently only starts
a single parent telemetry activity; add granular telemetry by creating a child
activity for each data element and a nested child (or measurement) around each
RunCalculation call to record duration, success/failure counters and exception
details; specifically, wrap the foreach body (iterating dataType/dataElement
from dataAccessor.GetDataElementsWithFormDataForTask) with a per-element
activity including tags taskId, dataType.Id and dataElement.Id, then inside
CalculateFormData/RunCalculation record a per-calculation activity or timer and
increment success/failure counters and log exceptions via _telemetry (use
StartCalculateActivity or a new StartActivity variant and
_telemetry.RecordException/_telemetry.IncrementCounter) so each element and each
RunCalculation is traced and failures are counted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24310868-03ba-4711-9b2f-9547dd8b4057
📒 Files selected for processing (1)
src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Features/DataProcessing/DataModelFieldCalculatorTests.cs`:
- Around line 38-39: The fields _dataElement and _instanceDataAccessor are
declared as nullable but are always initialized in Setup() and then passed to
methods that expect non-nullable parameters; change their declarations to use
the null-forgiving initializer so the compiler knows they're intentionally
initialized (e.g. replace "private DataElement? _dataElement;" and "private
IInstanceDataAccessor? _instanceDataAccessor;" with "private DataElement
_dataElement = null!;" and "private IInstanceDataAccessor _instanceDataAccessor
= null!;") to eliminate CS8604 warnings while keeping Setup() initialization
logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3a3ccba-620a-480b-b1f5-ba6df6cac4b8
📒 Files selected for processing (2)
src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs (2)
122-125:⚠️ Potential issue | 🟡 MinorGuard nullable
expected.LogMessagebefore asserting logger contents.
expected.LogMessageis nullable (string?) but is passed directly toAssert.Contains. Add an explicit guard/fail path so malformed test data gives a deterministic failure instead of nullable-flow issues.Suggested change
+ var loggedMessages = _logger.Collector.GetSnapshot().Select(x => x.Message).ToList(); foreach (var expected in testCase.Expects) { - Assert.Contains(expected.LogMessage, _logger.Collector.GetSnapshot().Select(x => x.Message)); + if (expected.LogMessage is null) + { + Assert.Fail($"Expected logMessage is null for test case \"{testCase.Name}\""); + } + Assert.Contains(expected.LogMessage, loggedMessages); }#!/bin/bash set -euo pipefail python - <<'PY' import json, pathlib roots=[p for p in pathlib.Path(".").rglob("data-field-value-calculator-tests") if p.is_dir()] for root in roots: target=root/"assert-logger" if not target.exists(): continue for f in target.rglob("*.json"): data=json.loads(f.read_text(encoding="utf-8")) for i, exp in enumerate(data.get("expects", [])): if exp.get("logMessage") is None: print(f"{f}: expects[{i}] missing logMessage") PYAs per coding guidelines
**/*.cs: Use Nullable Reference Types in C# code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 122 - 125, The test iterates testCase.Expects and calls Assert.Contains with expected.LogMessage which is nullable; guard against null to give a deterministic failure: check expected.LogMessage != null (e.g., Assert.NotNull or Assert.False with a clear message referencing the testCase and index) before calling Assert.Contains, and then use expected.LogMessage in the Assert.Contains against _logger.Collector.GetSnapshot().Select(x => x.Message); update the loop around testCase.Expects to validate expected.LogMessage for each expected entry and fail with a descriptive assertion if it's null.
134-143:⚠️ Potential issue | 🟡 MinorValidate nullable
expected.Fieldandexpected.Resultbefore dereferencing.
expected.Fieldandexpected.Resultare nullable, but both are dereferenced in assertions. Add explicit validation so invalid test fixture entries fail clearly.Suggested change
foreach (var expected in testCase.Expects) { - if (expected.Result.HasValue) + if (expected.Field is null) + { + Assert.Fail($"Expected field is null for test case \"{testCase.Name}\""); + } + else if (expected.Result.HasValue) { Assert.Equal(expected.Result.Value.ToObject(), result.Get(expected.Field)); } else { Assert.Fail($"Expected result for field {expected.Field} not found"); } }#!/bin/bash set -euo pipefail python - <<'PY' import json, pathlib roots=[p for p in pathlib.Path(".").rglob("data-field-value-calculator-tests") if p.is_dir()] for root in roots: for f in root.rglob("*.json"): if "assert-logger" in f.parts: continue data=json.loads(f.read_text(encoding="utf-8")) for i, exp in enumerate(data.get("expects", [])): if exp.get("field") is None or "result" not in exp: print(f"{f}: expects[{i}] missing field/result") PYAs per coding guidelines
**/*.cs: Use Nullable Reference Types in C# code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 134 - 143, In DataModelFieldCalculatorTests inside the foreach over testCase.Expects, validate that expected.Field and expected.Result are not null before dereferencing: add explicit assertions (e.g., Assert.NotNull(expected.Field) and Assert.True(expected.Result.HasValue, ...)) or fail with a clear message when either is missing, then proceed to use expected.Field and expected.Result.Value.ToObject() in the existing Assert.Equal against result.Get(expected.Field); reference the loop variable expected and its properties Field and Result and the result.Get(...) call so the null checks are added immediately before those dereferences.
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs (2)
208-236: Narrow nested test DTO visibility.
DataModelFieldCalculatorTestModelandExpectedare only used inside this test class; make them non-public to keep the type surface minimal.As per coding guidelines
**/*.cs: Use internal accessibility on types by default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 208 - 236, The two test DTO records DataModelFieldCalculatorTestModel and Expected are publicly visible but only used inside this test; change their accessibility from public to internal (per project guideline to default to internal for types) so they are non-public; update the declarations for DataModelFieldCalculatorTestModel and Expected accordingly.
22-23: Seal the test class unless inheritance is intentional.
DataModelFieldCalculatorTestsdoes not appear to be extended; making itsealedaligns with repository defaults and reduces accidental inheritance surface.Suggested change
-public class DataModelFieldCalculatorTests +public sealed class DataModelFieldCalculatorTestsAs per coding guidelines
**/*.cs: Use sealed for classes unless inheritance is considered a valid use-case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 22 - 23, The test class DataModelFieldCalculatorTests is not intended for inheritance—mark it sealed to follow repository conventions; update the class declaration for DataModelFieldCalculatorTests to add the sealed modifier so it becomes "sealed class DataModelFieldCalculatorTests", ensuring no other behavior changes.
🤖 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/Features/DataProcessing/DataModelFieldCalculatorTests.cs`:
- Around line 122-125: The test iterates testCase.Expects and calls
Assert.Contains with expected.LogMessage which is nullable; guard against null
to give a deterministic failure: check expected.LogMessage != null (e.g.,
Assert.NotNull or Assert.False with a clear message referencing the testCase and
index) before calling Assert.Contains, and then use expected.LogMessage in the
Assert.Contains against _logger.Collector.GetSnapshot().Select(x => x.Message);
update the loop around testCase.Expects to validate expected.LogMessage for each
expected entry and fail with a descriptive assertion if it's null.
- Around line 134-143: In DataModelFieldCalculatorTests inside the foreach over
testCase.Expects, validate that expected.Field and expected.Result are not null
before dereferencing: add explicit assertions (e.g.,
Assert.NotNull(expected.Field) and Assert.True(expected.Result.HasValue, ...))
or fail with a clear message when either is missing, then proceed to use
expected.Field and expected.Result.Value.ToObject() in the existing Assert.Equal
against result.Get(expected.Field); reference the loop variable expected and its
properties Field and Result and the result.Get(...) call so the null checks are
added immediately before those dereferences.
---
Nitpick comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs`:
- Around line 208-236: The two test DTO records
DataModelFieldCalculatorTestModel and Expected are publicly visible but only
used inside this test; change their accessibility from public to internal (per
project guideline to default to internal for types) so they are non-public;
update the declarations for DataModelFieldCalculatorTestModel and Expected
accordingly.
- Around line 22-23: The test class DataModelFieldCalculatorTests is not
intended for inheritance—mark it sealed to follow repository conventions; update
the class declaration for DataModelFieldCalculatorTests to add the sealed
modifier so it becomes "sealed class DataModelFieldCalculatorTests", ensuring no
other behavior changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2a3eab4-f47f-4460-af59-62bec7b7e961
📒 Files selected for processing (1)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs
|
Konrad-Simso
approved these changes
Mar 26, 2026
Contributor
Konrad-Simso
left a comment
Konrad-Simso
left a comment
There was a problem hiding this comment.
Nice work, this was my first time taking a deep dive into this code base so i can't guarantee i didn't miss something.
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
2 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 logic for setting data field by expression through an IDataWriteProcessor implementation called DataFieldValueCalculator. This data processor consumes calculation.json files where expressions that evaluates and sets data model fields is defined. Much of the logic is just a copy of whats found in ExpressionValidation.
Related PRs:
calculation.schema.V1.json - Altinn/app-frontend-react#4048
Docs - Altinn/altinn-studio-docs#2763
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
calculation.jsonconfiguration files, enabling automatic computation of data model fields using expressions based on other field values.