819: Start the building block version linked to the case - #879
819: Start the building block version linked to the case#879maarten-ritense wants to merge 5 commits into
Conversation
An action that starts a building block ran the newest version of that building block instead of the version linked to the case, failing when that version wrote to fields the linked version does not have. The process link carries the exact, versioned process definition id, but form submission forwarded only the process definition key. Resolving a key then fell back to "highest engine version not linked to a case definition", which does not exclude building-block-owned definitions, so a newer draft version won. Carry the versioned id through to the engine for process-link starts, and derive a document's blueprint as case or building block - caseDefinitionId() is null for building block documents, which is what made the lookup degrade to "latest". Resolving by key alone now refuses building-block definitions instead of guessing a version. processDefinitionId on the start-process requests is @JsonIgnore on purpose: ProcessDocumentResource binds two of them from client JSON with FAIL_ON_UNKNOWN_PROPERTIES disabled, so a bindable field would let a caller bypass key and blueprint resolution and reach a suspended draft or another case's definition.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughProcess startup now supports exact process-definition IDs and blueprint-aware resolution. Unlinked process starts select the latest definition that is not linked to a case or building block. Form, URL, and document services propagate resolved definition IDs through internal start requests. JSON binding excludes client-supplied IDs. Tests and BPMN fixtures cover linked, building block, unlinked, exact-version, and failure scenarios. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
13.40.0 is being released, so the entry moves to the next unreleased minor. Adds the 13.41.0 directory and registers it in SUMMARY.md so GitBook renders it.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d70da6c0-31fd-4e60-bec6-184961cf3d99
📒 Files selected for processing (2)
documentation/SUMMARY.mddocumentation/release-notes/13.x.x/13.41.0/README.md
|
Will create a test environment. This comment will be updated once it is available. This usually takes a few minutes.
|
|
Will create a test environment. This comment will be updated once it is available. This usually takes a few minutes.
Test environment metadata:
Observability: |
The specification that resolves a process definition from its key alone was written out three times: in OperatonProcessService, inlined in ProcessAuthorizationService and mirrored in the helper's integration test. Move it to OperatonProcessDefinitionSpecificationHelper so all three share one definition and the test covers the real specification.
Describe the changes
Link to the related Github issue: generiekzaakafhandelcomponent/gzac-issues#819
A building block linked to a case as an action ran the newest version of that building block instead of the version that is linked. After creating a draft version and changing its process, starting the action still ran the draft, which failed as soon as that version wrote to a document field the linked version does not have.
Everything up to the submit was already correct:
StartableBuildingBlockItemProviderreturns the version-specific process definition id, and the start form is fetched by that id. The version was lost inside the submit.DefaultFormSubmissionServiceresolved the right versionedOperatonProcessDefinitionand then forwarded onlyprocessDefinition.key. Resolving a key inOperatonProcessService.startProcessfirst tries the blueprint version tag — which for a case document isCD:<case>:<v>and can never match a building block process taggedBB:<key>:<v>— and then fell back tomaxVersionOf(byNotLinkedToCaseDefinition()). That filter excludesCD:andDETACHED:but notBB:, so it selected the highest engine version of the key: the draft.The engine call is already by id (
formService.submitStartForm(processDefinition.getId(), ...)); only the resolution from a key was lossy. So the versioned process definition id is now carried end to end for process-link starts, and key-based resolution was made blueprint-correct and fail-fast.Specify the code branch location:
bugfix/819-actions-respect-building-block-versions→next-minorRelevant comments:
Breaking changes
All additions are additive:
OperatonProcessService.startProcessById,withProcessDefinitionIdon three request DTOs, and aDocumentDefinition.Id.asBlueprintId()default method. No signature was removed or changed incompatibly, and the JSON contract of the request DTOs is unchanged. See the flagged behavioural change under Relevant comments — a building-block-owned process can no longer be started by process definition key alone. Previously that input had no defined outcome (it returned an arbitrary version), which is why I read it as a bug fix rather than a breaking change.Documentation
New features or changes that have been introduced have been documented.
Tests
Unit tests have been added that cover these changes
OperatonProcessServiceTest(6 new: start-by-id uses the exact id and never resolves by key, not-found,DETACHED:, suspended activate → start → re-suspend including on failure,UNDEFINED_BUSINESS_KEY→ null — this class had nostartProcesscoverage at all before),DefaultFormSubmissionServiceTest(3 new, including a building-block variant),OperatonProcessJsonSchemaDocumentServiceTest(2 new: start-by-id routing, building-block blueprint derivation),ProcessDocumentResourceTest(3 new, the binding/serialisation guards).Integration tests have been added that cover these changes
BuildingBlockStartableItemVersionIT(new — the bug itself, the fail-fast path, and a standalone building block start),UnlinkedProcessStartIntTest(new — system processes are not regressed), and two cases added toOperatonProcessDefinitionSpecificationHelperIntTestfor the composed unlinked specification.Ran green:
:backend:{core,process-document,form,process-link-url,form-view-model,case,building-block}:test,:backend:zgw:{portaaltaak,verzoek,zaken-api}:test, andintegrationTestingPostgresqlforbuilding-block,core,process-document,form,form-view-model,process-link-url,zgw:verzoekandzgw:portaaltaak. Checkstyle passes with no new warnings. Thezgw:verzoeksuite matters because the key-only fallback exists for the Verzoek plugin.Describe the testing steps
0.0.1final and linked to a case as an action,0.0.2a draft whose main process writes to a document field that does not exist in0.0.1— starting the action from a case runs0.0.1and completes without an error. See provided 'Hello World' building block exports below.0.0.1one (BB:<key>:0.0.1), and aBuildingBlockInstanceis created for0.0.1with the case document as itscase_document_id0.0.2further has no effect on starting the action/processesthat is not linked to a case definition still starts, and starts its latest version2 versions of the 'Hello World' building block which can be used for testing:
Security
The Secure by Design principle has been applied to these changes
The new
processDefinitionIdis deliberately not bindable from client JSON (see Relevant comments); allowing it would let a caller choose which process definition to start for a document, bypassing the key and blueprint resolution that couples the two. The value is only ever set server-side from the process link the request refers to. TheDETACHED:guard closes a second path to starting a definition that is no longer reachable through normal resolution.Added or changed REST API endpoints have authentication and authorization in place
No endpoints were added or changed. Two existing ones (
/api/v1/process-document/operation/{new,modify}-document-and-start-process) have request DTOs that gained a field, which is why that field is@JsonIgnoreand whyProcessDocumentResourceTestnow asserts a client-suppliedprocessDefinitionIdis not bound and never serialised.Valtimo access control checks have been implemented
The existing
RelatedEntityAuthorizationRequest(OperatonExecution, CREATE, OperatonProcessDefinition, <id>)check is kept on both start paths, and is now evaluated against the definition that is actually started. Before this change the form submission authorized the process link's definition while the start authorized whichever definition the key resolved to — potentially a different one. Permission rules that key offversionTagwill therefore see the linked version instead of the newest; that is the intended outcome.ProcessAuthorizationServiceinform-view-modelduplicates this resolution and was aligned for the same reason.Dependencies
Newly added dependencies do not introduce known vulnerabilities/CVE's and are in line with the Valtimo license
No dependencies were added or changed.