feat: building block hardening - #868
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBuilding-block process links now validate 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/plugin/src/main/kotlin/com/ritense/plugin/service/PluginService.kt (1)
636-656: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLog unresolved action properties also when they are omitted from the resolver result.
ValueResolverServiceImplstates that “An unresolved requestedValue will not be included in the returned map,” butlogUnresolvedActionPropertiesonly flags values whose key is present and mapped tonull. Add the missing keys to the warning path sopv:references that fail to resolve are reported instead of being silently handled as literals.
🧹 Nitpick comments (1)
backend/building-block/src/test/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockCallActivityListenerIT.kt (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated process-definition lookup helper.
Three test classes each add a private helper that performs the exact same
RepositoryServicequery (by key, latest version, throw if not deployed), just under different names/signatures. Since all three extendBaseIntegrationTest, moving this to that shared base (or a small test utility) avoids drift as more fixtures are added.
backend/building-block/src/test/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockCallActivityListenerIT.kt#L375-382: moveprocessDefinitionId(processKey: String = MAIN_PROCESS_KEY)toBaseIntegrationTestand remove the local copy.backend/building-block/src/test/kotlin/com/ritense/buildingblock/processlink/mapper/BuildingBlockProcessLinkIntegrationTest.kt#L412-419: replacelatestProcessDefinitionId(processKey)with a call to the shared helper.backend/building-block/src/test/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockFormPrefillIT.kt#L137-144: replaceprocessDefinitionId(processKey)with a call to the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e1e6b5d-99e7-45c3-8877-99610d0ce72e
📒 Files selected for processing (22)
.gitignorebackend/building-block/src/main/kotlin/com/ritense/buildingblock/configuration/BuildingBlockAutoConfiguration.ktbackend/building-block/src/main/kotlin/com/ritense/buildingblock/processlink/mapper/BuildingBlockProcessLinkMapper.ktbackend/building-block/src/main/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockCallActivityBusinessKeyValidator.ktbackend/building-block/src/main/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockCallActivityListener.ktbackend/building-block/src/main/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockProcessLinkCopyValidator.ktbackend/building-block/src/test/kotlin/com/ritense/buildingblock/TestApplication.ktbackend/building-block/src/test/kotlin/com/ritense/buildingblock/TestMailPlugin.ktbackend/building-block/src/test/kotlin/com/ritense/buildingblock/TestMailPluginFactory.ktbackend/building-block/src/test/kotlin/com/ritense/buildingblock/processlink/mapper/BuildingBlockProcessLinkIntegrationTest.ktbackend/building-block/src/test/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockCallActivityBusinessKeyValidatorTest.ktbackend/building-block/src/test/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockCallActivityListenerIT.ktbackend/building-block/src/test/kotlin/com/ritense/buildingblock/processlink/service/BuildingBlockFormPrefillIT.ktbackend/building-block/src/test/resources/config/building-block/bezwaar/1-0-0/bpmn/building-block-process.bpmnbackend/building-block/src/test/resources/config/building-block/bezwaar/1-0-0/document/definition/bezwaar.schema.document-definition.jsonbackend/building-block/src/test/resources/config/case/bb-case/1-0-0/bpmn/building-block-call-activity-main-dual-ns.bpmnbackend/building-block/src/test/resources/config/case/bb-case/1-0-0/bpmn/building-block-call-activity-main-wrong-key.bpmnbackend/building-block/src/test/resources/config/case/bb-case/1-0-0/document/definition/bb-case.schema.document-definition.jsonbackend/building-block/src/test/resources/config/plugin/test-mail.pluginconfig.jsonbackend/plugin/src/main/kotlin/com/ritense/plugin/service/PluginService.ktdocumentation/features/building-blocks/README.mddocumentation/release-notes/13.x.x/13.40.0/README.md
| @EventListener(ProcessLinksCopiedEvent::class) | ||
| fun validateCopiedBuildingBlockLinks(event: ProcessLinksCopiedEvent) { | ||
| val buildingBlockLinks = event.copiedProcessLinks.filterIsInstance<BuildingBlockProcessLink>() | ||
| if (buildingBlockLinks.isEmpty()) { | ||
| return | ||
| } | ||
| val bpmnModel = repositoryService.getBpmnModelInstance(event.processDefinitionId) ?: return | ||
|
|
||
| buildingBlockLinks.forEach { link -> | ||
| val callActivity = bpmnModel.getModelElementById<CallActivity>(link.activityId) | ||
| if (callActivity == null) { | ||
| logger.error { | ||
| "Building block process link '${link.id}' was copied to process definition " + | ||
| "'${event.processDefinitionId}', but activity '${link.activityId}' is not a call " + | ||
| "activity in that definition. The building block cannot start from this definition." | ||
| } | ||
| return@forEach | ||
| } | ||
| try { | ||
| BuildingBlockCallActivityBusinessKeyValidator.validate(callActivity, event.processDefinitionId) | ||
| } catch (e: IllegalStateException) { | ||
| logger.error { | ||
| "Building block process link '${link.id}' was copied to a process definition with an " + | ||
| "invalid call activity configuration. The building block will fail to start until " + | ||
| "the BPMN is fixed. ${e.message}" | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Broaden the catch to actually guarantee "never fails deployment."
The class doc states it must never fail deployment, but only IllegalStateException is caught around BuildingBlockCallActivityBusinessKeyValidator.validate. Any other unexpected exception (e.g. from unforeseen DOM/BPMN model issues) would propagate through the @EventListener and could break redeployment — contradicting the documented invariant.
🛡️ Proposed fix to widen the safety net
try {
BuildingBlockCallActivityBusinessKeyValidator.validate(callActivity, event.processDefinitionId)
- } catch (e: IllegalStateException) {
+ } catch (e: Exception) {
logger.error {
"Building block process link '${link.id}' was copied to a process definition with an " +
"invalid call activity configuration. The building block will fail to start until " +
"the BPMN is fixed. ${e.message}"
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @EventListener(ProcessLinksCopiedEvent::class) | |
| fun validateCopiedBuildingBlockLinks(event: ProcessLinksCopiedEvent) { | |
| val buildingBlockLinks = event.copiedProcessLinks.filterIsInstance<BuildingBlockProcessLink>() | |
| if (buildingBlockLinks.isEmpty()) { | |
| return | |
| } | |
| val bpmnModel = repositoryService.getBpmnModelInstance(event.processDefinitionId) ?: return | |
| buildingBlockLinks.forEach { link -> | |
| val callActivity = bpmnModel.getModelElementById<CallActivity>(link.activityId) | |
| if (callActivity == null) { | |
| logger.error { | |
| "Building block process link '${link.id}' was copied to process definition " + | |
| "'${event.processDefinitionId}', but activity '${link.activityId}' is not a call " + | |
| "activity in that definition. The building block cannot start from this definition." | |
| } | |
| return@forEach | |
| } | |
| try { | |
| BuildingBlockCallActivityBusinessKeyValidator.validate(callActivity, event.processDefinitionId) | |
| } catch (e: IllegalStateException) { | |
| logger.error { | |
| "Building block process link '${link.id}' was copied to a process definition with an " + | |
| "invalid call activity configuration. The building block will fail to start until " + | |
| "the BPMN is fixed. ${e.message}" | |
| } | |
| } | |
| } | |
| } | |
| `@EventListener`(ProcessLinksCopiedEvent::class) | |
| fun validateCopiedBuildingBlockLinks(event: ProcessLinksCopiedEvent) { | |
| val buildingBlockLinks = event.copiedProcessLinks.filterIsInstance<BuildingBlockProcessLink>() | |
| if (buildingBlockLinks.isEmpty()) { | |
| return | |
| } | |
| val bpmnModel = repositoryService.getBpmnModelInstance(event.processDefinitionId) ?: return | |
| buildingBlockLinks.forEach { link -> | |
| val callActivity = bpmnModel.getModelElementById<CallActivity>(link.activityId) | |
| if (callActivity == null) { | |
| logger.error { | |
| "Building block process link '${link.id}' was copied to a process definition " + | |
| "'${event.processDefinitionId}', but activity '${link.activityId}' is not a call " + | |
| "activity in that definition. The building block cannot start from this definition." | |
| } | |
| return@forEach | |
| } | |
| try { | |
| BuildingBlockCallActivityBusinessKeyValidator.validate(callActivity, event.processDefinitionId) | |
| } catch (e: Exception) { | |
| logger.error { | |
| "Building block process link '${link.id}' was copied to a process definition with an " + | |
| "invalid call activity configuration. The building block will fail to start until " + | |
| "the BPMN is fixed. ${e.message}" | |
| } | |
| } | |
| } | |
| } |
| require(invalidInputTargets.isEmpty()) { | ||
| "Input mapping targets must be building block fields. A value passed to a building block is " + | ||
| "stored in the building block document and can be referenced inside the building block " + | ||
| "with 'doc:'. Invalid targets: ${invalidInputTargets.joinToString()}" |
There was a problem hiding this comment.
That's a lot of words! I don't think this is a showstopper or anything, but if you have the chance, can you make this a bit less wordy? This applies to the other require as well
|
|
||
| * New bugfix. | ||
|
|
||
| ## Documentation |
There was a problem hiding this comment.
This isn't a section for the release notes. We link to relevant release notes on the individual release notes. I see the purpose of it, though, so maybe link it in the "Clearer rules for building block input and output mappings", or one of the other ones you added?
| * variable (pv:) inside a building block: values passed to a building block only exist in the | ||
| * building block document (doc:), never as process variables. | ||
| */ | ||
| private fun logUnresolvedActionProperties( |
There was a problem hiding this comment.
I'm not sure i like this. i think there can be valid cases where some data is null. if every times that is logged as warn this might cause all kinds of spam. I would either remove this, or change the log level to debug.
| * **Building block call activities are now validated** | ||
|
|
||
| New enhancement explanation. | ||
| The business key configuration of a building block call activity is checked when the building block link is saved |
There was a problem hiding this comment.
I feel that some of these release notes go too much into the technical detail. They should be easily understandable and preferably not too long.
…ies at debug (PR feedback)
…lding-block-hardening # Conflicts: # documentation/release-notes/13.x.x/13.40.0/README.md
ivo-ritense
added
the
test env
|
Will create a test environment. This comment will be updated once it is available. This usually takes a few minutes.
Test environment metadata:
Observability: |
|
Validation works for business key and input mappings when saving a process, but the feedback is in the console instead of to the user. |
ivo-ritense
added
tested
and removed
test env
3 participants
camunda:inthat is ignored becauseoperaton:inelements are present is detected and explained