Skip to content
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.

Sign up for GitHub

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

FEEL variable scope resolution #43

Merged
merged 7 commits into from
Jan 23, 2025
+334 −69
Merged

FEEL variable scope resolution #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
63ab591
chore(refactor): remove element after current def into separate function
abdul99ahad Jan 11, 2025
6df0756
test: adjust previous tests
abdul99ahad Jan 20, 2025
fa38b90
chore(deps): update extract-process-variables deps
abdul99ahad Jan 11, 2025
ab8dce1
fix: resolve scope priority for variables with the same name
abdul99ahad Jan 11, 2025
ef66905
fix: latest io mapping overwrite when duplicates
abdul99ahad Jan 11, 2025
ded973e
chore: code style
philippfromme Jan 23, 2025
3aafef6
chore: simplify tests
philippfromme Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/base/VariableResolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { getBusinessObject, is } from 'bpmn-js/lib/util/ModelUtil';
import CachedValue from './util/CachedValue';
import { getParents, getScope } from './util/scopeUtil';
import { uniqueBy } from 'min-dash';

/**
* @typedef {Object} AdditionalVariable
Expand Down Expand Up @@ -257,15 +258,18 @@ export class BaseVariableResolver {
const root = getRootElement(bo);
const allVariables = await this.getProcessVariables(root);

// keep only unique variables based on name property
const uniqueVariables = uniqueBy('name', allVariables.reverse());

// (1) get variables for given scope
var scopeVariables = allVariables.filter(function(variable) {
var scopeVariables = uniqueVariables.filter(function(variable) {
return variable.scope.id === bo.id;
});

// (2) get variables for parent scopes
var parents = getParents(bo);

var parentsScopeVariables = allVariables.filter(function(variable) {
var parentsScopeVariables = uniqueVariables.filter(function(variable) {
return parents.find(function(parent) {
return parent.id === variable.scope.id;
});
Expand Down
42 changes: 5 additions & 37 deletions lib/zeebe/VariableResolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { getProcessVariables } from '@bpmn-io/extract-process-variables/zeebe';
import { BaseVariableResolver } from '../base/VariableResolver';
import { parseVariables } from './util/feelUtility';
import {
parseVariables,
getElementNamesToRemove
} from './util/feelUtility';
import {
getBusinessObject,
is
Expand Down Expand Up @@ -36,42 +39,7 @@ export default class ZeebeVariableResolver extends BaseVariableResolver {
return variables;
}

const namesToFilter = [];

// Input: remove all inputs defined after the current input definition
if (is(moddleElement, 'zeebe:Input')) {
const allInputs = inputOutput.inputParameters;

const inputsToFilter =
allInputs
.slice(allInputs.indexOf(moddleElement))
.map(o => o.target);

namesToFilter.push(...inputsToFilter);
}

const allOutputs = inputOutput.outputParameters;

// Output: remove all outputs defined after the current output definition
if (is(moddleElement, 'zeebe:Output')) {

// Get all output mappings defined after the current element, including own name
const outputsToFilter = allOutputs
.slice(allOutputs.indexOf(moddleElement))
.map(o => o.target);

namesToFilter.push(...outputsToFilter);
}

// Input or general property: remove all outputs
else if (allOutputs) {

// Input or execution-related element, remove all outputs
const outputsToFilter = allOutputs
.map(o => o.target);

namesToFilter.push(...outputsToFilter);
}
const namesToFilter = getElementNamesToRemove(moddleElement, inputOutput);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with other commit in this PR, what problem does this fix solve? I'd apprechiate if we'd pack implementation + test case together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a fix, just a refactor to clean up the code into a separate function. I'll make sure the commits are clear for reviewers.


return variables.filter(v => {

Expand Down
56 changes: 53 additions & 3 deletions lib/zeebe/util/feelUtility.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
import { EntriesContext } from './VariableContext';
import { getExtensionElementsList } from '../../base/util/ExtensionElementsUtil';
import { getParents } from '../../base/util/scopeUtil';
import { is } from 'bpmn-js/lib/util/ModelUtil';

export function parseVariables(variables) {

Expand Down Expand Up @@ -158,15 +159,20 @@ export function getResultContext(expression, variables = {}) {
* @returns {{ expression: String, unresolved: Array<String> }}}
*/
function getExpressionDetails(variable, origin) {
const expression = getIoExpression(variable, origin) || getScriptExpression(variable, origin);

// if variable scope is !== origin (global), prioritize IoExpression over ScriptExpression
// if variable scope is === origin (local), prioritize ScriptExpression over IoExpression
const expression = variable.scope !== origin
? getIoExpression(variable, origin) || getScriptExpression(variable, origin)
: getScriptExpression(variable, origin) || getIoExpression(variable, origin);

if (!expression) {
return;
}

const result = getResultContext(expression);

const unresolved = findUnresolvedVariables(result) ;
const unresolved = findUnresolvedVariables(result);

return { expression, unresolved };
}
Expand Down Expand Up @@ -197,7 +203,7 @@ function getIoExpression(variable, origin) {
return;
}

