-
Notifications
You must be signed in to change notification settings - Fork 571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support decision table inputs and outputs without names/labels #8919
Conversation
Manually tested to see if it could be successfully exported to ES: {
"_index": "zeebe-record_decision-evaluation_1.4.0-snapshot_2022-03-16",
"_type": "_doc",
"_id": "1-50",
"_version": 1,
"_score": 1,
"_routing": "1",
"_source": {
"partitionId": 1,
"value": {
"decisionVersion": 5,
"decisionOutput": "\"Obi-Wan Kenobi\"",
"evaluatedDecisions": [
{
"decisionType": "DECISION_TABLE",
"evaluatedInputs": [
{
"inputId": "Input_1",
"inputName": "",
"inputValue": "\"blue\""
}
],
"decisionVersion": 4,
"decisionOutput": "\"Jedi\"",
"matchedRules": [
{
"ruleId": "DecisionRule_0zumznl",
"ruleIndex": 1,
"evaluatedOutputs": [
{
"outputId": "Output_1",
"outputName": "",
"outputValue": "\"Jedi\""
}
]
}
],
"decisionId": "jedi_or_sith",
"decisionName": "Jedi or Sith",
"decisionKey": 2251799813685265
},
{
"decisionType": "DECISION_TABLE",
"evaluatedInputs": [
{
"inputId": "InputClause_0qnqj25",
"inputName": "",
"inputValue": "\"Jedi\""
},
{
"inputId": "InputClause_0k64hys",
"inputName": "",
"inputValue": "185"
}
],
"decisionVersion": 5,
"decisionOutput": "\"Obi-Wan Kenobi\"",
"matchedRules": [
{
"ruleId": "DecisionRule_0uin2hk",
"ruleIndex": 2,
"evaluatedOutputs": [
{
"outputId": "OutputClause_0hhe1yo",
"outputName": "",
"outputValue": "\"Obi-Wan Kenobi\""
}
]
}
],
"decisionId": "force_user",
"decisionName": "Which force user?",
"decisionKey": 2251799813685266
}
],
"evaluationFailureMessage": "",
"failedDecisionId": "",
"decisionRequirementsId": "force_users",
"decisionRequirementsKey": 2251799813685264,
"decisionId": "force_user",
"decisionName": "Which force user?",
"decisionKey": 2251799813685266,
"bpmnProcessId": "force-user",
"processDefinitionKey": 2251799813685268,
"processInstanceKey": 2251799813685270,
"elementInstanceKey": 2251799813685274,
"elementId": "Activity_1sl5qf7"
},
"key": 2251799813685277,
"timestamp": 1647429371592,
"sourceRecordPosition": 46,
"intent": "EVALUATED",
"rejectionType": "NULL_VAL",
"rejectionReason": "",
"brokerVersion": "1.4.0",
"valueType": "DECISION_EVALUATION",
"recordType": "EVENT",
"position": 50
},
"fields": {
"value.decisionKey": [
2251799813685266
],
"value.evaluatedDecisions.decisionOutput": [
"\"Jedi\"",
"\"Obi-Wan Kenobi\""
],
"value.evaluatedDecisions.decisionVersion": [
4,
5
],
"sourceRecordPosition": [
46
],
"value.decisionRequirementsId": [
"force_users"
],
"value.evaluatedDecisions.decisionKey": [
2251799813685265,
2251799813685266
],
"value.evaluatedDecisions.evaluatedInputs.inputName": [
"",
"",
""
],
"rejectionType": [
"NULL_VAL"
],
"value.evaluatedDecisions.matchedRules.evaluatedOutputs.outputValue": [
"\"Jedi\"",
"\"Obi-Wan Kenobi\""
],
"value.decisionVersion": [
5
],
"value.evaluatedDecisions.evaluatedInputs.inputId": [
"Input_1",
"InputClause_0qnqj25",
"InputClause_0k64hys"
],
"valueType": [
"DECISION_EVALUATION"
],
"value.decisionId": [
"force_user"
],
"value.elementId": [
"Activity_1sl5qf7"
],
"key": [
2251799813685277
],
"timestamp": [
"2022-03-16T11:16:11.592Z"
],
"value.processInstanceKey": [
2251799813685270
],
"value.evaluatedDecisions.evaluatedInputs.inputValue": [
"\"blue\"",
"\"Jedi\"",
"185"
],
"value.elementInstanceKey": [
2251799813685274
],
"value.decisionRequirementsKey": [
2251799813685264
],
"value.evaluatedDecisions.matchedRules.ruleId": [
"DecisionRule_0zumznl",
"DecisionRule_0uin2hk"
],
"value.decisionOutput": [
"\"Obi-Wan Kenobi\""
],
"value.processDefinitionKey": [
2251799813685268
],
"partitionId": [
1
],
"value.evaluatedDecisions.matchedRules.evaluatedOutputs.outputName": [
"",
""
],
"recordType": [
"EVENT"
],
"value.decisionName": [
"Which force user?"
],
"value.failedDecisionId": [
""
],
"brokerVersion": [
"1.4.0"
],
"intent": [
"EVALUATED"
],
"value.evaluatedDecisions.matchedRules.evaluatedOutputs.outputId": [
"Output_1",
"OutputClause_0hhe1yo"
],
"value.bpmnProcessId": [
"force-user"
],
"value.evaluatedDecisions.decisionName": [
"Jedi or Sith",
"Which force user?"
],
"value.evaluatedDecisions.matchedRules.ruleIndex": [
1,
2
],
"value.evaluatedDecisions.decisionId": [
"jedi_or_sith",
"force_user"
],
"value.evaluatedDecisions.decisionType": [
"DECISION_TABLE",
"DECISION_TABLE"
],
"value.evaluationFailureMessage": [
""
],
"position": [
50
],
"rejectionReason": [
""
]
}
} |
Hiya @pihme, would you be willing to review this PR? If you want I can provide you with an intro to DMN in Zeebe. It's also fine if you'd rather someone else review. There's no need to rush this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving comments; will continue after we had a chance to talk about it
engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/BpmnDecisionBehavior.java
Show resolved
Hide resolved
engine/src/test/java/io/camunda/zeebe/engine/processing/bpmn/activity/BusinessRuleTaskTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🏷️
I'm postponing merging this until it is clear whether Operate needs Zeebe to provide input/output names in the same way Modeler shows them. |
A decision tables' inputs and outputs must have an id and can have a name. The name (or label) is not mandatory. This adds a failing test case that deploys a version of the jedi_or_sith decision where all inputis and outputs no longer have names or labels. The evaluated decision result should still be written to the log as normal, but with an empty string as name.
This was highlighted by IntelliJ.
The decision engine can evaluate decisions where the inputs and outputs of a decision table don't have names (or labels). In that case the name property is null. We can't write null in records, instead we should write an empty string "". See #8909
The assertions were very verbose and hid what was really relevant for this test. By only asserting that inputs and outputs must be present and have empty names we makes sure the test is much more focussed.
This method needs to be used in the upcoming decision table tests. These tests will verify that a decision table with inputs that are labeled or not have a different name in the evaluted input. (and likewise will test similar behavior for outputs). These tests will need to be able to evaluate decisions and it makes sense to just reuse this logic.
If the decision fails to evalute successfully, we should print the reason along with the assertion error.
Decision tables have some special logic when it comes to the name of the evaluated input/outputs. These tests work pretty simple, and similar to the existing DecisionTypeTests. Simply put, it refers to a different decision table for each test and then verifies something on the evaluted decision.
The inputs of decision tables can have an input, but these are not mandatory. If it's missing, the Modeler shows the expression (if filled in). Operate would like to have the same behavior as the Modeler in showcasing it. Zeebe can help Operate by deciding how to store the input name. This way Opeate can directly use the 'correct' name without having to parse the XML of the DMN model itself. This also makes the recordstream readable in a similar fashion.
The outputs of decision tables can have a name and can have a label, according to the following rules: - label is optional - name is optional, unless the table has multiple outputs The Modeler and Operate only need 1 name to display, but somehow we should decide which one: the label or the name. The Modeler favors the label if it is available it will be used, even if a name is specified. Zeebe should use this same behavior to decide which name to store in the EvaluatedOutput of a DecisionEvaluation. This way Operate can directly use the 'correct' name without having to parse the XML of the DMN model itself.
4fa9d98
to
e5dfa0f
Compare
The behavior of the evaluted input/output's name is now different.
@pihme To accommodate Operate, Zeebe now applies some logic to determine the name of evaluated inputs/outputs. Please have another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Looks good to me!
(One optional suggestion)
dmn/src/main/java/io/camunda/zeebe/dmn/impl/EvaluatedDmnScalaInput.java
Outdated
Show resolved
Hide resolved
To avoid getting really long expressions as names, let's truncate them at an arbitrary number of 30 chars. There's no specific reason for 30 over any other number. It's good enough for now, we can change it later when requested.
bors merge |
Canceled. |
bors merge |
8919: Support decision table inputs and outputs without names/labels r=korthout a=korthout ## Description <!-- Please explain the changes you made here. --> A decision tables' inputs and outputs must have an id and can have a name. The name (or label) is not mandatory. Even though the internal decision engine was already able to deal with this, the workflow engine wasn't. It would try to write the EvaluationEvent record with an EvaluatedInputRecord (or EvaluatedOutputRecord) and then fail writing it because of a NullPointerException. This makes sure these records can be written when the name of the input or output is undefined. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #8909 Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
Timed out. |
bors merge |
8919: Support decision table inputs and outputs without names/labels r=korthout a=korthout ## Description <!-- Please explain the changes you made here. --> A decision tables' inputs and outputs must have an id and can have a name. The name (or label) is not mandatory. Even though the internal decision engine was already able to deal with this, the workflow engine wasn't. It would try to write the EvaluationEvent record with an EvaluatedInputRecord (or EvaluatedOutputRecord) and then fail writing it because of a NullPointerException. This makes sure these records can be written when the name of the input or output is undefined. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #8909 8939: refactor: use foreign keys for decisions r=oleschoenburg a=oleschoenburg ## Description Uses `DbForeignKey` internally to maintain referential integrity within `DbDecisionState` relates to #8930 Co-authored-by: Nico Korthout <nico.korthout@camunda.com> Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
Build failed (retrying...): |
8919: Support decision table inputs and outputs without names/labels r=korthout a=korthout ## Description <!-- Please explain the changes you made here. --> A decision tables' inputs and outputs must have an id and can have a name. The name (or label) is not mandatory. Even though the internal decision engine was already able to deal with this, the workflow engine wasn't. It would try to write the EvaluationEvent record with an EvaluatedInputRecord (or EvaluatedOutputRecord) and then fail writing it because of a NullPointerException. This makes sure these records can be written when the name of the input or output is undefined. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #8909 Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
Build failed: |
Bors merge |
8919: Support decision table inputs and outputs without names/labels r=korthout a=korthout ## Description <!-- Please explain the changes you made here. --> A decision tables' inputs and outputs must have an id and can have a name. The name (or label) is not mandatory. Even though the internal decision engine was already able to deal with this, the workflow engine wasn't. It would try to write the EvaluationEvent record with an EvaluatedInputRecord (or EvaluatedOutputRecord) and then fail writing it because of a NullPointerException. This makes sure these records can be written when the name of the input or output is undefined. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #8909 Co-authored-by: Nico Korthout <nico.korthout@camunda.com>
Build failed: |
If a label is defined for an output, it will be used as the output name in the evaluted output, even if it already has a defined name. This is in line with the Modeler behavior.
Inputs always have a name, because if the label is missing they always still have an expression that was evaluted that can be used as name. No need to test for this anymore.
@pihme It seems there were some tests failing. I've adjusted the tests and updated the PR description to make it clear what this PR changes. Please have again another look 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
bors merge |
Description
A decision tables' inputs and outputs must have an id and can have a name. What we define as a name differs between inputs and outputs. For inputs, we used only the label from the XML which is not mandatory. For outputs, we used only the name, but it is only mandatory when multiple outputs exist.
Even though the internal decision engine was already able to deal with this, the workflow engine wasn't. It would try to write the EvaluationEvent record with an EvaluatedInputRecord (or EvaluatedOutputRecord) and then fail writing it because of a NullPointerException.
After discussing with the Operate team (who uses this name value), we came to the conclusion that we want the same behavior as the Modeler:
This PR adjusts which values are used as the names for inputs and outputs of evaluated decision tables. It also makes sure these records can be written when the name of the input or output is undefined.
Related issues
closes #8909
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation: