Business rule task can define decisionId as expression #8822

korthout · 2022-02-21T19:01:56Z

Description

A business rule task with a called decision needs to specify a decisionId for the decision that is called. It should be possible to refer to a decision based on the evaluation result of an expression. So, it should be possible to define the decisionId as an expression. See #8779.

This adds support to define the decisionId of a zeebe:calledDecision as an expression (i.e. prefixed with =; e.g. = variableContainingDecisionId), while maintaining the ability to define it as a static value (i.e. not prefixed by =; e.g. an_actual_decision_id). For example, if the decisionId is an expression = variableContainingDecisionId, then the value of the variableContainingDecisionId variable should be used as the id of the decision that is called.

The decisionId expression is evaluated during the activation of the business rule task, just after input mapping and just before the decision is evaluated. This means you can use the result of input mappings in the decisionId expression.

If the evaluation fails, an incident of type EXTRACT_VALUE_ERROR is raised, similar to other expression evaluation failures. Resolving the incident will re-apply input mappings, re-evaluate the decisionId expression and then call the decision.

Related issues

closes #8779

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.

A CalledDecision with an expression as its decisionId should be able to call the decision based on the evaluation result of the expression. For example, if the decisionId is an expression `= decisionId`, then the value of the `decisionId` variable should be used as the id of the decision that is called. The test can verify that it works by asserting that a result variable has been created for the process instance. Note that no decision with id `=decisionIdVariable` exists, so the test fails until the evaluation result of the expression is used, instead of the static value.

An incident should be raised when the decision id expression fails to be evalutated successfully. The error type should be EXTRACT_VALUE_ERROR like the other expression evaluation failures. In addition, it should be possible to resolve the incident by removing the problem (i.e. set the missing variable in this case).

The decisionId will become a calculated value. To reuse it, lets first extract it into a local variable.

The decisionId is validated to be valid expression (or static value) by the ZeebeRuntimeValidators. So we can safely transform it to an expression by parsing it using the expression language. To keep the behavior the same, the BpmnDecisionBehavior needs to extract the original string representation of the expression. For static values, this is simply the static value again. For expressions it would be the entire expression in string representation. Since expressions are not yet evaluated, this keeps the logic the same from a user's perspective. But the decisionId is now additionally parsed to allow evaluation to be added later.

korthout · 2022-02-21T19:04:37Z

@remcowesterhoud Please have a look at this one as well 🙇 It's probably easiest to review this on a per-commit basis. Please let me know if anything is unclear.

remcowesterhoud

@korthout I have a small suggestions and some questions. It's all small stuf, for the rest it LGTM 👍

❓ I'm assuming the Feel engine will decide if it is actually an expression or not (i.e. starts with an =) and just returns the String if it does not start with an =?

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

...va/io/camunda/zeebe/engine/processing/deployment/model/element/ExecutableCalledDecision.java

...da/zeebe/engine/processing/deployment/model/transformer/zeebe/CalledDecisionTransformer.java

remcowesterhoud · 2022-02-22T15:09:37Z

engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/BpmnDecisionBehavior.java

