Make it possible to use Json as storage format #1526
Conversation
|
Warning Rate limit exceeded@ivarne has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors storage APIs to be instance-centric: removes org/app string parameters, adds Instance/DataElement overloads, introduces DataElement-aware JSON/XML serialization (new Serialize/Deserialize overloads), updates IDataClient/DataClient and all call sites, marks legacy overloads Obsolete, and adapts tests and telemetry accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
ivarne
added
backport-ignore
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs
Dismissed
Show dismissed
Hide dismissed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
223-226: Don’t return null Stream on 404; callers use ‘await using’.Returning null violates NRT expectations and can NRE with ‘await using’. Prefer Stream.Null or throw NotFound.
-#nullable disable - return null; -#nullable restore + return Stream.Null;test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
291-299: Bug: UpdateFormData mock must pass DataElement to serializer.Without it, contentType may differ from existing DataElement.ContentType; Debug.Assert won’t catch in Release, causing divergence from production behavior.
- var (serializedBytes, contentType) = _modelSerialization.SerializeToStorage(dataToSerialize, dataType); + var (serializedBytes, contentType) = + _modelSerialization.SerializeToStorage(dataToSerialize, dataType, dataElement); - Debug.Assert(contentType == dataElement.ContentType, "Content type should not change when updating data"); + Debug.Assert( + contentType == dataElement.ContentType, + "Content type should not change when updating data" + );src/Altinn.App.Api/Controllers/InstancesController.cs (1)
1036-1049: Attachment copy path can fail hard; add null/exception handlingGetBinaryData may return null (404) and InsertBinaryData can throw. Your comment says “continue processing,” but code throws on first failure.
Apply:
- using var binaryDataStream = await _dataClient.GetBinaryData( + try + { + using var binaryDataStream = await _dataClient.GetBinaryData( instanceOwnerPartyId, sourceInstanceGuid, Guid.Parse(de.Id) - ); - - await _dataClient.InsertBinaryData( - targetInstance.Id, - de.DataType, - de.ContentType, - de.Filename, - binaryDataStream - ); + ); + if (binaryDataStream is null) + { + _logger.LogWarning("Source binary data {DataElementId} not found; skipping.", de.Id); + continue; + } + await _dataClient.InsertBinaryData( + targetInstance.Id, + de.DataType, + de.ContentType ?? "application/octet-stream", + de.Filename ?? $"copied-{de.Id}", + binaryDataStream + ); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed copying binary data element {DataElementId}; continuing.", de.Id); + continue; + }src/Altinn.App.Core/Internal/Data/IDataClient.cs (2)
268-289: XML doc typosFix minor typos to improve docs:
- “elemen” → “element”
- “immediatly” → “immediately”
[suggested diff]- /// Method that removes a data elemen from disk/storage immediatly or marks it as deleted. + /// Method that removes a data element from disk/storage immediately or marks it as deleted.
389-398: XML doc param typo: instanceIdMinor fix for clarity.
- /// <param name="instanceId">isntanceId = {instanceOwnerPartyId}/{instanceGuid}</param> + /// <param name="instanceId">instanceId = {instanceOwnerPartyId}/{instanceGuid}</param>
🧹 Nitpick comments (29)
test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs (3)
43-48: Good: explicit ContentType aligns with new JSON/XML paths.Consider also adding a JSON-path variant to cover content-type aware deserialization. Keeps legacy and new defaults tested.
Would you like me to add a test where ContentType="application/json" and data is serialized via SerializeToJson?
372-379: Avoid duplicated “application/xml” literals; derive from DataElement.Use the same source of truth to prevent drift.
- contentType: "application/xml", + contentType: _defaultDataElement.ContentType,
420-427: Same here: prefer the DataElement’s ContentType.- contentType: "application/xml", + contentType: _defaultDataElement.ContentType,src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
503-507: Minor: API URL composition mixes absolute and relative forms.Either rely on HttpClient.BaseAddress everywhere or compose absolute URLs consistently.
src/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cs (1)
17-18: Seal the class (and consider internal) per guidelines.Unless inheritance is required, mark as sealed; also verify if public is necessary.
-public class UniqueSignatureAuthorizer : IUserActionAuthorizer +public sealed class UniqueSignatureAuthorizer : IUserActionAuthorizerIs this type intended for external consumption? If not, make it internal. As per coding guidelines.
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (2)
224-225: Prefer the 3‑arg serializer form for parity with production.Not critical for inserts, but keeps intent explicit and avoids the obsolete 2‑arg path.
- var (serializedBytes, contentType) = _modelSerialization.SerializeToStorage(dataToSerialize, dataType); + var (serializedBytes, contentType) = _modelSerialization.SerializeToStorage(dataToSerialize, dataType, null);
215-223: Minor: derive org/app via TestData helper for consistency.You already use GetInstanceOrgApp elsewhere; reduces string splitting duplication.
- string org = instance.Org; - string app = instance.AppId.Split("/")[1]; + var (org, app) = TestData.GetInstanceOrgApp(instanceIdentifier);test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs (1)
85-85: Prefer explicit nullable return overdefault(UserProfile)!.For readability and null intent, use
ReturnsAsync((UserProfile?)null)if the API allows, orReturnsAsync((UserProfile)null!)to avoid the implicitness ofdefault!.test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanFull.verified.txt (1)
45-60: Snapshot ContentType added; consistent with XML-based test data.No issues. As JSON becomes the default, consider mirroring tests that assert behavior with
application/jsonto avoid regressions across formats.test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs (1)
150-155: ContentType matches serialization path; consider adding JSON-path coverage.
ContentType = "application/xml"aligns withSerializeToXmlinSetupDataClient. Add a sibling test usingapplication/json+SerializeToJsonto exercise the JSON storage path and validators.test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs (3)
172-178: Settingapplication/jsonis correct for JSON deserialization.Optional: also declare
AllowedContentTypes = new() { "application/json" }on the involvedDataTypeto make the constraint explicit and future‑proof against validation tightening.Apply near the
DataTypeinitialization:new() { Id = DefaultDataTypeName, TaskId = TaskId, AppLogic = new() { ClassRef = _classRef }, + AllowedContentTypes = new() { "application/json" }, },
235-241: Consistent JSON ContentType for the alternate data type.Same optional improvement: add
AllowedContentTypes = new() { "application/json" }to the"aa"data type to keep test intent explicit.new() { Id = "aa", AppLogic = new() { ClassRef = _classRef }, + AllowedContentTypes = new() { "application/json" }, },
278-278: Switched toSerializeToJson; aligns with ContentType.LGTM. If deserialization depends on ContentType strictly, consider an assertion in the test that the accessor resolves JSON (e.g., by verifying the model type was read from JSON) to guard against accidental XML fallback.
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (1)
115-132: Align mock strictness and drop redundant HttpContext DI.
- Mixed behaviors: several mocks are Strict while IInstantiationProcessor and IPrefill are Loose. Either:
- make them Strict too, or
- keep them Loose and default verifyInstantiationProcessor/verifyPrefill to false, to avoid brittle VerifyNoOtherCalls.
- Registering HttpContext in DI is atypical; since ControllerContext.HttpContext is set explicitly, consider removing the transient HttpContext registration or switch to IHttpContextAccessor if needed elsewhere.
Also applies to: 135-135
test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs (1)
693-699: Use of obsolete UpdatedDataModels API inside test action.AddUpdatedDataModel is marked [Obsolete]. If the build treats obsoletes as errors, suppress at the test scope or switch the test to assert the new tracking mechanism while keeping a separate compatibility test for the obsolete shape.
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cs (1)
93-94: InsertFormData overload: confirm serializer/content type and avoid XML-only prep.
- Good: migrated to instance-based InsertFormData.
- Risk: PrepareModelForXmlStorage above is XML-specific. With JSON as default, delegate storage-specific preparation to the serialization layer or gate the XML prep on the resolved content type to avoid unintended mutations under JSON.
Would you like a small refactor that moves storage-format preparation into a serializer abstraction (e.g., ModelSerializationService) so initializer remains format-agnostic?
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (3)
85-98: Test name contradicts behavior; it defaults, doesn’t throwThe case with AllowedContentTypes=["application/unsupported"] asserts XML fallback. Rename to reflect behavior.
Apply:
- public void SerializeToStorage_ThrowsOnUnsupportedContentType() + public void SerializeToStorage_UnsupportedContentType_DefaultsToXml()
65-83: OK to exercise obsolete overload, but consider adding a DataElement path testAdd a case preserving an existing dataElement.ContentType="application/xml".
1-10: Prefer xUnit asserts over FluentAssertions in test projectsPer guidelines, migrate Should().Be* to Assert.* incrementally. As per coding guidelines.
Also applies to: 214-221
src/Altinn.App.Api/Controllers/InstancesController.cs (2)
959-965: Minor: remove unused localsorg/app are assigned but never used post-refactor.
1016-1051: Optional: bound parallelism for many attachmentsIf instances can have many attachments, consider limited parallel copy (e.g., Parallel.ForEachAsync with small degree).
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (2)
601-606: Outdated comment“This should FAIL until we implement it” is now stale; binary copy is implemented and asserted. Update/remove the comment.
978-1011: Strengthen “no form data” assertion for new APIYou verify obsolete GetFormData overload wasn’t called. Also assert Times.Never on the new overload signature to guard regressions.
Suggested add:
fixture.Mock<IDataClient>().Verify( p => p.GetFormData(It.IsAny<Instance>(), It.IsAny<DataElement>(), It.IsAny<StorageAuthenticationMethod>(), It.IsAny<CancellationToken>()), Times.Never); fixture.Mock<IDataClient>().Verify( p => p.InsertFormData(It.IsAny<Instance>(), It.IsAny<string>(), It.IsAny<object>(), It.IsAny<StorageAuthenticationMethod>(), It.IsAny<CancellationToken>()), Times.Never);src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
190-234: Use Ordinal comparison for content type detectionContains without StringComparison is culture-sensitive. Safer to use Ordinal/OrdinalIgnoreCase for mime checks.
Apply:
- if (contentType?.Contains("application/xml") ?? true) + if (contentType?.Contains("application/xml", StringComparison.OrdinalIgnoreCase) ?? true) ... - else if (contentType.Contains("application/json")) + else if (contentType.Contains("application/json", StringComparison.OrdinalIgnoreCase))
265-299: Harden XML deserialization against XXEPrefer XmlReader.Create with XmlReaderSettings { DtdProcessing = Prohibit, XmlResolver = null } over XmlTextReader to reduce XXE risk. Wrap both primary and fallback deserializations with secure settings.
src/Altinn.App.Core/Internal/Data/IDataClient.cs (4)
63-78: Document new InsertFormData parametersAPI shape looks good. Please add meaningful XML docs for instance/dataTypeString/dataToSerialize to aid app devs switching to JSON.
237-255: Still exposes org/app on GetBinaryDataList — deprecate and add instance-aware overloadTo fully meet the PR objective (“Deprecate all method signatures that include org and app”), mark this obsolete and add an overload without org/app that accepts instanceOwnerPartyId and instanceGuid.
- Task<List<AttachmentList>> GetBinaryDataList( - string org, - string app, - int instanceOwnerPartyId, - Guid instanceGuid, - StorageAuthenticationMethod? authenticationMethod = null, - CancellationToken cancellationToken = default - ); + [Obsolete("Org and App parameters are not used, use the overload without these parameters")] + Task<List<AttachmentList>> GetBinaryDataList( + string org, + string app, + int instanceOwnerPartyId, + Guid instanceGuid, + StorageAuthenticationMethod? authenticationMethod = null, + CancellationToken cancellationToken = default + ) => GetBinaryDataList(instanceOwnerPartyId, instanceGuid, authenticationMethod, cancellationToken); + + Task<List<AttachmentList>> GetBinaryDataList( + int instanceOwnerPartyId, + Guid instanceGuid, + StorageAuthenticationMethod? authenticationMethod = null, + CancellationToken cancellationToken = default + );
320-330: Clarify obsolete message for InsertBinaryData(HttpRequest, org/app)To reinforce migration, mention both HttpRequest and org/app in the message, and point to the Stream + instanceId overload.
-[Obsolete("The overload that takes a HttpRequest is deprecated, use the overload that takes a Stream instead")] +[Obsolete("The overload that takes HttpRequest and org/app is deprecated; use the overload that takes a Stream and instanceId")]
458-476: Harden GetFormData extension: handle null and improve errorAvoid potential NRE when deserialization yields null and emit clearer errors.
- { - object formData = await dataClient.GetFormData(instance, dataElement, authenticationMethod, cancellationToken); - - if (formData is T typedFormData) - { - return typedFormData; - } - - throw new InvalidCastException( - $"Failed to cast form data of type {formData.GetType().FullName} to requested type {typeof(T).FullName}" - ); - } + { + object? formData = await dataClient.GetFormData(instance, dataElement, authenticationMethod, cancellationToken); + if (formData is null) + { + throw new InvalidOperationException( + $"Deserialized form data was null for dataElement '{dataElement.Id}' (contentType='{dataElement.ContentType}')" + ); + } + if (formData is T typedFormData) + { + return typedFormData; + } + throw new InvalidCastException( + $"Failed to cast form data of type '{formData.GetType().FullName}' to '{typeof(T).FullName}' for dataElement '{dataElement.Id}'" + ); + }As per coding guidelines (Nullable Reference Types).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
src/Altinn.App.Api/Controllers/DataController.cs(3 hunks)src/Altinn.App.Api/Controllers/InstancesController.cs(2 hunks)src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs(0 hunks)src/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cs(1 hunks)src/Altinn.App.Core/Features/Signing/Services/SigningReceiptService.cs(0 hunks)src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs(2 hunks)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs(3 hunks)src/Altinn.App.Core/Implementation/DefaultAppEvents.cs(0 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(9 hunks)src/Altinn.App.Core/Internal/Data/DataService.cs(0 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(16 hunks)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs(4 hunks)src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskCleaner.cs(0 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs(10 hunks)test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs(1 hunks)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs(8 hunks)test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(0 hunks)test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs(18 hunks)test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs(3 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanFull.verified.txt(1 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanIncremental.verified.txt(8 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyFull.verified.txt(1 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyIncremental.verified.txt(8 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs(1 hunks)test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
💤 Files with no reviewable changes (10)
- test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs
- src/Altinn.App.Core/Features/Signing/Services/SigningReceiptService.cs
- test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs
- test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs
- src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs
- src/Altinn.App.Core/Internal/Data/DataService.cs
- test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
- src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskCleaner.cs
- src/Altinn.App.Core/Implementation/DefaultAppEvents.cs
- test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cssrc/Altinn.App.Api/Controllers/DataController.cstest/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cssrc/Altinn.App.Api/Controllers/DataController.cstest/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
test/Altinn.App.Core.Tests/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Files:
test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cssrc/Altinn.App.Api/Controllers/InstancesController.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
src/Altinn.App.Core/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Files:
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cs
src/Altinn.App.Core/Features/Telemetry/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Treat telemetry shapes as a public contract; changes are breaking
Files:
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs
🧠 Learnings (3)
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
🧬 Code graph analysis (14)
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs (1)
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (1)
Mock(37-38)
test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs (2)
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (1)
DataType(214-221)src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
DataType(87-95)DataType(101-108)DataType(116-134)
test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs (1)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
ModelSerializationService(18-401)ModelSerializationService(29-33)
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs (6)
src/Altinn.App.Core/Features/Telemetry/Telemetry.Data.cs (3)
Activity(29-34)Activity(36-40)Activity(42-46)src/Altinn.App.Core/Features/Telemetry/TelemetryActivityExtensions.cs (9)
Activity(24-31)Activity(39-46)Activity(54-61)Activity(69-76)Activity(84-91)Activity(99-107)Activity(115-123)Activity(131-139)Activity(147-155)src/Altinn.App.Core/Features/Signing/Services/SigningDelegationService.cs (1)
Guid(146-156)test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (1)
Instance(195-207)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
DataElement(197-208)src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs (1)
DataElement(110-113)
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (5)
test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs (1)
Mock(51-64)test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs (1)
Mock(35-36)test/Altinn.App.Core.Tests/Features/Validators/ValidationServiceTests.cs (1)
Mock(70-98)test/Altinn.App.Api.Tests/Controllers/StatelessDataControllerTests.cs (1)
Mock(36-37)test/Altinn.App.Core.Tests/Internal/Process/ProcessEngineTest.cs (1)
Mock(1169-1170)
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (1)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (3)
ModelSerializationService(18-401)ModelSerializationService(29-33)DeserializeFromStorage(61-76)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(61-76)
src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs (1)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(61-76)
test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs (4)
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (1)
Instance(195-207)test/Altinn.App.Core.Tests/Features/Action/PaymentUserActionTests.cs (1)
Instance(149-158)test/Altinn.App.Api.Tests/Data/apps/tdd/task-action/config/models/Scheme.cs (1)
Scheme(7-16)src/Altinn.App.Core/Models/UserAction/UserActionResult.cs (4)
UserActionResult(29-134)UserActionResult(74-82)UserActionResult(88-101)UserActionResult(108-116)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (4)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
Obsolete(61-77)Obsolete(109-163)Obsolete(232-283)Obsolete(452-490)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
DataType(582-591)DataElement(197-208)test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (1)
DataType(214-221)src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (4)
DataType(87-95)DataType(101-108)DataType(116-134)DataElement(60-60)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (2)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
Obsolete(41-52)Obsolete(85-95)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (17)
Obsolete(61-77)Obsolete(109-163)Obsolete(232-283)Obsolete(452-490)Task(80-106)Task(166-195)Task(198-229)Task(286-314)Task(317-342)Task(345-382)Task(420-449)Task(493-542)Task(545-582)Task(585-626)Task(629-658)Task(661-698)Task(701-738)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (4)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
ApplicationMetadata(1194-1226)test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (1)
ApplicationMetadata(209-215)test/Altinn.App.Tests.Common/Auth/TestAuthentication.cs (1)
ApplicationMetadata(622-635)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
DataClient(27-739)DataClient(44-58)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (3)
Obsolete(41-52)Obsolete(85-95)DeserializeFromStorage(61-76)src/Altinn.App.Core/Internal/Data/IDataClient.cs (10)
Obsolete(26-38)Obsolete(51-61)Obsolete(93-105)Obsolete(134-144)Obsolete(171-180)Obsolete(209-218)Obsolete(264-266)Obsolete(279-289)Obsolete(320-330)Obsolete(343-356)src/Altinn.App.Core/Helpers/MemoryAsStream.cs (2)
MemoryAsStream(6-60)MemoryAsStream(10-13)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (3)
test/Altinn.App.Api.Tests/Data/TestData.cs (4)
org(69-90)TestData(9-221)GetDataBlobPath(116-120)GetDataDirectory(61-67)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (4)
DeserializeFromStorage(61-76)Type(301-312)Obsolete(41-52)Obsolete(85-95)src/Altinn.App.Core/Internal/Data/IDataClient.cs (9)
Obsolete(26-38)Obsolete(51-61)Obsolete(93-105)Obsolete(134-144)Obsolete(171-180)Obsolete(209-218)Obsolete(264-266)Obsolete(279-289)Obsolete(320-330)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (42)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
27-28: Sealing DataClient is appropriate.
94-105: InsertFormData uses serializer without DataElement; verify default content-type.Ensure SerializeToStorage(model, dataType, null) yields the configured default (JSON by default per PR) and respects per-dataType overrides.
Do we read default format from app template config when AllowedContentTypes contains both XML and JSON?
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
154-181: Good: test mock deserializes with DataElement-aware API.src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs (1)
73-78: ContentType validation is already in place; no changes needed.The 3-argument
DeserializeFromStoragemethod at line 61–75 already contains a comprehensive guard:return dataElement.ContentType switch { "application/xml" => DeserializeXml(data, type), "application/json" => DeserializeJson(data, type), null or "" => throw new InvalidOperationException( $"Data element {dataElement.Id} does not have a content type" ), _ => throw new InvalidOperationException(...), };This handles the legacy instance scenario: if
ContentTypeis null or empty, an explicitInvalidOperationExceptionis thrown with a clear message. The existing pattern-match validation is sufficient; a pre-check inPreviousDataAccessorwould be redundant.test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyFull.verified.txt (1)
45-60: Snapshot includes ContentType; looks consistent with XML fixtures.Good addition. Since default storage is moving to JSON, ensure complementary coverage exists where ContentType is application/json for the same flow, or that the XML path is intentionally exercised here.
test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyIncremental.verified.txt (1)
138-139: Snapshots updated for ContentType — OK.The added ContentType: application/xml fields align with the new ContentType-aware flow. Please ensure we also have complementary JSON-path snapshots to cover the new default.
Also applies to: 162-163, 189-190, 214-215, 251-252, 276-277, 290-291, 297-298, 304-305, 367-368, 374-375, 381-382
test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanIncremental.verified.txt (1)
117-118: ContentType included in clean snapshots — OK.Looks consistent with DirtyIncremental changes. As above, verify that JSON-default scenarios are covered by tests/snapshots elsewhere.
Also applies to: 141-142, 167-168, 192-193, 228-229, 253-254, 267-268, 274-275, 281-282, 343-344, 350-351, 357-358
src/Altinn.App.Api/Controllers/DataController.cs (1)
908-909: Migration to instance/element-centric IDataClient — verified complete.All three call sites correctly use the new IDataClient overloads:
- Line 908-909:
GetBinaryData(instanceOwnerPartyId, instanceGuid, dataGuid)✓- Line 950-951:
GetFormData(instance, dataElement)✓- Line 981-982:
UpdateFormData(instance, appModel, dataElement)✓No legacy overloads remain in use. The suggestion to pass
HttpContext.RequestAbortedis valid (IDataClient methods accept optionalCancellationToken), but would require addingCancellationTokenparameters to the controller methods as a prerequisite—consider for a future refactor.test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs (2)
220-225: LGTM: ContentType initialization for test fixture.The explicit ContentType initialization ensures deterministic test behavior. Setting it to "application/xml" is appropriate for this test fixture.
238-238: LGTM: Updated serialization call with DataElement.The SerializeToStorage call now correctly passes the dataElement parameter, enabling content-type aware serialization. This aligns with the broader refactoring to support JSON storage format.
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs (2)
16-30: LGTM: Telemetry overloads for domain-object-based APIs.The addition of StartUpdateDataActivity overloads supporting both primitive (Guid) and domain object (Instance, DataElement) parameters enables the migration to instance-aware data operations while maintaining backward compatibility.
101-106: LGTM: New telemetry method for instance-aware form data retrieval.The StartGetFormDataActivity(Instance?) overload correctly tracks telemetry for the new instance-based GetFormData operations introduced in this PR.
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (4)
131-134: LGTM: DataElement-aware deserialization.The GetDataElement call ensures the element exists and provides ContentType metadata required for format-aware deserialization. The DeserializeFromStorage call correctly passes the dataElement to enable XML/JSON format selection based on ContentType.
231-231: LGTM: Format selection for new form data elements.Passing null for dataElement triggers FindFirstContentType, which selects the format based on DataType configuration. This correctly implements the PR objective of defaulting to JSON in new apps while maintaining XML compatibility for legacy apps through configuration.
396-400: LGTM: Content-type preservation during updates.The SerializeToStorage call correctly passes the existing dataElement to preserve the ContentType during updates. This ensures data format consistency across save operations.
413-415: LGTM: DataElement-aware deserialization for patch operations.The DeserializeFromStorage call correctly uses the dataElement parameter to determine the format when deserializing previous data during patch operations.
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (4)
28-46: LGTM: content-type selection is validatedCovers json/xml/empty list → XML default correctly.
48-63: LGTM: JSON roundtrip assertionMatches JsonSerializerDefaults.Web (camelCase).
135-153: LGTM: mismatching model type pathVerifies type enforcement on deserialize.
155-173: LGTM: JSON/XML deserialize and default-to-XML pathsGood coverage for the new DataElement-aware overload and legacy defaulting.
Also applies to: 175-193, 195-212
src/Altinn.App.Api/Controllers/InstancesController.cs (1)
986-1005: Switched to Instance/DataElement APIs — goodForm data copy now uses GetFormData(sourceInstance, de) and InsertFormData(targetInstance, dt.Id, data). Aligns with new storage surface.
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
394-415: Mocks align with new IDataClient surface — goodPositive verifications match Instance/DataElement overloads.
Also applies to: 515-537, 775-799
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
381-400: LGTM: FindFirstContentType fallback strategySelects first supported type and defaults to XML for backward compat.
41-52: Obsolete guards for JSON are appropriateThe Obsolete attribute generates a compile-time warning when methods are called, providing developer notification without runtime enforcement. The implementation correctly adds runtime protection by throwing
NotImplementedExceptionwhen obsolete overloads detect JSON content types—this guard logic (lines 44-48 and 88-92) prevents accidental JSON mishandling at execution time.Examining the codebase via the provided grep results, the production code in
src/(includingInstanceDataUnitOfWork.cs,PreviousDataAccessor.cs, andDataClient.cs) consistently calls the correct non-obsolete overloads with properDataElementparameters or safenullvalues. Test files appropriately call both obsolete and non-obsolete methods to verify guard behavior. No unsafe calls to obsolete JSON-supporting overloads without required parameters were found in production code.The obsolete decorator combined with runtime validation provides appropriate layered protection against legacy API misuse.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (4)
39-39: LGTM: Initialize DataTypes to empty listAvoids null checks in tests relying on app metadata.
391-403: LGTM: Assertions updated to new endpoints and authURI/auth expectations and helper asserts look correct; good coverage across methods.
Also applies to: 429-442, 497-510, 593-603, 678-679, 718-724, 803-809, 850-851, 886-887, 927-928, 968-977
1017-1064: Good: tests through IDataClient interfaceTargets public surface; reduces coupling to implementation.
206-223: Namespace model assertions updated — OKKeeps XML path test realistic.
src/Altinn.App.Core/Internal/Data/IDataClient.cs (14)
26-38: Deprecation of legacy InsertFormData overloads is correctMoving away from org/app and Type aligns with instance-aware, content-type-driven storage. No issues.
51-62: Safe delegation for obsolete InsertFormData(..., Type)Good default interface redirection to the new overload. Keeps source compatibility.
93-105: Obsoleting UpdateData is appropriateDeprecation message points callers to the correct instance-aware API. LGTM.
107-121: New UpdateFormData aligns with JSON-in-storageIncludes DataElement to infer ContentType. Looks good.
134-145: Obsolete GetFormData overloadCorrectly deprecated; encourages DataElement-aware usage.
146-159: New GetFormData signature is soundInstance + DataElement enables correct deserialization based on ContentType. LGTM.
171-181: Obsolete GetBinaryData (org/app) delegationThe deprecation and redirection are consistent with the cleanup. LGTM.
198-219: Obsolete GetDataBytes (org/app) delegationConsistent with the org/app deprecation strategy. LGTM.
220-235: New GetDataBytes overloadClear, non-nullable semantics with bytes. LGTM.
291-307: New DeleteData overload (no org/app) looks goodMatches the deprecation strategy and supports delayed delete.
368-376: UpdateBinaryData (InstanceIdentifier, …, Stream) is appropriateModern, instance-aware signature. LGTM.
408-413: Update(DataElement metadata) — OKClear contract for metadata updates.
422-427: LockDataElement — OKSignature and naming are consistent.
436-441: UnlockDataElement — OKSymmetric with LockDataElement. LGTM.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
Outdated
Show resolved
Hide resolved
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
Outdated
Show resolved
Hide resolved
ivarne
force-pushed
the
feat/json-in-storage
branch
from
12eaba4 to
63a5e64
Compare
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cs (1)
100-114: Fix null stream handling in GetSigneeFromSignDocument.GetBinaryData can return null when the response status is NotFound, as evidenced by the implementation returning
null!at line 220 in DataClient. DataService demonstrates the required pattern by checking for null immediately after calling GetBinaryData and throwing ArgumentNullException if null.The current code lacks this check and will throw an unhandled
ArgumentNullExceptionif the stream is null, since the try-catch only handlesJsonException:private async Task<Signee?> GetSigneeFromSignDocument( InstanceIdentifier instanceIdentifier, DataElement dataElement ) { await using var data = await _dataClient.GetBinaryData( instanceIdentifier.InstanceOwnerPartyId, instanceIdentifier.InstanceGuid, Guid.Parse(dataElement.Id) ); + + if (data == null) + { + return null; + } + try { var signDocument = await JsonSerializer.DeserializeAsync<SignDocument>(data, _jsonSerializerOptions); return signDocument?.SigneeInfo; } catch (JsonException) { return null; } }test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (1)
1031-1042: Fix DI registrations and set a non-null SubscriptionKey for tests.Same DI issue here: register mocks by interface; also set SubscriptionKey to avoid null header failures in DataClient ctor.
- services.AddSingleton(mocks.AppModelMock.Object); + services.AddSingleton<IAppModel>(mocks.AppModelMock.Object); services.AddSingleton(mocks.HttpClientFactoryMock.Object); services.AddSingleton(mocks.MaskinportenClientMock.Object); services.AddSingleton(mocks.AppMetadataMock.Object); services.AddSingleton(mocks.AuthenticationContextMock.Object); + services.Configure<PlatformSettings>(o => + { + o.ApiStorageEndpoint = ApiStorageEndpoint; + o.SubscriptionKey = "test-subscription-key"; + });test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (2)
289-296: UpdateFormData must preserve content-type; use the DataElement-aware overload.Current call drops DataElement, potentially changing content-type and breaking the debug assertion.
- var (serializedBytes, contentType) = _modelSerialization.SerializeToStorage(dataToSerialize, dataType); + var (serializedBytes, contentType) = _modelSerialization.SerializeToStorage( + dataToSerialize, + dataType, + dataElement + );
541-547: Bug: incorrect path construction when reading files.Directory.GetFiles returns full paths; Path.Join(path, file) duplicates the path and fails.
- DataElement dataElement = await InstanceClientMockSi.ReadJsonFile<DataElement>( - Path.Join(path, file), - cancellationToken - ); + DataElement dataElement = await InstanceClientMockSi.ReadJsonFile<DataElement>( + file, + cancellationToken + );src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
53-56: Guard against null/empty SubscriptionKey before adding header.Avoid ArgumentNullException in tests and non-prod configs.
- httpClient.DefaultRequestHeaders.Add(General.SubscriptionKeyHeaderName, _platformSettings.SubscriptionKey); + if (!string.IsNullOrWhiteSpace(_platformSettings.SubscriptionKey)) + { + httpClient.DefaultRequestHeaders.Add( + General.SubscriptionKeyHeaderName, + _platformSettings.SubscriptionKey + ); + }
♻️ Duplicate comments (1)
test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs (1)
693-697: LGTM!The migration to instance-based data access is correct. The code properly uses
_dataClient.GetFormData(context.Instance, dataElement)with the new signature and correctly referencesdataElement.Idfor the update operation. The intentional use of the obsoleteAddUpdatedDataModelmethod in this test case is appropriate for validating backward compatibility during the migration period.
🧹 Nitpick comments (14)
test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs (1)
172-177: Consider adding ContentType to all test DataElements for consistency.Tests 3 and 4 now include
ContentType = "application/json"on the DataElement, but tests 1 and 2 (lines 73-76 and 116-119) don't set this property. While tests 1 and 2 may not require data deserialization (their expressions evaluategatewayActionrather thandataModel), setting ContentType consistently across all tests would improve clarity and ensure the test suite fully exercises the JSON serialization path introduced by this PR.Apply this diff to add ContentType to test 1:
Data = new() { - new() { Id = "cd9204e7-9b83-41b4-b2f2-9b196b4fafcf", DataType = DefaultDataTypeName }, + new() + { + Id = "cd9204e7-9b83-41b4-b2f2-9b196b4fafcf", + DataType = DefaultDataTypeName, + ContentType = "application/json", + }, },Apply this diff to add ContentType to test 2:
Data = new() { - new() { Id = "cd9204e7-9b83-41b4-b2f2-9b196b4fafcf", DataType = DefaultDataTypeName }, + new() + { + Id = "cd9204e7-9b83-41b4-b2f2-9b196b4fafcf", + DataType = DefaultDataTypeName, + ContentType = "application/json", + }, },Also applies to: 235-240
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs (1)
85-85: Prefer explicit nullable return over null-forgiving default(UserProfile)!Use a clear nullable cast to avoid masking nullability and intent.
- fixture.Mock<IProfileClient>().Setup(p => p.GetUserProfile(12345)).ReturnsAsync(default(UserProfile)!); + fixture.Mock<IProfileClient>().Setup(p => p.GetUserProfile(12345)).ReturnsAsync((UserProfile?)null);test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (1)
135-136: Avoid mocking HttpContext; use DefaultHttpContext to reduce brittlenessMocking HttpContext with Strict often breaks on unconfigured getters (Items, RequestServices, RequestAborted). DefaultHttpContext is simpler and safer.
- var httpContextMock = new Mock<HttpContext>(MockBehavior.Strict); - services.AddTransient(_ => httpContextMock.Object); + var httpContext = new DefaultHttpContext(); + services.AddTransient(_ => httpContext); ... - controller.ControllerContext = new() { HttpContext = httpContextMock.Object }; + controller.ControllerContext = new() { HttpContext = httpContext };Also applies to: 147-151
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormData_dataType=xmlDefaultDataType.verified.txt (1)
12-12: Snapshot has trailing comma in Accept header"application/json,application/xml," may be fragile if header serialization changes. If you control the snapshot formatter, consider trimming trailing commas to reduce churn.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormDataExtension_dataType=xmlDataType.verified.txt (1)
12-12: Normalize Accept header formatting in snapshotsTrailing comma after media types can cause noisy diffs across platforms/versions. Consider normalizing to a comma-separated list without trailing comma.
Also applies to: 42-42
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestUpdateFormData_dataType=jsonDefaultDataType.verified.txt (1)
17-17: Snapshot Accept header trailing commaSame minor formatting nit as other snapshots; optional cleanup to stabilize future diffs.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs (1)
89-95: Minor: typo in diagnostic message.“Availible” → “Available”.
- $"No response found for request uri: \n{request.Method} {request.RequestUri} {request.Content?.Headers.ContentType}\nAvailible:\n{string.Join("\n", _fakeResponses.Select(r => $"{r.Method} {r.Url} {r.RequestContentType}"))}" + $"No response found for request uri: \n{request.Method} {request.RequestUri} {request.Content?.Headers.ContentType}\nAvailable:\n{string.Join("\n", _fakeResponses.Select(r => $"{r.Method} {r.Url} {r.RequestContentType}"))}"src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
66-77: Make content-type handling case-insensitive.Content-Type is case-insensitive per RFC; current checks use case-sensitive equality/Contains.
- return dataElement.ContentType switch - { - "application/xml" => DeserializeXml(data, type), - "application/json" => DeserializeJson(data, type), + return dataElement.ContentType switch + { + var s when string.Equals(s, "application/xml", StringComparison.OrdinalIgnoreCase) + => DeserializeXml(data, type), + var s when string.Equals(s, "application/json", StringComparison.OrdinalIgnoreCase) + => DeserializeJson(data, type), null or "" => throw new InvalidOperationException( $"Data element {dataElement.Id} does not have a content type" ), // Consider defaulting to xml after initial testing? _ => throw new InvalidOperationException( $"Unsupported content type {dataElement.ContentType} on data element {dataElement.Id}" ), };- return contentType switch + return contentType switch { - "application/json" => (SerializeToJson(model), contentType), + var s when string.Equals(s, "application/json", StringComparison.OrdinalIgnoreCase) + => (SerializeToJson(model), s), // When the content type is missing, default to xml for backward compatibility - "application/xml" or null or "" => (SerializeToXml(model), "application/xml"), + var s when string.Equals(s, "application/xml", StringComparison.OrdinalIgnoreCase) || string.IsNullOrEmpty(s) + => (SerializeToXml(model), "application/xml"), _ => throw new InvalidOperationException($"Unsupported content type {contentType}"), };- if (contentType?.Contains("application/xml", StringComparison.Ordinal) ?? true) + if (contentType?.Contains("application/xml", StringComparison.OrdinalIgnoreCase) ?? true) ... - else if (contentType.Contains("application/json", StringComparison.Ordinal)) + else if (contentType.Contains("application/json", StringComparison.OrdinalIgnoreCase))Also applies to: 120-128, 189-210
7-7: Avoid dependency from serialization to storage client.ModelSerializationService calls DataClient.TypeAllowsJson(), coupling Helpers.Serialization to Infrastructure.Clients.Storage. Extract the JSON-allowance check into a shared internal utility (e.g., DataTypeContentNegotiation) to keep dependency direction clean.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (3)
972-979: Compare auth header to token string explicitly.Avoid implicit object vs string comparison; use token.ToString().
- Assert.Equal(authHeader.Parameter, expectedAuth ?? _defaultAuth.Token); + Assert.Equal(expectedAuth?.ToString() ?? _defaultAuth.Token, authHeader.Parameter);
297-299: Prefer xUnit assertions over FluentAssertions (project guideline).Replace Should().Be*/BeEquivalentTo() with Assert.* for consistency.
Example conversions:
- invocations.Should().Be(1) → Assert.Equal(1, invocations)
- result.Should().BeTrue() → Assert.True(result)
- response.Should().BeEquivalentTo(expected) → Assert.Equal(expected, response)
As per coding guidelines.
Also applies to: 366-369, 406-407, 509-510, 599-601, 674-675, 713-721, 797-805, 845-847, 882-883, 923-924, 959-960
981-992: Redundant URI checks; keep one.Assert.Equivalent on Uri is redundant given the subsequent Uri.Compare. Remove one to reduce noise.
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
289-291: Use Path.Join instead of string concatenation for directories.Small cross-platform hygiene improvement.
- Directory.CreateDirectory(dataPath + @"blob"); + Directory.CreateDirectory(Path.Join(dataPath, "blob")); ... - Directory.CreateDirectory(dataPath + @"blob"); + Directory.CreateDirectory(Path.Join(dataPath, "blob"));Also applies to: 351-354
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
545-547: Optional: structured logging instead of string interpolation.Use message templates to avoid unnecessary string allocations on non-enabled levels.
- _logger.LogError( - $"Storing attachment for instance {instanceId} failed with status code {response.StatusCode} - content {await response.Content.ReadAsStringAsync(cancellationToken)}" - ); + var content = await response.Content.ReadAsStringAsync(cancellationToken); + _logger.LogError("Storing attachment for instance {InstanceId} failed with {StatusCode}. Content: {Content}", + instanceId, response.StatusCode, content);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
src/Altinn.App.Api/Controllers/DataController.cs(3 hunks)src/Altinn.App.Api/Controllers/InstancesController.cs(2 hunks)src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs(0 hunks)src/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cs(1 hunks)src/Altinn.App.Core/Features/Signing/Services/SigningReceiptService.cs(0 hunks)src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs(2 hunks)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs(6 hunks)src/Altinn.App.Core/Implementation/DefaultAppEvents.cs(0 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(10 hunks)src/Altinn.App.Core/Internal/Data/DataService.cs(0 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(16 hunks)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs(4 hunks)src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskCleaner.cs(0 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs(11 hunks)test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs(1 hunks)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs(8 hunks)test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(0 hunks)test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs(18 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetBinaryData_AllVariants.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetBinaryData_TeaPotResult.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormDataExtension_dataType=jsonDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormDataExtension_dataType=jsonDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormDataExtension_dataType=xmlDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormDataExtension_dataType=xmlDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormData_dataType=jsonDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormData_dataType=jsonDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormData_dataType=xmlDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormData_dataType=xmlDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestInsertFormDataWithInstance_dataType=jsonDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestInsertFormDataWithInstance_dataType=jsonDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestInsertFormDataWithInstance_dataType=xmlDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestInsertFormDataWithInstance_dataType=xmlDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteGetFormData_dataType=xmlDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormDataWithInstance_dataType=jsonDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormDataWithInstance_dataType=jsonDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormDataWithInstance_dataType=xmlDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormDataWithInstance_dataType=xmlDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormData_dataType=jsonDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormData_dataType=jsonDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormData_dataType=xmlDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormData_dataType=xmlDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteUpdateData_dataType=xmlDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestUpdateFormData_dataType=jsonDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestUpdateFormData_dataType=jsonDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestUpdateFormData_dataType=xmlDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestUpdateFormData_dataType=xmlDefaultDataType.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.UpdateBinaryData.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs(3 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanFull.verified.txt(1 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanIncremental.verified.txt(8 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyFull.verified.txt(1 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyIncremental.verified.txt(8 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs(1 hunks)test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
💤 Files with no reviewable changes (10)
- test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs
- test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs
- src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskCleaner.cs
- src/Altinn.App.Core/Internal/Data/DataService.cs
- test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs
- src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs
- test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs
- src/Altinn.App.Core/Features/Signing/Services/SigningReceiptService.cs
- src/Altinn.App.Core/Implementation/DefaultAppEvents.cs
- test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
✅ Files skipped from review due to trivial changes (11)
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestUpdateFormData_dataType=xmlDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestInsertFormDataWithInstance_dataType=xmlDefaultDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormDataExtension_dataType=jsonDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormData_dataType=xmlDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormData_dataType=xmlDefaultDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetBinaryData_AllVariants.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormDataWithInstance_dataType=xmlDefaultDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormData_dataType=jsonDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestInsertFormDataWithInstance_dataType=jsonDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormDataWithInstance_dataType=jsonDefaultDataType.verified.txt
- test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormData_dataType=jsonDataType.verified.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cs
- test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanIncremental.verified.txt
- test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs
- test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs
- src/Altinn.App.Api/Controllers/DataController.cs
- test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cstest/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cstest/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cssrc/Altinn.App.Api/Controllers/InstancesController.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cstest/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
src/Altinn.App.Core/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Files:
src/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs
test/Altinn.App.Core.Tests/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Files:
test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs
src/Altinn.App.Core/Features/Telemetry/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Treat telemetry shapes as a public contract; changes are breaking
Files:
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs
🧠 Learnings (4)
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
PR: Altinn/app-lib-dotnet#1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanFull.verified.txttest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.UpdateBinaryData.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
🧬 Code graph analysis (14)
src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs (1)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(62-77)
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs (1)
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (1)
Mock(37-38)
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (5)
test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs (1)
Mock(35-36)test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs (1)
Mock(51-64)test/Altinn.App.Core.Tests/Features/Validators/ValidationServiceTests.cs (1)
Mock(70-98)test/Altinn.App.Api.Tests/Controllers/StatelessDataControllerTests.cs (1)
Mock(36-37)test/Altinn.App.Core.Tests/Internal/Process/ProcessEngineTest.cs (1)
Mock(1169-1170)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs (3)
test/Altinn.App.Tests.Common/Auth/TestAuthentication.cs (1)
TestAuthentication(102-664)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (14)
DataClient(27-747)DataClient(44-58)Task(80-106)Task(162-191)Task(194-224)Task(293-321)Task(324-349)Task(352-389)Task(427-456)Task(500-549)Task(593-634)Task(637-666)Task(669-706)Task(709-746)src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-67)
test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs (3)
test/Altinn.App.Core.Tests/Features/Action/PaymentUserActionTests.cs (1)
Instance(149-158)test/Altinn.App.Api.Tests/Data/apps/tdd/task-action/config/models/Scheme.cs (1)
Scheme(7-16)src/Altinn.App.Core/Models/UserAction/UserActionResult.cs (4)
UserActionResult(29-134)UserActionResult(74-82)UserActionResult(88-101)UserActionResult(108-116)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(62-77)
test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs (3)
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (1)
DataType(260-267)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
DataType(582-591)src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (3)
DataType(87-95)DataType(101-108)DataType(116-134)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (9)
Obsolete(61-77)Obsolete(109-159)Obsolete(227-274)Obsolete(459-497)Obsolete(552-590)DataClient(27-747)DataClient(44-58)TypeAllowsJson(276-279)TypeAllowsJson(281-290)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (5)
src/Altinn.App.Api/Controllers/InstancesController.cs (1)
Instance(1266-1291)test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (1)
Instance(195-207)src/Altinn.App.Core/Models/DataElementIdentifier.cs (1)
ToString(70-73)src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (1)
Mock(37-38)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (2)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
ApplicationMetadata(1219-1251)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
DataClient(27-747)DataClient(44-58)
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs (2)
src/Altinn.App.Core/Features/Telemetry/Telemetry.Data.cs (3)
Activity(29-34)Activity(36-40)Activity(42-46)src/Altinn.App.Core/Features/Telemetry/TelemetryActivityExtensions.cs (9)
Activity(24-31)Activity(39-46)Activity(54-61)Activity(69-76)Activity(84-91)Activity(99-107)Activity(115-123)Activity(131-139)Activity(147-155)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (17)
Obsolete(61-77)Obsolete(109-159)Obsolete(227-274)Obsolete(459-497)Obsolete(552-590)Task(80-106)Task(162-191)Task(194-224)Task(293-321)Task(324-349)Task(352-389)Task(427-456)Task(500-549)Task(593-634)Task(637-666)Task(669-706)Task(709-746)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
Obsolete(42-53)Obsolete(86-96)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (3)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (3)
Obsolete(42-53)Obsolete(86-96)DeserializeFromStorage(62-77)src/Altinn.App.Core/Internal/Data/IDataClient.cs (10)
Obsolete(26-38)Obsolete(51-61)Obsolete(93-105)Obsolete(134-144)Obsolete(171-180)Obsolete(209-218)Obsolete(264-266)Obsolete(279-289)Obsolete(320-330)Obsolete(343-356)src/Altinn.App.Core/Helpers/MemoryAsStream.cs (2)
MemoryAsStream(6-60)MemoryAsStream(10-13)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (4)
test/Altinn.App.Api.Tests/Data/TestData.cs (4)
org(69-90)TestData(9-221)GetDataBlobPath(116-120)GetDataDirectory(61-67)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (17)
Task(80-106)Task(162-191)Task(194-224)Task(293-321)Task(324-349)Task(352-389)Task(427-456)Task(500-549)Task(593-634)Task(637-666)Task(669-706)Task(709-746)Obsolete(61-77)Obsolete(109-159)Obsolete(227-274)Obsolete(459-497)Obsolete(552-590)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (5)
Task(174-236)DeserializeFromStorage(62-77)Type(300-311)Obsolete(42-53)Obsolete(86-96)src/Altinn.App.Core/Internal/Data/IDataClient.cs (9)
Obsolete(26-38)Obsolete(51-61)Obsolete(93-105)Obsolete(134-144)Obsolete(171-180)Obsolete(209-218)Obsolete(264-266)Obsolete(279-289)Obsolete(320-330)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
🔇 Additional comments (24)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.UpdateBinaryData.verified.txt (1)
1-59: Skip review — verified snapshot file.This is a verification snapshot fixture file (
*.verified.txt), which documents expected test behavior rather than production code. Per guidance from previous reviews, these files should not be subject to code review suggestions, as they are auto-managed snapshots meant to detect unintended behavioral changes, not to be critiqued as source code.test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanFull.verified.txt (1)
1-112: Skip review — verified snapshot file.Per learnings,
*.verified.txtfiles are verification snapshots and should not generate code review suggestions. The changes align with the broader PR effort to introduce explicitContentTypemetadata to DataElements, consistent with the JSON/XML storage format awareness being added in this PR.test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteInsertFormData_dataType=jsonDefaultDataType.verified.txt (1)
1-68: Test fixture properly captures obsolete method behavior with JSON serialization.This verified test data file appropriately captures the mock HTTP interaction for testing the deprecated
InsertFormDatamethod with JSON as the default data type. The request/response structure is consistent:
- Request correctly posts with
dataType=jsonDefaultDataTypequery parameter- Request payload is valid JSON (
name: ivar, age: 36)- Content-Type headers are properly set to
application/json(request and response)- InstanceGuid is consistent across request and response
- Response includes complete DataElement metadata
The fixture aligns well with the PR objectives of enabling JSON as a storage format while maintaining backward compatibility with deprecated method signatures.
To ensure completeness, please verify that the corresponding test method (if not already provided for review) validates that:
- The deprecated method signature still functions correctly
- JSON serialization/deserialization is working as expected
- The ContentType is properly preserved through the storage round-trip
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormDataExtension_dataType=jsonDefaultDataType.verified.txt (2)
7-50: Clarify the intent of duplicate HTTP calls in the test fixture.The snapshot contains two identical HTTP calls (lines 8–28 and 29–49) with the same request URI, headers, and response data. Confirm whether this is intentional (e.g., testing idempotence or multiple sequential operations) or an artifact of the snapshot collection process.
1-51: Snapshot fixture structure is appropriate for JSON storage format testing.The test fixture correctly captures the expected behavior for
GetFormDataExtensionwith JSON as the default data type:
- Mock HTTP responses return status 200 with correct
Content-Type: application/json; charset=utf-8- Accept header appropriately includes both
application/jsonandapplication/xml- Content-Length (24 bytes) is consistent with the serialized JSON payload
- Authorization header is properly scrubbed for test safety
- Mock data (Name: ivar, Age: 36) is realistic and aligns with the PR objective to enable JSON storage
test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs (1)
278-278: LGTM! Serialization format correctly updated to JSON.The change from
SerializeToXmltoSerializeToJsoncorrectly aligns with the JSON storage format introduced in this PR and matches theContentType = "application/json"settings added to the test DataElements.test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteUpdateData_dataType=xmlDataType.verified.txt (2)
5-6: The single TestObsoleteUpdateData snapshot is consistent; however, only an XML variant exists for verification.The XML snapshot correctly uses
application/xmlfor the request andapplication/jsonfor the API response—this is standard HTTP behavior where requests and responses can differ in content type. The deprecated method properly infers and passes the content type.However, the review comment requests verification across multiple data type snapshots (XML and JSON), but only the xmlDataType variant exists. The JSON data type variant does not have a corresponding TestObsoleteUpdateData snapshot. Verify whether test coverage for JSON data types in the deprecated UpdateData method is intentional or should be added for complete consistency validation.
1-71: ****The review comment's assumptions are incorrect. The
TestObsoleteUpdateDatamethod is parameterized with[Theory]and[MemberData(nameof(DataTypes))], but the test implementation intentionally only generates snapshots forxmlDataType. For other data types (jsonDataType, jsonDefaultDataType, etc.), the method throwsInvalidOperationExceptionby design, as documented in the code comment: "The tests share the same ClassRef, and the compatibility check will fail if any type supports json."This is fundamentally different from the modern
TestUpdateFormDatamethod, which successfully processes all data types and generates 4 snapshots. The singleTestObsoleteUpdateData_dataType=xmlDataType.verified.txtsnapshot file is complete and correct—no JSON variant is expected or should exist for this particular deprecated overload.Likely an incorrect or invalid review comment.
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (1)
115-133: Strict mocks properly configured in testsThe fixture creates 19 strict mocks without default setups, but tests correctly configure them. Examination of actual test implementations shows the established pattern:
// Tests call fixture.Mock<T>().Setup(...) before invoking controller methods fixture.Mock<HttpContext>().Setup(httpContext => httpContext.User) .Returns(TestAuthentication.GetUserPrincipal(...)); fixture.Mock<IAppMetadata>().Setup(a => a.GetApplicationMetadata()) .ReturnsAsync(...); fixture.Mock<IPDP>().Setup(p => p.GetDecisionForRequest(...)) .ReturnsAsync(...);All examined test methods properly set up required mocks before calling controller methods, and consistently use
fixture.VerifyNoOtherCalls()to catch unintended interactions. The pattern is well-established and working correctly.src/Altinn.App.Api/Controllers/InstancesController.cs (1)
984-1002: LGTM - Clean refactoring to instance-based APIThe updated data access calls correctly use the new instance-based signatures:
GetFormData(sourceInstance, de)replaces the old org/app/type-based signatureInsertFormData(targetInstance, dt.Id, data)similarly removes org/app parametersThe refactoring aligns with the PR objectives to simplify the API surface and enable DataElement-aware serialization.
src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs (1)
73-77: LGTM - Correct DataElement-aware deserializationThe updated code correctly retrieves the
DataElementbefore deserialization and passes it toDeserializeFromStorage. This enables the serialization service to select the appropriate format (JSON/XML) based ondataElement.ContentType, which is essential for the PR's goal of supporting JSON storage format.test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyIncremental.verified.txt (1)
138-138: LGTM!The systematic addition of
ContentType: application/xmlto all DataElement entries correctly documents the expected test output after the PR's changes to include content-type information in DataElement objects.Also applies to: 162-162, 189-189, 214-214, 251-251, 276-276, 290-290, 297-297, 304-304, 367-367, 374-374, 381-381
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestObsoleteGetFormData_dataType=xmlDataType.verified.txt (1)
1-40: LGTM!Test fixture correctly documents the expected behavior for obsolete GetFormData with XML data type, including proper Content-Type headers and XML response format.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormDataExtension_dataType=xmlDefaultDataType.verified.txt (1)
1-70: LGTM with observation.Test fixture correctly documents the expected XML response format. Note that the mock HTTP client captures two identical GET calls to the same endpoint (lines 8-37 and 38-67), which likely reflects the test exercising the extension method multiple times to verify idempotency or repeated invocation behavior.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetFormData_dataType=jsonDefaultDataType.verified.txt (1)
1-31: LGTM!Test fixture correctly documents the expected JSON response format with proper Content-Type header and compact JSON payload (24 bytes vs. 220 bytes for XML), demonstrating one of the storage efficiency benefits mentioned in the PR objectives.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestGetBinaryData_TeaPotResult.verified.txt (1)
1-79: LGTM!Test fixture documents edge-case handling for GetBinaryData with the unusual "306 Unused" HTTP status code. The five identical HTTP calls likely validate retry or repeated access behavior. The fixture name "TeaPotResult" and use of status 306 suggest intentional testing of non-standard response handling.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestInsertFormDataWithInstance_dataType=xmlDataType.verified.txt (1)
1-76: LGTM!Test fixture correctly documents the InsertFormData operation with instance-based context, including proper XML serialization in the request (Content-Type: application/xml) and the returned DataElement metadata with ContentType field, aligning with the PR's migration to instance-based data operations.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.TestUpdateFormData_dataType=xmlDefaultDataType.verified.txt (1)
1-71: LGTM!Test fixture correctly documents the UpdateFormData operation using PUT with XML serialization and proper Content-Type handling. The response includes the updated DataElement with ContentType metadata, consistent with the PR's migration to DataElement-aware serialization.
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (4)
131-135: LGTM!The addition of
dataElementparameter toDeserializeFromStoragecorrectly enables content-type-aware deserialization (JSON vs XML) based onDataElement.ContentType, aligning with the new serialization service signature.
231-231: LGTM!Passing
nullfordataElementis appropriate here since this is creating a new data element that doesn't exist yet. The serialization service determines the content type from thedataTypeconfiguration.
396-400: LGTM!Passing
dataElementtoSerializeToStoragecorrectly enables content-type-aware serialization for updates, ensuring the existing element's format is preserved.
414-414: LGTM!Passing
dataElementtoDeserializeFromStorageensures correct format handling when reconstructing previous form data for change comparison.src/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cs (1)
78-78: LGTM!Removing the
AppIdentifierparameter aligns with the instance-based data access pattern established across the PR.src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs (1)
16-30: The review comment contains factually unsupported claims about removed telemetry methods.The search found no evidence of
StartDeleteBinaryDataActivityexisting anywhere in the codebase. Additionally, the current Telemetry.DataClient.cs file shows only the newStartInsertFormDataActivity(Instance? instance)signature, but no evidence of the claimed old signature(Guid?, int?).More importantly, per the coding guideline, "telemetry shapes" refer to the output data contract (activity names and tags), which remain unchanged. The methods themselves are
internalimplementation details. Other methods likeStartUpdateDataActivityandStartGetFormDataActivitymaintain both old and new signatures through overloading, so they are not breaking changes.The review comment's concern about removed signatures appears to be based on an inaccurate AI summary and does not reflect the actual codebase state.
Likely an incorrect or invalid review comment.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs
Outdated
Show resolved
Hide resolved
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
581-589: Critical: wrong path composition when reading DataElements.Directory.GetFiles(path) returns absolute file paths. Joining them with path duplicates the directory portion, breaking reads.
Apply:
- string[] files = Directory.GetFiles(path); + string[] files = Directory.GetFiles(path, "*.json"); ... - DataElement dataElement = await InstanceClientMockSi.ReadJsonFile<DataElement>( - Path.Join(path, file), - cancellationToken - ); + DataElement dataElement = await InstanceClientMockSi.ReadJsonFile<DataElement>( + file, + cancellationToken + );test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
2945-2984: Add org/app-less GetBinaryDataList and mark org/app overload obsolete for API consistency.The review comment correctly identifies a gap:
GetBinaryDataListis the only method in the interface lacking an org/app-less overload and an[Obsolete]marker on the org/app version. All similar methods (DeleteData,GetBinaryData,GetDataBytes) follow the pattern of providing both overloads with the org/app version marked obsolete.To maintain consistency:
- Add a new
GetBinaryDataList(int instanceOwnerPartyId, Guid instanceGuid, ...)overload (without org/app parameters) to bothIDataClientandDataClient- Mark the existing org/app overload as
[Obsolete("Org and App parameters are not used, use the overload without these parameters")]in both files- Update the implementation in
DataClientaccordingly- Consider updating tests and mocks (
DataClientTests.cs,DataClientMock.cs) to use the new org/app-less overload
♻️ Duplicate comments (2)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
172-181: Contract mismatch + stale remarks in summary (does not buffer; impl returns null on 404).
- The XML doc says this buffers the entire response; current impl returns HttpContent.ReadAsStreamAsync (streaming).
- Implementation still returns null for 404 while signature is non-nullable Task. This was flagged earlier and should throw a PlatformHttpException instead.
Suggest:
- Fix DataClient.GetBinaryData to throw on 404.
- Update the summary to reflect streaming and point callers to GetBinaryDataStream for unbuffered semantics.
- /// Gets the data as is. Note: This method buffers the entire response in memory before returning the stream. - /// For memory-efficient processing of large files, use <see cref="GetBinaryDataStream"/> instead. + /// Gets the data as a stream. For large files and true unbuffered processing, prefer <see cref="GetBinaryDataStream"/>.test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs (1)
107-114: Register mocks with explicit interfaces and provide a subscription key (prevents brittle resolution/header issues).Be explicit and add a test subscription key to satisfy DataClient header initialization.
- _services.AddSingleton<HttpClient>(_mockHttpClient); - _services.AddSingleton(_appModelMock.Object); - _services.AddSingleton<IDataClient, DataClient>(); - _services.AddSingleton(_tokenResolverMock.Object); - _services.AddSingleton(_appMetadataMock.Object); - _services.AddSingleton(Options.Create(new PlatformSettings() { ApiStorageEndpoint = ApiStorageEndpoint })); - _services.AddSingleton<ModelSerializationService>(); + _services.AddSingleton<HttpClient>(_mockHttpClient); + _services.AddSingleton<IAppModel>(_appModelMock.Object); + _services.AddSingleton<IAuthenticationTokenResolver>(_tokenResolverMock.Object); + _services.AddSingleton<IAppMetadata>(_appMetadataMock.Object); + _services.AddSingleton<IDataClient, DataClient>(); + _services.AddSingleton( + Options.Create(new PlatformSettings + { + ApiStorageEndpoint = ApiStorageEndpoint, + SubscriptionKey = "test-subscription-key" + }) + ); + _services.AddSingleton<ModelSerializationService>();
🧹 Nitpick comments (7)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (3)
199-217: Exception type in docs diverges from behavior used in tests.Tests assert PlatformHttpResponseSnapshotException from GetBinaryDataStream on non‑OK responses. The XML doc here mentions PlatformHttpException. Align docs with behavior or vice‑versa.
- /// Throws <see cref="Altinn.App.Core.Helpers.PlatformHttpException"/> if the data element is not found or other HTTP errors occur. + /// Throws <see cref="Altinn.App.Core.Helpers.PlatformHttpResponseSnapshotException"/> when the data element is not found or other HTTP errors occur.Also consider documenting the new timeout parameter ordering and default (100s).
229-239: Return description is wrong (bytes, not HttpResponseMessage).- /// <returns>The raw HttpResponseMessage from the call to platform</returns> + /// <returns>The raw bytes from storage</returns>
285-288: Obsolete member has error:true but still provides a body.This default interface implementation will never be callable in valid code (compile‑time error), so the forwarding body is confusing. Consider throwing NotSupportedException or removing the body to avoid dead code.
-[Obsolete("Use method DeleteData with delayed=false instead.", error: true)] -Task<bool> DeleteBinaryData(string org, string app, int instanceOwnerPartyId, Guid instanceGuid, Guid dataGuid) => - DeleteData(org, app, instanceOwnerPartyId, instanceGuid, dataGuid, false); +[Obsolete("Use method DeleteData with delayed=false instead.", error: true)] +Task<bool> DeleteBinaryData(string org, string app, int instanceOwnerPartyId, Guid instanceGuid, Guid dataGuid);test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
91-126: Align GetBinaryDataStream mock with production exception type.Prod tests assert PlatformHttpResponseSnapshotException on non‑OK; the mock throws FileNotFoundException. Consider mirroring the platform exception for consistency across test suites.
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
174-209: Propagate cancellation and use case‑insensitive content‑type checks.- await body.CopyToAsync(memoryStream); + await body.CopyToAsync(memoryStream, CancellationToken.None); @@ - if (contentType?.Contains("application/xml", StringComparison.Ordinal) ?? true) + if (contentType?.Contains("application/xml", StringComparison.OrdinalIgnoreCase) ?? true) @@ - else if (contentType.Contains("application/json", StringComparison.Ordinal)) + else if (contentType.Contains("application/json", StringComparison.OrdinalIgnoreCase))If you have a token available in the caller, thread it through here as an extra parameter for full cancellation support.
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
2250-2267: Null handling in new DataElement-aware serialization is correct; deprecation guards migration safely.The 2-arg obsolete overloads found in tests are intentional (validating deprecated behavior). Null-safety verified:
SerializeToStorage(model, dataType, null)safely falls back toFindFirstContentType(dataType)then defaults to XML (lines 122, 125–126).DeserializeFromStorage(3-arg)validates ContentType presence; throws if missing.- No production code calls obsolete 2-arg methods; the 2-arg methods throw
InvalidOperationExceptionwhen dataType allows JSON, forcing migration.Consider adding
[System.ComponentModel.EditorBrowsable(System.ComponentModel.EditorBrowsableState.Never)]to the obsoleteDeserializeFromStorage(2-arg)andSerializeToStorage(2-arg)methods to further reduce discoverability in IDE autocomplete.src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
333-342: Consider simplifying the double-negation logic.The expression
!dataType.AllowedContentTypes.TrueForAll(ct => !ct.Equals(...))is logically correct but unnecessarily complex. It's equivalent to checking if any content type equals "application/json".Apply this diff to improve readability:
internal static bool TypeAllowsJson(DataType? dataType) { if (dataType is null) return false; if (dataType.AllowedContentTypes is null) return false; - return !dataType.AllowedContentTypes.TrueForAll(ct => - !ct.Equals("application/json", StringComparison.OrdinalIgnoreCase) + return dataType.AllowedContentTypes.Any(ct => + ct.Equals("application/json", StringComparison.OrdinalIgnoreCase) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs(6 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(11 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(17 hunks)src/Altinn.App.Core/Models/DataElementChanges.cs(1 hunks)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs(10 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs(18 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs(5 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Altinn.App.Core/Models/DataElementChanges.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs
🧠 Learnings (5)
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
src/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
PR: Altinn/app-lib-dotnet#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/**/*.{cs} : Use strongly-typed configuration classes and register options/services properly in the DI container
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs
🧬 Code graph analysis (6)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
DataClient(28-818)DataClient(47-62)TypeAllowsJson(328-331)TypeAllowsJson(333-342)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Obsolete(65-81)Obsolete(113-169)Obsolete(278-326)Obsolete(514-553)Obsolete(612-651)Task(84-110)Task(172-201)Task(204-236)Task(239-275)Task(345-373)Task(376-402)Task(405-443)Task(481-511)Task(556-609)Task(654-696)Task(699-729)Task(732-776)Task(779-817)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
Obsolete(42-53)Obsolete(86-96)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (2)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
ApplicationMetadata(1219-1251)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
DataClient(28-818)DataClient(47-62)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (5)
test/Altinn.App.Api.Tests/Data/TestData.cs (4)
org(69-90)TestData(9-221)GetDataBlobPath(116-120)GetDataDirectory(61-67)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Task(84-110)Task(172-201)Task(204-236)Task(239-275)Task(345-373)Task(376-402)Task(405-443)Task(481-511)Task(556-609)Task(654-696)Task(699-729)Task(732-776)Task(779-817)Obsolete(65-81)Obsolete(113-169)Obsolete(278-326)Obsolete(514-553)Obsolete(612-651)src/Altinn.App.Core/Internal/Data/IDataClient.cs (10)
Task(72-78)Task(115-121)Obsolete(26-38)Obsolete(51-61)Obsolete(93-105)Obsolete(134-144)Obsolete(172-181)Obsolete(230-239)Obsolete(285-287)Obsolete(300-310)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (5)
Task(174-236)DeserializeFromStorage(62-77)Type(300-311)Obsolete(42-53)Obsolete(86-96)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs (4)
test/Altinn.App.Tests.Common/Auth/TestAuthentication.cs (1)
TestAuthentication(102-664)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
DataClient(28-818)DataClient(47-62)src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-67)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
ModelSerializationService(19-401)ModelSerializationService(30-34)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (10)
Obsolete(26-38)Obsolete(51-61)Obsolete(93-105)Obsolete(134-144)Obsolete(172-181)Obsolete(230-239)Obsolete(285-287)Obsolete(300-310)Obsolete(341-351)Obsolete(364-377)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (3)
Obsolete(42-53)Obsolete(86-96)DeserializeFromStorage(62-77)src/Altinn.App.Core/Helpers/MemoryAsStream.cs (2)
MemoryAsStream(6-60)MemoryAsStream(10-13)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (15)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
479-497: Typed extension looks good.Clear cast guard with precise message; aligns with new instance+DataElement flow.
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsDataTypes.cs (1)
361-376: Obsolete API calls are intentional here; make sure code scanning findings are triaged.These tests validate backward‑compat behavior. Keep them, but suppress or justify the warnings in the analyzer config for the test project.
Also applies to: 503-569
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs (1)
411-442: Tests rely on null on 404; prepare to assert exception after bug fix.GetBinaryData should throw on 404 (non‑nullable Task). Once fixed, update:
- GetBinaryData_returns_empty_stream_when_storage_returns_notfound → assert PlatformHttpException (or the chosen exception) instead of null.
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
120-129: SerializeToStorage content‑type handling looks solid.Defaults to XML for back‑compat; supports JSON when configured or carried by DataElement. Good.
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
47-59: The review comment references properties that don't exist on the DeleteStatus model.The OpenAPI schema confirms that
DeleteStatusonly containsisHardDeletedandhardDeletedproperties. The propertiesIsSoftDeletedandSoftDeletedthat the review suggests adding exist only onInstance.Status, not onDataElement.DeleteStatus.The current code at line 54 is correct for the available model properties. The review has conflated
Instance.Statussemantics withDataElement.DeleteStatussemantics—they are distinct models on different domain objects.Likely an incorrect or invalid review comment.
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
2982-2984: New IDataClientExtensions.GetFormData(Instance, DataElement, …) — LGTM.Generic constraint and parameters align with the new content‑type aware flow.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (9)
28-28: LGTM: Properly sealed the class.Following the coding guideline to use
sealedfor classes unless inheritance is needed.
65-81: LGTM: Obsolete method correctly redirects to new API.The deprecated method properly forwards to the Instance-based overload.
113-169: LGTM: Obsolete guard correctly prevents JSON data types.The deprecation guard properly checks for JSON-configured data types and throws a clear exception directing users to the new
UpdateFormDatamethod. The fallback to XML serialization maintains backward compatibility.
171-201: LGTM: UpdateFormData correctly uses DataElement-aware serialization.The new method properly passes the
DataElementtoSerializeToStorage, allowing it to determine the correct format (JSON/XML) based on the element'sContentType. This is the correct approach for updates where the DataElement already exists.
278-326: LGTM: Obsolete GetFormData properly guards against JSON types.The deprecation guard correctly prevents usage with JSON-configured data types and provides clear guidance to use the new
GetFormData(Instance, DataElement)overload.
344-373: LGTM: New GetFormData properly implements DataElement-aware deserialization.The method correctly:
- Validates null parameters upfront
- Retrieves the DataType from application metadata
- Uses DataElement-aware deserialization to support both JSON and XML formats
514-514: LGTM: HttpRequest-based overloads properly deprecated.The obsolete markers on HttpRequest-based methods (lines 514 and 612) correctly guide users toward the Stream-based overloads, which provide better resource management and flexibility.
602-607: LGTM: Improved structured logging.The logging statement now uses structured logging with named parameters (
InstanceId,StatusCode,Content), which improves observability and enables better log querying.
84-110: No issues found—verification confirms null DataElement is correctly handled.The
SerializeToStoragemethod with nullDataElementis designed exactly for new data inserts. Line 120 ofModelSerializationService.csshows that whendataElementis null, the method determines content type viaFindFirstContentType(dataType), which returns the first allowed content type from theDataTypeconfiguration. Comprehensive unit tests confirm this behavior works correctly for both JSON and XML formats.
.../Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Show resolved
Hide resolved
…torage
When creating a new data element
1. Check appmetadata.DataTypes[type].AllowedContentTypes and see if `application/xml` or `application/json` appears first
2. Use fallback `application/xml`
When getting or updating a data element
1. Look at dataElement.ContentType and use xml serialization unless it says `application/json`
* [Obsolete] methods that does not work when storage format is dependent on DataElement.ContentType
* Obsolete methods only work if DataType.AllowedContentTypes does not contain "application/json"
* [Obsolete] methods in IDataClient that accepts org and app arguments and replace with similar methods without, as those are not required and is a hassle to provide.
ivarne
force-pushed
the
feat/json-in-storage
branch
from
fe4f38c to
44e29da
Compare
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
238-251: Fix hardcoded content type when seeding test data.
CreateInstanceDataMutatorWithDataAndLayoutalways setsdataElement.ContentTypeto"application/xml"even whendefaultDataType.AllowedContentTypescontains JSON. The serializer right below may produce JSON, leaving storage bytes and metadata out of sync and causing JSON flows to deserialize with the wrong MIME. Please setContentType(and the stored payload) to the actualcontentTypereturned bySerializeToStorage(or at least the first allowed type) before adding data toStorage.
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (1)
133-133: Consider discarding unused tuple element.The
datavariable is assigned but never used. Consider using a discard to make the intent clearer:- var (data, contentType) = _sut.SerializeToStorage(testObject, dataType, null); + var (_, contentType) = _sut.SerializeToStorage(testObject, dataType, null);test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
265-283: Populate DataElement metadata when inserting form dataThe mock now creates DataElements without setting
Size,Created, orLastChanged, so any code that inspects metadata (e.g., attachment lists or regression tests comparing instance payloads) will see zero length and null timestamps, which diverges from how Storage behaves. Please hydrate those fields when persisting the element so the mock remains faithful to production semantics.DataElement dataElement = new() { Id = dataGuid.ToString(), InstanceGuid = instanceIdentifier.InstanceGuid.ToString(), DataType = dataTypeString, - ContentType = contentType, + ContentType = contentType, + Size = serializedBytes.Length, + Created = DateTime.UtcNow, + LastChanged = DateTime.UtcNow, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
src/Altinn.App.Api/Controllers/DataController.cs(3 hunks)src/Altinn.App.Api/Controllers/InstancesController.cs(2 hunks)src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs(0 hunks)src/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cs(1 hunks)src/Altinn.App.Core/Features/Signing/Services/SigningReceiptService.cs(0 hunks)src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs(2 hunks)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs(6 hunks)src/Altinn.App.Core/Implementation/DefaultAppEvents.cs(0 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(11 hunks)src/Altinn.App.Core/Internal/Data/DataService.cs(0 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(17 hunks)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs(4 hunks)src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskCleaner.cs(0 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cs(1 hunks)src/Altinn.App.Core/Internal/Texts/TranslationService.cs(1 hunks)src/Altinn.App.Core/Models/DataElementChanges.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs(11 hunks)test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs(1 hunks)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs(10 hunks)test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(0 hunks)test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs(19 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs(0 hunks)test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs(5 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanFull.verified.txt(1 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanIncremental.verified.txt(8 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyFull.verified.txt(1 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyIncremental.verified.txt(8 hunks)test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs(1 hunks)test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs(10 hunks)test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs(4 hunks)
💤 Files with no reviewable changes (10)
- src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskCleaner.cs
- src/Altinn.App.Core/Features/Signing/Services/SigningReceiptService.cs
- test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs
- src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs
- test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
- src/Altinn.App.Core/Internal/Data/DataService.cs
- src/Altinn.App.Core/Implementation/DefaultAppEvents.cs
- test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs
- test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs
- test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Altinn.App.Core/Models/DataElementChanges.cs
- test/Altinn.App.Api.Tests/Controllers/ActionsControllerTests.cs
- test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanFull.verified.txt
- test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs
- test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyFull.verified.txt
- test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.DirtyIncremental.verified.txt
- test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/CleanDataAccessor/TestValidateCleanData.CleanIncremental.verified.txt
- test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs
- test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cstest/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cssrc/Altinn.App.Core/Internal/Texts/TranslationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cstest/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cssrc/Altinn.App.Core/Internal/Texts/TranslationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cssrc/Altinn.App.Api/Controllers/InstancesController.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cssrc/Altinn.App.Core/Internal/Texts/TranslationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
src/Altinn.App.Core/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Files:
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cs
src/Altinn.App.Core/Features/Telemetry/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Treat telemetry shapes as a public contract; changes are breaking
Files:
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs
🧠 Learnings (26)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/**/*.cs : New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cstest/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cssrc/Altinn.App.Api/Controllers/DataController.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/Telemetry/**/*.cs : Treat telemetry shapes as a public contract; changes are breaking
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cstest/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cssrc/Altinn.App.Core/Internal/Texts/TranslationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cstest/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Api/Controllers/InstancesController.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cssrc/Altinn.App.Core/Features/Action/UniqueSignatureAuthorizer.cssrc/Altinn.App.Core/Internal/Texts/TranslationService.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Api/Controllers/InstancesController.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs : Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Core.Tests/Features/**/*.cs : Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Applied to files:
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cssrc/Altinn.App.Api/Controllers/InstancesController.cssrc/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTests.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/**/*.cs : Mock external dependencies with Moq
Applied to files:
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/**/AppFixture.*.cs : Follow the AppFixture pattern as a central orchestrator for integration tests; add new API operation coverage via AppFixture.{Feature}.cs
Applied to files:
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/**/*.{cs} : Use strongly-typed configuration classes and register options/services properly in the DI container
Applied to files:
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/InstancesController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/InstancesController.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs : For HTTP APIs, define DTOs with ...Request and ...Response naming
Applied to files:
src/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs
📚 Learning: 2025-08-29T13:29:47.532Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1411
File: src/Altinn.App.Core/Models/Expressions/ExpressionConverter.cs:21-35
Timestamp: 2025-08-29T13:29:47.532Z
Learning: Expression syntax in Altinn App Core only supports array-based format ["funcname", arg1, arg2...] for function calls, not object-based format {"function":"...", "args":[...]}. The ExpressionConverter correctly throws JsonException for objects.
Applied to files:
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs
📚 Learning: 2025-09-18T07:40:01.660Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1470
File: src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs:411-418
Timestamp: 2025-09-18T07:40:01.660Z
Learning: In InstanceDataUnitOfWork.UpdateDataElement, the _binaryCache is intentionally NOT updated after successful UpdateBinaryData calls. This preserves the ability to repeatedly detect which elements were changed during a session, even after they've been saved to storage. The "dirty" detection is by design for audit/tracking purposes.
Applied to files:
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
📚 Learning: 2025-10-08T07:45:43.421Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs:230-233
Timestamp: 2025-10-08T07:45:43.421Z
Learning: In the eFormidling service, an empty or null `DataTypes` configuration intentionally means "send no data elements" rather than "send all data elements". This is the established behavior from `applicationMetadata.EFormidling.DataTypes` and must be preserved for backward compatibility. Applications must explicitly configure which data types to send.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
🧬 Code graph analysis (15)
test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (3)
test/Altinn.App.Core.Tests/LayoutExpressions/TestDataModel.cs (2)
TestDataModel(13-406)Name(434-439)test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
DataType(132-151)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(62-77)
test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (3)
test/Altinn.App.Core.Tests/Internal/Process/ProcessEngineTest.cs (1)
Mock(1200-1201)test/Altinn.App.Core.Tests/Features/Validators/ValidationServiceTests.cs (1)
Mock(70-98)test/Altinn.App.Api.Tests/Controllers/StatelessDataControllerTests.cs (1)
Mock(36-37)
test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs (3)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
DataType(132-151)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
ModelSerializationService(19-401)ModelSerializationService(30-34)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (2)
DataClientMock(16-621)DataClientMock(22-31)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs (2)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (7)
MockedServiceCollection(31-183)MockedServiceCollection(54-58)TryAddCommonServices(65-122)AddDataType(124-130)Task(187-256)DataType(132-151)VerifyMocks(176-182)test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (3)
AddInstance(47-51)Task(73-104)DataElement(58-71)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
DataClient(28-828)DataClient(47-62)TypeAllowsJson(340-343)TypeAllowsJson(345-352)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (3)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (1)
DataElement(58-71)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
DataElement(227-238)StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)src/Altinn.App.Core/Models/DataElementIdentifier.cs (1)
ToString(70-73)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (3)
src/Altinn.App.Core/Internal/Language/LanguageConst.cs (1)
LanguageConst(3-8)test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
ApplicationMetadata(1219-1251)test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
DataType(132-151)
src/Altinn.App.Core/Internal/Data/PreviousDataAccessor.cs (1)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(62-77)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (3)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (3)
StorageClientInterceptor(12-333)StorageClientInterceptor(31-42)AddDataRaw(53-56)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
ModelSerializationService(19-401)ModelSerializationService(30-34)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
DataClient(28-828)DataClient(47-62)
src/Altinn.App.Core/Features/Telemetry/Telemetry.DataClient.cs (3)
src/Altinn.App.Core/Features/Telemetry/Telemetry.Processes.cs (8)
Activity(36-41)Activity(43-49)Activity(51-56)Activity(58-63)Activity(65-71)Activity(73-78)Activity(80-85)Activity(87-92)src/Altinn.App.Core/Features/Telemetry/Telemetry.Data.cs (3)
Activity(29-34)Activity(36-40)Activity(42-46)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
DataElement(227-238)
test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs (1)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
DataType(132-151)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (4)
test/Altinn.App.Api.Tests/Data/TestData.cs (2)
org(69-90)TestData(9-221)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Obsolete(65-87)Obsolete(119-181)Obsolete(290-338)Obsolete(524-563)Obsolete(622-661)Task(90-116)Task(184-213)Task(216-248)Task(251-287)Task(355-383)Task(386-412)Task(415-453)Task(491-521)Task(566-619)Task(664-706)Task(709-739)Task(742-786)Task(789-827)src/Altinn.App.Core/Internal/Data/IDataClient.cs (10)
Obsolete(26-38)Obsolete(51-69)Obsolete(101-113)Obsolete(142-152)Obsolete(180-189)Obsolete(238-247)Obsolete(293-295)Obsolete(308-318)Obsolete(349-359)Obsolete(372-385)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
DeserializeFromStorage(62-77)Type(300-311)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (3)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
Obsolete(42-53)Obsolete(86-96)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Obsolete(65-87)Obsolete(119-181)Obsolete(290-338)Obsolete(524-563)Obsolete(622-661)Task(90-116)Task(184-213)Task(216-248)Task(251-287)Task(355-383)Task(386-412)Task(415-453)Task(491-521)Task(566-619)Task(664-706)Task(709-739)Task(742-786)Task(789-827)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (7)
Obsolete(176-189)Obsolete(222-237)Obsolete(286-301)Obsolete(350-363)Task(33-76)Task(78-89)Task(91-126)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(62-77)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
Obsolete(42-53)DeserializeFromStorage(62-77)src/Altinn.App.Core/Internal/Data/IDataClient.cs (10)
Obsolete(26-38)Obsolete(51-69)Obsolete(101-113)Obsolete(142-152)Obsolete(180-189)Obsolete(238-247)Obsolete(293-295)Obsolete(308-318)Obsolete(349-359)Obsolete(372-385)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (20)
Obsolete(176-189)Obsolete(222-237)Obsolete(286-301)Obsolete(350-363)Task(33-76)Task(78-89)Task(91-126)Task(128-142)Task(144-174)Task(191-220)Task(239-284)Task(303-348)Task(365-416)Task(418-470)Task(472-484)Task(486-500)Task(502-523)Task(525-547)Task(549-565)Task(567-604)src/Altinn.App.Core/Helpers/MemoryAsStream.cs (2)
MemoryAsStream(6-60)MemoryAsStream(10-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (15)
src/Altinn.App.Core/Internal/Texts/TranslationService.cs (1)
265-265: LGTM: Defensive null-safety improvement.The addition of the null-conditional operator when accessing
Title.Countprevents a potentialNullReferenceExceptionifTitleis null whileappMetadatais not. This aligns with nullable reference types best practices and maintains the existing fallback logic.test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs (1)
115-136: LGTM! Stricter mock behavior improves test determinism.The explicit
MockBehavior.Strictconfiguration enforces precise interaction verification, reducing false negatives. The selective use ofMockBehavior.LooseforIInstantiationProcessor,IPrefill, andIProcessEngineappropriately accommodates optional or variable interactions in these dependencies.test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs (2)
150-155: LGTM! DataElement initialization updated for content-type handling.The addition of
ContentType = "application/xml"aligns with the PR's objective to enable JSON storage format support. Defaulting to XML maintains backward compatibility.
325-338: LGTM! GetDataBytes call updated to instance-based signature.The removal of
organdappparameters aligns with the refactored IDataClient interface, which now uses instance-centric identifiers.test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs (2)
220-225: LGTM! DataElement initialization consistent with content-type handling.The addition of
ContentType = "application/xml"is consistent with the broader test updates and supports the new DataElement-aware serialization logic.
238-238: LGTM! SerializeToStorage call updated with DataElement parameter.The call correctly uses the new three-parameter overload
SerializeToStorage(data, dataType, dataElement)and extracts the data from the returned tuple.src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskInitializer.cs (1)
93-93: LGTM! InsertFormData call simplified.Removing the type parameter from
InsertFormDatastreamlines the data creation flow. Type resolution is now handled internally via theDataTypeparameter, reducing coupling toIAppModelat call sites.test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (2)
1-275: LGTM! Comprehensive test coverage for ModelSerializationService.The test suite thoroughly validates:
- Content-type negotiation (JSON/XML)
- Obsolete method behavior with guards
- DataElement-aware serialization/deserialization
- Error cases (mismatching types, unsupported content types)
- Backward compatibility (XML defaults)
This aligns well with the PR objectives to enable JSON storage format support.
64-82: Obsolete method call is intentional in this test.The test
SerializeToStorage_SerializesXmlWithObsoleteMethodexplicitly validates the behavior of the obsolete overload. This is appropriate for ensuring backward compatibility during the deprecation period. The static analysis warning can be safely ignored.src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (6)
42-53: LGTM! Obsolete method properly guarded.The obsolete
DeserializeFromStorageoverload correctly guards against JSON types and directs users to the DataElement-aware overload. The exception message is clear and actionable.
55-77: LGTM! DataElement-aware deserialization correctly implemented.The switch on
dataElement.ContentTypeclearly handles XML and JSON cases. The explicit error for missing content type (line 70-72) prevents ambiguity, though the comment suggests potential future defaulting to XML for backward compatibility—appropriate for now during the transition.
86-96: LGTM! Obsolete SerializeToStorage properly guarded and delegates to new overload.The guard correctly prevents misuse with JSON-enabled types, and the exception message accurately references
SerializeToStorage. Delegation to the new overload withnulldataElement maintains consistency.
98-129: LGTM! DataElement-aware serialization correctly implemented.The logic properly:
- Validates model type matches DataType expectation
- Preserves existing DataElement content type or selects via
FindFirstContentType- Defaults to XML for backward compatibility (line 126)
- Handles both JSON and XML serialization paths
380-400: LGTM! FindFirstContentType provides clear content-type selection logic.The helper correctly iterates through
AllowedContentTypes, returning the first supported type (XML or JSON), and defaults to XML for backward compatibility. The order-dependent selection aligns with the test coverage (seeSerializeToStorage_MultipleAllowedContentTypes_PicksTheFirstOnetest).
189-209: LGTM! String comparisons optimized with explicit ordinal comparison.Using
StringComparison.Ordinalfor content-type checks improves performance and ensures culture-invariant matching, aligning with coding guidelines for efficient code.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
266-283: Populate metadata on newly inserted data elementsThe newly created
DataElementnever getsCreated,LastChanged, orSizeset. Downstream code (e.g., attachment listings and telemetry assertions) reads those fields, so leaving them at their defaults makes the mock diverge from the real storage API and breaks size-dependent checks. Please stamp the timestamps and size when writing the blob, similar to whatInsertBinaryData/UpdateFormDataalready do.- DataElement dataElement = new() - { - Id = dataGuid.ToString(), - InstanceGuid = instanceIdentifier.InstanceGuid.ToString(), - DataType = dataTypeString, - ContentType = contentType, - }; + var now = DateTime.UtcNow; + DataElement dataElement = new() + { + Id = dataGuid.ToString(), + InstanceGuid = instanceIdentifier.InstanceGuid.ToString(), + DataType = dataTypeString, + ContentType = contentType, + Created = now, + LastChanged = now, + Size = serializedBytes.Length, + };
♻️ Duplicate comments (1)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
503-521: Add null check before dereferencing formData.The extension method assumes
GetFormDatanever returns null, but callingformData.GetType()at Line 519 will throwNullReferenceExceptionif the deserializer returns null or storage responds with empty payload.Apply this diff to add a null guard:
public static async Task<T> GetFormData<T>( this IDataClient dataClient, Instance instance, DataElement dataElement, StorageAuthenticationMethod? authenticationMethod = null, CancellationToken cancellationToken = default ) { object formData = await dataClient.GetFormData(instance, dataElement, authenticationMethod, cancellationToken); + if (formData is null) + { + throw new InvalidOperationException( + $"Form data deserialized to null for instance {instance.Id}, data element {dataElement.Id}, requested type {typeof(T).FullName}" + ); + } + if (formData is T typedFormData) { return typedFormData; } throw new InvalidCastException( $"Failed to cast form data of type {formData.GetType().FullName} to requested type {typeof(T).FullName}" ); }
🧹 Nitpick comments (1)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
87-89: Fix typo in comment.Line 87: "theses" should be "these".
- // There is no TryAddHttpClient, but theses are the core of the mocked service collection + // There is no TryAddHttpClient, but these are the core of the mocked service collection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(11 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(18 hunks)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs(10 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs(10 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cs
🧠 Learnings (19)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/**/AppFixture.*.cs : Follow the AppFixture pattern as a central orchestrator for integration tests; add new API operation coverage via AppFixture.{Feature}.cs
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/**/*.cs : Mock external dependencies with Moq
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/**/*.cs : New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Core.Tests/Features/**/*.cs : Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/Telemetry/**/*.cs : Treat telemetry shapes as a public contract; changes are breaking
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs : Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-10-08T07:45:43.421Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs:230-233
Timestamp: 2025-10-08T07:45:43.421Z
Learning: In the eFormidling service, an empty or null `DataTypes` configuration intentionally means "send no data elements" rather than "send all data elements". This is the established behavior from `applicationMetadata.EFormidling.DataTypes` and must be preserved for backward compatibility. Applications must explicitly configure which data types to send.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧬 Code graph analysis (4)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (4)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (3)
StorageClientInterceptor(12-333)StorageClientInterceptor(31-42)AddDataRaw(53-56)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
ModelSerializationService(19-401)ModelSerializationService(30-34)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
DataClient(28-826)DataClient(47-62)test/Altinn.App.Api.Tests/Mocks/AppModelMock.cs (1)
AppModelMock(6-18)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (3)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (11)
Obsolete(26-38)Obsolete(51-69)Obsolete(101-113)Obsolete(142-152)Obsolete(180-189)Obsolete(238-247)Obsolete(276-284)Obsolete(309-311)Obsolete(324-334)Obsolete(365-375)Obsolete(388-401)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(62-77)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Obsolete(65-87)Obsolete(119-181)Obsolete(290-338)Obsolete(522-561)Obsolete(620-659)Task(90-116)Task(184-213)Task(216-248)Task(251-287)Task(355-383)Task(386-412)Task(415-451)Task(489-519)Task(564-617)Task(662-704)Task(707-737)Task(740-784)Task(787-825)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (4)
test/Altinn.App.Api.Tests/Data/TestData.cs (4)
org(69-90)TestData(9-221)GetDataBlobPath(116-120)GetDataDirectory(61-67)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Task(90-116)Task(184-213)Task(216-248)Task(251-287)Task(355-383)Task(386-412)Task(415-451)Task(489-519)Task(564-617)Task(662-704)Task(707-737)Task(740-784)Task(787-825)Obsolete(65-87)Obsolete(119-181)Obsolete(290-338)Obsolete(522-561)Obsolete(620-659)src/Altinn.App.Core/Internal/Data/IDataClient.cs (14)
Task(80-86)Task(123-129)Task(162-167)Obsolete(26-38)Obsolete(51-69)Obsolete(101-113)Obsolete(142-152)Obsolete(180-189)Obsolete(238-247)Obsolete(276-284)Obsolete(309-311)Obsolete(324-334)Obsolete(365-375)Obsolete(388-401)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
DeserializeFromStorage(62-77)Type(300-311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
51-69: LGTM: Default interface implementation correctly validates and delegates.The obsolete default interface method properly validates the type parameter against the runtime type before delegating to the new non-generic overload.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (5)
28-28: LGTM: Class sealed per coding guidelines.Sealing the class follows best practices since DataClient is not designed for inheritance.
65-87: LGTM: Obsolete method correctly delegates to new API.The deprecated overload properly validates the type parameter and delegates to the instance-based
InsertFormDatamethod.
90-116: LGTM: Instance-based InsertFormData correctly serializes and delegates.The new method properly resolves the data type from metadata, serializes using
ModelSerializationService, and delegates toInsertBinaryDatawith the correct content type.
340-352: LGTM: TypeAllowsJson implementation correctly filters and checks content types.The helper methods properly:
- Filter data types by class reference with null-safe navigation
- Guard against null
AllowedContentTypes- Use correct logic to detect if JSON is allowed (at least one content type is JSON)
610-614: Verify log injection risk is mitigated.The log statement includes
instanceIdwhich is user-provided input. While structured logging inMicrosoft.Extensions.Loggingtypically handles parameter escaping, verify that the logging infrastructure properly sanitizes this input to prevent log injection attacks.Based on the static analysis hint at Line 612, consider whether additional validation or sanitization of
instanceIdis needed before logging, or confirm that the structured logging framework adequately escapes the parameter.test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (4)
42-56: LGTM: Storage property correctly initialized in constructor.Moving the
StorageClientInterceptorinitialization to the constructor aligns with theAppMetadataproperty dependency at Line 40.
132-151: LGTM: AddDataType correctly returns DataType with sensible defaults.The method now returns the created
DataType, allowing callers to reference it. The defaultAllowedContentTypesof["application/xml"]preserves backward compatibility.
176-182: LGTM: Centralized mock verification.The new
VerifyMocksmethod consolidates verification of all mocked dependencies, promoting consistent test cleanup.
233-253: Verify ContentType consistency between DataElement and serialization.The
DataElementis created with a hardcodedContentType = "application/xml"at Line 238, butSerializeToStorageat Line 252 returns a content type based on theDataTypeconfiguration. IfdefaultDataTypeallows JSON (viaallowedContentTypesparameter inAddDataType<T>), this creates an inconsistency where theDataElement.ContentTypedoesn't match the actual serialized format.Consider updating the code to use the content type returned from
SerializeToStorage:+ var (serializedData, serializedContentType) = serializer.SerializeToStorage(model, defaultDataType, data); + var data = new DataElement() { Id = dataGuid.ToString(), InstanceGuid = instanceGuid.ToString(), DataType = defaultDataType.Id, - ContentType = "application/xml", + ContentType = serializedContentType, }; // ... (instance creation) appServices.Storage.AddDataRaw( dataGuid, - serializer.SerializeToStorage(model, defaultDataType, data).data.ToArray() + serializedData.ToArray() );This ensures the
DataElement.ContentTypematches the actual serialization format, supporting both XML and JSON test scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
241-245: 404 returns null stream — keep for now, but plan the breaking changeReturning null violates the non-nullable return contract and surprises “await using” callers. Keep for compatibility if required, but schedule the change to throw PlatformHttpException and update callers. In the interim, ensure callers null-check.
Suggested future diff:
- if (response.StatusCode == HttpStatusCode.NotFound) - { - // ! TODO: Remove null return in v9 and throw exception instead - return null!; - } + if (response.StatusCode == HttpStatusCode.NotFound) + { + throw await PlatformHttpException.CreateAsync(response); + }
🧹 Nitpick comments (4)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (2)
169-180: Document 404 → null behavior to match current implementation until v9The interface promises a non-null Stream, but the current DataClient returns null on 404. Document this here (and in XML comments) so callers know to null-check until the breaking change lands.
Example doc tweak:
- “May return null if the data element is not found (HTTP 404). This will change to throwing PlatformHttpException in the next major version.”
503-521: Guard null form data before castingIf deserialization returns null, the cast throws with a “null” type name. Add an explicit null check to throw a clearer InvalidOperationException before the cast.
object formData = await dataClient.GetFormData(instance, dataElement, authenticationMethod, cancellationToken); - if (formData is T typedFormData) + if (formData is null) + { + throw new InvalidOperationException( + $"Form data for data element {dataElement.Id} was null; cannot cast to {typeof(T).FullName}." + ); + } + if (formData is T typedFormData) { return typedFormData; }test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (1)
15-24: Remove unused IAppMetadata mock and related helpersDataService no longer depends on IAppMetadata. _mockAppMetadata field, CreateAppMetadata, and setups are unused. Remove to simplify tests.
- private readonly Mock<IAppMetadata> _mockAppMetadata; ... - _mockAppMetadata = new Mock<IAppMetadata>(); - _dataService = new DataService(_mockDataClient.Object); + _dataService = new DataService(_mockDataClient.Object); ... - ApplicationMetadata applicationMetadata = CreateAppMetadata(instance); ... - _mockAppMetadata.Setup(x => x.GetApplicationMetadata()).ReturnsAsync(applicationMetadata); ... - ApplicationMetadata applicationMetadata = CreateAppMetadata(instance); ... - _mockAppMetadata.Setup(x => x.GetApplicationMetadata()).ReturnsAsync(applicationMetadata); @@ - private static ApplicationMetadata CreateAppMetadata(Instance instance) - { - return new ApplicationMetadata(instance.AppId) - { - CopyInstanceSettings = new CopyInstanceSettings { Enabled = true }, - }; - } + // Removed obsolete helpersAlso applies to: 35-36, 54-55, 90-91, 109-110
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs (1)
78-89: Remove duplicate InsertFormData call (no added coverage)elementInstanceObsolete and elementObsoleteType call the same overload with the same args. Drop one and adjust expected request count to 3.
- // Obsolete endpoint with instance and type - var elementInstanceObsolete = await dataClient.InsertFormData(_instance, dataType, data, data.GetType()); - Assert.Equal(dataType, elementInstanceObsolete.DataType); - Assert.Equal(requestContentType, elementInstanceObsolete.ContentType); - - // Obsolete endpoint with instance and type - var elementObsoleteType = await dataClient.InsertFormData(_instance, dataType, data, data.GetType()); - Assert.Equal(dataType, elementObsoleteType.DataType); - Assert.Equal(requestContentType, elementObsoleteType.ContentType); + // Obsolete endpoint with instance and type + var elementInstanceObsolete = await dataClient.InsertFormData(_instance, dataType, data, data.GetType()); + Assert.Equal(dataType, elementInstanceObsolete.DataType); + Assert.Equal(requestContentType, elementInstanceObsolete.ContentType); @@ - Assert.Equal(4, _mockedServiceCollection.Storage.RequestsResponses.Count); + Assert.Equal(3, _mockedServiceCollection.Storage.RequestsResponses.Count);Also consider adding a small test for GetBinaryDataStream to validate non-buffered, response-tied streaming semantics.
Also applies to: 103-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Altinn.App.Api/Controllers/DataController.cs(4 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(12 hunks)src/Altinn.App.Core/Internal/Data/DataService.cs(1 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(18 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskCleaner.cs(0 hunks)src/Altinn.App.Core/Internal/Texts/TranslationService.cs(2 hunks)src/Altinn.App.Core/Models/DataElementChanges.cs(2 hunks)test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs(1 hunks)test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs(10 hunks)
💤 Files with no reviewable changes (1)
- src/Altinn.App.Core/Internal/Process/ProcessTasks/Common/ProcessTaskCleaner.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.App.Core/Internal/Data/DataService.cs
- src/Altinn.App.Core/Internal/Texts/TranslationService.cs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cssrc/Altinn.App.Core/Models/DataElementChanges.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cssrc/Altinn.App.Core/Models/DataElementChanges.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Models/DataElementChanges.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cs
🧠 Learnings (17)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cssrc/Altinn.App.Core/Models/DataElementChanges.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/**/*.cs : Mock external dependencies with Moq
Applied to files:
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/Telemetry/**/*.cs : Treat telemetry shapes as a public contract; changes are breaking
Applied to files:
src/Altinn.App.Core/Models/DataElementChanges.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/**/*.cs : New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Applied to files:
src/Altinn.App.Core/Models/DataElementChanges.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/**/AppFixture.*.cs : Follow the AppFixture pattern as a central orchestrator for integration tests; add new API operation coverage via AppFixture.{Feature}.cs
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Core.Tests/Features/**/*.cs : Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs : Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Api/Controllers/DataController.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cs
📚 Learning: 2025-10-08T07:45:43.421Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs:230-233
Timestamp: 2025-10-08T07:45:43.421Z
Learning: In the eFormidling service, an empty or null `DataTypes` configuration intentionally means "send no data elements" rather than "send all data elements". This is the established behavior from `applicationMetadata.EFormidling.DataTypes` and must be preserved for backward compatibility. Applications must explicitly configure which data types to send.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Core/Internal/Data/IDataClient.cs
🧬 Code graph analysis (6)
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (1)
src/Altinn.App.Core/Internal/Data/DataService.cs (2)
DataService(8-117)DataService(17-20)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (2)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (3)
StorageClientInterceptor(12-333)StorageClientInterceptor(31-42)AddDataRaw(53-56)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
ModelSerializationService(19-401)ModelSerializationService(30-34)
src/Altinn.App.Api/Controllers/DataController.cs (2)
test/Altinn.App.Api.Tests/Data/TestData.cs (1)
org(69-90)src/Altinn.App.Core/Models/AppIdentifier.cs (1)
org(83-90)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (11)
Obsolete(26-38)Obsolete(51-69)Obsolete(101-113)Obsolete(142-152)Obsolete(180-189)Obsolete(238-247)Obsolete(276-284)Obsolete(309-311)Obsolete(324-334)Obsolete(365-375)Obsolete(388-401)src/Altinn.App.Core/Helpers/MemoryAsStream.cs (2)
MemoryAsStream(6-60)MemoryAsStream(10-13)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(62-77)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (3)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Obsolete(65-87)Obsolete(119-181)Obsolete(290-338)Obsolete(508-547)Obsolete(606-645)Task(90-116)Task(184-213)Task(216-248)Task(251-287)Task(341-369)Task(372-398)Task(401-437)Task(475-505)Task(550-603)Task(648-690)Task(693-723)Task(726-770)Task(773-811)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (7)
Obsolete(177-190)Obsolete(223-238)Obsolete(287-302)Obsolete(351-364)Task(33-76)Task(78-89)Task(91-126)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
Obsolete(42-53)Obsolete(86-96)
test/Altinn.App.Core.Tests/Infrastructure/Clients/Storage/DataClientTestsXmlJson.cs (3)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (8)
MockedServiceCollection(31-183)MockedServiceCollection(54-58)AddXunitLogging(60-63)TryAddCommonServices(65-122)AddDataType(124-130)Task(187-259)DataType(132-151)VerifyMocks(176-182)test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (3)
AddInstance(47-51)Task(73-104)DataElement(58-71)test/Altinn.App.Core.Tests/Models/ModelSerializationServiceTests.cs (12)
Theory(27-45)DataType(259-266)Fact(47-62)Fact(64-82)Fact(84-102)Fact(104-122)Fact(124-137)Fact(139-160)Fact(162-175)Fact(177-198)Fact(200-218)Fact(220-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
src/Altinn.App.Core/Models/DataElementChanges.cs (1)
37-43: Constructor visibility tightening and doc fix look goodprivate protected constructor fits the intended construction pattern; doc typo fix is correct.
Also applies to: 185-191
src/Altinn.App.Api/Controllers/DataController.cs (3)
544-549: Good switch to binary vs form split with new APIsRouting to GetFormData/UpdateFormData for app logic types and to GetBinaryData for attachments matches the new storage format model.
905-919: Null check preserves backward-compat with current GetBinaryData 404 behaviorThis complements the pending breaking change to throw on 404. The try/catch in the caller also covers the future behavior.
944-977: Correct use of instance/dataElement overloads for form data read/writeUsing GetFormData and UpdateFormData with Instance/DataElement is the right adaptation for JSON/XML awareness.
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
87-90: Fixture wiring aligns with new storage modelHttpClient interception, AddDataType defaults, and SerializeToStorage usage look correct for JSON/XML paths.
Also applies to: 132-151, 195-203, 239-253
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (2)
183-213: JSON-aware UpdateFormData path is correctSerializeToStorage with dataElement context and delegating to UpdateBinaryData is the right shape.
813-825: Json-allowed check now correct and null-safeThe classRef/DataType predicates are filtered and guard null AllowedContentTypes; good fix.
src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
215-225: No action required—parameter reordering is safeVerification confirms all call sites use defensive patterns: they either pass only the first 3 positional parameters or use named arguments (e.g.,
authenticationMethod:,cancellationToken:). The new optionaltimeoutparameter precedescancellationTokenwithout risk since existing code doesn't passtimeoutpositionally. As noted in learnings, this pattern is acceptable provided the change is documented in release notes and samples use named arguments.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
Outdated
Show resolved
Hide resolved
bjorntore
left a comment
bjorntore
left a comment
There was a problem hiding this comment.
Good job! 🎉
Probably a lot of devs that will appreciate being able to work with json all the way.
The new methods in IDataClient are much cleaner as well. Breaking changes in the interface ofc, but I can't imagine that anyone have made their own implementation of IDataClient. Have you asked @bjosttveit to double check, just in case?
keep it for the old signatures in case someone had `dataTypeString: "default"` in their app Activate the warning about not matching interface parameter names and fix the warning I got.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (1)
331-347: Update dataElement.ContentType when SerializeToStorage returns a different value.If
SerializeToStoragereturns a different content type (e.g., during XML→JSON migration), the staleContentTypeis persisted to the file. WhenDeserializeFromStoragelater switches ondataElement.ContentType, it will attempt to parse the data with the wrong format and fail.Apply this diff to keep metadata in sync:
var (serializedBytes, contentType) = _modelSerialization.SerializeToStorage( dataToSerialize, dataType, dataElement ); -Debug.Assert(contentType == dataElement.ContentType, "Content type should not change when updating data"); +if (!string.Equals(contentType, dataElement.ContentType, StringComparison.OrdinalIgnoreCase)) +{ + dataElement.ContentType = contentType; +} await File.WriteAllBytesAsync( Path.Join(dataPath, "blob", dataElement.Id), serializedBytes.ToArray(), cancellationToken ); dataElement.LastChanged = DateTime.UtcNow; dataElement.Size = serializedBytes.Length; await WriteDataElementToFile(dataElement, org, app, instanceIdentifier.InstanceOwnerPartyId, cancellationToken);
🧹 Nitpick comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
813-825: Simplify double negation in TypeAllowsJson for clarity.The logic is correct but the double negation (
!TrueForAll(ct => !ct.Equals(...))) is harder to read than necessary.Consider this clearer alternative:
internal static bool TypeAllowsJson(DataType? dataType) { - if (dataType?.AllowedContentTypes is null) - return false; - return !dataType.AllowedContentTypes.TrueForAll(ct => - !ct.Equals("application/json", StringComparison.OrdinalIgnoreCase) - ); + return dataType?.AllowedContentTypes?.Any(ct => + ct.Equals("application/json", StringComparison.OrdinalIgnoreCase) + ) ?? false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.editorconfig(1 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs(12 hunks)src/Altinn.App.Core/Internal/Data/IDataClient.cs(18 hunks)test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs(9 hunks)test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(0 hunks)test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
💤 Files with no reviewable changes (1)
- test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧰 Additional context used
📓 Path-based instructions (5)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cssrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
test/Altinn.App.Core.Tests/Features/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Files:
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
🧠 Learnings (16)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to **/*.cs : Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Applied to files:
.editorconfig
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cstest/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs : For HTTP APIs, define DTOs with ...Request and ...Response naming
Applied to files:
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/Telemetry/**/*.cs : Treat telemetry shapes as a public contract; changes are breaking
Applied to files:
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/**/*.cs : New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Applied to files:
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-09-18T07:40:01.660Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1470
File: src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs:411-418
Timestamp: 2025-09-18T07:40:01.660Z
Learning: In InstanceDataUnitOfWork.UpdateDataElement, the _binaryCache is intentionally NOT updated after successful UpdateBinaryData calls. This preserves the ability to repeatedly detect which elements were changed during a session, even after they've been saved to storage. The "dirty" detection is by design for audit/tracking purposes.
Applied to files:
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Core.Tests/Features/**/*.cs : Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/IDataClient.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Data/IDataClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs : Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
📚 Learning: 2025-10-08T07:45:43.421Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs:230-233
Timestamp: 2025-10-08T07:45:43.421Z
Learning: In the eFormidling service, an empty or null `DataTypes` configuration intentionally means "send no data elements" rather than "send all data elements". This is the established behavior from `applicationMetadata.EFormidling.DataTypes` and must be preserved for backward compatibility. Applications must explicitly configure which data types to send.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs
🧬 Code graph analysis (3)
test/Altinn.App.Api.Tests/Mocks/DataClientMock.cs (4)
test/Altinn.App.Api.Tests/Data/TestData.cs (4)
org(69-90)TestData(9-221)GetDataBlobPath(116-120)GetDataDirectory(61-67)src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Task(90-116)Task(184-213)Task(216-248)Task(251-287)Task(341-369)Task(372-398)Task(401-437)Task(475-505)Task(550-603)Task(648-690)Task(693-723)Task(726-770)Task(773-811)Obsolete(65-87)Obsolete(119-181)Obsolete(290-338)Obsolete(508-547)Obsolete(606-645)src/Altinn.App.Core/Internal/Data/IDataClient.cs (14)
Task(80-86)Task(123-129)Task(162-167)Obsolete(26-38)Obsolete(51-69)Obsolete(101-113)Obsolete(142-152)Obsolete(180-189)Obsolete(238-247)Obsolete(276-284)Obsolete(309-311)Obsolete(324-334)Obsolete(365-375)Obsolete(388-401)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (2)
DeserializeFromStorage(62-77)Type(300-311)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (18)
Obsolete(65-87)Obsolete(119-181)Obsolete(290-338)Obsolete(508-547)Obsolete(606-645)Task(90-116)Task(184-213)Task(216-248)Task(251-287)Task(341-369)Task(372-398)Task(401-437)Task(475-505)Task(550-603)Task(648-690)Task(693-723)Task(726-770)Task(773-811)
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (4)
src/Altinn.App.Core/Internal/Data/IDataClient.cs (11)
Obsolete(26-38)Obsolete(51-69)Obsolete(101-113)Obsolete(142-152)Obsolete(180-189)Obsolete(238-247)Obsolete(276-284)Obsolete(309-311)Obsolete(324-334)Obsolete(365-375)Obsolete(388-401)src/Altinn.App.Core/Helpers/MemoryAsStream.cs (2)
MemoryAsStream(6-60)MemoryAsStream(10-13)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)src/Altinn.App.Core/Helpers/Serialization/ModelSerializationService.cs (1)
DeserializeFromStorage(62-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (7)
test/Altinn.App.Core.Tests/Features/Options/Altinn2Provider/Altinn2MetadataApiClientHttpMessageHandlerMoq.cs (1)
10-17: LGTM!The parameter rename from
httpRequestMessagetorequestimproves readability and follows common .NET naming conventions. All usages have been updated correctly..editorconfig (1)
138-140: CA1725 rule addition appropriately supports the method signature refactoring.Enabling CA1725 at warning severity is well-suited to the PR's instance-centric refactoring work. When introducing new method overloads (instance-based) while deprecating old ones (org/app-based), enforcing parameter naming consistency across base and derived declarations prevents subtle bugs and maintains API clarity.
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
1-5057: Snapshot file – acknowledged.Per project policy, *.verified.txt API snapshots are not reviewed for code issues here. Proceed assuming the snapshot update is intentional.
src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs (1)
28-28: Verify the intent to make DataClient public.The coding guidelines recommend using internal accessibility by default. Making
DataClientpublic expands the API surface significantly. Is this intentional, or should consumers continue to use theIDataClientinterface for dependency injection?Based on coding guidelines.
src/Altinn.App.Core/Internal/Data/IDataClient.cs (3)
51-69: LGTM - Runtime type check provides safety during migration.The default implementation validates that the
typeparameter matchesdataToSerialize.GetType()before delegating to the new overload. This prevents misuse during the transition period while the Type parameter is being phased out.
503-521: LGTM - Null handling is correct.The null-conditional operator on line 519 (
formData?.GetType()) properly handles null values. IfGetFormDatareturns null, the pattern match on line 513 will fail, and the exception message will display "null" as the type name.
180-189: Well-structured migration from org/app to instance-based signatures.The Obsolete overloads cleanly delegate to the new instance-based methods, allowing consumers to migrate incrementally while maintaining backward compatibility.
Also applies to: 238-247, 276-284, 324-334
|
✅ All clear @bjorntore |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs (2)
549-556: Consider traditional foreach for clarity.While the
ForEachlambda is functionally correct, a traditionalforeachloop might be more readable, especially since it mutates outer variables. Both approaches are valid.
13-13: Seal the class per coding guidelines.Per project guidelines, classes should be sealed unless inheritance is required. Since this is a concrete test mock, sealing it would prevent accidental inheritance.
As per coding guidelines
Apply this diff:
-public class InstanceClientMockSi : IInstanceClient +public sealed class InstanceClientMockSi : IInstanceClient
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Analyzers/FormDataWrapper/FormDataWrapperAnalyzer.cs(1 hunks)src/Altinn.App.Analyzers/Utils/EquatableArray.cs(1 hunks)test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Analyzers/Utils/EquatableArray.cssrc/Altinn.App.Analyzers/FormDataWrapper/FormDataWrapperAnalyzer.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Analyzers/Utils/EquatableArray.cssrc/Altinn.App.Analyzers/FormDataWrapper/FormDataWrapperAnalyzer.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
🧠 Learnings (8)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/**/*.cs : New features should follow the established feature pattern under /src/Altinn.App.Core/Features (feature folder, DI registration, telemetry, and tests)
Applied to files:
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Integration.Tests/**/*.cs : Include integration tests for platform service interactions (using Docker Testcontainers)
Applied to files:
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to src/Altinn.App.Core/Features/Telemetry/**/*.cs : Treat telemetry shapes as a public contract; changes are breaking
Applied to files:
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
📚 Learning: 2025-08-29T11:06:37.385Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T11:06:37.385Z
Learning: Applies to test/Altinn.App.Core.Tests/Features/**/*.cs : Provide corresponding test coverage for each Core feature under /test/Altinn.App.Core.Tests/Features
Applied to files:
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
🧬 Code graph analysis (1)
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs (4)
src/Altinn.App.Api/Controllers/InstancesController.cs (10)
Task(920-937)Task(953-1049)Task(1074-1106)Task(1108-1170)Task(1172-1256)Task(1293-1306)Task(1318-1340)Task(1342-1359)Task(1361-1377)Instance(1266-1291)test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.cs (2)
Task(532-555)Task(557-565)test/Altinn.App.Core.Tests/Features/Action/PaymentUserActionTests.cs (1)
Instance(149-158)test/Altinn.App.Api.Tests/Data/TestData.cs (2)
org(69-90)GetInstancePath(141-145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Static code analysis
🔇 Additional comments (5)
src/Altinn.App.Analyzers/FormDataWrapper/FormDataWrapperAnalyzer.cs (1)
9-14: LGTM! Parameter rename improves readability.The parameter rename from
analysisContexttocontextis consistent throughout the method and aligns with common Roslyn analyzer conventions.test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs (3)
36-60: LGTM: CreateInstance correctly implements instance-template pattern.The method now mutates and returns the
instanceTemplateparameter, aligning with the broader refactoring to instance-centric APIs. The change correctly writes the populated template and returns it.
223-223: LGTM: Modern collection expression syntax.The collection expression
[...]is more concise and leverages C# 12 features.
489-492: LGTM: Prefer TryGetValue over ContainsKey.Using
TryGetValueis more idiomatic and avoids a second dictionary lookup.src/Altinn.App.Analyzers/Utils/EquatableArray.cs (1)
33-35: LGTM! Parameter rename follows .NET conventions.Renaming the parameter from
arraytootheraligns with standard .NET naming conventions forIEquatable<T>.Equals(T other)implementations.
|
3 participants
This required the deprecation of some methods in
IDataClientwitch provoked me to do a bigger cleanup of suboptimal signatures.Required deprecations:
Nice additions:
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests