Support service task being first element after StartEvent.#1667
Open
Support service task being first element after StartEvent.#1667
Conversation
Contributor
Author
|
/publish |
bjorntore
added
kind/feature-request
Contributor
📝 WalkthroughWalkthroughReplaces ad-hoc event handling with a new IProcessEngine.Start(...) workflow that accepts user, prefill, language and CancellationToken, returns a ProcessChangeResult with MutatedInstance, auto-runs initial service tasks, and updates controllers, engine, models, and tests to use Start for process initiation and storage updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
153-169: Missing telemetry instrumentation for the newStartmethod.Other entry points like
GenerateProcessStartEventsandNextuse_telemetry?.Start*Activity(...). TheStartmethod is a new public-facing operation on the process engine and should have its own activity span for observability. As per coding guidelines, comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Process/ProcessEngine.cs` around lines 153 - 169, The Start method lacks OpenTelemetry instrumentation; add a telemetry activity at the top of Start (similar to GenerateProcessStartEvents/Next) by calling the telemetry helper (e.g. _telemetry?.StartProcessStartActivity(...) or the existing _telemetry?.Start*Activity API) and wrap the method body in the activity's scope (dispose/stop the activity after the awaited operations). Ensure the activity encompasses the call to HandleEventsAndUpdateStorage and the subsequent IsServiceTask check and return, and pass relevant context (instance, user, language/prefill) to the telemetry call so traces correlate with the Start operation.test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
385-405: Consider extracting the repeatedStartmock setup into a helper method.The identical
IProcessEngine.Startmock setup (.Setup+.ReturnsAsynclambda) is duplicated verbatim across five test methods (lines 385–405, 517–538, 729–750, 940–961, 1158–1179). A small helper would reduce boilerplate and make future signature changes a single-point fix.Also,
System.Security.Claims.ClaimsPrincipalis fully qualified in every occurrence. Addingusing System.Security.Claims;at the top would clean this up.Example helper
// At the top of the file using System.Security.Claims; // Helper method in the test class private static void SetupProcessEngineStart(InstancesControllerFixture fixture) { fixture .Mock<IProcessEngine>() .Setup(p => p.Start( It.IsAny<Instance>(), It.IsAny<List<InstanceEvent>>(), It.IsAny<ClaimsPrincipal>(), It.IsAny<Dictionary<string, string>>(), It.IsAny<string>(), It.IsAny<CancellationToken>() ) ) .ReturnsAsync( ( Instance inst, List<InstanceEvent>? _, ClaimsPrincipal _, Dictionary<string, string>? _, string? _, CancellationToken _ ) => new ProcessChangeResult { Success = true, MutatedInstance = inst } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs` around lines 385 - 405, Extract the duplicated IProcessEngine.Start mock in InstancesController_CopyInstanceTests into a single helper method (e.g., SetupProcessEngineStart) that accepts the test fixture and performs the .Mock<IProcessEngine>().Setup(...Start(...)) and .ReturnsAsync(...) logic; replace the five verbatim setups with calls to this helper. Also add using System.Security.Claims; to the top of the test file and use ClaimsPrincipal in the Setup/ReturnsAsync signatures instead of the fully-qualified System.Security.Claims.ClaimsPrincipal to simplify the mock lambda parameters and make future signature changes a single-point edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 385-395: The use of the null-forgiving operator on
ProcessChangeResult.MutatedInstance is unsafe; update the Sites in
InstancesController (where you call _processEngine.Start and then use
startProcessResult.MutatedInstance) to explicitly guard against null instead of
using "!" — check startProcessResult.Success and also verify
startProcessResult.MutatedInstance is not null before assigning/using it,
returning an appropriate error (e.g., Conflict or InternalServerError) if it is
null; alternatively, if you prefer a type-level fix, add
[MemberNotNullWhen(true, nameof(MutatedInstance))] to the
ProcessChangeResult.Success property (and ensure all code paths set
MutatedInstance when Success is true, including the non-service-task Start path)
so the compiler can trust the invariant.
In `@src/Altinn.App.Api/Controllers/ProcessController.cs`:
- Around line 150-163: startProcessResult.MutatedInstance is being dereferenced
with a null-forgiving operator; instead mirror the fix from InstancesController
by validating MutatedInstance after a successful Start call and handling the
unexpected null safely. After calling _processEngine.Start(...) and confirming
startProcessResult.Success, check if startProcessResult.MutatedInstance is null
and return an appropriate error response (e.g., Conflict or BadRequest) rather
than using !; then assign instance = startProcessResult.MutatedInstance and
continue to call ConvertAndAuthorizeActions(instance, instance.Process).
- Around line 156-159: The code path in ProcessController.Start currently
returns Conflict(startProcessResult.ErrorMessage) for all failures, which forces
a 409 regardless of the actual ErrorType; change the failure return to use
GetResultForError(startProcessResult) so the controller maps Start/Next errors
(Internal, Unauthorized, etc.) to the correct HTTP status codes. Locate the
failing branch that checks if (!startProcessResult.Success) and replace the
Conflict(...) return with a call to GetResultForError(startProcessResult),
keeping the same startProcessResult object to preserve the error message and
type mapping.
In `@src/Altinn.App.Core/Internal/Process/ProcessEngine.cs`:
- Around line 153-183: The Start method returns new
ProcessChangeResult(instance) with Success = true but never sets
ProcessStateChange, violating the [MemberNotNullWhen(true,
nameof(ProcessStateChange))] contract; modify Start to capture the result/state
produced by HandleEventsAndUpdateStorage (or the state mutation it produces) and
assign it to the ProcessStateChange property on the returned ProcessChangeResult
when Success is true (e.g., store the change/result from
HandleEventsAndUpdateStorage and use it to populate ProcessStateChange before
returning), keeping existing behavior for IsServiceTask and the Next call
intact.
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs`:
- Around line 387-394: The mock setup for IProcessEngine.Start uses non-nullable
matchers but the real method signature has nullable parameters
(List<InstanceEvent>?, Dictionary<string,string>?, string?); update the mock
call in the test to use nullable-compatible matchers—replace
It.IsAny<List<InstanceEvent>>(), It.IsAny<Dictionary<string,string>>(),
It.IsAny<string>() with It.IsAny<List<InstanceEvent>?>(),
It.IsAny<Dictionary<string,string>?>(), It.IsAny<string?>() so the mocked
invocation matches the actual Start signature and VerifyAll() can correctly
validate the call.
---
Duplicate comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 610-620: The code uses the null-forgiving operator on
startProcessResult.MutatedInstance (after calling _processEngine.Start) which
can propagate a null-forgiving pipeline failure like in the Post method; update
the handling of startProcessResult to check that
startProcessResult.MutatedInstance is not null before assigning to instance (or
return an error/Conflict if it is null) instead of using the ! operator,
mirroring the fix applied to the Post method so _processEngine.Start,
startProcessResult, and instance are safely handled.
- Around line 742-751: The code currently assigns
startProcessResult.MutatedInstance! to targetInstance using a null-forgiving
operator which can cause a pipeline null-forgiving failure; instead, check that
startProcessResult.MutatedInstance is not null before assignment (e.g., if
(!startProcessResult.MutatedInstance.HasValue or == null) return Conflict or
appropriate error), and only set targetInstance =
startProcessResult.MutatedInstance when it is non-null—apply the same
null-check/early-return pattern used in the Post method around
_processEngine.Start/startProcessResult to avoid the null-forgiving operator.
---
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Process/ProcessEngine.cs`:
- Around line 153-169: The Start method lacks OpenTelemetry instrumentation; add
a telemetry activity at the top of Start (similar to
GenerateProcessStartEvents/Next) by calling the telemetry helper (e.g.
_telemetry?.StartProcessStartActivity(...) or the existing
_telemetry?.Start*Activity API) and wrap the method body in the activity's scope
(dispose/stop the activity after the awaited operations). Ensure the activity
encompasses the call to HandleEventsAndUpdateStorage and the subsequent
IsServiceTask check and return, and pass relevant context (instance, user,
language/prefill) to the telemetry call so traces correlate with the Start
operation.
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs`:
- Around line 385-405: Extract the duplicated IProcessEngine.Start mock in
InstancesController_CopyInstanceTests into a single helper method (e.g.,
SetupProcessEngineStart) that accepts the test fixture and performs the
.Mock<IProcessEngine>().Setup(...Start(...)) and .ReturnsAsync(...) logic;
replace the five verbatim setups with calls to this helper. Also add using
System.Security.Claims; to the top of the test file and use ClaimsPrincipal in
the Setup/ReturnsAsync signatures instead of the fully-qualified
System.Security.Claims.ClaimsPrincipal to simplify the mock lambda parameters
and make future signature changes a single-point edit.
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/Altinn.App.Api.Tests/Process/ServiceTasks/ServiceTaskFirstTests.cs`:
- Line 8: Replace FluentAssertions usage in the ServiceTaskFirstTests class with
xUnit asserts: remove the using FluentAssertions; and change expressions like
.Should().BeTrue() -> Assert.True(...), .Should().BeFalse() ->
Assert.False(...), .Should().Be(expected) -> Assert.Equal(expected, actual),
.Should().NotBeNull() -> Assert.NotNull(...), .Should().HaveCount(n) ->
Assert.Equal(n, collection.Count), and similar conversions inside each test
method; update any chained calls to capture the actual value passed as the first
xUnit parameter and ensure test methods in ServiceTaskFirstTests compile with
xUnit Assert methods.
test/Altinn.App.Api.Tests/Process/ServiceTasks/ServiceTaskFirstTests.cs
Outdated
Show resolved
Hide resolved
Contributor
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/Models/Process/ProcessChangeResult.cs (1)
10-10: 🛠️ Refactor suggestion | 🟠 MajorSeal
ProcessChangeResult.The class is a concrete result model with no intended inheritance. Per coding guidelines: "Use
sealedfor classes unless inheritance is considered a valid use-case."♻️ Proposed fix
-public class ProcessChangeResult +public sealed class ProcessChangeResultAs per coding guidelines: "Use sealed for classes unless inheritance is considered a valid use-case."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Models/Process/ProcessChangeResult.cs` at line 10, The ProcessChangeResult class is a concrete result model that should be sealed; update its declaration to add the sealed modifier on the class definition (ProcessChangeResult) so it cannot be inherited, leaving the rest of the class unchanged.
🧹 Nitpick comments (1)
src/Altinn.App.Core/Models/Process/ProcessChangeResult.cs (1)
58-61: Constructor does not enforceSuccess = true— consider setting it in-body.The constructor initialises
MutatedInstancebut leavesSuccess = false(thebooldefault). Callers must remember to append{ Success = true }via object initialiser, which is an easy-to-miss footgun. Becauseinitproperties can be assigned from within a constructor of the same type, settingSuccess = truedirectly in the body encodes the invariant explicitly:♻️ Proposed fix
internal ProcessChangeResult(Instance mutatedInstance) { + Success = true; MutatedInstance = mutatedInstance; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Models/Process/ProcessChangeResult.cs` around lines 58 - 61, The constructor ProcessChangeResult(Instance mutatedInstance) currently only sets MutatedInstance and leaves the Success property at its default false; modify the constructor body to also assign Success = true so the invariant is enforced at construction time (i.e., set Success within the ProcessChangeResult(Instance) constructor alongside MutatedInstance) to avoid requiring callers to use an object initializer to mark success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Core/Models/Process/ProcessChangeResult.cs`:
- Around line 15-18: The MemberNotNullWhen contract is currently invalid because
callers create ProcessChangeResult instances with Success=true while leaving
ProcessStateChange or MutatedInstance null; fix by making ProcessChangeResult
sealed, change the internal constructor ProcessChangeResult(Instance
mutatedInstance) to assign MutatedInstance and set Success = true (so callers
can simply return new ProcessChangeResult(instance)), and update any
factory/return sites that use "new ProcessChangeResult { Success = true }" to
instead call the appropriate constructor or provide both required non-null
properties; alternatively remove the [MemberNotNullWhen(true,
nameof(MutatedInstance))] attribute if you prefer to keep current
patterns—ensure the final contract guarantees that when Success == true both
ProcessStateChange and MutatedInstance are non-null.
---
Outside diff comments:
In `@src/Altinn.App.Core/Models/Process/ProcessChangeResult.cs`:
- Line 10: The ProcessChangeResult class is a concrete result model that should
be sealed; update its declaration to add the sealed modifier on the class
definition (ProcessChangeResult) so it cannot be inherited, leaving the rest of
the class unchanged.
---
Nitpick comments:
In `@src/Altinn.App.Core/Models/Process/ProcessChangeResult.cs`:
- Around line 58-61: The constructor ProcessChangeResult(Instance
mutatedInstance) currently only sets MutatedInstance and leaves the Success
property at its default false; modify the constructor body to also assign
Success = true so the invariant is enforced at construction time (i.e., set
Success within the ProcessChangeResult(Instance) constructor alongside
MutatedInstance) to avoid requiring callers to use an object initializer to mark
success.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
test/Altinn.App.Api.Tests/Process/ServiceTasks/ServiceTaskFirstTests.cs
Dismissed
Show dismissed
Hide dismissed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Altinn.App.Api/Controllers/ProcessController.cs (1)
150-163: Null-forgiving operator removed —MutatedInstanceis now correctly narrowed by[MemberNotNullWhen].The
ProcessStateChangeis properly threaded through, andMutatedInstanceis safely accessed after theSuccesscheck without!. Good fix.However, lines 156-159 still return
Conflict(startProcessResult.ErrorMessage)for all failure cases. SinceStartcan delegate toNext(for initial service tasks), failures may includeInternalorUnauthorizederror types. UseGetResultForError(startProcessResult)for correct HTTP status mapping, consistent with theNextElementendpoint (line 292).Proposed fix
if (!startProcessResult.Success) { - return Conflict(startProcessResult.ErrorMessage); + return GetResultForError(startProcessResult); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Controllers/ProcessController.cs` around lines 150 - 163, Replace the unconditional Conflict return for start failures with the standardized error mapper: after calling _processEngine.Start and checking if (!startProcessResult.Success) return GetResultForError(startProcessResult) instead of Conflict(startProcessResult.ErrorMessage); reference the startProcessResult variable and the Start call and mirror the pattern used in NextElement (GetResultForError) so Internal/Unauthorized are mapped correctly.
🧹 Nitpick comments (4)
src/Altinn.App.Core/Internal/Process/Interfaces/IProcessEngine.cs (1)
40-51: Add<param>documentation tags to the newStartmethod.The existing
HandleEventsAndUpdateStoragehas<param>tags for each parameter, but the newStartmethod only has a summary. The parameters (instance,processStateChange,user,prefill,language,ct) are non-obvious and deserve inline documentation, especiallyprocessStateChangewhose doc comment currently only loosely referencesGenerateProcessStartEventswithout clarifying that it expects the non-nullProcessStateChangemember from a successfulProcessChangeResult.📝 Proposed doc additions
/// <summary> /// Dispatches process start events to storage and auto-runs any initial service tasks. /// Call after instance creation and data storage, with the result from <see cref="GenerateProcessStartEvents"/>. /// </summary> + /// <param name="instance">The newly created instance.</param> + /// <param name="processStateChange"> + /// The non-null <see cref="ProcessStateChange"/> from a successful <see cref="GenerateProcessStartEvents"/> result. + /// </param> + /// <param name="user">The authenticated principal initiating the process.</param> + /// <param name="prefill">Optional prefill data forwarded to the first user task.</param> + /// <param name="language">Optional language hint for the process execution.</param> + /// <param name="ct">Cancellation token.</param> Task<ProcessChangeResult> Start( Instance instance, ProcessStateChange processStateChange, ClaimsPrincipal user, Dictionary<string, string>? prefill = null, string? language = null, CancellationToken ct = default );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Process/Interfaces/IProcessEngine.cs` around lines 40 - 51, Add XML <param> documentation for the Start method describing each parameter: describe Instance instance (the instance to start), ProcessStateChange processStateChange (must be the non-null ProcessStateChange returned from a successful GenerateProcessStartEvents / ProcessChangeResult), ClaimsPrincipal user (the caller/actor), Dictionary<string,string>? prefill (optional prefill key/value data), string? language (optional language code), and CancellationToken ct (cancellation token); keep the existing summary and mirror the style used on HandleEventsAndUpdateStorage so consumers understand the required non-null processStateChange contract and purpose of each parameter.src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (3)
225-233: Prefill is forwarded on every loop iteration — verify this is intentional.Unlike
ActionandActionOnBehalfOf(which are cleared after the first iteration at lines 229-230),Prefillis passed through on every iteration of thedo-whileloop. This is currently safe because the loop only continues whileIsServiceTaskis true, andHandleMoveToNext(line 799) nullifies prefill for service-task destinations. However, the asymmetry withAction/ActionOnBehalfOfcould confuse future maintainers.Consider adding a brief comment explaining why
Prefillisn't cleared, or clearing it after it's been applied (i.e., after the first non-service-task iteration, which is also the last iteration).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Process/ProcessEngine.cs` around lines 225 - 233, The code constructs a ProcessNextRequest with Prefill left set for every loop iteration while Action and ActionOnBehalfOf are cleared after the first iteration; either add a short explanatory comment near the ProcessNextRequest construction referencing why Prefill must persist across service-task iterations (mention IsServiceTask and HandleMoveToNext) or change behavior to mirror Action/ActionOnBehalfOf by clearing Prefill after its first use (i.e., set Prefill = null after the first non-service-task iteration or when firstIteration becomes false) so future maintainers understand the intent and symmetry with Action/ActionOnBehalfOf.
784-803: Prefill-gating logic inHandleMoveToNextis correct but subtly depends on mutation ordering.Line 799 calls
IsServiceTask(instance)afterMoveProcessStateToNextAndGenerateEvents(line 790) has already mutatedinstance.Processin-place to point to the next element. So this correctly checks whether the destination task is a service task. This is the right behavior (skip prefill for service tasks, apply it for user tasks).A brief inline comment clarifying that
instance.Processalready reflects the next element at this point would help readability, sinceHandleMoveToNextdoesn't make this mutation ordering obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Process/ProcessEngine.cs` around lines 784 - 803, Add a short inline comment after the call to MoveProcessStateToNextAndGenerateEvents in HandleMoveToNext to state that MoveProcessStateToNextAndGenerateEvents mutates instance.Process to point to the destination element, so the subsequent IsServiceTask(instance) check is intentionally inspecting the next task (destination) to decide whether to apply prefill; reference these symbols: HandleMoveToNext, MoveProcessStateToNextAndGenerateEvents, instance.Process, and IsServiceTask.
153-183: Start method correctly threadsProcessStateChangeand handles service-task auto-continuation.The previous review's concern about
ProcessChangeResultcontract violation (Success = truewithoutProcessStateChange) is resolved — line 169 now properly setsProcessStateChange = processStateChange.One observation: this new public method lacks its own telemetry
Activityspan, unlike other public entry points (GenerateProcessStartEventsat line 86,Nextat line 190). The downstream calls are instrumented, but having a top-level span forStartwould improve trace visibility. As per coding guidelines, "Comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry".🔭 Suggested telemetry addition
public async Task<ProcessChangeResult> Start( Instance instance, ProcessStateChange processStateChange, ClaimsPrincipal user, Dictionary<string, string>? prefill = null, string? language = null, CancellationToken ct = default ) { + using var activity = _telemetry?.StartProcessStartActivity(instance); + // HandleEvents mutates instance in-place; we discard the DispatchToStorage // return (same as callers did before this method existed). bool isServiceTask = IsServiceTask(instance); await HandleEventsAndUpdateStorage(instance, isServiceTask ? null : prefill, processStateChange.Events); if (!isServiceTask) - return new ProcessChangeResult(instance) { Success = true, ProcessStateChange = processStateChange }; + { + var result = new ProcessChangeResult(instance) { Success = true, ProcessStateChange = processStateChange }; + activity?.SetProcessChangeResult(result); + return result; + } // Auto-run through initial service tasks, forwarding prefill to the first user task return await Next( new ProcessNextRequest { Instance = instance, User = user, Action = null, Language = language, Prefill = prefill, }, ct ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Internal/Process/ProcessEngine.cs` around lines 153 - 183, Add a top-level OpenTelemetry Activity/span around the Start method to match other public entry points (like GenerateProcessStartEvents and Next): create and start an Activity (e.g., "ProcessEngine.Start") at the start of Start, ensure it is disposed/stopped after the method completes (wrapping the existing logic including HandleEventsAndUpdateStorage and the conditional Next call), and attach relevant attributes (instance id, isServiceTask flag, language) and exceptions to the span so the Start call is visible in traces and errors are recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs`:
- Around line 396-405: The mock-returned ProcessChangeResult from the mocked
Start call in InstancesController_CopyInstanceTests.cs sets Success = true and
MutatedInstance = inst but omits ProcessStateChange, which violates the
[MemberNotNullWhen(true, nameof(ProcessStateChange))] contract; update each
mocked .ReturnsAsync for Start to also set a non-null ProcessStateChange (e.g.,
a minimal ProcessStateChange instance/placeholder) whenever Success is true so
the contract holds for ProcessChangeResult, keeping MutatedInstance as-is and
applying this fix to all five test methods that mock Start.
---
Duplicate comments:
In `@src/Altinn.App.Api/Controllers/ProcessController.cs`:
- Around line 150-163: Replace the unconditional Conflict return for start
failures with the standardized error mapper: after calling _processEngine.Start
and checking if (!startProcessResult.Success) return
GetResultForError(startProcessResult) instead of
Conflict(startProcessResult.ErrorMessage); reference the startProcessResult
variable and the Start call and mirror the pattern used in NextElement
(GetResultForError) so Internal/Unauthorized are mapped correctly.
---
Nitpick comments:
In `@src/Altinn.App.Core/Internal/Process/Interfaces/IProcessEngine.cs`:
- Around line 40-51: Add XML <param> documentation for the Start method
describing each parameter: describe Instance instance (the instance to start),
ProcessStateChange processStateChange (must be the non-null ProcessStateChange
returned from a successful GenerateProcessStartEvents / ProcessChangeResult),
ClaimsPrincipal user (the caller/actor), Dictionary<string,string>? prefill
(optional prefill key/value data), string? language (optional language code),
and CancellationToken ct (cancellation token); keep the existing summary and
mirror the style used on HandleEventsAndUpdateStorage so consumers understand
the required non-null processStateChange contract and purpose of each parameter.
In `@src/Altinn.App.Core/Internal/Process/ProcessEngine.cs`:
- Around line 225-233: The code constructs a ProcessNextRequest with Prefill
left set for every loop iteration while Action and ActionOnBehalfOf are cleared
after the first iteration; either add a short explanatory comment near the
ProcessNextRequest construction referencing why Prefill must persist across
service-task iterations (mention IsServiceTask and HandleMoveToNext) or change
behavior to mirror Action/ActionOnBehalfOf by clearing Prefill after its first
use (i.e., set Prefill = null after the first non-service-task iteration or when
firstIteration becomes false) so future maintainers understand the intent and
symmetry with Action/ActionOnBehalfOf.
- Around line 784-803: Add a short inline comment after the call to
MoveProcessStateToNextAndGenerateEvents in HandleMoveToNext to state that
MoveProcessStateToNextAndGenerateEvents mutates instance.Process to point to the
destination element, so the subsequent IsServiceTask(instance) check is
intentionally inspecting the next task (destination) to decide whether to apply
prefill; reference these symbols: HandleMoveToNext,
MoveProcessStateToNextAndGenerateEvents, instance.Process, and IsServiceTask.
- Around line 153-183: Add a top-level OpenTelemetry Activity/span around the
Start method to match other public entry points (like GenerateProcessStartEvents
and Next): create and start an Activity (e.g., "ProcessEngine.Start") at the
start of Start, ensure it is disposed/stopped after the method completes
(wrapping the existing logic including HandleEventsAndUpdateStorage and the
conditional Next call), and attach relevant attributes (instance id,
isServiceTask flag, language) and exceptions to the span so the Start call is
visible in traces and errors are recorded.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
AGENTS.mdsrc/Altinn.App.Api/Controllers/InstancesController.cssrc/Altinn.App.Api/Controllers/ProcessController.cssrc/Altinn.App.Core/Internal/Process/Interfaces/IProcessEngine.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.App.Api/Controllers/InstancesController.cs
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
Show resolved
Hide resolved
…oller and test mock contracts.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
387-411:Startmock updated to new 6-parameter signature withProcessStateChangeproperly populated in all return values.The previously flagged missing
ProcessStateChangein theStartmock return has been fixed —ProcessStateChange = new()is now present in all five test methods, satisfying the[MemberNotNullWhen]contract.The non-nullable type matchers for the nullable
Dictionary<string, string>?andstring?parameters (It.IsAny<Dictionary<string, string>>(),It.IsAny<string>()) are functionally equivalent to their nullable counterparts at Moq runtime (C# nullable reference annotations are compile-time only). For consistency with the actual interface signature, consider using nullable variants.✨ Optional: align matchers with nullable interface signature
- It.IsAny<Dictionary<string, string>>(), - It.IsAny<string>(), + It.IsAny<Dictionary<string, string>?>(), + It.IsAny<string?>(),Apply the same diff to all five
Startmock setups (lines 391–392, 530–531, 748–749, 965–966, 1189–1190).Also applies to: 526-549, 744-768, 961-985, 1185-1209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs` around lines 387 - 411, The Start mock setups should use nullable matchers to match the interface signature and consistently populate ProcessStateChange in the returned ProcessChangeResult; update each Start(...) ReturnsAsync lambda (the five occurrences that return ProcessChangeResult) to keep ProcessStateChange = new() and change the non-nullable It.IsAny matchers to their nullable equivalents (e.g., It.IsAny<Dictionary<string,string>?>() and It.IsAny<string?>()) so the mock signature matches the Start method's nullable parameters across all Start setups referenced in the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs`:
- Around line 387-411: The Start mock setups should use nullable matchers to
match the interface signature and consistently populate ProcessStateChange in
the returned ProcessChangeResult; update each Start(...) ReturnsAsync lambda
(the five occurrences that return ProcessChangeResult) to keep
ProcessStateChange = new() and change the non-nullable It.IsAny matchers to
their nullable equivalents (e.g., It.IsAny<Dictionary<string,string>?>() and
It.IsAny<string?>()) so the mock signature matches the Start method's nullable
parameters across all Start setups referenced in the tests.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Api/Controllers/ProcessController.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Api.Tests/Process/ServiceTasks/ServiceTaskFirstTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Api.Tests/Process/ServiceTasks/ServiceTaskFirstTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
384-411: Mock setup correctly matches the updatedIProcessEngine.Startsignature.The callback lambda parameters properly use nullable annotations (
Dictionary<string, string>?,string?), andProcessChangeResultnow includes bothMutatedInstanceandProcessStateChange.Note: The past review flagged using
It.IsAny<Dictionary<string, string>>()instead of nullable-annotated matchers. In practice, Moq'sIt.IsAny<T>()will match null values for reference types since nullable reference types are a compile-time feature only. The mock will function correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs` around lines 384 - 411, The mock setup for IProcessEngine.Start is correct—ensure the callback lambda parameters use nullable annotations matching the updated Start signature (e.g., Dictionary<string, string>?, string?), keep the It.IsAny<T>() matchers (they will accept null at runtime), and return a ProcessChangeResult that populates both MutatedInstance and ProcessStateChange as shown; no code change required other than keeping these nullable parameter types and the two result properties in the ReturnsAsync lambda for the Start mock.src/Altinn.App.Api/Controllers/InstancesController.cs (1)
748-763: Same early-check issue as PostSimplified.
Apply the same ordering here—return onstartResult.Success == falsebefore creating/copying the instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Controllers/InstancesController.cs` around lines 748 - 763, The code calls _processEngine.Start and mutates targetInstance before checking startResult.Success; reorder the logic to return early when startResult.Success is false (i.e., keep the existing "if (!startResult.Success) return Conflict(...)" immediately after computing startResult and before any creation/copying of the instance or calling _processEngine.Start), then only call _processEngine.Start and assign targetInstance = startProcessResult.MutatedInstance after confirming startResult succeeded; adjust the block around startResult, _processEngine.Start, startProcessResult, and targetInstance to follow this ordering.
🧹 Nitpick comments (1)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
1314-1391: Good test coverage forStartfailure path.This test correctly validates that the controller returns
ConflictObjectResultwhenIProcessEngine.StartreturnsSuccess = false. TheProcessChangeResultsetup is appropriate—omittingMutatedInstanceandProcessStateChangeis correct since[MemberNotNullWhen(true, ...)]only requires them to be non-null whenSuccessis true.Minor note: Some other test methods in this file call
fixture.Mock<T>().VerifyAll()andfixture.VerifyNoOtherCalls(), but this test does not. Consider adding these verification calls for consistency if the mocks are expected to be invoked exactly as set up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs` around lines 1314 - 1391, The test CopyInstance_StartFails_ReturnsConflict is missing mock verification for consistency; after the Assert, call VerifyAll() on the mocks you set up (HttpContext, IAppMetadata, IPDP, IInstanceClient, IInstantiationValidator, IProcessEngine) to ensure expected invocations (e.g., fixture.Mock<HttpContext>().VerifyAll(), fixture.Mock<IAppMetadata>().VerifyAll(), etc.), then call fixture.VerifyNoOtherCalls() to assert no unexpected calls occurred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 611-627: The check for processResult.Success must be moved
earlier: after the call that sets processResult (likely
GenerateProcessStartEvents) and before any instance creation/copying (methods
CreateInstance/CopyInstance). Change the flow in InstancesController.cs so that
you immediately return Conflict(processResult.ErrorMessage) if
processResult.Success is false, and only proceed to call
CreateInstance/CopyInstance and later _processEngine.Start when
processResult.Success is true; keep subsequent checks for startProcessResult
as-is. Ensure you reference the same processResult variable and preserve passing
processResult.ProcessStateChange into _processEngine.Start.
- Around line 384-395: Wrap the body of ProcessEngine.Start with an
OpenTelemetry span by calling _telemetry?.StartProcessStartActivity(instance) at
the start of the method and ensuring it is disposed/stopped around the existing
logic (including the call to Next()), so the Start flow is instrumented
end-to-end; locate the Start method in the ProcessEngine class, call
_telemetry?.StartProcessStartActivity(instance) to create the span, execute the
original logic (including Next()), and stop/dispose the activity in a finally
block to guarantee the span is ended even on error.
---
Duplicate comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 748-763: The code calls _processEngine.Start and mutates
targetInstance before checking startResult.Success; reorder the logic to return
early when startResult.Success is false (i.e., keep the existing "if
(!startResult.Success) return Conflict(...)" immediately after computing
startResult and before any creation/copying of the instance or calling
_processEngine.Start), then only call _processEngine.Start and assign
targetInstance = startProcessResult.MutatedInstance after confirming startResult
succeeded; adjust the block around startResult, _processEngine.Start,
startProcessResult, and targetInstance to follow this ordering.
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs`:
- Around line 384-411: The mock setup for IProcessEngine.Start is correct—ensure
the callback lambda parameters use nullable annotations matching the updated
Start signature (e.g., Dictionary<string, string>?, string?), keep the
It.IsAny<T>() matchers (they will accept null at runtime), and return a
ProcessChangeResult that populates both MutatedInstance and ProcessStateChange
as shown; no code change required other than keeping these nullable parameter
types and the two result properties in the ReturnsAsync lambda for the Start
mock.
---
Nitpick comments:
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs`:
- Around line 1314-1391: The test CopyInstance_StartFails_ReturnsConflict is
missing mock verification for consistency; after the Assert, call VerifyAll() on
the mocks you set up (HttpContext, IAppMetadata, IPDP, IInstanceClient,
IInstantiationValidator, IProcessEngine) to ensure expected invocations (e.g.,
fixture.Mock<HttpContext>().VerifyAll(),
fixture.Mock<IAppMetadata>().VerifyAll(), etc.), then call
fixture.VerifyNoOtherCalls() to assert no unexpected calls occurred.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
386-411: Nullable type mismatch inStartmock setup (pre-existing pattern).
It.IsAny<Dictionary<string, string>>()andIt.IsAny<string>()are used as matchers for parameters typedDictionary<string, string>?andstring?inIProcessEngine.Start. TheReturnsAsynclambda already uses the correct nullable annotations (Dictionary<string, string>? _,string? _), making the Setup/ReturnsAsync inconsistent.This pattern is repeated across all
Startmock setups in the file (lines 525–550, 743–768, 960–985, 1183–1209, 1365–1383).p.Start( It.IsAny<Instance>(), It.IsAny<ProcessStateChange>(), It.IsAny<System.Security.Claims.ClaimsPrincipal>(), - It.IsAny<Dictionary<string, string>>(), - It.IsAny<string>(), + It.IsAny<Dictionary<string, string>?>(), + It.IsAny<string?>(), It.IsAny<CancellationToken>() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs` around lines 386 - 411, The mock Setup for IProcessEngine.Start uses non-nullable matchers It.IsAny<Dictionary<string,string>>() and It.IsAny<string>() while the Start signature and ReturnsAsync lambda use nullable types; update the Setup call to use nullable generic matchers It.IsAny<Dictionary<string,string>?>() and It.IsAny<string?>() to match the method signature. Apply the same fix to all repeated Start mock setups in this file so the generic matcher types align with the parameter types used in the ReturnsAsync lambdas (refer to the Start method invocation and the ReturnsAsync lambda that returns ProcessChangeResult/MutatedInstance).src/Altinn.App.Api/Controllers/InstancesController.cs (1)
748-762:⚠️ Potential issue | 🟡 Minor
CopyInstance:GenerateProcessStartEventsfailure check placed afterCreateInstanceandCopyDataFromSourceInstance.If
GenerateProcessStartEventsfails (lines 748–749), the target instance has already been created (line 742) and source data already copied (line 744). This leaves both an orphaned storage instance and copied data elements.The null-safety rationale for placing the guard here (before accessing
startResult.ProcessStateChangeat line 753) is correct, but moving the check immediately after line 740 would prevent orphaned resources:🛡️ Suggested fix
ProcessChangeResult startResult = await _processEngine.GenerateProcessStartEvents(processStartRequest); +if (!startResult.Success) + return Conflict(startResult.ErrorMessage); + targetInstance = await _instanceClient.CreateInstance(org, app, targetInstance); await CopyDataFromSourceInstance(application, targetInstance, sourceInstance); targetInstance = await _instanceClient.GetInstance(targetInstance); -if (!startResult.Success) - return Conflict(startResult.ErrorMessage); - var startProcessResult = await _processEngine.Start(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Controllers/InstancesController.cs` around lines 748 - 762, In CopyInstance the result of GenerateProcessStartEvents (startResult) is checked only after CreateInstance and CopyDataFromSourceInstance, which can leave an orphaned instance and copied data if GenerateProcessStartEvents failed; move the null/success guard for startResult immediately after the GenerateProcessStartEvents call (before calling CreateInstance and CopyDataFromSourceInstance) so you return Conflict(startResult.ErrorMessage) early when startResult.Success is false, and only proceed to call _processEngine.Start and assign targetInstance when startResult is valid.
🧹 Nitpick comments (2)
test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_ServiceOwner.verified.txt (1)
165-195: BothProcess.StartandProcess.StartExecutionrecord identical process-change events — potential double-counting.The
Eventspayloads (process_StartEvent+process_StartTask,to.started,to.task.name) are identical in both spans. IfStartExecutionis an outer wrapper aroundStart, recording the same change events in both means downstream telemetry consumers (e.g., Grafana LGTM, Honeycomb) that aggregate span events will see each process change twice per instantiation. This is worth a conscious decision — ifStartExecutionis meant to be the authoritative carrier of these events, consider not re-emitting them on the innerProcess.Startspan (or vice versa).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_ServiceOwner.verified.txt` around lines 165 - 195, The test shows duplicate process-change events emitted on both Process.StartExecution and its inner Process.Start span; update the span/event construction so these events are only emitted once—either emit the process_StartEvent/process_StartTask and tags (to.started, to.task.name) only on Process.StartExecution or add a conditional in the Process.Start event creation to skip adding those tags when the span has a parent named Process.StartExecution (or when HasParent is true). Locate the code that builds Events for Process.Start and Process.StartExecution and implement the dedupe: move the event emission to the outer Process.StartExecution span or add a parent-check guard in Process.Start to avoid double-counting.test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
1393-1457:CopyInstance_GenerateProcessStartEventsFails_ReturnsConflict: addfixture.VerifyNoOtherCalls()for completeness.The test correctly sets up
CreateInstanceandGetInstance(confirming both are called even on aGenerateProcessStartEventsfailure, which documents the ordering issue in the controller), but unlike simpler tests in this file, there is nofixture.VerifyNoOtherCalls()to guard against unexpected calls — e.g.,IProcessEngine.Startbeing called accidentally. While Moq's loose mock would returnnullforTask<ProcessChangeResult>causing aNullReferenceExceptionthat would fail the test, an explicitVerifyNoOtherCalls()makes the intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs` around lines 1393 - 1457, Add a final verification to the test CopyInstance_GenerateProcessStartEventsFails_ReturnsConflict to ensure no unexpected mock calls occur: after asserting the ConflictObjectResult, call fixture.VerifyNoOtherCalls() (using the InstancesControllerFixture instance named fixture) so all mocked dependencies (e.g., IInstanceClient, IProcessEngine with GenerateProcessStartEvents) have no extra interactions such as an accidental IProcessEngine.Start invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 748-762: In CopyInstance the result of GenerateProcessStartEvents
(startResult) is checked only after CreateInstance and
CopyDataFromSourceInstance, which can leave an orphaned instance and copied data
if GenerateProcessStartEvents failed; move the null/success guard for
startResult immediately after the GenerateProcessStartEvents call (before
calling CreateInstance and CopyDataFromSourceInstance) so you return
Conflict(startResult.ErrorMessage) early when startResult.Success is false, and
only proceed to call _processEngine.Start and assign targetInstance when
startResult is valid.
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs`:
- Around line 386-411: The mock Setup for IProcessEngine.Start uses non-nullable
matchers It.IsAny<Dictionary<string,string>>() and It.IsAny<string>() while the
Start signature and ReturnsAsync lambda use nullable types; update the Setup
call to use nullable generic matchers It.IsAny<Dictionary<string,string>?>() and
It.IsAny<string?>() to match the method signature. Apply the same fix to all
repeated Start mock setups in this file so the generic matcher types align with
the parameter types used in the ReturnsAsync lambdas (refer to the Start method
invocation and the ReturnsAsync lambda that returns
ProcessChangeResult/MutatedInstance).
---
Nitpick comments:
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs`:
- Around line 1393-1457: Add a final verification to the test
CopyInstance_GenerateProcessStartEventsFails_ReturnsConflict to ensure no
unexpected mock calls occur: after asserting the ConflictObjectResult, call
fixture.VerifyNoOtherCalls() (using the InstancesControllerFixture instance
named fixture) so all mocked dependencies (e.g., IInstanceClient, IProcessEngine
with GenerateProcessStartEvents) have no extra interactions such as an
accidental IProcessEngine.Start invocation.
In
`@test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_ServiceOwner.verified.txt`:
- Around line 165-195: The test shows duplicate process-change events emitted on
both Process.StartExecution and its inner Process.Start span; update the
span/event construction so these events are only emitted once—either emit the
process_StartEvent/process_StartTask and tags (to.started, to.task.name) only on
Process.StartExecution or add a conditional in the Process.Start event creation
to skip adding those tags when the span has a parent named
Process.StartExecution (or when HasParent is true). Locate the code that builds
Events for Process.Start and Process.StartExecution and implement the dedupe:
move the event emission to the outer Process.StartExecution span or add a
parent-check guard in Process.Start to avoid double-counting.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Altinn.App.Api/Controllers/InstancesController.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.Processes.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_SelfIdentifiedUser.verified.txttest/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_ServiceOwner.verified.txttest/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_SystemUser.verified.txttest/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_User.verified.txt
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Altinn.App.Api/Controllers/InstancesController.cs (1)
738-762:⚠️ Potential issue | 🟠 MajorMove the
GenerateProcessStartEventssuccess guard before creating/copying the instance.Line 748 checks
startResult.Successafter instance creation and copy. If the process start fails, you still create and populate an instance, leaving orphaned data.🛠️ Proposed fix
ProcessChangeResult startResult = await _processEngine.GenerateProcessStartEvents(processStartRequest); + if (!startResult.Success) + return Conflict(startResult.ErrorMessage); targetInstance = await _instanceClient.CreateInstance(org, app, targetInstance); await CopyDataFromSourceInstance(application, targetInstance, sourceInstance); targetInstance = await _instanceClient.GetInstance(targetInstance); - if (!startResult.Success) - return Conflict(startResult.ErrorMessage); - var startProcessResult = await _processEngine.Start( targetInstance, startResult.ProcessStateChange, User, language: language, ct: HttpContext.RequestAborted );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Controllers/InstancesController.cs` around lines 738 - 762, The code calls _processEngine.GenerateProcessStartEvents (creating ProcessStartRequest and storing startResult) but only checks startResult.Success after creating the target instance and copying data; move the success guard so you check startResult.Success immediately after the GenerateProcessStartEvents call (and return Conflict(startResult.ErrorMessage) if false) before calling _instanceClient.CreateInstance or CopyDataFromSourceInstance, ensuring you only create/populate an instance when GenerateProcessStartEvents succeeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 738-762: The code calls _processEngine.GenerateProcessStartEvents
(creating ProcessStartRequest and storing startResult) but only checks
startResult.Success after creating the target instance and copying data; move
the success guard so you check startResult.Success immediately after the
GenerateProcessStartEvents call (and return Conflict(startResult.ErrorMessage)
if false) before calling _instanceClient.CreateInstance or
CopyDataFromSourceInstance, ensuring you only create/populate an instance when
GenerateProcessStartEvents succeeded.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If a service task is the first element after StartEvent, run process next automatically to execute it and move to the next element. Forward prefill to first user task.
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
Documentation