Set data field by expression#1683
Merged
Merged
Conversation
…ve method can be used during calculation of data fields using expressions. Added unit tests for the DataFieldValueCalculator
olavsorl
added
feature
Contributor
|
Warning Review limit reached
More reviews will be available in 44 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds calculation-config support and a runtime that computes and writes calculated data fields. Includes app settings, DI registrations, DataModelFieldCalculator (plus processor integration), collection-aware key-resolution enhancements across DataModelWrapper/form-data layers, a new expression evaluation API returning ExpressionValue, telemetry hooks, expression validator refactoring, comprehensive tests and fixtures, and CI workflow trigger updates. ChangesCalculation configuration and runtime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
* Workflow tweaking Run tests on all PRs, but only check labels on main and release. * Also run on pull_request edited when the edit is a base.ref change
ExpressionValidation now checks for null by it self, not relying on isCalculating = false. (added a shared test for this)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs (1)
120-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard nullable expected fields before asserting.
Expected.LogMessageandExpected.Fieldare nullable, but both are used directly in assertions. This can fail tests for fixture-shape reasons rather than feature behavior.Proposed fix
foreach (var expected in testCase.Expects) { - Assert.Contains(expected.LogMessage, _logger.Collector.GetSnapshot().Select(x => x.Message)); + if (expected.LogMessage is not null) + { + Assert.Contains(expected.LogMessage, _logger.Collector.GetSnapshot().Select(x => x.Message)); + } } 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)); Assert.Empty(_logger.Collector.GetSnapshot()); } else { Assert.Fail($"Expected result for field {expected.Field} not found"); } }As per coding guidelines: “Use Nullable Reference Types in C# code.”
Also applies to: 132-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs` around lines 120 - 123, The test assertions are using nullable fields expected.LogMessage and expected.Field without null guards, which can cause tests to fail for fixture shape reasons rather than actual feature behavior. Add null checks to guard both nullable fields before using them in assertions within the foreach loop that iterates over testCase.Expects. Only perform the Assert.Contains call for expected.LogMessage when it is not null, and similarly guard the expected.Field usage at the other affected location. This ensures assertions only execute when the required data is present.Source: Coding guidelines
🧹 Nitpick comments (3)
test/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGetResolvedKeys.cs (1)
11-11: 💤 Low valuePrimary constructor with empty parameters is unnecessary.
The class uses primary constructor syntax
public class TestGetResolvedKeys()but has no constructor parameters. This can be simplified topublic class TestGetResolvedKeyswithout the parentheses.Proposed fix
-public class TestGetResolvedKeys() +public class TestGetResolvedKeys🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGetResolvedKeys.cs` at line 11, The TestGetResolvedKeys class declaration uses primary constructor syntax with empty parentheses, which is unnecessary since no constructor parameters are defined. Remove the empty parentheses from the class declaration by changing public class TestGetResolvedKeys() to public class TestGetResolvedKeys, simplifying the syntax without affecting functionality.src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs (1)
111-127: ⚡ Quick winCache
GetLayoutEvaluatorState()result to avoid repeated calls.
GetLayoutEvaluatorState()is called on line 113 and again on lines 123-124 inside the loop. Caching the result once at the start of the method would improve efficiency.♻️ Proposed refactor
{ + var evaluatorState = dataAccessor.GetLayoutEvaluatorState(); var formDataWrapper = await dataAccessor.GetFormDataWrapper(dataElement); var hiddenFields = await LayoutEvaluator.GetHiddenFieldsForRemoval( - dataAccessor.GetLayoutEvaluatorState(), + evaluatorState, evaluateRemoveWhenHidden: false ); var validationIssues = new List<ValidationIssue>(); DataElementIdentifier dataElementIdentifier = dataElement; var expressionValidations = ParseExpressionValidationConfig(rawValidationConfig, _logger); foreach (var (baseField, validations) in expressionValidations) { - var resolvedFields = await dataAccessor - .GetLayoutEvaluatorState() - .GetResolvedKeys( + var resolvedFields = await evaluatorState.GetResolvedKeys( new DataReference() { Field = baseField, DataElementIdentifier = dataElementIdentifier } );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs` around lines 111 - 127, The GetLayoutEvaluatorState() method is called multiple times within the ExpressionValidator method—once when fetching hidden fields and again inside the foreach loop. Cache the result of dataAccessor.GetLayoutEvaluatorState() into a local variable at the start of the method (after line 111), then replace the direct calls on lines 113 (in the LayoutEvaluator.GetHiddenFieldsForRemoval call) and lines 123-124 (inside the foreach loop) with references to this cached variable to avoid repeated method invocations.src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs (1)
42-53: 🏗️ Heavy liftBatch async work with bounded parallelism instead of awaiting inside loops.
The current loop structure awaits each async operation sequentially, which can significantly increase processing time for instances with many data elements/fields. Please refactor to bounded concurrency (e.g.,
Parallel.ForEachAsync/SemaphoreSlim) where ordering is not required.As per coding guidelines: “Don't await async operations in a loop (prefer batching, but have an upper bound on parallelism that makes sense).”
Also applies to: 67-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs` around lines 42 - 53, The current implementation awaits async operations sequentially inside the foreach loop (in the loop containing the CalculateFormData call), which causes inefficient processing. Refactor this loop and the similar pattern mentioned at the alternate location to use bounded concurrency instead, such as Parallel.ForEachAsync or SemaphoreSlim, to process multiple data elements concurrently while respecting an upper bound on parallelism. This will eliminate sequential awaiting and improve performance for instances with many data elements.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt`:
- Line 2159: The ExpressionValidator constructor signature has been changed from
accepting ILayoutEvaluatorStateInitializer to IServiceProvider, which breaks
backward compatibility for external code. Add a constructor overload to
ExpressionValidator that accepts the old signature with
ILayoutEvaluatorStateInitializer as a parameter and forwards the call to the new
constructor with IServiceProvider, ensuring the old parameter is properly
converted or adapted to work with the new implementation.
---
Duplicate comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs`:
- Around line 120-123: The test assertions are using nullable fields
expected.LogMessage and expected.Field without null guards, which can cause
tests to fail for fixture shape reasons rather than actual feature behavior. Add
null checks to guard both nullable fields before using them in assertions within
the foreach loop that iterates over testCase.Expects. Only perform the
Assert.Contains call for expected.LogMessage when it is not null, and similarly
guard the expected.Field usage at the other affected location. This ensures
assertions only execute when the required data is present.
---
Nitpick comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs`:
- Around line 42-53: The current implementation awaits async operations
sequentially inside the foreach loop (in the loop containing the
CalculateFormData call), which causes inefficient processing. Refactor this loop
and the similar pattern mentioned at the alternate location to use bounded
concurrency instead, such as Parallel.ForEachAsync or SemaphoreSlim, to process
multiple data elements concurrently while respecting an upper bound on
parallelism. This will eliminate sequential awaiting and improve performance for
instances with many data elements.
In `@src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs`:
- Around line 111-127: The GetLayoutEvaluatorState() method is called multiple
times within the ExpressionValidator method—once when fetching hidden fields and
again inside the foreach loop. Cache the result of
dataAccessor.GetLayoutEvaluatorState() into a local variable at the start of the
method (after line 111), then replace the direct calls on lines 113 (in the
LayoutEvaluator.GetHiddenFieldsForRemoval call) and lines 123-124 (inside the
foreach loop) with references to this cached variable to avoid repeated method
invocations.
In
`@test/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGetResolvedKeys.cs`:
- Line 11: The TestGetResolvedKeys class declaration uses primary constructor
syntax with empty parentheses, which is unnecessary since no constructor
parameters are defined. Remove the empty parentheses from the class declaration
by changing public class TestGetResolvedKeys() to public class
TestGetResolvedKeys, simplifying the syntax without affecting functionality.
🪄 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: 1b0081ff-f0e2-4a6c-abf5-fcc083280000
📒 Files selected for processing (17)
src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cssrc/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Internal/Data/IFormDataWrapper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cstest/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_FailingValidator_ReturnsValidationErrors.verified.txttest/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_PdfFails_DataIsUnlocked.verified.txttest/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cstest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/calculate-in-group.jsontest/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/Validators/Default/ExpressionValidatorTests.cstest/Altinn.App.Core.Tests/Features/Validators/expression-validation-tests/shared/ignore-value-null.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.SourceGenerator.Integration.Tests/UnitTest/TestGetResolvedKeys.cs
💤 Files with no reviewable changes (2)
- test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_FailingValidator_ReturnsValidationErrors.verified.txt
- test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_PdfFails_DataIsUnlocked.verified.txt
✅ Files skipped from review due to trivial changes (1)
- test/Altinn.App.Core.Tests/Features/Validators/expression-validation-tests/shared/ignore-value-null.json
When resolving keys that end in an enumerable, there are two interpetations that makes sense You either want the key to the list, or the key for each row in the list model.group => ["model.group"] or model.group => ["model.group[0]", "model.group[1]"] To resolve this we disambiguate by having you specify `model.group[]` if you want the latter.
ivarne
force-pushed
the
feature/set-data-field-by-expression
branch
from
3888591 to
ebcb7a5
Compare
github-project-automation
Bot
moved this from 🔎 In review
to ✅ Done
in Team Altinn Studio
|
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Added 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.jsonfilename.group[]and related null/index cases) affecting calculations and validation context.