Uow aggregate endpoint - #19919
Draft
vxkc wants to merge 10 commits into
Draft
Conversation
Thread instance, process-state, and blob version metadata from Storage responses into unit-of-work state and conditional write preconditions, and commit staged changes through the aggregate instance-mutation endpoint. Carry aggregate mutation and conflict handling through API controllers, workflow and process execution, PDF, Fiks, EFormidling, and test plumbing. InstanceClient and DataClient directly implement the internal metadata and mutation views. Resolve one authentication method per aggregate save, preserving each call-site default when no data method contributes and a lone method unchanged. Union distinct ServiceOwner additional scopes in ordinal order, and reject CurrentUser/ServiceOwner or unsafe Custom mixtures before Storage I/O while allowing Delegate.Equals-equivalent Custom providers. Track StorageVersionMetadata as an immutable authoritative snapshot without lock, merge, wrapper, or restore escape hatches; replace it after successful aggregate saves and authoritative replay rebuilds. Keep workflows persisted by the prior callback protocol resumable: accept the exact main-era discriminatorless process-state payload on MutateProcessState and the legacy SaveProcessStateToStorage key, retain bounded support for the registered legacy discriminator, derive lock/unlock payloads only for main-era null payloads, reject malformed non-null command payloads permanently, and hydrate versionless callback states with an exact ServiceOwner metadata read before the controller-owned single aggregate save. Evict stale-by-version data caches after aggregate saves so a subsequent read never serves content the save invalidated. Replace the app instance lock with durable process status. Storage carries an idle/processing status inside the instance process object, and every aggregate mutation states the status it expects. AcquireProcessingStatus is command index 0 of every chain-initiating workflow and stages expected idle to new processing before any other side effect; engine-owned dependent workflows inherit processing. CommitProcessState normally clears back to idle, but keeps processing when the sequence appends ExecuteServiceTask so the target service task is durable before its side effects run. Process-end cleanup folds into the terminal CommitProcessState aggregate, replacing DeleteDataElementsIfConfigured and DeleteInstanceIfConfigured. Each chain is fenced on one authoritative snapshot, a lost acquire is classified as its own failure kind, user-facing mutations answer 409 instance-processing while an instance is processing, and GETs stay readable through a transition. InstanceLocker, IInstanceLock, InstanceLockClient and its telemetry, the Altinn-Storage-Lock-Token plumbing, and the lock token on AppWorkflowContext, AppCallbackPayload, and the engine wire contract are removed. A service task releases processing ownership only where it genuinely pauses: when the pipeline's Finally concludes without auto-advancing, which includes the null a legacy implementation can still return. A completed stage keeps it, because the pipeline has further engine steps to run, and a deferral keeps it because a deferral is stateless — staging the transition would make the callback fail on the controller's no-data-changes guard, and a parked chain is still mid-transition. Require StepId on the callback payload. The engine's step id is what this revision sends Storage as the aggregate mutation's idempotency key, and it was deliberately optional only to tolerate an engine that predates the field; while both sides are pre-release, an absent id should fail the callback rather than default to Guid.Empty, which would be one key shared by every step of every instance. Rename DataElement.ContentEtag to BlobVersionId so the app models the raw blob version id Storage carries on the wire rather than a quoted HTTP etag. Vendor the Storage.Interface source under src/_vendor so the backend can compile against the approved blob-version-id and process-status contracts before a corresponding Altinn.Platform.Storage.Interface package is released, and block packing the app packages while that vendored copy exists. Delete the directory and restore the released package reference once the package ships; see its README for the exact steps. LocalTest support and LocalTest parity land separately, so the LocalTest-backed integration tests are expected red and are not run here. The independent parts of this work — the service-task execution reference time, the Fiks Arkiv major-version simplification, the data-accessor threading for PDF and eFormidling, and the LF and Docker-context hygiene — are below this revision in the stack. The retry-stable idempotency key those parts once carried is the workflow engine's own StepId, which main now supplies on every callback.
Copy the Storage.Interface sources from altinn-storage change lnlnxrxumkorxorksormotltoxtvkqwv, which types the process status as a ProcessStatus enum instead of a string, and adjust the app to it. The wire spelling is unchanged, so this is a compile-time change only: the enum still serializes as "idle" and "processing", and the OpenAPI documents process.status as that closed string enum. The copy is a full sync of Models and Enums rather than a targeted patch, so it also drops the vendored InstanceMutationRequest members upstream no longer has -- newProcessStatus and the update-operation deleteStatus. Neither reaches the wire: the app builds its own StorageInstanceMutationRequest, and a test already pins that newProcessStatus is never sent. TransitionProcessStatus loses its argument range guards, since the enum parameters make an out-of-range status unrepresentable, and their test goes with them. The blocking-status flow through ProcessStatusHelper, ProcessChangeResult and ProcessStatusProblemResult is typed the same way; the problem's machine-readable processStatus extension still serializes to the wire spelling, while its human-readable detail now names the enum member. FakeWorkflowEngineClient had a duplicated createdAt local and StartAt member that made Altinn.App.Api.Tests uncompilable; removed so the test project can build.
… review Copy the Storage.Interface sources from altinn-storage change rmoxyzkoxxrsnlpyvnzmvtwkwmpozkzr, which acts on review of the process-status enum, and follow it in the app. ProcessStatus moved to Interface.Enums, so the using follows it wherever the app names the enum. Both serializers now reject the numeric form of the status, which the app never sent, so nothing changes on the wire. Storage's 409 body carries the wire spelling again rather than the enum member name, and the app's own status messages follow it: the instance-processing problem detail, the unit-of-work guard, and the Storage fake that mirrors the platform's conflict response. The problem's machine-readable processStatus extension was already the wire spelling, so detail and extension agree again, and its assertion pins that spelling rather than round-tripping it through the converter that produced it. Correcting the parent: it says the enum parameters make an out-of-range status unrepresentable, which is the same mistake the storage side had to walk back -- (ProcessStatus)99 is representable, and the argument range guards TransitionProcessStatus lost were what kept one out of serialization. The risk is small enough to accept, since the method is internal and every caller passes a literal, but the reason given for dropping the guards was wrong.
feat: commit complete confirmations through the aggregate mutation endpoint The eFormidling task's Delivered branch confirmed completion with a direct POST .../complete -- the last Storage write left outside the unit of work. Every write bumps the instance version, so that call moved Storage past the version the callback's own workflow-owned save was fencing against: 412, then InstanceDataStaleException, then a non-retryable 422, and the process never left Task_Send_EFormidling. Staging the confirmation puts it in the same version-fenced mutation as the shipment status, which is what the callback saves; ProcessNext_EFormidlingTask_WaitsForDelivery_AndSendsTheShipmentOnce reaches EndEvent_1 again. Staging a confirmation the instance already carries is a no-op. Storage records the confirmation for the calling organisation, which for an app is the instance's own org, and it is idempotent in outcome but not version-neutral: asking again would bump the instance version for a write that changes nothing, and invalidate every fence held elsewhere with it. Deciding that here is safe because it only ever suppresses a request -- a confirmation added after this snapshot was captured is still sent, and Storage settles it. LocalTest mirrors Storage's operation, since apps must behave the same locally: the same InstanceComplete escalation, the same organisation-scoped stakeholder, only the added confirmation on the instance update, and the same division of labour -- its controller does not filter on the snapshot, and its repository skips a stakeholder that already has a confirmation while holding the instance lock, where mergeinstanceupdate does in production storage. LocalTest has no controller-test harness, so the controller wiring is covered by Storage's tests for the same logic and by the integration test end to end; the repository's skip has a test of its own. A staged confirmation refuses to commit under anything but service owner authentication. Storage records it for the calling organisation, and a data type demanding another authentication method resolves the whole aggregate to that method -- which would confirm as the wrong stakeholder, or as none at all. Like the other workflow-owned staged mutations, it is also rejected on a user-facing save. EFormidlingServiceTask holds no IInstanceClient any more: nothing in it writes to Storage directly, and the delivery tests assert the staged confirmation instead of a client call.
IDE0005: AcquireProcessingStatus and ProcessStatusProblemResult resolve ProcessStatus from Interface.Enums and reference nothing in Interface.Models; DataController is the other way round -- its only remaining "ProcessStatus" is a log message template. CS8600: VersionPreconditionHelper.TryParse returns (VersionPreconditions, ActionResult?), and LocalTest's ProcessController deconstructed the error into a non-nullable ActionResult at both call sites. It is the only storage controller the project's nullable context reaches; the eleven identical deconstructions in the other controllers sit in "#nullable disable" files and warn about nothing, so they are left as they are.
LocalTest stamped LastChanged/LastChangedBy on every data element update, which is what applyinstancemutation did before it learned to stamp content updates only. Gate it on the same signal production uses: /currentBlobVersion in the property list is what becomes the SQL's newblobversion, and only a content update carries it. Without this an app behaves differently locally than in the cloud in a way that is easy to miss and hard to explain: ending a task would rewrite every locked element's LastChangedBy to the app's own ServiceOwner identity, where real Storage leaves the content author in place.
…ion is in flight The test asserted type "instance-processing" plus a processStatus extension and failed with KeyNotFoundException on type. Neither is on the response. ProcessEngine.ProcessNext evaluates GetCurrentTaskWorkflowState before the blocking-status check, and an in-flight transition leaves an active collection head, so the request returns the Retrying conflict -- 409, title "Task is still being processed.", processNextState "retrying" -- and never reaches CreateProcessStatusBlockedResult. That ordering is deliberate rather than something to patch here. ProcessEngineTest pins both sides: Next_blocks_when_current_task_workflow_is_retrying with an active head and a processing status, and Next_blocks_non_idle_process_status_after_workflow_recovery_check without one. It is also the ordering the frontend depends on -- useProcessNext branches on processNextState, swallowing "retrying" to show the advancing screen and treating "resumeRequired" as a terminal failure. Hoisting the status guard would collapse both into instance-processing, because a terminally failed workflow also leaves the status non-idle, and the user would get an error page where the transition is merely still running. instance-processing is therefore unobservable on this endpoint while a workflow is live; its surface is the data and action endpoints, where DataElementAccessChecker, ActionsController, InstancesController and PaymentController all emit it. Its absence is asserted so the next reader does not restore the expectation. No coverage is lost: the durable processing status is still asserted on the GET above the competing call, and taskEndInvocations still proves the second transition never ran. Renamed because "WhileAuthoritativeStatusIsProcessing" named a guard this endpoint cannot reach.
…hots Every write mints a fresh blob version -- LocalTest through BlobVersionId.Encode(Guid.NewGuid()), production through Azure -- and nothing scrubbed it, so it reached the snapshot verbatim twice: as BlobVersionId and inside the versioned blobStoragePath. AutoVerify(includeBuildServer: false) rewrote those files silently on every local run while CI, where it is off, failed them. InstanceStringScrubber now replaces the value alongside the data element id. Two verifications reused a scrubber built from the instantiation instance for a snapshot taken after a patch had minted a new version, so the value never matched and the scrub was a no-op: BasicAppTests' PatchFormData (16 files) and WorkflowEngineFailureTests' InstanceAfterFailure. Both now build the scrubber from the instance in the response they verify. The failure mode is silent -- the snapshot just keeps a volatile value -- so a shape-based fallback, matching the token wherever it appears rather than depending on which instance the scrubber came from, would make this class of mistake impossible at the cost of the per-element index. The re-baselined content also carries what the stack changed and the snapshots had never been regenerated for. Process.Status is on the wire (73 files). ReadStatus is the Unread default that Verify omits rather than UpdatedSinceLastReview (13). A data element's post-save Size and LastChanged now reach the response for the write that made them instead of the creation-time values (16 and 34), from the unit-of-work stale-cache eviction. LastChangedBy moves the other way, from the app's own ServiceOwner identity back to the party that actually wrote the content, which is the content-only stamp gate in applyinstancemutation and its LocalTest mirror -- the localtest image had to be rebuilt from the checkout before these snapshots could see it. Generated from a full suite run (70 passed, 1 skipped, 0 failed) and confirmed stable: a repeat run over BasicAppTests, WorkflowEngineFailureTests and CustomScopesTests rewrote nothing, and no raw 22-character blob token remains in any snapshot in the project.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
the
skip-releasenotes
vxkc
marked this pull request as draft
August 11, 2026 15:19
…on plan Storage only grants ignoreLock to a metadata-only update that unlocks, so an update that re-locks an already locked data element is rejected and fails the whole aggregate with 409 "Data element ... is locked and cannot be updated or deleted". Every signing flow produced exactly that request: storage's sign endpoint creates the signature document already locked, and LockTaskData then stages a lock for every data type bound to the task, including the signature data type. The resulting lock-only update was a no-op that could only fail. The legacy data lock endpoint short-circuits an already locked element, so this only became reachable when lock mutation moved into the aggregate save. The unit of work now drops a lock-only update whose target value already matches the instance snapshot. The workflow-owned save carries both version preconditions and any lock change bumps the instance version, so a snapshot that disagrees with storage fails the save on the precondition instead of silently skipping a lock that was still needed. When nothing else is staged the plan is empty and the save is skipped without a storage round trip.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #18110.
Description
Verification