const mapping = mappings.find(mapping => mapping.target === variable.name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd apprechiate if we'd group the fix with the actual test that verifies the fix. This way it is easier to understand what we fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix corresponds to

When multiple input or output variables with same name exists within the same process/activity, the later one overwrites all the previous ones.

I'll see if the test case doesn't exist for this, I'll add one. Thanks for pointing this out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also search mappings from last to first:

mappings.reverse().find(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason we're sticking to find(...) and avoid filter(...).pop() ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Find aborts early, while filter goes through the whole list. But in the end it is a matter of taste.

pop() is also not side-effect free, so it is not fully functional, and may cause (in some situations) issues.

const mapping = mappings.slice().reverse().find(mapping => mapping.target === variable.name);

if (!mapping || !mapping.source) {
return;
Expand Down Expand Up @@ -374,4 +380,48 @@ function filterForScope(context, variable) {
}

return scopedResults;
}

/**
* Remove input/output element name after current definition
*/
export function getElementNamesToRemove(moddleElement, inputOutput) {
const namesToFilter = [];

// Input: remove all inputs defined after the current input definition
if (is(moddleElement, 'zeebe:Input')) {
const allInputs = inputOutput.inputParameters;

const inputsToFilter =
allInputs
.slice(allInputs.indexOf(moddleElement))
.map(o => o.target);

namesToFilter.push(...inputsToFilter);
}

const allOutputs = inputOutput.outputParameters;

// Output: remove all outputs defined after the current output definition
if (is(moddleElement, 'zeebe:Output')) {

// Get all output mappings defined after the current element, including own name
const outputsToFilter = allOutputs
.slice(allOutputs.indexOf(moddleElement))
.map(o => o.target);

namesToFilter.push(...outputsToFilter);
}

// Input or general property: remove all outputs
else if (allOutputs) {

// Input or execution-related element, remove all outputs
const outputsToFilter = allOutputs
.map(o => o.target);

namesToFilter.push(...outputsToFilter);
}

return namesToFilter;
}
15 changes: 8 additions & 7 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"author": "bpmn.io contributors",
"license": "MIT",
"dependencies": {
"@bpmn-io/extract-process-variables": "^1.0.0",
"@bpmn-io/extract-process-variables": "^1.0.1",
"@lezer/common": "^1.2.3",
"lezer-feel": "^1.4.0",
"min-dash": "^4.2.2"
Expand Down
143 changes: 132 additions & 11 deletions test/fixtures/zeebe/mappings/scope.bpmn
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,151 @@
<?xml version="1.0" encoding="UTF-8"?>
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" xmlns:zeebe="http://camunda.org/schema/zeebe/1.0" xmlns:modeler="http://camunda.org/schema/modeler/1.0" id="Definitions_1noegy9" targetNamespace="http://bpmn.io/schema/bpmn" exporter="Camunda Modeler" exporterVersion="5.6.0-rc.1" modeler:executionPlatform="Camunda Cloud" modeler:executionPlatformVersion="8.1.0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit contains a lot of changes to the XML with no corresponding tests. Furthermore, some of the elements you added don't seem to be used in any tests later on (e.g. Participant_2) please help me out here and ensure that every commit bundles the tests and the changes to the XML that are necessary.

Copy link
Contributor Author

@abdul99ahad abdul99ahad Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have combined commits and fixes later that's why this XML is still intact as a separate commit as I was finding it difficult to combine it. But I'll see what I can do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have combined xml commits with respective fixes and tests.

<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" xmlns:zeebe="http://camunda.org/schema/zeebe/1.0" xmlns:modeler="http://camunda.org/schema/modeler/1.0" id="Definitions_1noegy9" targetNamespace="http://bpmn.io/schema/bpmn" exporter="Camunda Modeler" exporterVersion="5.31.0" modeler:executionPlatform="Camunda Cloud" modeler:executionPlatformVersion="8.1.0">
<bpmn:collaboration id="Collaboration_0e60zir">
<bpmn:participant id="Participant_1" name="Participant_1" processRef="Process_1" />
<bpmn:participant id="Participant_2" name="Participant_2" processRef="Process_2" />
<bpmn:participant id="Participant_3" name="Participant_3" processRef="Process_3" />
<bpmn:participant id="Participant_4" name="Participant_4" processRef="Process_4" />
<bpmn:participant id="Participant_5" name="Participant_5" processRef="Process_5" />
<bpmn:participant id="Participant_6" name="Participant_6" processRef="Process_6" />
<bpmn:participant id="Participant_7" name="Participant_7" processRef="Process_7" />
</bpmn:collaboration>
<bpmn:process id="Process_1" isExecutable="true">
<bpmn:serviceTask id="scopedContext" name="Scoped Context">
<bpmn:serviceTask id="ServiceTask_1" name="Input &#38; output in scope">
<bpmn:extensionElements>
<zeebe:ioMapping>
<zeebe:input source="=globalVariable" target="scopedInput" />
<zeebe:output source="=scopedInput" target="validMapping" />
<zeebe:input source="=globalVariable" target="fooInputVariable" />
<zeebe:output source="=fooInputVariable" target="fooOutputVariable" />
</zeebe:ioMapping>
</bpmn:extensionElements>
</bpmn:serviceTask>
<bpmn:serviceTask id="Activity_0kafybg">
<bpmn:serviceTask id="ServiceTask_2" name="Input out of scope">
<bpmn:extensionElements>
<zeebe:ioMapping>
<zeebe:output source="=scopedInput" target="invalidMapping" />
<zeebe:output source="=fooInputVariable" target="barOutputVariable" />
</zeebe:ioMapping>
</bpmn:extensionElements>
</bpmn:serviceTask>
</bpmn:process>
<bpmn:process id="Process_2" isExecutable="false">
<bpmn:scriptTask id="ScriptTask_1" name="Result variable &#38; input with same name">
<bpmn:extensionElements>
<zeebe:script expression="=&#34;1&#34;" resultVariable="foo" />
<zeebe:ioMapping>
<zeebe:input source="=&#34;2&#34;" target="foo" />
<zeebe:output source="=&#34;3&#34;" target="output" />
</zeebe:ioMapping>
</bpmn:extensionElements>
</bpmn:scriptTask>
</bpmn:process>
<bpmn:process id="Process_3" isExecutable="false">
<bpmn:scriptTask id="ScriptTask_2" name="Result variable &#38; outputs">
<bpmn:extensionElements>
<zeebe:script expression="=&#34;1&#34;" resultVariable="resultVariable" />
<zeebe:ioMapping>
<zeebe:output source="=&#34;2&#34;" target="output" />
</zeebe:ioMapping>
</bpmn:extensionElements>
</bpmn:scriptTask>
</bpmn:process>
<bpmn:process id="Process_4" isExecutable="false">
<bpmn:scriptTask id="ScriptTask_3" name="Result variable &#38; no outputs">
<bpmn:extensionElements>
<zeebe:script expression="=&#34;1&#34;" resultVariable="resultVariable" />
</bpmn:extensionElements>
</bpmn:scriptTask>
</bpmn:process>
<bpmn:process id="Process_5" isExecutable="false">
<bpmn:scriptTask id="ScriptTask_4" name="Result variable &#38; output with same name">
<bpmn:extensionElements>
<zeebe:script expression="=&#34;1&#34;" resultVariable="foo" />
<zeebe:ioMapping>
<zeebe:output source="=&#34;2&#34;" target="foo" />
<zeebe:output source="=&#34;3&#34;" target="output" />
</zeebe:ioMapping>
</bpmn:extensionElements>
</bpmn:scriptTask>
</bpmn:process>
<bpmn:process id="Process_6" isExecutable="false">
<bpmn:serviceTask id="ServiceTask_3" name="Input &#38; output with same name">
<bpmn:extensionElements>
<zeebe:ioMapping>
<zeebe:input source="=&#34;1&#34;" target="foo" />
<zeebe:output source="=&#34;2&#34;" target="foo" />
<zeebe:output source="=&#34;3&#34;" target="output" />
</zeebe:ioMapping>
</bpmn:extensionElements>
</bpmn:serviceTask>
</bpmn:process>
<bpmn:process id="Process_7" isExecutable="false">
<bpmn:serviceTask id="ServiceTask_4" name="Inputs with same name">
<bpmn:extensionElements>
<zeebe:ioMapping>
<zeebe:input source="=&#34;1&#34;" target="foo" />
<zeebe:input source="=&#34;2&#34;" target="foo" />
</zeebe:ioMapping>
</bpmn:extensionElements>
</bpmn:serviceTask>
</bpmn:process>
<bpmndi:BPMNDiagram id="BPMNDiagram_1">
<bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_1">
<bpmndi:BPMNShape id="Activity_1n52sm3_di" bpmnElement="scopedContext">
<dc:Bounds x="120" y="70" width="100" height="80" />
<bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Collaboration_0e60zir">
<bpmndi:BPMNShape id="Participant_10jajek_di" bpmnElement="Participant_1" isHorizontal="true">
<dc:Bounds x="180" y="60" width="300" height="120" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Activity_1n52sm3_di" bpmnElement="ServiceTask_1">
<dc:Bounds x="230" y="80" width="100" height="80" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Activity_1hlau9e_di" bpmnElement="ServiceTask_2">
<dc:Bounds x="350" y="80" width="100" height="80" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Participant_0o4apj6_di" bpmnElement="Participant_2" isHorizontal="true">
<dc:Bounds x="500" y="60" width="300" height="120" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Activity_06g1sod_di" bpmnElement="ScriptTask_1">
<dc:Bounds x="550" y="80" width="100" height="80" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Participant_13eqjzi_di" bpmnElement="Participant_3" isHorizontal="true">
<dc:Bounds x="820" y="60" width="300" height="120" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Activity_1c85kz4_di" bpmnElement="ScriptTask_2">
<dc:Bounds x="870" y="80" width="100" height="80" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Activity_1hlau9e_di" bpmnElement="Activity_0kafybg">
<dc:Bounds x="270" y="70" width="100" height="80" />
<bpmndi:BPMNShape id="BPMNShape_0o2k4bk" bpmnElement="Participant_4" isHorizontal="true">
<dc:Bounds x="180" y="200" width="300" height="120" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="BPMNShape_1wqur3c" bpmnElement="ScriptTask_3">
<dc:Bounds x="230" y="220" width="100" height="80" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Participant_08w9t3p_di" bpmnElement="Participant_5" isHorizontal="true">
<dc:Bounds x="500" y="200" width="300" height="120" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Activity_0jkek8a_di" bpmnElement="ScriptTask_4">
<dc:Bounds x="550" y="220" width="100" height="80" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Participant_1cpioa5_di" bpmnElement="Participant_6" isHorizontal="true">
<dc:Bounds x="820" y="200" width="300" height="120" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Activity_0pvyvzm_di" bpmnElement="ServiceTask_3">
<dc:Bounds x="870" y="220" width="100" height="80" />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Participant_0hcijo0_di" bpmnElement="Participant_7" isHorizontal="true">
<dc:Bounds x="180" y="340" width="300" height="120" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Activity_1533gjq_di" bpmnElement="ServiceTask_4">
<dc:Bounds x="230" y="360" width="100" height="80" />
<bpmndi:BPMNLabel />
</bpmndi:BPMNShape>
</bpmndi:BPMNPlane>
</bpmndi:BPMNDiagram>
Expand Down
Loading
Loading