Business rule task can evaluate a DMN decision #8755

korthout · 2022-02-09T17:21:13Z

Description

This adds the ability to process business rule tasks that refer to a called decision (see #8060). Business rule tasks that refer to a task definition instead of a called decision work like they did before, i.e. by creating a job for job workers.

On activation of a business rule task with called decision, the decision specified by decisionId is retrieved from the deployed decisions state. If present, the engine uses the dmn module to evaluate the decision. If that is successful, the decision result's output is stored in a result variable (defined by resultVariable).

The result variable stores the decision result's output according to the following rules:

if the business rule task defines no output mappings, the result variable is propagated
if there are output mappings defined on the business rule task, the result variable is set as a local variable of the business rule task

If any of this fails, Zeebe raises an incident on the business rule task. For example, if the decision was not deployed yet, then an incident is raised. This allows you to:

deploy the decision to fix the problem
retry activating the business rule task by resolving the incident

Related issues

closes #8080

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually
The change has been verified by a QA run
The impact of the changes is verified by a benchmark

Documentation:

The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
New content is added to the release announcement
If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

korthout · 2022-02-09T17:51:42Z

🤔 It looks like the OS Smoke Tests and the Code QL fail for the same reason:

[INFO] -------------------------------------------------------------
Error:  COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
Error:  /home/runner/work/zeebe/zeebe/engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/BpmnDecisionBehavior.java:[13,28] cannot find symbol
  symbol:   class DecisionResult
  location: package io.camunda.zeebe.dmn
Error:  /home/runner/work/zeebe/zeebe/engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/BpmnDecisionBehavior.java:[59,26] cannot find symbol
  symbol:   class DecisionResult
  location: class io.camunda.zeebe.engine.processing.bpmn.behavior.BpmnDecisionBehavior
Error:  /home/runner/work/zeebe/zeebe/engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/BpmnDecisionBehavior.java:[123,27] cannot find symbol
  symbol:   class DecisionResult
  location: class io.camunda.zeebe.engine.processing.bpmn.behavior.BpmnDecisionBehavior
Error:  /home/runner/work/zeebe/zeebe/engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/BpmnDecisionBehavior.java:[138,13] cannot find symbol
  symbol:   class DecisionResult
  location: class io.camunda.zeebe.engine.processing.bpmn.behavior.BpmnDecisionBehavior
[INFO] 4 errors

This is strange, because the jenkins build does succeed. It looks like it is able to find the other classes from io.camunda.zeebe.dmn, but just not the DecisionResult. Perhaps this is a concurrency problem 🤔 . All those tests run: mvn ... -T1C, so perhaps the dmn jar is not installed locally before it tries to compile the workflow-engine. I'll have another look tomorrow.

EDIT: I've had another look. It isn't a problem with -T1C, but a merging issue. Both these GH checks run on a checkout of refs/remotes/pull/number/merge (in other words, on the merge between the head of this pr and the target). This differs from what I tested locally and from the jenkins pipeline, which simply run against the head of the pr. After merging main into this branch I saw the compile error locally as well. Seems that DecisionResult was renamed to DecisionEvaluationResult. I'll make sure to rebase.

Recently, we encountered compilation errors in the github checks: OS Smoke Tests and Code QL checks, while the Jenkins pipeline was able to complete succesfully. It seems that the difference might stem from the multi threadedness of these builds, i.e. the mvn command is ran with -T1C. See #8755 (comment) for an example of the compilation error.

The business rule task will work as both job worker task as well as a being able to call a decision, depending on how it's modeled. We'll need to modify its transformer slightly. This extracts the common parts (TaskDefinition and TaskHeaders transformers) and reuses them in a new BusinessRuleTaskTransformer

The zeebe transformers used slightly different implementation patterns, which leads to different call patterns. Should the tranformer be called if the specific extension element is not in the model? I think the validators should make sure that all model parts are defined as expected. The transformers should simply be able to deal with the case that it was optional. This matters, because we're going to change 1 of the tasks that currently always has a task definition but it will be optional after the future changes (i.e. business rule task). Therefore, I wanted to align the implementation of these classes so their usage can be kept the same over multiple elements that may contain these extension elements. Lastly, I chose to move the getSingleExtensionElement call to the caller side, to make it more clear at the caller side what this transformer does. This also makes it very easy to move the 'if element is null do nothing' check into the caller side, if we do happen to decide that the transformers should only be called when they have the element.

The business rule task is either a job worker task or it has a called decision. To represent this, the class should be able to be both: it is a job worker task and it can have a called decision. In reality only 1 of these should be present. This should at the minimum be checked by the validators, but we could also consider adding a safety check in this class. For now, I've left it out.

The business rule task can be either a job worker task or have a called decision. If it's a job worker task, we can simply reuse the existing JobWorkerTaskProcessor. If it has a decision to call, we can process it differently. This requires some specialised behavior classes, because we can't simply use something like: ``` private final BpmnElementProcessor<ExecutableJobWorkerTask> jobWorkerTaskBehavior; private final BpmnElementProcessor<ExecutableBusinessRuleTask> calledDecisionBehavior; ``` This would fail because at the calling site of the .onX methods we'll notice that the types don't match anymore. Instead, we can simply define a BusinessRuleTaskBehavior. We can implement it for the job worker task using delegation, while we can implement it independently for the called decision behavior.

This adds the failing tests for the called decision behavior when processing a business rule task that specifies a called decision id. These tests are inspired by the JobWorkerElementTest and follow a similar structure, with the major difference that instead of verifying the creation of a job, it verifies the writing of the result variable.

This adds the failing tests for the called decision behavior related to incident when processing a business rule task that specifies a called decision id. Specifically, it makes sure that failing to evaluate the decision leads to a resolvable incident. In addition, it makes sure that incidents due to output mappings behave the same for such business rule tasks as for service tasks. Small refactorings of the OutputMappingIncidentTest were necessary to deploy the dmn along with the process model.

This method can be used to easily convert an Optional into an Either. Optional provides an .orElse method to successfully retrieve a result from it, but this always has to be the same type as the Optional's type parameter. In the case of Either, we want to be able to express the 'other' value (i.e. left) with a potentially different type. Users are already used to using .orElse in the context of Optionals. When converting an Optional into an Either, it then makes sense to provide a similarly named method to supply the left value.

This introduces a new BpmnDecisionBehavior class to provide decision behavior to the bpmn processors, and it is immediately used in the BusinessRuleTaskProcessor with called decision behavior. The implementatino is pretty straightforward. It needs to: - find the drg in the state (this has some potential failure cases) - evaluate the decision in the drg (this can also fail) - trigger a process event with the result variable if successfully evaluated. The process event triggering record is used to accomodate the async nature of evaluating the decision on element activations, while the result variable is only used on completino of the element. That is, the output mapping behavior uses it to determine whether it will propagate it as a process variable, or write it as a local variable and use it in the output mappings. This combined with the potential to raise incidents as part of the output mappings behavior, means that the value must be available in state and thus on record. The event trigger that this creates is cleaned up by the ProcessInstanceElementCompletedApplier as the normal part of cleanup involved with the element being completed. All failure cases mentioned above result in an incident on the business rule task during while activating, with the exception of the incidents raised during output mapping. Lastly, it felt a bit cleaner to provide a smaller interface to the BpmnDecisionBehavior.evaluateDecision method. So, I extracted the ExecutableCalledDecision interface from the ExecutableBusinessRuleTask.

korthout · 2022-02-11T15:56:25Z

Manually tested in Camunda Cloud SaaS (by using the QA run to let testbench create a new tmp generation based on this code and quickly using this generation to create a personal cluster).

Here's a screenshot of a business rule task with called decision that calls the jedi_or_sith decision from the drg-force-user.dmn test resource. It requires a lightsaberColor variable. Its result variable is set to result.

Next to this, I've also manually tested:

using a decision id for which no decisions are deployed:
- correctly produced an incident
not passing in one of the required variables:
- correctly produced an incident
- incident could be resolved after adding the variable to the process instance
using an input mapping
- correctly passes the input mapping's result to the decision
using an output mapping
- correctly makes the result variable into a local variable
- correctly uses the local result variable in the output mapping

saig0

@korthout well done 🥇

The code looks simple and clean. Thanks for splitting the PR into the commits and adding the descriptions 👍

I just have some minor suggestions. Feel free to merge 🚀

saig0 · 2022-02-14T06:01:55Z

.../io/camunda/zeebe/engine/processing/deployment/model/element/ExecutableBusinessRuleTask.java

+
+public final class ExecutableBusinessRuleTask extends ExecutableJobWorkerTask {
+
+  private String decisionId;


ℹ️ I created a follow-up issue to support an expression as decision id #8779.

...ne/src/main/java/io/camunda/zeebe/engine/processing/bpmn/task/BusinessRuleTaskProcessor.java

engine/src/test/java/io/camunda/zeebe/engine/processing/bpmn/activity/BusinessRuleTaskTest.java

saig0 · 2022-02-14T07:45:13Z

.../src/test/java/io/camunda/zeebe/engine/processing/incident/BusinessRuleTaskIncidentTest.java

+            """
+            Expected to evaluate decision 'jedi-or-sith', \
+            but failed to evaluate expression 'lightsaberColor': \
+            no variable found for name 'lightsaberColor'\
+            """);


Nice usage of the new multi-line strings 🤩

engine/src/test/java/io/camunda/zeebe/engine/processing/incident/OutputMappingIncidentTest.java

saig0 · 2022-02-14T07:55:51Z

util/src/main/java/io/camunda/zeebe/util/Either.java

+
+    private final Optional<R> right;
+
+    public EitherOptional(final Optional<R> right) {


🔧 We could change the visibility modifier to private.

We don't expect that the EitherOptional class is created directly.

@saig0 We also turn it into a record.

record EitherOptional<R>(Optional<R> right) { public <L> Either<L, R> orElse(final L left) { return right.<Either<L, R>>map(Either::right).orElse(Either.left(left)); } }

In this case, you have an extra method available after using Either.ofOptional that allows the caller to retrieve the wrapped Optional:

WDYT?

I will already merge, so feel free to answer this any time that suits you and we can always give it a try later.

Using the BpmnProcessingException we can easily print the related context. This can be useful during debugging.

This was a copy-pasta mistake.

These test cases used the same scenario and could easily be merged.

This makes for a cleaner test.

The constructor should only be used by the Either class itself.

korthout · 2022-02-14T14:59:31Z

bors merge

8755: Business rule task can evaluate a DMN decision r=korthout a=korthout ## Description This adds the ability to process business rule tasks that refer to a called decision (see #8060). Business rule tasks that refer to a [task definition](https://docs.camunda.io/docs/components/modeler/bpmn/business-rule-tasks/business-rule-tasks/#defining-a-task) instead of a called decision work like they did before, i.e. by creating a job for job workers. On activation of a business rule task with called decision, the decision specified by `decisionId` is retrieved from the deployed decisions state. If present, the engine uses the `dmn` module to evaluate the decision. If that is successful, the decision result's output is stored in a result variable (defined by `resultVariable`). The result variable stores the decision result's output according to the following rules: - if the business rule task defines no output mappings, the result variable is [propagated](https://docs.camunda.io/docs/components/concepts/variables/#variable-propagation) - if there are [output mappings](https://docs.camunda.io/docs/components/concepts/variables/#output-mappings) defined on the business rule task, the result variable is set as a [local variable](https://docs.camunda.io/docs/components/concepts/variables/#local-variables) of the business rule task If any of this fails, Zeebe raises an incident on the business rule task. For example, if the decision was not deployed yet, then an incident is raised. This allows you to: 1. deploy the decision to fix the problem 2. retry activating the business rule task by resolving the incident ## Related issues  closes #8080 Co-authored-by: Nico Korthout <nico.korthout@camunda.com>

ghost · 2022-02-14T15:28:22Z

Build failed:

continuous-integration/jenkins/branch

korthout · 2022-02-14T16:21:54Z

bors retry

8755: Business rule task can evaluate a DMN decision r=korthout a=korthout ## Description This adds the ability to process business rule tasks that refer to a called decision (see #8060). Business rule tasks that refer to a [task definition](https://docs.camunda.io/docs/components/modeler/bpmn/business-rule-tasks/business-rule-tasks/#defining-a-task) instead of a called decision work like they did before, i.e. by creating a job for job workers. On activation of a business rule task with called decision, the decision specified by `decisionId` is retrieved from the deployed decisions state. If present, the engine uses the `dmn` module to evaluate the decision. If that is successful, the decision result's output is stored in a result variable (defined by `resultVariable`). The result variable stores the decision result's output according to the following rules: - if the business rule task defines no output mappings, the result variable is [propagated](https://docs.camunda.io/docs/components/concepts/variables/#variable-propagation) - if there are [output mappings](https://docs.camunda.io/docs/components/concepts/variables/#output-mappings) defined on the business rule task, the result variable is set as a [local variable](https://docs.camunda.io/docs/components/concepts/variables/#local-variables) of the business rule task If any of this fails, Zeebe raises an incident on the business rule task. For example, if the decision was not deployed yet, then an incident is raised. This allows you to: 1. deploy the decision to fix the problem 2. retry activating the business rule task by resolving the incident ## Related issues  closes #8080 Co-authored-by: Nico Korthout <nico.korthout@camunda.com>

ghost · 2022-02-14T16:36:36Z

Build failed:

continuous-integration/jenkins/branch

korthout · 2022-02-14T16:58:20Z

Build failed:

* [continuous-integration/jenkins/branch](https://ci.zeebe.camunda.cloud/job/camunda-cloud/job/zeebe/job/staging/1819/display/redirect)

Ugh, aborted due to connection error.

bors merge

ghost · 2022-02-14T17:23:30Z

Build succeeded:

korthout requested a review from saig0 February 10, 2022 10:30

korthout added 8 commits February 10, 2022 13:52

korthout force-pushed the korthout-8080-business-rule-task-call-decision branch from 9005ee8 to 1af80b8 Compare February 10, 2022 13:21

saig0 approved these changes Feb 14, 2022

View reviewed changes

korthout added 6 commits February 14, 2022 15:12

fix(engine): add context to exception

06fc19c

Using the BpmnProcessingException we can easily print the related context. This can be useful during debugging.

docs: fix incorrect wording in javadoc

14dcc46

This was a copy-pasta mistake.

test: merge similar cases

19c7c0e

These test cases used the same scenario and could easily be merged.

test: extract dmn resource as constant

8f801a9

test: replace set-variable with input mapping

e9bad89

This makes for a cleaner test.

refactor(util): weaken EitherOptional const visibility

3e5f789

The constructor should only be used by the Either class itself.

ghost merged commit 926d47e into main Feb 14, 2022

ghost deleted the korthout-8080-business-rule-task-call-decision branch February 14, 2022 17:23

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Business rule task can evaluate a DMN decision #8755

Business rule task can evaluate a DMN decision #8755

korthout commented Feb 9, 2022 •

edited

Loading

korthout commented Feb 9, 2022 •

edited

Loading

korthout commented Feb 11, 2022

saig0 left a comment

saig0 Feb 14, 2022

saig0 Feb 14, 2022

saig0 Feb 14, 2022

korthout Feb 14, 2022 •

edited

Loading

korthout commented Feb 14, 2022

ghost commented Feb 14, 2022

korthout commented Feb 14, 2022

ghost commented Feb 14, 2022

korthout commented Feb 14, 2022

ghost commented Feb 14, 2022


		public final class ExecutableBusinessRuleTask extends ExecutableJobWorkerTask {

		private String decisionId;


		private final Optional<R> right;

		public EitherOptional(final Optional<R> right) {

Business rule task can evaluate a DMN decision #8755

Business rule task can evaluate a DMN decision #8755

Conversation

korthout commented Feb 9, 2022 • edited Loading

Description

Related issues

Definition of Done

korthout commented Feb 9, 2022 • edited Loading

korthout commented Feb 11, 2022

saig0 left a comment

Choose a reason for hiding this comment

saig0 Feb 14, 2022

Choose a reason for hiding this comment

saig0 Feb 14, 2022

Choose a reason for hiding this comment

saig0 Feb 14, 2022

Choose a reason for hiding this comment

korthout Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

korthout commented Feb 14, 2022

ghost commented Feb 14, 2022

korthout commented Feb 14, 2022

ghost commented Feb 14, 2022

korthout commented Feb 14, 2022

ghost commented Feb 14, 2022

korthout commented Feb 9, 2022 •

edited

Loading

korthout commented Feb 9, 2022 •

edited

Loading

korthout Feb 14, 2022 •

edited

Loading