-    final var decisionId = element.getDecisionId().getExpression();
+    final var decisionIdOrFailure = evalDecisionIdExpression(element, scopeKey);
+    if (decisionIdOrFailure.isLeft()) {
+      return decisionIdOrFailure.map(null);


❓ Why does this return a decisionIfOrFailure.map(null)? It seems like this is just a trick to fix types of the Either, correct?

Yes that is correct. The type of decisionIdOrFailure is Either<Failure, String> but the return type expects an Either<Failure, DecisionEvaluationResult>.

The only alternative I know is to wrap the left's value into a new left, but that's less functional. I this case it would be return Either.left(decisionIdOrFailure.getLeft());.

After having worked with Eithers for some time, I'm still not sure which I like better. My preference goes slightly to the map. The map looks strange at first but is really concise and accurate. We simply map the right side of a left to the correct type and null makes it clear that we don't know its value. But feel free to counter this. I'm also open to other ideas.

As someone who has barely worked with Either the map seems very weird. You're mapping something, but because it's a left you won't actually map anything. I would expect this code to throw a NPE at first since it seems like it applies the given function to something, but in reality the entire map() method doesn't do anything because it's a left. So I think my preference goes to Either.left(decisionIdOrFailure.getLeft()). Then again it's not a very strong preference since it's all documented quite well.

A separate method which would do this for us to make it more readable would be nice. But the problem is that it would then be on Either, and Right would need an implementation of it which is not really possible. It's a hard problem 😄

Also I do like the more functional approach

TBH, you're absolutely right. And it should be clear for first-time readers. I've changed it to the non-functional style.

I've also looked around our codebase for similar situations and found 2 cases that used Either.left(x.getLeft()) and 1 case that uses map. 😆 Nothing really helpful. This other map case can't be changed though, since it's really mapping a right as well. See https://github.com/camunda-cloud/zeebe/blob/main/engine/src/main/java/io/camunda/zeebe/engine/processing/common/CatchEventBehavior.java#L104

korthout · 2022-02-22T16:20:54Z

❓ I'm assuming the Feel engine will decide if it is actually an expression or not (i.e. starts with an =) and just returns the String if it does not start with an =?

Yes, that is correct. When you call the evaluateExpression on the expression language it executes some real simple logic that handles the case that the expression actually failed to parse, and immediately returns the static value as that was the result of parsing it. https://github.com/camunda-cloud/zeebe/blob/main/expression-language/src/main/java/io/camunda/zeebe/el/impl/FeelExpressionLanguage.java#L74-L85

❓ Should I add this knowledge to any of the specific commits?

remcowesterhoud · 2022-02-22T16:23:35Z

I don't think you need to add it to any commit. I fully expected this was the case, I just couldn't quite pinpoint where it happened. Thanks!

This adds evaluation of the decision id expression to the evaluateDecision logic of the BpmnDecisionBehavior. From now on, if a CalledDecision has a decisionId that is prefixed by the expression symbol (`=`), then the expression is evaluated. Otherwise, its used as a static value. If the decision evaluation fails, a Left<Failure, ?> is returned which can be used to raise an incident. This is already the case for the business rule task.

korthout · 2022-02-22T17:50:21Z

bors merge

ghost · 2022-02-22T18:17:36Z

Build succeeded:

korthout added 4 commits February 21, 2022 17:31

refactor: reuse decisionId local var

93ef852

The decisionId will become a calculated value. To reuse it, lets first extract it into a local variable.

remcowesterhoud self-requested a review February 22, 2022 14:08

remcowesterhoud approved these changes Feb 22, 2022

View reviewed changes

korthout force-pushed the korthout-8779-decision-id-expression branch from e73edc7 to d1f8c49 Compare February 22, 2022 16:58

ghost merged commit dfd0c9b into main Feb 22, 2022

ghost deleted the korthout-8779-decision-id-expression branch February 22, 2022 18:17

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Business rule task can define decisionId as expression #8822

Business rule task can define decisionId as expression #8822

korthout commented Feb 21, 2022

korthout commented Feb 21, 2022

remcowesterhoud left a comment •

edited

Loading

remcowesterhoud Feb 22, 2022

korthout Feb 22, 2022

remcowesterhoud Feb 22, 2022

remcowesterhoud Feb 22, 2022

korthout Feb 22, 2022

korthout commented Feb 22, 2022 •

edited

Loading

remcowesterhoud commented Feb 22, 2022

korthout commented Feb 22, 2022

ghost commented Feb 22, 2022

Business rule task can define decisionId as expression #8822

Business rule task can define decisionId as expression #8822

Conversation

korthout commented Feb 21, 2022

Description

Related issues

Definition of Done

korthout commented Feb 21, 2022

remcowesterhoud left a comment • edited Loading

Choose a reason for hiding this comment

remcowesterhoud Feb 22, 2022

Choose a reason for hiding this comment

korthout Feb 22, 2022

Choose a reason for hiding this comment

remcowesterhoud Feb 22, 2022

Choose a reason for hiding this comment

remcowesterhoud Feb 22, 2022

Choose a reason for hiding this comment

korthout Feb 22, 2022

Choose a reason for hiding this comment

korthout commented Feb 22, 2022 • edited Loading

remcowesterhoud commented Feb 22, 2022

korthout commented Feb 22, 2022

ghost commented Feb 22, 2022

remcowesterhoud left a comment •

edited

Loading

korthout commented Feb 22, 2022 •

edited

Loading