Open
Conversation
…ve method can be used during calculation of data fields using expressions. Added unit tests for the DataFieldValueCalculator
olavsorl
added
feature
Contributor
📝 WalkthroughWalkthroughAdds a data-field calculation feature: new calculation engine and processor, support for loading per-model calculation JSON, expression evaluation extension, propagation of an isCalculating flag through key-resolution, telemetry hooks, DI registrations, model/refactor changes, and accompanying tests and test data. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Comment on lines
+185
to
+186
| services.TryAddTransient<IDataWriteProcessor, DataFieldValueCalculatorProcessor>(); | ||
| services.TryAddTransient<DataFieldValueCalculator>(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all IDataWriteProcessor registrations and usage
echo "=== IDataWriteProcessor registrations and references ==="
rg -n "IDataWriteProcessor" --type=cs -B2 -A2
echo ""
echo "=== GetAll<IDataWriteProcessor> usage ==="
rg -n "GetAll.*IDataWriteProcessor" --type=cs -B3 -A3
echo ""
echo "=== DataFieldValueCalculatorProcessor class definition ==="
rg -n "class DataFieldValueCalculatorProcessor" --type=cs -B2 -A10Repository: Altinn/app-lib-dotnet
Length of output: 10062
Use AddTransient instead of TryAddTransient for IDataWriteProcessor.
The code uses GetAll<IDataWriteProcessor>() to retrieve all registered implementations (InternalPatchService.cs line 269), iterating over each in a loop. With TryAddTransient, if an app registers its own IDataWriteProcessor, the core's DataFieldValueCalculatorProcessor will be skipped entirely, preventing the default processor from executing alongside app-provided implementations.
Change line 185 to services.AddTransient<IDataWriteProcessor, DataFieldValueCalculatorProcessor>(); to ensure the core processor is always included.
Additionally, DataFieldValueCalculatorProcessor should be marked sealed per coding guidelines unless inheritance is a valid use-case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs` around lines
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.
Comment on lines
+40
to
+55
| public async Task Calculate(IInstanceDataAccessor dataAccessor, string taskId) | ||
| { | ||
| foreach (var (dataType, dataElement) in dataAccessor.GetDataElementsWithFormDataForTask(taskId)) | ||
| { | ||
| if (await _dataElementAccessChecker.CanRead(dataAccessor.Instance, dataType) is false) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| var calculationConfig = _appResourceService.GetCalculationConfiguration(dataType.Id); | ||
| if (!string.IsNullOrEmpty(calculationConfig)) | ||
| { | ||
| await CalculateFormData(dataAccessor, dataElement, taskId, calculationConfig); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add OpenTelemetry instrumentation for this feature path.
This new calculation engine currently logs, but lacks explicit tracing/metrics around calculation lifecycle and failures. Please add spans/counters (e.g., per data element, per calculation, failures, conversion 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/DataFieldValueCalculator.cs`
around lines 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.
Comment on lines
+81
to
+84
| hiddenFields.Exists(d => | ||
| d.DataElementIdentifier == resolvedField.DataElementIdentifier | ||
| && resolvedField.Field.StartsWith(d.Field, StringComparison.InvariantCulture) | ||
| ) |
Contributor
There was a problem hiding this comment.
StartsWith filtering can skip unrelated fields.
resolvedField.Field.StartsWith(d.Field, ...) is too broad and can match sibling names. Use an exact-or-descendant path check with segment boundaries.
🐛 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 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.
Comment on lines
+124
to
133
| public string[] GetResolvedKeys(string field, bool isCalculating = false) | ||
| { | ||
| if (_dataModel is null) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| var fieldParts = field.Split('.'); | ||
| return GetResolvedKeysRecursive(fieldParts, _dataModel); | ||
| return GetResolvedKeysRecursive(fieldParts, _dataModel, _dataModel.GetType(), isCalculating: isCalculating); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs | sed -n '124,245p'Repository: Altinn/app-lib-dotnet
Length of output: 5511
🏁 Script executed:
find . -name "*.csproj" -path "*/Altinn.App.Core*" | head -5Repository: Altinn/app-lib-dotnet
Length of output: 167
🏁 Script executed:
cat ./src/Altinn.App.Core/Altinn.App.Core.csproj | grep -i "nullable\|LangVersion"Repository: Altinn/app-lib-dotnet
Length of output: 47
🏁 Script executed:
cat ./src/Altinn.App.Core/Altinn.App.Core.csprojRepository: Altinn/app-lib-dotnet
Length of output: 3205
🏁 Script executed:
find ./src -name "Directory.Build.props" -o -name ".editorconfig" | head -5Repository: Altinn/app-lib-dotnet
Length of output: 92
🏁 Script executed:
cat ./src/Directory.Build.props | grep -A 5 -B 5 -i "nullable"Repository: Altinn/app-lib-dotnet
Length of output: 1008
🏁 Script executed:
rg "GetResolvedKeysRecursive" --type csRepository: Altinn/app-lib-dotnet
Length of output: 692
Build-breaking nullability mismatch in recursive key resolver.
With Nullable Reference Types enabled, GetResolvedKeys returns string[] but calls GetResolvedKeysRecursive which declares string?[] as both parameter type and return type. This creates a type mismatch at line 132 (and similar mismatches at recursive call sites). Additionally, the input fieldParts from field.Split('.') is string[], not string?[].
All return statements in GetResolvedKeysRecursive produce non-nullable strings, making the nullable signature incorrect. Change both the parameter and return type to string[]:
Proposed fix
- private static string?[] GetResolvedKeysRecursive(
- string?[] keyParts,
+ private static string[] GetResolvedKeysRecursive(
+ string[] keyParts,
object? currentModel,
Type currentType,
int currentIndex = 0,
string currentKey = "",
bool isCalculating = false
)🧰 Tools
🪛 GitHub Check: Run dotnet build and test (macos-latest)
[failure] 132-132:
Nullability of reference types in value of type 'string?[]' doesn't match target type 'string[]'.
[failure] 132-132:
Nullability of reference types in value of type 'string?[]' doesn't match target type 'string[]'.
🤖 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 -
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).
Comment on lines
+8
to
+30
| public class DataFieldCalculation | ||
| { | ||
| /// <summary> | ||
| /// Condition to evaluate | ||
| /// </summary> | ||
| public Expression Condition { get; set; } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Raw value calculation expression from the calculaiton configuration file | ||
| /// </summary> | ||
| public class RawDataFieldValueCalculation | ||
| { | ||
| /// <summary> | ||
| /// Condition to evaluate | ||
| /// </summary> | ||
| public Expression? Condition { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Reference to expression definitions | ||
| /// </summary> | ||
| public string? Ref { get; set; } | ||
| } |
Contributor
There was a problem hiding this comment.
Reduce surface area of these config models.
These types look internal to calculation parsing and should not be public extension points. Consider internal sealed for both classes.
Suggested change
-public class DataFieldCalculation
+internal sealed class DataFieldCalculation
{
@@
-public class RawDataFieldValueCalculation
+internal sealed class RawDataFieldValueCalculation
{As per coding guidelines Use internal accessibility on types by default and Use sealed for classes unless inheritance is considered a valid use-case.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public class DataFieldCalculation | |
| { | |
| /// <summary> | |
| /// Condition to evaluate | |
| /// </summary> | |
| public Expression Condition { get; set; } | |
| } | |
| /// <summary> | |
| /// Raw value calculation expression from the calculaiton configuration file | |
| /// </summary> | |
| public class RawDataFieldValueCalculation | |
| { | |
| /// <summary> | |
| /// Condition to evaluate | |
| /// </summary> | |
| public Expression? Condition { get; set; } | |
| /// <summary> | |
| /// Reference to expression definitions | |
| /// </summary> | |
| public string? Ref { get; set; } | |
| } | |
| internal sealed class DataFieldCalculation | |
| { | |
| /// <summary> | |
| /// Condition to evaluate | |
| /// </summary> | |
| public Expression Condition { get; set; } | |
| } | |
| /// <summary> | |
| /// Raw value calculation expression from the calculaiton configuration file | |
| /// </summary> | |
| internal sealed class RawDataFieldValueCalculation | |
| { | |
| /// <summary> | |
| /// Condition to evaluate | |
| /// </summary> | |
| public Expression? Condition { get; set; } | |
| /// <summary> | |
| /// Reference to expression definitions | |
| /// </summary> | |
| public string? Ref { get; set; } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Altinn.App.Core/Models/RawDataFieldValueCalculation.cs` around lines 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).
| /// <summary> | ||
| /// Condition to evaluate | ||
| /// </summary> | ||
| public Expression Condition { get; set; } |
Contributor
There was a problem hiding this comment.
Initialize non-nullable Condition explicitly.
Condition is non-nullable and currently left uninitialized. Use required (or constructor initialization) to align with nullable-reference-type expectations.
Suggested change
- public Expression Condition { get; set; }
+ public required Expression Condition { get; set; }As per coding guidelines Use Nullable Reference Types in C# code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public Expression Condition { get; set; } | |
| public required Expression Condition { get; set; } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Altinn.App.Core/Models/RawDataFieldValueCalculation.cs` at 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.
Comment on lines
+5
to
+7
| "field": "form.name", | ||
| "result": "feil" | ||
| } |
Contributor
There was a problem hiding this comment.
This test does not prove calculation execution.
Expected and initial values are identical ("feil"), so the case can pass even if calculation is skipped. Please make input and expected output observably different so the hidden-field behavior is actually validated.
Also applies to: 19-22
🤖 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-field.json`
around lines 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.
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
Comment on lines
+187
to
+188
| private DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger) | ||
| { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file to see the method implementation
head -n 250 src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs | tail -n +180Repository: Altinn/app-lib-dotnet
Length of output: 1807
🏁 Script executed:
# Check if _jsonSerializerOptions is static or instance field
rg "_jsonSerializerOptions" src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs -A 2 -B 2Repository: Altinn/app-lib-dotnet
Length of output: 491
🏁 Script executed:
# Search for all calls to ResolveDataFieldCalculation
rg "ResolveDataFieldCalculation" src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.csRepository: Altinn/app-lib-dotnet
Length of output: 293
Make ResolveDataFieldCalculation static.
The method only uses its parameters and the static _jsonSerializerOptions field—no instance members are accessed. CA1822 correctly flags this in CI.
🛠️ Proposed fix
- private DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger)
+ private static DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger) | |
| { | |
| private static DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger) | |
| { |
🧰 Tools
🪛 GitHub Actions: Build and Test on windows, macos and ubuntu
[error] 188-188: CA1822: Member 'ResolveDataFieldCalculation' does not access instance data and can be marked as static.
🪛 GitHub Actions: CodeQL
[error] 188-188: CA1822: Member 'ResolveDataFieldCalculation' does not access instance data and can be marked as static (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822)
🪛 GitHub Check: Analyze (csharp)
[failure] 188-188:
Member 'ResolveDataFieldCalculation' does not access instance data and can be marked as static (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822)
[failure] 188-188:
Member 'ResolveDataFieldCalculation' does not access instance data and can be marked as static (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822)
[failure] 188-188:
Member 'ResolveDataFieldCalculation' does not access instance data and can be marked as static (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822)
🪛 GitHub Check: Run dotnet build and test (macos-latest)
[failure] 188-188:
Member 'ResolveDataFieldCalculation' does not access instance data and can be marked as static (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822)
[failure] 188-188:
Member 'ResolveDataFieldCalculation' does not access instance data and can be marked as static (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822)
🤖 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 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.
Comment on lines
+201
to
+208
| var elementAt = GetElementAt(childModelList, groupIndex.Value); | ||
| return GetResolvedKeysRecursive( | ||
| keyParts, | ||
| elementAt, | ||
| elementType, | ||
| currentIndex + 1, | ||
| JoinFieldKeyParts(currentKey, key + "[" + groupIndex + "]"), | ||
| isCalculating |
Contributor
There was a problem hiding this comment.
Stop resolving paths when the indexed row does not exist.
When groupIndex is explicit and GetElementAt returns null, the recursion still continues in calculation mode and can resolve keys like group[99].field for rows that are not there. Bail out before recursing on a missing element.
🐛 Proposed fix
var elementAt = GetElementAt(childModelList, groupIndex.Value);
+ if (elementAt is null)
+ {
+ return [];
+ }
return GetResolvedKeysRecursive(
keyParts,
elementAt,🤖 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 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".
Comment on lines
+40
to
+51
| var accessCheckerMock = new Mock<IDataElementAccessChecker>(); | ||
| accessCheckerMock.Setup(x => x.CanRead(It.IsAny<Instance>(), It.IsAny<DataType>())).ReturnsAsync(true); | ||
|
|
||
| var dataElementAccessChecker = new Mock<IDataElementAccessChecker>(); | ||
|
|
||
| _output = output; | ||
| _dataFieldValueCalculator = new DataFieldValueCalculator( | ||
| _logger, | ||
| _layoutInitializer.Object, | ||
| _appResources.Object, | ||
| dataElementAccessChecker.Object | ||
| ); |
Contributor
There was a problem hiding this comment.
Pass the new telemetry dependency and inject the configured access-checker mock.
This constructor call no longer matches DataFieldValueCalculator, so the test class will not compile. It also configures one IDataElementAccessChecker mock and injects a different instance.
🐛 Proposed fix
- 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);
@@
_dataFieldValueCalculator = new DataFieldValueCalculator(
_logger,
_layoutInitializer.Object,
_appResources.Object,
- dataElementAccessChecker.Object
+ dataElementAccessChecker.Object,
+ telemetry: null
);🤖 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 - 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.
| public object? GetModelData(string field, System.ReadOnlySpan<int> rowIndexes = default) { } | ||
| public int? GetModelDataCount(string field, System.ReadOnlySpan<int> rowIndexes = default) { } | ||
| public string[] GetResolvedKeys(string field) { } | ||
| public string[] GetResolvedKeys(string field, bool isCalculating = false) { } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Public API snapshot entries =="
fd 'PublicApi_ShouldNotChange_Unintentionally\.verified\.txt$' . -x sed -n '2284,2290p;3191,3197p' {}
echo
echo "== Source definitions for GetResolvedKeys =="
fd 'DataModelWrapper\.cs$' src test -x rg -n 'GetResolvedKeys\s*\(' {}
fd 'LayoutEvaluatorState\.cs$' src test -x rg -n 'GetResolvedKeys\s*\(' {}Repository: Altinn/app-lib-dotnet
Length of output: 1849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All GetResolvedKeys definitions in DataModelWrapper.cs =="
fd 'DataModelWrapper\.cs$' src test -x cat -n {} | grep -A 5 -B 5 'GetResolvedKeys'
echo
echo "== All GetResolvedKeys definitions in LayoutEvaluatorState.cs =="
fd 'LayoutEvaluatorState\.cs$' src test -x cat -n {} | grep -A 5 -B 5 'GetResolvedKeys'
echo
echo "== Check git log for recent changes to these methods =="
git log --oneline -20 -- '**/DataModelWrapper.cs' '**/LayoutEvaluatorState.cs' 2>/dev/null || echo "Git history unavailable"
echo
echo "== Check if old signatures without isCalculating parameter exist in .verified.txt =="
rg 'GetResolvedKeys\(string field\)' test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt || echo "No match found"
rg 'GetResolvedKeys\(Altinn\.App\.Core\.Models\.Layout\.DataReference reference\)' test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt || echo "No match found"Repository: Altinn/app-lib-dotnet
Length of output: 4162
Keep the old GetResolvedKeys overloads for backward compatibility.
The existing public methods DataModelWrapper.GetResolvedKeys(string field) and LayoutEvaluatorState.GetResolvedKeys(DataReference reference) have been replaced with new signatures that include an optional isCalculating parameter. This is a binary-breaking change—applications compiled against the previous package version will fail with MissingMethodException at runtime after upgrading, even though the optional parameter makes this source-compatible.
Add new forwarding overloads that call the parameterized versions with isCalculating: false to preserve the original signatures alongside the new ones.
🤖 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`
at 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.
Comment on lines
+2914
to
2915
| string? GetCalculationConfiguration(string dataTypeId); | ||
| string GetClassRefForLogicDataType(string dataType); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Public API snapshot for IAppResources =="
fd 'PublicApi_ShouldNotChange_Unintentionally\.verified\.txt$' . -x sed -n '2903,2918p' {}
echo
echo "== Interface and implementation definitions =="
fd 'IAppResources\.cs$' src test -x rg -n 'interface IAppResources|GetCalculationConfiguration\s*\(' {}
fd 'AppResourcesSI\.cs$' src test -x rg -n 'class AppResourcesSI|GetCalculationConfiguration\s*\(' {}Repository: Altinn/app-lib-dotnet
Length of output: 1485
🏁 Script executed:
cat -n src/Altinn.App.Core/Internal/App/IAppResources.cs | head -200Repository: Altinn/app-lib-dotnet
Length of output: 7852
🏁 Script executed:
rg 'class.*IAppResources|implements.*IAppResources' src test --type cs -A 2Repository: Altinn/app-lib-dotnet
Length of output: 352
🏁 Script executed:
grep -r "class.*:.*IAppResources" src test --include="*.cs"Repository: Altinn/app-lib-dotnet
Length of output: 161
🏁 Script executed:
rg 'implements.*IAppResources|:\s*IAppResources' src test --type cs -B 1 -A 3Repository: Altinn/app-lib-dotnet
Length of output: 481
🏁 Script executed:
sed -n '528,545p' src/Altinn.App.Core/Implementation/AppResourcesSI.csRepository: Altinn/app-lib-dotnet
Length of output: 677
🏁 Script executed:
rg 'IAppResources' test --type cs -B 2 -A 2 | head -100Repository: Altinn/app-lib-dotnet
Length of output: 10057
🏁 Script executed:
fd 'CHANGELOG|VERSION|version' . --type f | grep -E '\.(md|txt|json)$' | head -10Repository: Altinn/app-lib-dotnet
Length of output: 47
IAppResources is a breaking interface change: external implementations must add GetCalculationConfiguration or compilation fails.
The new abstract method string? GetCalculationConfiguration(string dataTypeId) added to the public interface will require any external implementation to provide an implementation. If this is not intended for the next major version, consider using a default interface method with a null return value or extracting this into a separate interface.
🤖 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 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.
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
Comment on lines
+215
to
+222
| public record Expected | ||
| { | ||
| public string Field { get; set; } | ||
|
|
||
| public ExpressionValue Result { get; set; } | ||
|
|
||
| public string LogMessageWarning { get; set; } | ||
| } |
Contributor
There was a problem hiding this comment.
Expected record properties should be nullable or marked required.
Field, Result, and LogMessageWarning are non-nullable but lack the required modifier. When deserializing JSON that omits these properties (e.g., the test at line 94 only provides logMessageWarning), they'll be null at runtime, causing potential NullReferenceException when accessed (e.g., expected.Result.ToObject() at line 72).
🛡️ 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; }
}Then update the assertion loops to handle nulls:
foreach (var expected in testCase.Expects)
{
- Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field));
+ 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 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.
|
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
1 participant
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
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes