Subform PDF service task #1512
Subform PDF service task #1512
Conversation
…eration and eFormidling.
…ult enum and use result classes instead, for expandability.
…elds from result class and instead return generic failed ProcessChangeResult.
…served key word Next.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (2)
39-62: Add an upper bound on parallel execution.The parallel execution path creates concurrent tasks for all subform data elements without limiting concurrency. Consider using
SemaphoreSlimorParallel.ForEachAsyncwith aMaxDegreeOfParallelismto prevent overwhelming the PDF microservice.As per coding guidelines: "Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)."
10-17: Addsealedkeyword to the class declaration.Per coding guidelines, classes should be
sealedunless inheritance is a valid use case. This service task class has no need for inheritance.-internal class SubformPdfServiceTask( +internal sealed class SubformPdfServiceTask( IProcessReader processReader, IPdfService pdfService, IDataClient dataClient, IProcessTaskCleaner processTaskCleaner, ILogger<SubformPdfServiceTask> logger ) : IServiceTasktest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (2)
484-509: DisposeCancellationTokenSourceto avoid resource leak.
CancellationTokenSourceimplementsIDisposableand should be disposed after use.[Fact] public async Task Execute_WithCancellationToken_Should_PropagateCancellation() { // Arrange SetupProcessReader(parallelExecution: false); var instance = CreateInstanceWithSubformData(); - var cts = new CancellationTokenSource(); - cts.Cancel(); // Already cancelled - var context = CreateServiceTaskContext(instance, cts.Token); + using var cts = new CancellationTokenSource(); + cts.Cancel(); // Already cancelled + var context = CreateServiceTaskContext(instance, cts.Token);
656-692: DisposeCancellationTokenSourceto avoid resource leak.Same issue as the other test -
CancellationTokenSourceshould be disposed.[Fact] public async Task Execute_Should_PassCancellationTokenToAllDependencies() { // Arrange SetupProcessReader(parallelExecution: false); var instance = CreateInstanceWithSubformData(); - var cts = new CancellationTokenSource(); + using var cts = new CancellationTokenSource(); var context = CreateServiceTaskContext(instance, cts.Token);
🧹 Nitpick comments (4)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (1)
98-104: Remove or update the misleading comment.The comment on line 100 says "return a default valid configuration. No required config as of now" but the code immediately throws an
ApplicationConfigException. The comment contradicts the actual behavior.if (subformPdfConfiguration == null) { - // If no PDF configuration is specified, return a default valid configuration. No required config as of now. throw new ApplicationConfigException( "The subformPdfConfig node is missing in the subform pdf process task configuration." ); }test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (3)
189-202: Update test name to reflect actual behavior.The test name
Execute_WithNoPdfConfiguration_Should_Use_DefaultConfigurationsuggests a default configuration is used, but the test actually verifies that anApplicationConfigExceptionis thrown. Consider renaming for clarity.[Fact] -public async Task Execute_WithNoPdfConfiguration_Should_Use_DefaultConfiguration() +public async Task Execute_WithNoPdfConfiguration_Should_ThrowApplicationConfigException() {
513-543: Test doesn't verify execution order.The test name and comment claim to verify that cleanup happens before PDF generation, but the assertions only verify both methods are called—not their order. Consider using
MockSequenceor callback counters to enforce ordering if this is an important invariant.// Example with callback counters: var callOrder = new List<string>(); _processTaskCleanerMock .Setup(x => x.RemoveAllDataElementsGeneratedFromTask(It.IsAny<Instance>(), It.IsAny<string>())) .Callback(() => callOrder.Add("cleanup")) .Returns(Task.CompletedTask); _pdfServiceMock .Setup(x => x.GenerateAndStoreSubformPdf(...)) .Callback(() => callOrder.Add("pdf")) .ReturnsAsync(...); // Assert Assert.Equal(new[] { "cleanup", "pdf", "pdf" }, callOrder);
76-161: Good test coverage for execution paths.Tests appropriately cover parallel execution, sequential execution, and no matching data elements scenarios. The verification of call counts and parameter matching is thorough.
Note: Per coding guidelines, xUnit asserts are preferred over FluentAssertions in tests. The
result.Should().BeOfType<ServiceTaskSuccessResult>()calls could be replaced withAssert.IsType<ServiceTaskSuccessResult>(result), but this is a minor stylistic preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
🧠 Learnings (13)
📓 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-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/ServiceTasks/SubformPdfServiceTask.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.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:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Use sealed for classes unless inheritance is considered a valid use-case
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-08-27T06:58:28.126Z
Learnt from: cammiida
Repo: Altinn/app-lib-dotnet PR: 1451
File: src/Altinn.App.Api/Controllers/DataTagsController.cs:0-0
Timestamp: 2025-08-27T06:58:28.126Z
Learning: In the Altinn app-lib-dotnet codebase, validation endpoints follow a pattern where validation only runs when the ignoredValidators query parameter is provided (not null). This is a conscious design choice for consistency across validation endpoints.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.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/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.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/Process/ServiceTasks/SubformPdfServiceTaskTests.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/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.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.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (3)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (5)
Task(17-17)Task(29-35)Task(48-55)Task(63-63)Task(76-76)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (2)
ValidAltinnSubformPdfConfiguration(38-58)AltinnSubformPdfConfiguration(10-67)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/IServiceTask.cs (1)
ServiceTaskSuccessResult(42-42)
⏰ 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 (3)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (1)
109-124: LGTM!The metadata helper correctly sets the subform component ID and data element ID, then persists via
dataClient.Updatewith proper cancellation token propagation.test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (2)
16-74: LGTM!Test setup is well-structured with clearly defined mocks and a reusable
_serviceTaskinstance. The mock configurations forGenerateAndStoreSubformPdfanddataClient.Updateproperly capture and return relevant data for verification.
694-792: LGTM!Helper methods are well-organized, reducing duplication across tests. The parameterized
CreateInstanceWithManySubformDatahelper is a good pattern for testing edge cases with varying data sizes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (2)
513-538: Dispose theCancellationTokenSourceto prevent resource leaks.The
CancellationTokenSourceat line 519 is not disposed. While this may not cause issues in short-lived test runs, it's good practice to dispose it.Apply this diff:
[Fact] public async Task Execute_WithCancellationToken_Should_PropagateCancellation() { // Arrange SetupProcessReader(degreeOfParallelism: 1); var instance = CreateInstanceWithSubformData(); - var cts = new CancellationTokenSource(); - cts.Cancel(); // Already cancelled - var context = CreateServiceTaskContext(instance, cts.Token); + using var cts = new CancellationTokenSource(); + cts.Cancel(); // Already cancelled + var context = CreateServiceTaskContext(instance, cts.Token);
685-721: Dispose theCancellationTokenSourceto prevent resource leaks.The
CancellationTokenSourceat line 691 is not disposed.Apply this diff:
[Fact] public async Task Execute_Should_PassCancellationTokenToAllDependencies() { // Arrange SetupProcessReader(degreeOfParallelism: 1); var instance = CreateInstanceWithSubformData(); - var cts = new CancellationTokenSource(); + using var cts = new CancellationTokenSource(); var context = CreateServiceTaskContext(instance, cts.Token);src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (2)
7-10: Fix the XML documentation comment.The documentation incorrectly states "Configuration properties for payments" but this class is for subform PDF configuration.
Apply this diff:
/// <summary> -/// Configuration properties for payments in a process task +/// Configuration properties for subform PDF generation in a process task /// </summary>
39-66: Accumulate all validation errors before throwing.The current logic throws immediately after the first validation failure. If both
SubformComponentIdandSubformDataTypeIdare invalid, only the first error is reported. This forces developers to fix errors one at a time.Apply this diff to accumulate all errors before throwing:
internal ValidAltinnSubformPdfConfiguration Validate() { List<string>? errorMessages = null; string? normalizedFilename = string.IsNullOrWhiteSpace(FilenameTextResourceKey) ? null : FilenameTextResourceKey.Trim(); - if (SubformComponentId.IsNullOrWhitespace(ref errorMessages, $"{nameof(SubformComponentId)} is missing.")) - ThrowApplicationConfigException(errorMessages); - - if (SubformDataTypeId.IsNullOrWhitespace(ref errorMessages, $"{nameof(SubformDataTypeId)} is missing.")) - ThrowApplicationConfigException(errorMessages); + SubformComponentId.IsNullOrWhitespace(ref errorMessages, $"{nameof(SubformComponentId)} is missing."); + SubformDataTypeId.IsNullOrWhitespace(ref errorMessages, $"{nameof(SubformDataTypeId)} is missing."); if (DegreeOfParallelism < 1) { errorMessages ??= []; errorMessages.Add($"{nameof(DegreeOfParallelism)} must be at least 1."); - ThrowApplicationConfigException(errorMessages); } + if (errorMessages is not null) + ThrowApplicationConfigException(errorMessages); + return new ValidAltinnSubformPdfConfiguration( normalizedFilename, SubformComponentId, SubformDataTypeId, DegreeOfParallelism ); }
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (1)
87-88: Consider using xUnit asserts for consistency.The tests use a mix of FluentAssertions (
.Should().BeOfType<>()) and xUnit asserts (Assert.ThrowsAsync). Per coding guidelines, prefer xUnit asserts in tests.Example replacement:
- result.Should().BeOfType<ServiceTaskSuccessResult>(); + Assert.IsType<ServiceTaskSuccessResult>(result);Also applies to: 117-117, 146-146, 643-643, 670-670
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (1)
116-120: Simplify collection initialization.Per static analysis hint, the collection initialization can be simplified using a collection expression.
Apply this diff:
- pdfDataElement.Metadata = new List<KeyValueEntry> - { - new() { Key = "subformComponentId", Value = subformComponentId }, - new() { Key = "subformDataElementId", Value = subformDataElementId }, - }; + pdfDataElement.Metadata = + [ + new() { Key = "subformComponentId", Value = subformComponentId }, + new() { Key = "subformDataElementId", Value = subformDataElementId }, + ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
🧠 Learnings (11)
📓 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-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/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.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/Process/ServiceTasks/SubformPdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-08-27T06:58:28.126Z
Learnt from: cammiida
Repo: Altinn/app-lib-dotnet PR: 1451
File: src/Altinn.App.Api/Controllers/DataTagsController.cs:0-0
Timestamp: 2025-08-27T06:58:28.126Z
Learning: In the Altinn app-lib-dotnet codebase, validation endpoints follow a pattern where validation only runs when the ignoredValidators query parameter is provided (not null). This is a conscious design choice for consistency across validation endpoints.
Applied to files:
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.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:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.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/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Use sealed for classes unless inheritance is considered a valid use-case
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
🧬 Code graph analysis (2)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (3)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (4)
Task(17-17)Task(29-35)Task(48-55)Task(63-63)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-82)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (1)
AltinnSubformPdfConfiguration(10-75)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (1)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnExtensionConfigValidationExtensions.cs (1)
IsNullOrWhitespace(7-21)
🪛 GitHub Check: SonarCloud Code Analysis
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
[warning] 116-116: Collection initialization can be simplified
🔇 Additional comments (9)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (3)
16-74: Well-structured test setup with comprehensive mocking.The test class setup properly mocks all dependencies and configures sensible default behaviors. The constructor-based initialization pattern keeps tests clean and maintainable.
76-103: LGTM - Parallel execution test.This test correctly verifies that PDF generation is called for all subform data elements when parallel execution is configured.
723-821: Well-designed helper methods for test data creation.The helper methods provide clean, reusable test data creation with clear naming conventions (
CreateInstanceWithSubformData,CreateInstanceWithoutSubformData, etc.). The parameterizedCreateInstanceWithManySubformData(int count)is particularly useful for edge case testing.src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (3)
10-18: LGTM - Class declaration follows coding guidelines.The class is properly declared as
internal sealedand uses primary constructor syntax. TheTypeproperty clearly identifies this service task.
39-85: Well-implemented bounded parallelism pattern.The semaphore-based approach correctly limits concurrent PDF generation to
degreeOfParallelism. Theusingstatement ensures proper disposal, and the try/finally ensures semaphore release even on failure.
92-106: Configuration validation approach is correct.The method appropriately throws
ApplicationConfigExceptionwhen configuration is missing, providing clear guidance to developers. The delegation toValidate()keeps validation logic centralized in the configuration class.src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (1)
77-82: LGTM - Clean validated configuration record.The
ValidAltinnSubformPdfConfigurationreadonly record struct provides a clean, immutable representation of validated configuration with non-nullable required fields.test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (2)
3265-3271: Public API change acknowledged: confirm migration and DI stability
- GenerateAndStorePdf now returns DataElement; new overloads added; PdfService ctor extended/reordered. Please:
- Confirm all call sites and implementations updated.
- Add release notes/upgrade guide highlighting the return-type change and new overloads.
- Verify DI registrations still resolve unambiguously (constructor injection by type should be unaffected).
As per project learnings about accepting source-only compatibility with documented changes.
Also applies to: 3287-3294
3433-3444: New subform PDF configuration: validate defaults and docs
- Ensure DegreeOfParallelism has sane defaults and input validation.
- Confirm XML schema/docs for subformPdfConfig are updated and examples provided.
- Verify end-to-end tests cover SubformPdfServiceTask happy-path and error cases.
Based on learnings, proposing follow-up docs/tests rather than code changes in snapshot.
Also applies to: 3459-3461
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (3)
7-14: DI ofInstanceDataUnitOfWorkInitializerviaIServiceProviderResolving
InstanceDataUnitOfWorkInitializerlazily throughIServiceProvideris a reasonable way to keep the dependency optional and maintain backwards compatibility. Two things to double‑check:
- Ensure the lifetime of
PdfServiceis notSingletonifInstanceDataUnitOfWorkInitializeris registered asScoped/Transient, to avoid capturing a scoped service in a singleton.- Confirm
InstanceDataUnitOfWorkInitializer(and any cached state it returns) is safe to use from multiple concurrent calls toGetFileName(e.g., whenSubformPdfServiceTaskruns in parallel).If both are satisfied, the pattern is fine; otherwise it would be cleaner to inject
InstanceDataUnitOfWorkInitializer?directly instead of going viaIServiceProvider.Also applies to: 33-63
177-205: Subform URL construction inBuildUriThe extended
GeneratePdfContent/BuildUripipeline for subform PDFs is generally sound: you only inject the/currentTaskId/subform/{subformComponentId}/{subformDataElementId}segment when both subform identifiers are present, and you preserve existing query handling and thelangparameter.Two small points:
- The branch that inserts before
?pdf=1currently builds:Sinceurl = $"{beforePdf}/{subformComponentId}/{subformDataElementId}/{afterPdf}";afterPdfstarts with?pdf=1..., this produces a URL with an extra/immediately before the query (.../{subformDataElementId}/?pdf=1...). That’s usually tolerated by routing, but slightly odd. You can tighten this up by dropping the extra slash:
string beforePdf = $"{url[..pdfIndex]}/{currentTaskId}/subform";string afterPdf = url[pdfIndex..];url = $"{beforePdf}/{subformComponentId}/{subformDataElementId}/{afterPdf}";
string beforePdf = $"{url[..pdfIndex]}/{currentTaskId}/subform";string afterPdf = url[pdfIndex..]; // starts with '?pdf=1...'url = $"{beforePdf}/{subformComponentId}/{subformDataElementId}{afterPdf}";
- Because
currentTaskIdis supplied separately, behavior relies oninstance.Process?.CurrentTask?.ElementIdmatching the logical task you want to encode in the URL. If you ever call this whileCurrentTaskhas advanced or isnull, you’ll get an empty or stale segment. If that’s a concern, consider threading the intended task id down intoGeneratePdfContentand intoBuildUriinstead of re‑reading it from the instance.Also applies to: 224-252
293-351: Filename generation with variable substitution and escapingThe new
GetFileNamelogic looks good:
- When
_instanceDataUnitOfWorkInitializerand a custom text key are available, you useLayoutEvaluatorState+ComponentContext+DataElementIdentifierto enable variable substitution.- You fall back to simple translation (with
"backend.pdf_default_file_name"as default key) and finally a hardcoded"Altinn PDF.pdf"if the translation is missing.- Returning
Uri.EscapeDataString(fileName.AsFileName(false))and then enforcing aTwo minor suggestions:
- The
InvalidOperationException("LayoutEvaluatorState should not be null.")is a bit opaque in production. Including at least theinstance.Idand perhapscustomFileNameTextResourceKey(and/or current task id) would make misconfiguration much easier to diagnose.- If
InstanceDataUnitOfWorkInitializer.Initever becomes more expensive or cancellable, consider threading aCancellationTokenintoGetFileNameso the service task can be cancelled cleanly during filename evaluation.Functionally, though, this implementation is solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/Pdf/PdfService.cs
🧠 Learnings (7)
📓 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-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/Pdf/PdfService.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Internal/Pdf/PdfService.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:
src/Altinn.App.Core/Internal/Pdf/PdfService.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:
src/Altinn.App.Core/Internal/Pdf/PdfService.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.Core/Internal/Pdf/PdfService.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.Core/Internal/Pdf/PdfService.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (4)
DataElement(227-238)InstanceDataUnitOfWork(25-729)InstanceDataUnitOfWork(64-97)LayoutEvaluatorState(179-206)
⏰ 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 (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (2)
67-95: Public API shape and internal refactor aroundGenerateAndStorePdfThe split into:
Task GenerateAndStorePdf(Instance, string taskId, CancellationToken)(fire‑and‑forget wrapper), andTask<DataElement> GenerateAndStorePdf(Instance, string taskId, string? customFileNameTextResourceKey, List<string>? autoGeneratePdfForTaskIds = null, CancellationToken ct = default)with a shared
GenerateAndStorePdfInternalreturningDataElementlooks consistent and improves reuse.However, changing the return type of the overload that now returns
Task<DataElement>is a source‑level breaking change for any consumers that previously called it and ignored the result only implicitly. Given this sits on a public abstraction, please:
- Verify all usages in this repo and any known downstreams compile cleanly after the change.
- Mention the behavioral/API change in release notes/changelog, in line with the project’s stance on source‑only compatibility for NuGet consumers. Based on learnings, this is expected for public API shifts.
The internal flow (language resolution,
GeneratePdfContent,InsertBinaryData, and returning theDataElement) looks correct.Also applies to: 128-135, 137-175
96-115: NewGenerateAndStoreSubformPdfoverloadThe dedicated
GenerateAndStoreSubformPdfthat threadssubformComponentIdandsubformDataElementIdintoGenerateAndStorePdfInternalis straightforward and aligns with howSubformPdfServiceTaskuses it (per the provided snippet). PassingnullforautoGeneratePdfForTaskIdshere is also appropriate.No issues from a correctness standpoint.
...pp.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs
Outdated
Show resolved
Hide resolved
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
Outdated
Show resolved
Hide resolved
src/Altinn.App.Core/Internal/Process/ProcessEngineAuthorizer.cs
Outdated
Show resolved
Hide resolved
…bform pdf service task.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (2)
460-462: Dispose theCancellationTokenSourceto prevent resource leaks.The
CancellationTokenSourcecreated here is never disposed. Wrap it in ausingstatement.- var cts = new CancellationTokenSource(); - cts.Cancel(); // Already cancelled - var context = CreateServiceTaskContext(instance, cts.Token); + using var cts = new CancellationTokenSource(); + cts.Cancel(); // Already cancelled + var context = CreateServiceTaskContext(instance, cts.Token);As per coding guidelines, remember to dispose IDisposable/IAsyncDisposable instances.
632-633: Dispose theCancellationTokenSourceto prevent resource leaks.Same issue as above - the
CancellationTokenSourceis not disposed.- var cts = new CancellationTokenSource(); - var context = CreateServiceTaskContext(instance, cts.Token); + using var cts = new CancellationTokenSource(); + var context = CreateServiceTaskContext(instance, cts.Token);
🧹 Nitpick comments (2)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (1)
10-10: Seal the class per coding guidelines.Per the coding guidelines, classes should be sealed unless inheritance is a valid use case. Since this is a configuration class for XML deserialization with no apparent inheritance scenario, it should be sealed.
Apply this diff:
-public class AltinnSubformPdfConfiguration +public sealed class AltinnSubformPdfConfigurationtest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (1)
10-10: Consider using xUnit asserts instead of FluentAssertions.The coding guidelines prefer xUnit asserts over FluentAssertions in tests. The file already uses
Assert.ThrowsAsyncin several places but uses FluentAssertions for type assertions (e.g., lines 88, 117, 584, 611).Example replacement:
- result.Should().BeOfType<ServiceTaskSuccessResult>(); + Assert.IsType<ServiceTaskSuccessResult>(result);As per coding guidelines, prefer xUnit asserts over FluentAssertions in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs(1 hunks)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/IPdfService.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
🧠 Learnings (9)
📓 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-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/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/IPdfService.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.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:
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cstest/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:
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/IPdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-27T06:58:28.126Z
Learnt from: cammiida
Repo: Altinn/app-lib-dotnet PR: 1451
File: src/Altinn.App.Api/Controllers/DataTagsController.cs:0-0
Timestamp: 2025-08-27T06:58:28.126Z
Learning: In the Altinn app-lib-dotnet codebase, validation endpoints follow a pattern where validation only runs when the ignoredValidators query parameter is provided (not null). This is a conscious design choice for consistency across validation endpoints.
Applied to files:
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Use sealed for classes unless inheritance is considered a valid use-case
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
🧬 Code graph analysis (4)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (2)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (1)
ValidAltinnSubformPdfConfiguration(76-90)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnExtensionConfigValidationExtensions.cs (1)
IsNullOrWhitespace(7-21)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (2)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (2)
Task(20-74)Task(92-107)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
Task(34-56)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (3)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (3)
SubformPdfServiceTask(10-108)Task(20-74)Task(92-107)src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (5)
Task(17-17)Task(29-35)Task(48-55)Task(63-63)Task(76-76)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (1)
AltinnSubformPdfConfiguration(10-54)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (4)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (5)
Task(17-17)Task(29-35)Task(48-55)Task(63-63)Task(76-76)src/Altinn.App.Core/Internal/Pdf/PdfService.cs (11)
Task(67-72)Task(75-94)Task(97-115)Task(118-129)Task(132-135)Task(137-175)Task(177-222)Task(293-351)Task(353-361)Task(363-402)List(404-419)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (2)
ValidAltinnSubformPdfConfiguration(30-45)AltinnSubformPdfConfiguration(10-54)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-82)
⏰ 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: Analyze (csharp)
- GitHub Check: Static code analysis
- 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)
🔇 Additional comments (7)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (1)
30-60: LGTM!The validation logic correctly validates required fields and returns a strongly-typed record struct with non-nullable validated values. The optional
FilenameTextResourceKeyis appropriately normalized and remains nullable in the result.test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (1)
16-74: LGTM! Well-structured test class with comprehensive setup.The test class is well-organized with:
- Clear mock setup in the constructor
- Appropriate constants for test data
- Good separation of concerns between test setup and assertions
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (2)
28-35: LGTM! Return type enhancement with backward-compatible default.The change from
TasktoTask<DataElement>enables callers to use the returned data element (as demonstrated inSubformPdfServiceTask). The default implementation throwingNotImplementedExceptionensures existing implementations that don't override this method will fail fast rather than silently.
37-55: LGTM! Internal method addition is well-documented.The new
GenerateAndStoreSubformPdfmethod:
- Uses appropriate
internalvisibility for internal-only consumption- Has complete XML documentation for all parameters
- Follows the established pattern with a
NotImplementedExceptiondefaultsrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (3)
10-18: LGTM! Class declaration follows all coding guidelines.The class correctly uses:
internalaccessibilitysealedmodifier- Primary constructor for dependency injection
- Descriptive type identifier
38-69: Sequential PDF generation is acceptable for this use case.While the coding guidelines generally recommend avoiding
awaitin loops, the sequential approach here is reasonable because:
- Each PDF generation may modify shared state (metadata updates)
- Simpler error handling and debugging
- Previous parallel implementation was removed (per past reviews)
If performance becomes a concern with many subform elements, consider adding bounded parallelism in a follow-up.
92-107: LGTM! Metadata update method is clean and correct.The method properly:
- Sets the required metadata keys
- Passes the cancellation token to the async operation
- Uses the injected
dataClientdependency
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
Show resolved
Hide resolved
|
/publish |
PR release:
|
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)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
224-273: Consider adding input validation for URL construction edge cases.The string manipulation logic to inject subform segments (lines 238-252) has potential fragility concerns:
- URL construction assumes specific structure: The code searches for
"?pdf=1"as a query string marker, which is brittle if URL format changes.- Empty currentTaskId with subform parameters: While theoretically possible (line 197 returns
string.Empty), in practice the calling code structure mitigates this—SubformPdfServiceTaskrequires a validCurrentTask, and subform parameters are only passed from that context.- No explicit validation: The code checks that subform parameters are non-empty but doesn't validate
currentTaskIdwhen subform insertion is needed.Recommended improvements:
- Add comments documenting the expected URL format and why
"?pdf=1"is the insertion marker- Consider adding a validation check that
currentTaskIdis not empty if subform parameters are provided, or document why this invariant is guaranteed- Add unit tests directly for
BuildUriwith various parameter combinations, including edge cases
🤖 Fix all issues with AI Agents
In @src/Altinn.App.Core/Internal/Pdf/PdfService.cs:
- Around line 196-204: GenerateAndStorePdfInternal currently accepts
subformComponentId/subformDataElementId without ensuring instance.Process (and
its CurrentTask) is present, which can lead to empty currentTaskId being passed
into BuildUri; add a guard clause at the start of GenerateAndStorePdfInternal
that if either subformComponentId or subformDataElementId is non-empty, validate
instance.Process and instance.Process.CurrentTask are non-null and throw/return
a clear error (or ArgumentException/InvalidOperationException) if they are
missing; update the method to perform this check before calling BuildUri so
BuildUri always receives a valid currentTaskId when subform params are provided.
🧹 Nitpick comments (2)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (2)
33-33: Verify: Service Locator pattern for optional dependency.Resolving
InstanceDataUnitOfWorkInitializerviaIServiceProvider?.GetService<>()couples this service to the DI container and makes the dependency graph less explicit. While this appears intentional to make the feature optional (for variable substitution in filenames), consider whether this dependency could be injected directly as an optional parameter with a default null value instead.If this pattern is necessary for backward compatibility or gradual rollout, please confirm this is the intended approach.
Alternative: Direct optional injection
public PdfService( IDataClient dataClient, IHttpContextAccessor httpContextAccessor, IPdfGeneratorClient pdfGeneratorClient, IOptions<PdfGeneratorSettings> pdfGeneratorSettings, IOptions<GeneralSettings> generalSettings, ILogger<PdfService> logger, IAuthenticationContext authenticationContext, ITranslationService translationService, - IServiceProvider? serviceProvider = null, + InstanceDataUnitOfWorkInitializer? instanceDataUnitOfWorkInitializer = null, Telemetry? telemetry = null ) { // ... - _instanceDataUnitOfWorkInitializer = serviceProvider?.GetService<InstanceDataUnitOfWorkInitializer>(); + _instanceDataUnitOfWorkInitializer = instanceDataUnitOfWorkInitializer; _telemetry = telemetry; }Also applies to: 50-50, 62-62
293-351: LGTM! Variable substitution added with proper fallback.The enhanced
GetFileNamemethod:
- ✅ Supports variable substitution when
InstanceDataUnitOfWorkInitializeris available- ✅ Correctly creates subform-specific
DataElementIdentifierwhen needed- ✅ Falls back to simple translation if substitution is unavailable
- ✅ Bug fix correctly applied: returns
escapedFileNameinstead of unescapedfileNameMinor suggestion: The fallback behavior (lines 331-338) when
_instanceDataUnitOfWorkInitializeris null butcustomFileNameTextResourceKeyis provided means variable expressions in the custom filename won't be substituted. This is reasonable but could surprise users. Consider adding a log message or documentation noting when substitution is skipped.Optional: Log when substitution is unavailable
else { + if (customFileNameTextResourceKey != null && _instanceDataUnitOfWorkInitializer == null) + { + _logger.LogDebug( + "Variable substitution not available for custom filename '{Key}'. Using simple translation.", + customFileNameTextResourceKey + ); + } // Fall back to simple translation without variable substitution fileName = await _translationService.TranslateTextKey( customFileNameTextResourceKey ?? "backend.pdf_default_file_name", language ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Core/Internal/Pdf/PdfService.cstest/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/Pdf/PdfService.cs
🧠 Learnings (7)
📓 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-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Internal/Pdf/PdfService.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:
src/Altinn.App.Core/Internal/Pdf/PdfService.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:
src/Altinn.App.Core/Internal/Pdf/PdfService.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:
src/Altinn.App.Core/Internal/Pdf/PdfService.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.Core/Internal/Pdf/PdfService.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.Core/Internal/Pdf/PdfService.cs
⏰ 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: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (4)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (4)
7-7: LGTM! Imports support new functionality.The new imports are necessary for variable substitution (
Expressions,Models.Expressions) and optional dependency resolution (DependencyInjection).Also applies to: 10-10, 13-13
67-94: LGTM! Signature extended to return DataElement.The existing void-returning overload now discards the result appropriately, and the new overload returns
Task<DataElement>for callers that need the created data element. The null values for subform parameters correctly indicate this is not a subform PDF generation.
96-115: LGTM! New subform PDF generation method.The new
GenerateAndStoreSubformPdfmethod properly accepts subform-specific parameters and delegates to the internal implementation. The signature is clear and follows established patterns.
137-175: LGTM! Internal method updated for subform support.The method signature correctly propagates subform context to child methods and returns the created
DataElementfor upstream callers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/Altinn.App.Api/Controllers/SigningController.cs:
- Around line 91-95: When computing finalTaskId you currently access
instance.Process.CurrentTask.ElementId directly which throws if CurrentTask is
null; change usages to use the null-conditional operator and null-coalescing so
you read taskId ?? instance.Process.CurrentTask?.ElementId (and similarly where
you construct other taskId variables at the other occurrences), then ensure
VerifyIsSigningTask and the NotSigningTask call paths handle a null finalTaskId
(i.e., allow VerifyIsSigningTask to return false for null). Update the three
places that follow this pattern in SigningController (the initial finalTaskId
and the two other similar computations around the later checks) to use
?.ElementId and guarded null checks instead of dereferencing CurrentTask
directly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.App.Api/Controllers/SigningController.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/SigningController.cs
🧠 Learnings (10)
📓 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-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.Api/Controllers/SigningController.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Api/Controllers/SigningController.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:
src/Altinn.App.Api/Controllers/SigningController.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:
src/Altinn.App.Api/Controllers/SigningController.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:
src/Altinn.App.Api/Controllers/SigningController.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/SigningController.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/SigningController.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Api/Controllers/SigningController.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.Api/Controllers/SigningController.cs
🧬 Code graph analysis (1)
src/Altinn.App.Api/Controllers/SigningController.cs (6)
src/Altinn.App.Core/Internal/Linq/Extensions.cs (1)
IsNullOrEmpty(21-24)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
IInstanceDataAccessor(144-154)IInstanceDataAccessor(159-175)src/Altinn.App.Core/Features/IInstanceDataAccessor.cs (2)
IInstanceDataAccessor(52-52)IInstanceDataAccessor(58-58)src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (1)
AltinnSignatureConfiguration(140-154)src/Altinn.App.Core/Internal/Process/Elements/ProcessTask.cs (1)
ProcessTask(9-25)src/Altinn.App.Core/Internal/Process/Elements/ExtensionElements.cs (1)
ExtensionElements(9-22)
⏰ 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 (6)
src/Altinn.App.Api/Controllers/SigningController.cs (6)
3-3: LGTM! Using statements support new functionality.The added namespaces are necessary for IInstanceDataAccessor and ProcessTask usage in the new code.
Also applies to: 12-12
97-101: LGTM! Proper initialization of IInstanceDataAccessor.The use of
InstanceDataUnitOfWorkInitializerto initialize the data accessor with the finalTaskId is correct and follows the established pattern.
173-225: Implementation follows consistent pattern.The changes to
GetAuthorizedOrganizationsmirror the approach inGetSigneesState, properly usingfinalTaskIdandIInstanceDataAccessor. The same NullReferenceException concern at line 191 was already flagged in the previous comment.
251-292: LGTM! Efficient implementation without unnecessary data accessor initialization.This endpoint correctly uses
finalTaskIdto fetch signing configuration and filter data elements. Unlike the other endpoints, it doesn't need to initializeIInstanceDataAccessorsince it operates directly on the instance's data elements, which is more efficient.The NullReferenceException concern at line 267 was already flagged in the previous comment.
295-301: LGTM! Clean implementation of task type validation.The helper method correctly validates whether a given taskId corresponds to a signing task by inspecting the process task's extension elements.
303-314: LGTM! Error message clearly explains both usage scenarios.The updated message properly informs users that the endpoint works either when the current task is a signing task OR when the taskId query parameter points to one.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (2)
10-10: Consider using xUnit asserts instead of FluentAssertions.Per coding guidelines, xUnit asserts are preferred over FluentAssertions in tests. Multiple test methods use
.Should().BeOfType<>()which could be replaced withAssert.IsType<>().🔎 Example replacement
- result.Should().BeOfType<ServiceTaskSuccessResult>(); + Assert.IsType<ServiceTaskSuccessResult>(result);Also applies to: 87-87
477-506: Test verifies ordering but doesn't enforce strict sequence.The test name
Execute_Should_CleanupBeforePdfGenerationimplies ordering verification, but Moq'sVerifycalls don't actually enforce that cleanup happens before PDF generation - they only verify both were called. Consider usingMockSequenceorCallbackwith counters to verify the actual order if ordering is a critical requirement.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/Pdf/IPdfService.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
🧠 Learnings (14)
📓 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-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/Pdf/IPdfService.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.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/Pdf/IPdfService.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Internal/Pdf/PdfService.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:
src/Altinn.App.Core/Internal/Pdf/PdfService.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:
src/Altinn.App.Core/Internal/Pdf/PdfService.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.Core/Internal/Pdf/PdfService.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.Core/Internal/Pdf/PdfService.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Use sealed for classes unless inheritance is considered a valid use-case
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs
📚 Learning: 2025-08-27T06:58:28.126Z
Learnt from: cammiida
Repo: Altinn/app-lib-dotnet PR: 1451
File: src/Altinn.App.Api/Controllers/DataTagsController.cs:0-0
Timestamp: 2025-08-27T06:58:28.126Z
Learning: In the Altinn app-lib-dotnet codebase, validation endpoints follow a pattern where validation only runs when the ignoredValidators query parameter is provided (not null). This is a conscious design choice for consistency across validation endpoints.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.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
🧬 Code graph analysis (3)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (2)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (2)
Task(20-73)Task(90-105)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
Task(34-56)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (2)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (5)
Task(17-17)Task(29-35)Task(47-53)Task(61-61)Task(74-74)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (2)
Task(20-73)Task(90-105)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (4)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (5)
Task(17-17)Task(29-35)Task(47-53)Task(61-61)Task(74-74)src/Altinn.App.Core/Internal/Pdf/PdfService.cs (11)
Task(67-72)Task(75-93)Task(96-112)Task(115-126)Task(129-132)Task(134-176)Task(178-215)Task(285-343)Task(345-353)Task(355-394)List(396-411)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnSubformPdfConfiguration.cs (2)
ValidAltinnSubformPdfConfiguration(30-45)AltinnSubformPdfConfiguration(10-54)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-82)
⏰ 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: Analyze (csharp)
- 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
🔇 Additional comments (16)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (2)
28-35: LGTM - Return type change and new parameters well documented.The signature change from
TasktoTask<DataElement>enables callers to access the created PDF data element. The default implementation throwingNotImplementedExceptionensures existing implementations continue to work via the simpler overload on line 17.
47-53: Internal interface method correctly scoped.The
internalaccess modifier on this default interface method appropriately restricts visibility to within the assembly, preventing external consumers from calling or needing to implement it. This aligns with the previous review discussion to keep this method internal.test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/SubformPdfServiceTaskTests.cs (2)
75-101: Good test coverage for core functionality.The test properly verifies that
GenerateAndStoreSubformPdfis called for each matching data element with the correct parameters. The verification of component ID and call count is thorough.
173-211: Good coverage of cleanup and error propagation scenarios.Tests properly verify that:
- Cleanup is called with correct task ID
- Cleanup exceptions propagate correctly
- Configuration validation throws appropriate exceptions
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/SubformPdfServiceTask.cs (4)
10-18: LGTM - Class properly marked asinternal sealed.The class follows coding guidelines with
internal sealedmodifier and uses primary constructor for dependency injection. TheTypeproperty correctly returns"subformPdf"matching the renamed identifier from past review discussions.
38-68: Sequential PDF generation is acceptable here.While coding guidelines advise against awaiting async operations in a loop, the sequential approach was intentionally chosen (per past review discussion) to avoid overwhelming the PDF microservice. The code properly propagates cancellation tokens and logs progress for each data element.
75-88: Configuration validation is thorough.The method correctly validates that
SubformPdfConfigurationexists and delegates toValidate()which checks required fields (SubformComponentId,SubformDataTypeId). The error message clearly indicates the missing configuration node.
90-105: Metadata update happens after each PDF is stored.The metadata linking the PDF to its source subform data element is correctly added. However, consider whether the metadata update failure should be handled differently than PDF generation failure, since the PDF was already stored successfully at this point.
If metadata update fails after a PDF is stored, the cleanup at the start of the next retry attempt will remove the orphaned PDF. Verify this is the intended behavior for partial failures.
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (7)
50-63: Service locator pattern used for optional dependency.Using
IServiceProviderto optionally resolveInstanceDataUnitOfWorkInitializeris acceptable for features that may not always be available. The null-conditional ensures graceful fallback when the service isn't registered.
95-112: LGTM - Subform PDF generation method correctly delegates to internal implementation.The method follows the existing pattern, passing through the
SubformPdfContextto the internal implementation. No telemetry activity is started here - consider whether subform PDF generation should have its own telemetry span.Based on learnings, additional OTel spans for service tasks were deferred. Confirm this is intentional for subform PDF generation as well.
229-244: URL construction for subform paths looks correct.The logic properly inserts the subform path segments (
/{taskId}/subform/{componentId}/{dataElementId}/) before the?pdf=1query parameter when present, or appends them to the end otherwise. This maintains URL structure integrity.
294-321: Variable substitution for custom filenames is well-implemented.The code properly initializes the unit of work, creates a component context with the subform data element identifier, and uses the translation service for variable substitution. The fallback to simple translation when
_instanceDataUnitOfWorkInitializeris null ensures backward compatibility.
339-342: Filename escaping and suffix logic is correct.The code properly escapes the filename using
Uri.EscapeDataStringand ensures the
414-419: LGTM -SubformPdfContextrecord is well-designed.The
public sealed recordwith clear XML documentation appropriately exposes the subform context for PDF generation. Using a record provides value semantics and immutability.
297-301: No action needed. TheInitmethod is explicitly designed to accept a nullabletaskIdparameter (signature:string? taskId), so passinginstance.Process?.CurrentTask?.ElementIdis correct and safe. The contrast withSubformPdfServiceTask.Executeis not relevant—that's a different context whereProcess.CurrentTaskis guaranteed to exist, whereasPdfServiceappropriately uses null-coalescing to handle cases whereProcessorCurrentTaskmay be null.Likely an incorrect or invalid review comment.
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
3265-3299: No review on .verified Public API snapshotThis file is an auto-generated PublicApi snapshot (
*.verified.txt). Per project conventions, it should reflect the C# public surface but is not manually maintained. I’m intentionally not reviewing or suggesting changes to the contents here; any updates should come from regenerating the snapshot after adjusting the real source code.Also applies to: 3438-3447, 3462-3463
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Altinn.App.Api/Controllers/SigningController.cs (1)
91-101: Consider extracting repeated task verification and initialization pattern.The pattern of resolving
finalTaskId, verifying it's a signing task, and initializingIInstanceDataAccessoris repeated in bothGetSigneesStateandGetAuthorizedOrganizations. Consider extracting this into a private helper method to reduce duplication.🔎 Example refactoring approach
+private async Task<(string taskId, IInstanceDataAccessor dataAccessor)> ValidateAndInitializeSigningContext( + Instance instance, + string? requestedTaskId, + string? language) +{ + string? finalTaskId = requestedTaskId ?? instance.Process?.CurrentTask?.ElementId; + if (string.IsNullOrEmpty(finalTaskId) || !VerifyIsSigningTask(finalTaskId)) + { + throw new InvalidOperationException("Not a signing task"); + } + + IInstanceDataAccessor dataAccessor = await _instanceDataUnitOfWorkInitializer.Init( + instance, + finalTaskId, + language + ); + + return (finalTaskId, dataAccessor); +}Then use it in the endpoint methods with appropriate error handling.
Also applies to: 191-201
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Api/Controllers/SigningController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/SigningController.cs
🧠 Learnings (10)
📓 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-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.Api/Controllers/SigningController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Api/Controllers/SigningController.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:
src/Altinn.App.Api/Controllers/SigningController.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:
src/Altinn.App.Api/Controllers/SigningController.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:
src/Altinn.App.Api/Controllers/SigningController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 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/SigningController.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/SigningController.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.Api/Controllers/SigningController.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
🧬 Code graph analysis (1)
src/Altinn.App.Api/Controllers/SigningController.cs (3)
src/Altinn.App.Core/Internal/Process/ProcessTasks/SigningProcessTask.cs (1)
AltinnSignatureConfiguration(140-154)src/Altinn.App.Core/Internal/Process/Elements/ProcessTask.cs (1)
ProcessTask(9-25)src/Altinn.App.Core/Internal/Process/Elements/ExtensionElements.cs (1)
ExtensionElements(9-22)
⏰ 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: Static code analysis
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (macos-latest)
🔇 Additional comments (8)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (4)
3265-3270: Public API snapshot entry for newIPdfServiceoverloadThis file is a generated PublicApi snapshot. The added
GenerateAndStorePdfoverload returningDataElementis just being recorded here; no changes should be made directly in this file. As long as the implementation and tests reflect this signature and intended behavior, this snapshot update looks fine.Based on learnings, this file is not used for semantic review of the API.
3287-3299: Snapshot update forPdfServiceconstructor/methods andSubformPdfContextThese lines capture the new
PdfServiceconstructor parameter (serviceProvider), the overloads returningDataElement, and the newGenerateAndStoreSubformPdfplusSubformPdfContexttype. Since this is a PublicApi snapshot, no direct code changes are expected here; just ensure the underlying implementation, tests, and release notes for public API changes are aligned with this surface.Based on learnings, semantic review should be done on the actual source, not this snapshot.
3438-3447: Snapshot entry forAltinnSubformPdfConfigurationThe new
AltinnSubformPdfConfigurationtype and its XML-mapped properties are correctly reflected in the public API snapshot. No modifications should be made in this file; just verify the corresponding implementation and BPMN/XML handling behave as intended.Based on learnings, this snapshot is only guarding the public surface.
3462-3463: Snapshot entry forAltinnTaskExtension.SubformPdfConfigurationThis property addition records the new
<subformPdfConfig>extension on tasks. The snapshot change itself is expected and requires no edit; ensure the process parsing and service-task wiring forsubform-pdfare covered in source and tests.Based on learnings, review of behavior belongs in the implementation files, not here.
src/Altinn.App.Api/Controllers/SigningController.cs (4)
256-293: Verify that GetDataElements intentionally doesn't use IInstanceDataAccessor.Unlike
GetSigneesStateandGetAuthorizedOrganizations, this method doesn't initialize anIInstanceDataAccessor. It only filtersinstance.Databased on the signing configuration. Confirm this is intentional and that the method doesn't need task-specific data access.
295-301: LGTM!The helper method correctly validates whether a task is a signing task by checking the
TaskTypeproperty. The null-conditional operators ensure safe navigation when the task or its extension elements are not found.
309-310: LGTM!The updated error message clearly communicates that the endpoint can be called either when the current task is a signing task OR when the
taskIdquery parameter specifies a signing task. This improves clarity for API consumers.
110-110: PassfinalTaskIdto GetSigneeContexts instead oftaskId.Line 91 resolves
finalTaskIdfromtaskId ?? instance.Process?.CurrentTask?.ElementId, and line 92 validates it's non-empty and a signing task. UsefinalTaskIdin the call to GetSigneeContexts (line 110) to maintain consistency with how it's used elsewhere in the method and to ensure the service receives the resolved, validated task identifier rather than the potentially null user input.
|
3 participants
Is it OK to add this to the PdfService, or should I rather create a separate service for only SubformPdf?
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.