Feat/service task #745
Feat/service task #745
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Process/ServiceTasks/EFormidling/EFormidlingServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Api.Tests/Process/ServiceTasks/EFormidling/EFormidlingServiceTaskTests.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Process/ServiceTasks/EFormidling/EFormidlingServiceTaskTests.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Process/ServiceTasks/Pdf/PdfServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Api.Tests/Process/ServiceTasks/Pdf/PdfServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
|
There was a problem hiding this comment.
API and stuff LGTM. We discussed
- Validation error for trying to invoke actions when current task is a service task
- Clear/simple migration story (both old and new apps work) - can we just not run the legacy tasks if a PDF service task refers to the current task for example
- Service tasks failing should probably not have 500 status code for the user/client?
- Talk to frontenders and Studio team on how the task should be configured
bjorntore
force-pushed
the
feat/pdf-service-task
branch
from
7584f9a to
6aacedf
Compare
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Outdated
Show resolved
Hide resolved
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
Outdated
Show resolved
Hide resolved
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
Outdated
Show resolved
Hide resolved
|
bjorntore
added
the
feature
|
1 similar comment
|
|
bjorntore
added
the
backport-ignore
bjorntore
force-pushed
the
feat/pdf-service-task
branch
from
1d3fd8d to
9ec36c7
Compare
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Process/ServiceTasks/EFormidling/EFormidlingServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Api.Tests/Process/ServiceTasks/Pdf/PdfServiceTaskTests.cs
Dismissed
Show dismissed
Hide dismissed
...Altinn.App.Core.Tests/Internal/Process/EventHandlers/ProcessTask/EndTaskEventHandlerTests.cs
Dismissed
Show dismissed
Hide dismissed
...Altinn.App.Core.Tests/Internal/Process/EventHandlers/ProcessTask/EndTaskEventHandlerTests.cs
Dismissed
Show dismissed
Hide dismissed
bjorntore
force-pushed
the
feat/pdf-service-task
branch
5 times, most recently
from
6ef10b3 to
9b6b142
Compare
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Fixed
Show fixed
Hide fixed
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
Fixed
Show fixed
Hide fixed
bjorntore
force-pushed
the
feat/pdf-service-task
branch
from
bb80fbe to
70a69eb
Compare
|
/publish |
PR release:
|
|
![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
🧹 Nitpick comments (1)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (1)
27-27: Clarify the default value for AutoPdfTaskIds.The property is nullable (
List<string>?) but defaults to an empty list (= []). This means instances will never have a null value unless explicitly set to null, which contradicts the nullable type.Consider:
- If empty and absent should be treated the same, make it non-nullable:
public List<string> AutoPdfTaskIds { get; set; } = [];- If null has semantic meaning (e.g., "feature not configured" vs "configured with no tasks"), remove the default:
public List<string>? AutoPdfTaskIds { get; set; }Looking at the usage in
CreateAutoPdfTaskIdsQueryParams, the null check suggests null means "not configured", so removing the default initializer would be more appropriate.- public List<string>? AutoPdfTaskIds { get; set; } = []; + public List<string>? AutoPdfTaskIds { get; set; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs(1 hunks)src/Altinn.App.Core/Internal/Pdf/PdfService.cs(9 hunks)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Pdf/IPdfService.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Pdf/IPdfService.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Pdf/IPdfService.cs
🧠 Learnings (2)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
🧬 Code graph analysis (6)
test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs (2)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (9)
Task(71-76)Task(79-98)Task(101-114)Task(117-120)Task(122-158)Task(160-196)Task(246-260)List(403-418)Uri(198-226)src/Altinn.App.Core/Infrastructure/Clients/Pdf/PdfGeneratorClient.cs (2)
Task(69-72)Task(75-158)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (2)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
List(403-418)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
ValidAltinnPdfConfiguration(55-67)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs (7)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (3)
PdfServiceTask(13-68)PdfServiceTask(22-27)Task(33-53)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-76)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (14)
AltinnTaskExtension(209-221)List(41-45)List(48-52)List(62-66)List(69-73)List(83-87)List(90-94)List(97-101)List(104-108)List(118-122)List(125-129)List(165-182)List(185-194)List(197-206)src/Altinn.App.Core/Internal/Process/Interfaces/IProcessReader.cs (3)
AltinnTaskExtension(123-123)List(16-16)List(22-22)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (1)
AltinnPdfConfiguration(8-36)src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (4)
Task(17-17)Task(29-36)Task(44-44)Task(57-57)src/Altinn.App.Core/Internal/Pdf/PdfService.cs (8)
Task(71-76)Task(79-98)Task(101-114)Task(117-120)Task(122-158)Task(160-196)Task(246-260)List(403-418)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (5)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (2)
Task(17-17)Task(29-36)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/IServiceTask.cs (1)
Task(14-14)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (2)
ValidAltinnPdfConfiguration(29-35)AltinnPdfConfiguration(8-36)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-76)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (1)
AltinnTaskExtension(209-221)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (2)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (4)
Task(17-17)Task(29-36)Task(44-44)Task(57-57)src/Altinn.App.Core/Internal/Process/ProcessTasks/PaymentProcessTask.cs (3)
Task(54-65)Task(68-92)Task(95-99)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (1)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
List(403-418)
⏰ 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 (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
🔇 Additional comments (16)
test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs (1)
302-353: LGTM!The test correctly verifies that
autoGeneratePdfForTaskIdsare properly included as query parameters in the PDF generator URI. The test structure follows xUnit best practices and uses appropriate mocking with Moq.src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (2)
15-15: LGTM!Documentation typo fix from "witch" to "which" improves clarity.
19-36: LGTM!The new overload correctly extends the interface to support richer PDF generation configuration while maintaining backward compatibility through the default implementation that forwards to the simpler overload.
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (2)
13-53: LGTM!The service task implementation follows best practices:
- Properly sealed and internal as per guidelines
- Dependencies injected via constructor
- Clean separation of concerns with helper method
- Appropriate error handling through the service task result pattern
The CodeQL alerts about logging (lines 38, 50) are noted in past reviews; the taskId originates from the process definition rather than direct user input, making debug-level logging acceptable in this context.
55-67: LGTM!The helper method correctly retrieves and validates PDF configuration, providing sensible defaults when configuration is absent.
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (9)
70-76: LGTM!The existing method correctly delegates to the internal implementation with null defaults for the new parameters, maintaining backward compatibility.
78-98: LGTM!The new overload properly implements the extended interface, starting telemetry and forwarding all parameters to the internal implementation.
101-114: LGTM!The GeneratePdf method correctly passes null for autoGeneratePdfForTaskIds since this feature is specific to the GenerateAndStorePdf flow.
160-196: LGTM!The PDF content generation correctly incorporates autoGeneratePdfForTaskIds into the URI through query parameters.
198-226: LGTM!The URI building logic correctly appends additional query parameters while preserving the existing hash fragment handling.
262-296: LGTM!The filename resolution logic correctly:
- Supports text resource lookup for i18n
- Falls back to literal value if resource not found
- Maintains backward compatibility with existing app-based naming
- Ensures .pdf extension through GetValidFileName
336-350: LGTM!The refactoring to extract
AddPdfFileTypeIfMissingimproves code clarity and ensures consistent .pdf extension handling.
403-418: LGTM!The helper correctly creates repeated query parameters for task IDs, following standard URL array parameter conventions.
122-158: No further action required for dataTypeId normalization.All existing callers either pass null or a trimmed/validated dataTypeId (from ValidAltinnPdfConfiguration.Validate()), which converts whitespace-only inputs to null. No additional defensive handling in GenerateAndStorePdfInternal is necessary.
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs (2)
12-66: LGTM!The test class follows best practices:
- Mocks initialized as fields for reuse
- Clean setup in constructor
- Proper arrange-act-assert pattern
- Appropriate use of Moq's
It.IsandIt.IsAnyfor verificationThe first test correctly verifies the basic PDF generation flow with filename resolution.
68-104: LGTM!The test correctly verifies that
AutoPdfTaskIdsare properly passed through to the PDF service. Creating a new service task instance (line 94) is appropriate here since it requires specific mock configuration.
|
/publish |
PR release:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (1)
19-21: Consider consistent null handling.The property is marked nullable (
List<string>?) withIsNullable = true, but defaults to a non-null empty list[]. This creates ambiguity about whethernullis a valid state. Consider either:
- Removing the default initializer to allow
nullas the explicit "not configured" state, or- Making the property non-nullable if an empty list is always valid
The current code works, but explicit semantics would improve clarity.
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
259-293: Consider logging when text resource key is not found.Lines 265-275: When
fileNameTextResourceElementIdis provided but the corresponding text resource is not found, the code falls back to using the key itself as the filename (line 274). This might produce unexpected filenames like"pdf.filename.key.pdf".Consider adding a warning log before line 274 to help identify misconfigurations:
if (textResourceElement is not null) return GetValidFileName(textResourceElement.Value); // Log warning about missing text resource _logger.LogWarning("Text resource key '{Key}' not found, using key as filename.", fileNameTextResourceElementId); return GetValidFileName(fileNameTextResourceElementId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs(1 hunks)src/Altinn.App.Core/Internal/Pdf/PdfService.cs(9 hunks)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Internal/Pdf/IPdfService.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Internal/Pdf/IPdfService.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Pdf/IPdfService.cssrc/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cssrc/Altinn.App.Core/Internal/Pdf/PdfService.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.cs: Test projects mirror source structure
Prefer xUnit asserts over FluentAssertions
Mock external dependencies with Moq
Files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
🧠 Learnings (4)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
PR: Altinn/app-lib-dotnet#1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
PR: Altinn/app-lib-dotnet#1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#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.txt
🧬 Code graph analysis (5)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (1)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
List(400-415)
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs (6)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (3)
PdfServiceTask(13-67)PdfServiceTask(22-27)Task(33-52)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-76)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (14)
AltinnTaskExtension(209-221)List(41-45)List(48-52)List(62-66)List(69-73)List(83-87)List(90-94)List(97-101)List(104-108)List(118-122)List(125-129)List(165-182)List(185-194)List(197-206)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (1)
AltinnPdfConfiguration(8-29)src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (4)
Task(17-17)Task(28-34)Task(42-42)Task(55-55)src/Altinn.App.Core/Internal/Pdf/PdfService.cs (8)
Task(71-76)Task(79-96)Task(99-112)Task(115-118)Task(120-155)Task(157-193)Task(243-257)List(400-415)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (4)
src/Altinn.App.Core/Internal/Process/Elements/Process.cs (1)
Process(8-62)src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
List(400-415)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (13)
List(41-45)List(48-52)List(62-66)List(69-73)List(83-87)List(90-94)List(97-101)List(104-108)List(118-122)List(125-129)List(165-182)List(185-194)List(197-206)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
ValidAltinnPdfConfiguration(54-66)
src/Altinn.App.Core/Internal/Pdf/PdfService.cs (1)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (4)
Task(17-17)Task(28-34)Task(42-42)Task(55-55)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (4)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (3)
Task(17-17)Task(28-34)Task(42-42)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (2)
ValidAltinnPdfConfiguration(23-28)AltinnPdfConfiguration(8-29)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-76)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (1)
AltinnTaskExtension(209-221)
⏰ 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 (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (18)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (2)
15-15: Documentation typo fixes look good.The corrections from "witch" to "which" improve the documentation quality.
Also applies to: 40-40
19-34: No action required:PdfServiceexplicitly overrides the new overload and forwards all parameters, so the default implementation is never used and there’s no silent parameter ignore.src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (3)
8-14: LGTM!The class is correctly marked as
sealed, follows the coding guidelines, and theFilenameproperty is properly configured with XML serialization attributes to support text resource keys.
23-28: LGTM!The
Validate()method correctly normalizesFilenameby trimming and converting whitespace-only values tonull, which prevents empty strings from being passed downstream. TheAutoPdfTaskIdslist is passed through as-is, which is appropriate for a collection property.
31-31: LGTM!The internal
readonly record structis an appropriate and efficient choice for encapsulating validated configuration data.src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (3)
8-17: LGTM!The class is correctly marked as
internal sealedper coding guidelines, and the constructor dependencies are appropriate for the PDF generation task.
33-52: Confirm taskId is not user-controlled input.Static analysis flagged Lines 38 and 49 for logging user-provided values. The
taskIdcomes frominstance.Process.CurrentTask.ElementId, which should be a BPMN process model identifier controlled by the app developer rather than end-user input.Please confirm this is the case to dismiss the security concern.
54-66: LGTM!The helper method correctly handles both configured and unconfigured scenarios, providing a sensible default when no PDF configuration is specified in the BPMN model. The null-safe navigation and validation flow are appropriate.
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs (3)
12-34: LGTM!The test setup follows good practices with inline mock initialization and appropriate default configuration for the ProcessReader mock. The structure supports test reusability.
36-65: LGTM!The test correctly verifies that
ExecutecallsGenerateAndStorePdfwith the expected parameters. The use ofIt.IsAny<>()forAutoPdfTaskIdsandCancellationTokenis appropriate given this test focuses on the core invocation.
67-103: LGTM!The test correctly verifies that
AutoPdfTaskIdsare propagated through toGenerateAndStorePdf. Creating a separatePdfServiceTaskinstance for this test is appropriate to avoid interference with the default setup.src/Altinn.App.Core/Internal/Pdf/PdfService.cs (7)
70-76: LGTM!The original method now correctly delegates to
GenerateAndStorePdfInternalwithnullfor the new parameters, maintaining backward compatibility.
78-96: LGTM!The new overload correctly extends the public API to support filename and auto-PDF task configuration, with appropriate default parameter values.
120-155: LGTM!The internal method correctly consolidates the PDF generation and storage logic, properly disposes the stream with
await using, and passes through the extended configuration parameters appropriately.
99-111: LGTM!The changes correctly integrate
autoGeneratePdfForTaskIdsinto the PDF content generation flow, converting task IDs to query parameters and appending them to the URI.Also applies to: 157-193
195-223: LGTM!The
BuildUrimethod correctly handles additional query parameters, and the string concatenation approach was acknowledged as acceptable by the maintainer for this use case.
339-347: LGTM!The helper correctly ensures the
400-415: LGTM!The helper correctly converts task IDs into query parameters with repeated
taskkeys, which is the standard approach for array-like query parameters in URLs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
1-7: Missing using directive prevents compilation.The
IProcessReadertype (line 17) requires a namespace import. This issue was flagged in a previous review.Apply this diff:
using Altinn.App.Core.Helpers; using Altinn.App.Core.Internal.Pdf; +using Altinn.App.Core.Internal.Process; using Altinn.App.Core.Internal.Process.Elements.AltinnExtensionProperties; using Altinn.Platform.Storage.Interface.Models; using Microsoft.Extensions.Logging;
🧹 Nitpick comments (1)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
427-432: Clarify error handling for service task implementation bugs.If a service task fails to handle abandon issues, the thrown exception will be caught by the generic catch block (line 440), which logs "Service task failed with an exception!" This masks the specific implementation error. Consider either:
- Rethrowing this exception type so it's not caught by the generic handler, or
- Logging a more specific message before throwing
This would help distinguish between service task execution failures and implementation bugs.
Apply this diff to log a specific message before throwing:
if (cachedDataMutator.HasAbandonIssues) { + _logger.LogError( + "Service task {ServiceTaskType} implementation error: Abandon issues found in data elements but not handled by the service task.", + serviceTask.Type + ); throw new Exception( "Abandon issues found in data elements. Abandon issues should be handled by the service task." ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs(11 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs
🧠 Learnings (3)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#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-25T11:15:19.192Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: src/Altinn.App.Core/Internal/Process/ProcessEngine.cs:276-276
Timestamp: 2025-09-25T11:15:19.192Z
Learning: In ProcessEngine.Next(), HandleMoveToNext intentionally uses request.Action (original, potentially null) rather than checkedAction (resolved with defaults). This distinction is important for navigation logic, even though it may appear inconsistent with how authorization and service task execution use the resolved action.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#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/ProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
🧬 Code graph analysis (3)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (7)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs (2)
Task(44-77)Task(79-95)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (1)
Task(34-56)src/Altinn.App.Api/Controllers/ProcessController.cs (3)
Task(479-519)Task(623-653)Task(655-658)src/Altinn.App.Core/Helpers/LogSanitizer.cs (2)
LogSanitizer(9-45)Sanitize(22-38)src/Altinn.App.Core/Internal/Process/ProcessReaderExtensions.cs (1)
IsActionAllowedForTask(13-23)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (3)
InstanceDataUnitOfWork(24-701)InstanceDataUnitOfWork(65-98)DataElementChanges(351-428)src/Altinn.App.Core/Models/Validation/ValidationIssueWithSource.cs (2)
ValidationIssueWithSource(10-118)ValidationIssueWithSource(15-32)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs (6)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (12)
IServiceTask(313-324)Task(74-141)Task(144-292)Task(295-310)Task(326-378)Task(380-453)Task(458-487)Task(492-514)Task(516-588)Task(590-643)Task(645-658)Task(663-676)src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs (3)
Task(71-74)Task(77-80)Task(82-138)src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/IServiceTask.cs (1)
Task(14-14)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnEFormidlingConfiguration.cs (2)
ValidAltinnEFormidlingConfiguration(66-96)AltinnEFormidlingConfiguration(10-199)src/Altinn.App.Core/Helpers/LogSanitizer.cs (2)
LogSanitizer(9-45)Sanitize(22-38)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (1)
AltinnTaskExtension(209-221)
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (5)
src/Altinn.App.Core/Internal/Pdf/IPdfService.cs (3)
Task(17-17)Task(28-34)Task(42-42)src/Altinn.App.Core/Helpers/LogSanitizer.cs (2)
LogSanitizer(9-45)Sanitize(22-38)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPdfConfiguration.cs (2)
ValidAltinnPdfConfiguration(23-28)AltinnPdfConfiguration(8-29)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnTaskExtension.cs (1)
AltinnTaskExtension(9-76)src/Altinn.App.Core/Internal/Process/ProcessReader.cs (1)
AltinnTaskExtension(209-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (5)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (2)
703-760: LGTM: Well-structured validation helper.The
TryGetCurrentTaskIdAndAltinnTaskTypemethod follows C# conventions properly, uses nullability attributes correctly, and provides clear error messages for each validation case. The extraction of this logic from the mainNextmethod improves readability and maintainability.
144-292: LGTM: Well-structured refactor of the Next method.The refactored
Nextmethod follows a clear flow: authorization → service task or user action execution → conditional validation → process advancement. Key improvements:
- Authorization and validation are properly integrated
- Service tasks and user actions are routed correctly
- Error handling provides specific error types and messages
- Telemetry tracks each phase
- Validation is appropriately skipped for reject actions and service tasks
The logic is well-organized and maintains backward compatibility while adding new capabilities.
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/PdfServiceTask.cs (3)
39-39: Static analysis warnings are false positives.The static analysis tool flagged logging
taskIdas a security risk (log injection). However, the code correctly appliesLogSanitizer.Sanitize()before logging, which removes control characters and limits length, mitigating injection risks.Also applies to: 50-53
36-36: Remove null check suggestion for CurrentTask.ElementIdService task contexts always supply a non-null
CurrentTaskand the codebase consistently accessesElementIddirectly, so a null check here is unnecessary.Likely an incorrect or invalid review comment.
58-70: Exception handling for missing task extensions is consistent. All service tasks propagate theProcessExceptionfromGetAltinnTaskExtensionand default null configurations.
src/Altinn.App.Core/Internal/Process/ProcessTasks/ServiceTasks/EFormidlingServiceTask.cs
Show resolved
Hide resolved
|
/publish |
PR release:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Altinn.App.Api/Controllers/ProcessController.cs (1)
47-68: Avoid service locator; inject InstanceDataUnitOfWorkInitializer directlyThe constructor still uses
IServiceProviderto resolveInstanceDataUnitOfWorkInitializeron line 67. This anti-pattern hides dependencies and complicates testing.As per the previous review, make
InstanceDataUnitOfWorkInitializeraccessible for direct injection by either:
- Making the class
publicinsrc/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWorkInitializer.cs, OR- Adding
[assembly: InternalsVisibleTo("Altinn.App.Api")]in CoreThen update the constructor:
public ProcessController( ILogger<ProcessController> logger, IInstanceClient instanceClient, IProcessClient processClient, IValidationService validationService, IAuthorizationService authorization, IProcessReader processReader, IProcessEngine processEngine, - IServiceProvider serviceProvider, + InstanceDataUnitOfWorkInitializer instanceDataUnitOfWorkInitializer, IProcessEngineAuthorizer processEngineAuthorizer ) { _logger = logger; _instanceClient = instanceClient; _processClient = processClient; _authorization = authorization; _processReader = processReader; _processEngine = processEngine; _processEngineAuthorizer = processEngineAuthorizer; _validationService = validationService; - _instanceDataUnitOfWorkInitializer = serviceProvider.GetRequiredService<InstanceDataUnitOfWorkInitializer>(); + _instanceDataUnitOfWorkInitializer = instanceDataUnitOfWorkInitializer; }
🧹 Nitpick comments (1)
src/Altinn.App.Api/Controllers/ProcessController.cs (1)
508-556: Consider exhaustive pattern matching for ProcessErrorTypeThe centralized error handling is well-designed. For additional safety, consider using an exhaustive switch expression or adding a guard clause to ensure all
ProcessErrorTypeenum values are explicitly handled:private ActionResult<AppProcessState> GetResultForError(ProcessChangeResult result) { - switch (result.ErrorType) + return result.ErrorType switch { - case ProcessErrorType.Conflict: - return Conflict( + ProcessErrorType.Conflict => Conflict( new ProblemDetails() { Detail = result.ErrorMessage, Status = StatusCodes.Status409Conflict, Title = result.ErrorTitle, Extensions = new Dictionary<string, object?> { { "validationIssues", result.ValidationIssues }, }, } ), - case ProcessErrorType.Internal: - return StatusCode( + ProcessErrorType.Internal => StatusCode( 500, new ProblemDetails() { Detail = result.ErrorMessage, Status = StatusCodes.Status500InternalServerError, Title = result.ErrorTitle ?? "Internal server error", } ), - case ProcessErrorType.Unauthorized: - return StatusCode( + ProcessErrorType.Unauthorized => StatusCode( 403, new ProblemDetails() { Detail = result.ErrorMessage, Status = StatusCodes.Status403Forbidden, Title = result.ErrorTitle ?? "Unauthorized", } ), - default: - return StatusCode( + _ => StatusCode( 500, new ProblemDetails() { Detail = $"Unknown ProcessErrorType {result.ErrorType}", Status = StatusCodes.Status500InternalServerError, Title = result.ErrorTitle ?? "Internal server error", } - ); - } + ), + }; }This makes the compiler warn if new enum values are added without updating the switch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Api/Controllers/ProcessController.cs(9 hunks)src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs(2 hunks)test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_FailingValidator_ReturnsValidationErrors.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_PdfFails_DataIsUnlocked.verified.txt(0 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(18 hunks)
💤 Files with no reviewable changes (1)
- test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_PdfFails_DataIsUnlocked.verified.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
- test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_FailingValidator_ReturnsValidationErrors.verified.txt
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)
Files:
src/Altinn.App.Api/Controllers/ProcessController.cs
**/*.{cs,csproj}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Nullable Reference Types
Files:
src/Altinn.App.Api/Controllers/ProcessController.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming
Files:
src/Altinn.App.Api/Controllers/ProcessController.cs
🧠 Learnings (3)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#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-25T11:15:19.192Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#745
File: src/Altinn.App.Core/Internal/Process/ProcessEngine.cs:276-276
Timestamp: 2025-09-25T11:15:19.192Z
Learning: In ProcessEngine.Next(), HandleMoveToNext intentionally uses request.Action (original, potentially null) rather than checkedAction (resolved with defaults). This distinction is important for navigation logic, even though it may appear inconsistent with how authorization and service task execution use the resolved action.
Applied to files:
src/Altinn.App.Api/Controllers/ProcessController.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#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.txt
🧬 Code graph analysis (1)
src/Altinn.App.Api/Controllers/ProcessController.cs (5)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWorkInitializer.cs (2)
InstanceDataUnitOfWorkInitializer(15-71)InstanceDataUnitOfWorkInitializer(29-48)src/Altinn.App.Core/Models/Process/ProcessChangeResult.cs (1)
ProcessChangeResult(9-42)src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (13)
ConvertTaskTypeToAction(755-772)Task(80-147)Task(150-213)Task(218-349)Task(366-381)Task(397-449)Task(451-524)Task(529-558)Task(563-585)Task(587-659)Task(661-714)Task(716-729)Task(734-747)src/Altinn.App.Core/Internal/Process/Elements/ProcessTask.cs (2)
ElementType(21-24)ProcessTask(9-25)src/Altinn.App.Core/Internal/Process/Elements/AppProcessTaskTypeInfo.cs (1)
AppProcessTaskTypeInfo(10-29)
⏰ 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). (4)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
🔇 Additional comments (5)
src/Altinn.App.Api/Controllers/ProcessController.cs (5)
256-303: Simplified flow correctly delegates iteration to ProcessEngineThe
NextElementmethod now constructs a singleProcessNextRequestand delegates to_processEngine.Next(), which handles iterative service-task execution internally. This resolves the stale-state loop concern from previous reviews where the controller was checkingIsServiceTaskon the pre-Next instance.The structured logging improvement on line 296 is also a good practice.
612-642: Validation helper correctly supports CompleteProcess flowThe
GetValidationProblemDetailsmethod provides necessary pre-validation for theCompleteProcessloop (lines 366-409). While similar logic exists inProcessEngine.ProcessNext, this duplication is intentional:CompleteProcessmust validate before each iteration to determine whether to continue the loop, whereasProcessNextvalidates internally as part of the process change.The method correctly uses
InstanceDataUnitOfWorkInitializerandIValidationService, and formats errors consistently withGetResultForError.
391-398: LGTM: Clean ProcessNextRequest constructionThe use of target-typed
new()and proper action conversion viaConvertTaskTypeToActionis clean and idiomatic.
486-501: ElementType wiring supports new BPMN surfaceThe additions of
ElementTypetoCurrentTask(line 486) andProcessTasks(line 497) correctly populate the new API surface properties. This aligns with the PR's service-task feature and enables element-type-based behavior in the frontend.The use of
flowElement.ElementType()andprocessElement.ElementType()properly delegates to the process element model's type identification.
649-666: Action mapping correctly extends for service tasksAdding
"pdf"and"eFormidling"to the action conversion is consistent with the engine-side mapping (seeProcessEngine.ConvertTaskTypeToAction) and appropriately maps these service-task types to the"write"action.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (4)
3582-3587: IProcessEngine additions are breaking for implementers — use default interface bodies.This was already flagged earlier; ensure default bodies exist to preserve compat (especially
CheckIfServiceTaskand oldNextsignature forwarding).#!/bin/bash ast-grep --pattern $'interface IProcessEngine { $$$ }' --lang cs -n -C3
3628-3633: ProcessEngine ctor expanded — add an [Obsolete] overload that forwards.Previously raised; confirm a compat ctor exists to avoid binary/source breaks.
#!/bin/bash rg -nP --type=cs 'class\s+ProcessEngine\b' -C5
3381-3383: elementType casing consistency (lowerCamelCase).Already noted earlier; ensure overrides return lowerCamelCase to match JSON/client expectations.
Also applies to: 3405-3406
4705-4705: Plumb new fields to API responses and telemetry.Ensure ErrorTitle and ValidationIssues are wired through controllers and telemetry tags; previously raised.
Also applies to: 4721-4722
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(18 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bjorntore
PR: Altinn/app-lib-dotnet#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
PR: Altinn/app-lib-dotnet#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.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (2)
4737-4744: Verify whether ProcessNextRequest sealing is a new change in this PR.ProcessNextRequest is currently defined as
public sealed record. I cannot determine from the codebase alone whether sealing is new in this PR or was pre-existing. If the sealing is a new addition in this PR, the concern about breaking inheritance is valid—sealed records prevent consumers from deriving custom implementations. If it was already sealed, no action is needed.Check the PR diff to confirm: (1) was ProcessNextRequest previously unsealed, and (2) is this sealing intentional per versioning policy (breaking change acceptable for next major version)?
191-199: ****The interfaces
IEFormidlingReceiversandIEFormidlingServicealready have default implementations for the new overload methods. The secondGetEFormidlingReceiversoverload forwards to the first method, and the secondSendEFormidlingShipmentoverload forwards to the first method with an explicit backward-compatibility comment. BothDefaultEFormidlingReceiversandDefaultEFormidlingServicesuccessfully implement all overloads without compilation issues. No breaking change exists, and no modifications are needed.Likely an incorrect or invalid review comment.
| public DefaultEFormidlingService(Microsoft.Extensions.Logging.ILogger<Altinn.App.Core.EFormidling.Implementation.DefaultEFormidlingService> logger, Altinn.App.Core.Internal.Auth.IUserTokenProvider userTokenProvider, Altinn.App.Core.Internal.App.IAppMetadata appMetadata, Altinn.App.Core.Internal.Data.IDataClient dataClient, Altinn.App.Core.Internal.Events.IEventsClient eventClient, System.IServiceProvider sp, Altinn.App.Core.EFormidling.Implementation.IEFormidlingLegacyConfigurationProvider configurationProvider, Microsoft.Extensions.Options.IOptions<Altinn.App.Core.Configuration.AppSettings>? appSettings = null, Microsoft.Extensions.Options.IOptions<Altinn.App.Core.Configuration.PlatformSettings>? platformSettings = null, Altinn.Common.EFormidlingClient.IEFormidlingClient? eFormidlingClient = null, Altinn.Common.AccessTokenClient.Services.IAccessTokenGenerator? tokenGenerator = null) { } | ||
| public System.Threading.Tasks.Task SendEFormidlingShipment(Altinn.Platform.Storage.Interface.Models.Instance instance) { } | ||
| public System.Threading.Tasks.Task SendEFormidlingShipment(Altinn.Platform.Storage.Interface.Models.Instance instance, Altinn.App.Core.Internal.Process.Elements.AltinnExtensionProperties.ValidAltinnEFormidlingConfiguration configuration) { } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
DefaultEFormidlingService ctor expanded — keep a compat overload.
Manual consumers will break. Add an [Obsolete] overload with the previous parameter list that chains to the new ctor.
Example (source: Altinn.App.Core.EFormidling.Implementation.DefaultEFormidlingService):
public class DefaultEFormidlingService : Altinn.App.Core.EFormidling.Interface.IEFormidlingService
{
+ [System.Obsolete("Use the overload that takes IEFormidlingLegacyConfigurationProvider. Will be removed in V9.")]
+ public DefaultEFormidlingService(
+ Microsoft.Extensions.Logging.ILogger<Altinn.App.Core.EFormidling.Implementation.DefaultEFormidlingService> logger,
+ Altinn.App.Core.Internal.Auth.IUserTokenProvider userTokenProvider,
+ Altinn.App.Core.Internal.App.IAppMetadata appMetadata,
+ Altinn.App.Core.Internal.Data.IDataClient dataClient,
+ Altinn.App.Core.Internal.Events.IEventsClient eventClient,
+ System.IServiceProvider sp,
+ Microsoft.Extensions.Options.IOptions<Altinn.App.Core.Configuration.AppSettings>? appSettings = null,
+ Microsoft.Extensions.Options.IOptions<Altinn.App.Core.Configuration.PlatformSettings>? platformSettings = null,
+ Altinn.Common.EFormidlingClient.IEFormidlingClient? eFormidlingClient = null,
+ Altinn.Common.AccessTokenClient.Services.IAccessTokenGenerator? tokenGenerator = null)
+ : this(
+ logger, userTokenProvider, appMetadata, dataClient, eventClient, sp,
+ sp.GetService<Altinn.App.Core.EFormidling.Implementation.IEFormidlingLegacyConfigurationProvider>()
+ ?? throw new System.InvalidOperationException("IEFormidlingLegacyConfigurationProvider not registered"),
+ appSettings, platformSettings, eFormidlingClient, tokenGenerator) { }
}Note: add using Microsoft.Extensions.DependencyInjection; for GetService.
Scan for direct new DefaultEFormidlingService( usages:
🏁 Script executed:
#!/bin/bash
rg -nP 'new\s+DefaultEFormidlingService\s*\(' -C2 --type=csLength of output: 0
I need to gather more context to verify the concern. Let me explore alternative search strategies.
🏁 Script executed:
#!/bin/bash
# Search for DefaultEFormidlingService in C# files more broadly
rg -n 'DefaultEFormidlingService' --type=cs -A2 -B2 | head -100Length of output: 7483
🏁 Script executed:
#!/bin/bash
# Search for IEFormidlingService registration patterns (DI container)
rg -n 'AddScoped|AddTransient|AddSingleton' --type=cs | grep -i 'eform' -A2 -B2Length of output: 8990
🏁 Script executed:
#!/bin/bash
# Check the actual DefaultEFormidlingService implementation
fd -e cs -x grep -l 'class DefaultEFormidlingService' {} \;Length of output: 167
🏁 Script executed:
#!/bin/bash
# Extract the complete constructor definition from DefaultEFormidlingService.cs
cat src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs | head -80Length of output: 3388
🏁 Script executed:
#!/bin/bash
# Verify there's no existing [Obsolete] overload already present
rg -n '\[Obsolete\]' src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.csLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if constructor is actually in verified.txt file to confirm it's public API
cat test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt | grep -A3 'public DefaultEFormidlingService'Length of output: 1295
Add [Obsolete] backward-compatibility overload for DefaultEFormidlingService constructor.
The new required parameter IEFormidlingLegacyConfigurationProvider breaks public API consumers who manually instantiate this class. Add the suggested [Obsolete] overload (lines 43–54 in src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs) that chains to the new constructor, deriving the missing parameter from IServiceProvider.GetService(). Include using Microsoft.Extensions.DependencyInjection; at the top of the file.
🤖 Prompt for AI Agents
In src/Altinn.App.Core/EFormidling/Implementation/DefaultEFormidlingService.cs
around the constructor region corresponding to lines 43–54, add a
backward-compatible [Obsolete] constructor overload that accepts the previous
parameter list (without IEFormidlingLegacyConfigurationProvider) and chains to
the new constructor by resolving the missing
IEFormidlingLegacyConfigurationProvider from the provided IServiceProvider via
sp.GetService<Altinn.App.Core.EFormidling.Implementation.IEFormidlingLegacyConfigurationProvider>()
and pass that result into the new constructor; also add using
Microsoft.Extensions.DependencyInjection; at the top of the file so GetService<>
is available. Ensure the attribute marks the ctor as obsolete for consumers
while still delegating to the new implementation.
# Conflicts: # src/Altinn.App.Core/Internal/Pdf/PdfService.cs
|
3 participants
Implementing basic support for service task in bpmn.
Moving PDF and eFormidling into service tasks, but also keep legacy implementations and old way of configuring for backwards compatibility for now.
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Tests