Conversation
Buckwich
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request implements functionality to extract and track variables used in FEEL expressions within BPMN process definitions. The feature analyzes input mappings and script task expressions to identify which variables are required as inputs, creating "input requirement" entries that track dependencies between variables.
Changes:
- Added FEEL expression analysis using
@bpmn-io/feel-analyzerto extract input variables from expressions - Introduced
usedByproperty on ProcessVariable objects to track which target variables depend on each input - Added
getInputRequirementsForElementAPI method to retrieve input requirements for specific elements
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added dependencies for @bpmn-io/feel-analyzer and @camunda/feel-builtins to enable FEEL expression analysis |
| package-lock.json | Updated dependency tree with new packages and peer dependency resolutions |
| lib/zeebe/util/feelUtility.js | Core implementation: analyzes expressions with FeelAnalyzer, builds input requirements with deduplication and entry merging, filters scoped variables |
| lib/zeebe/VariableResolver.js | Integrates input requirements into the variable resolution pipeline |
| lib/base/VariableResolver.js | Adds getInputRequirementsForElement method, updates scope filtering to exclude input requirements, fixes async return type documentation |
| test/spec/zeebe/Mappings.spec.js | Comprehensive tests for input requirement extraction covering simple/nested expressions, deduplication, merging, scoping, and script tasks |
| test/fixtures/zeebe/mappings/input-requirements.bpmn | Test fixture with various input mapping scenarios |
| test/fixtures/zeebe/mappings/script-task-inputs.bpmn | Test fixture for script task input extraction |
| test/fixtures/zeebe/mappings/script-task-with-input-mappings.bpmn | Test fixture combining script tasks with input mappings |
Comments suppressed due to low confidence (3)
lib/base/VariableResolver.js:320
- The new public method
getInputRequirementsForElementhas no test coverage. Consider adding tests to verify that it correctly filters input requirements by element and handles edge cases like elements with no input requirements, multiple input requirements, and both diagram-js elements and moddle elements as parameters.
async getInputRequirementsForElement(element) {
const allVariablesByRoot = await this.getVariables()
.catch(() => {
return {};
});
const allVariables = Object.values(allVariablesByRoot).flat();
return allVariables.filter(v =>
v.usedBy && v.usedBy.length > 0
&& v.origin.length === 1 && v.origin[0].id === element.id
);
}
lib/base/VariableResolver.js:305
- The parameter type annotation for
elementshould be{ModdleElement}to be consistent with other methods in this class likegetProcessVariablesandgetVariablesForElementwhich use the same parameter type.
* @param {Object} element
lib/base/VariableResolver.js:318
- The method compares
v.origin[0].idwithelement.iddirectly, but the method might be called with a diagram-js element (which has a.businessObjectproperty) or a moddle element directly. Consider usinggetBusinessObject(element)likegetVariablesForElementdoes on line 256 to handle both cases correctly.
&& v.origin.length === 1 && v.origin[0].id === element.id
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bpmn-io-tasks
bot
added
needs review
Buckwich
requested review from
a team,
barmac and
jarekdanielak
and removed request for
a team
|
One problem which I noticed is that we don't take into account the order in which inputs are defined. This is already a problem with existing variable resolver as we suggest the local variables even before they are declared. You can consider this to be an edge case, but it may be confusing for the users (potential follow-up). On the example, 
In this PR, this manifests by not prefiling 
|
|
@barmac I've adjusted the behavior to follow the input mapping chain |
Hmm it looks that we still remove the variable which is required for the execution: Screen.Recording.2026-02-20.at.14.07.03.mov |
|
Sorry I didn't update the task testing PR yet. you need to test with (updated description) Screen.Recording.2026-02-20.at.14.20.03.mov |
|
Do we want to handle script and business rule tasks correctly in this PR? Asking because there are many places where a variable can be used, and not necessarily defined. 
|
|
Variables ordering in script:
So the script can use the inputs contrary to what the screenshot indicates. |
|
ATM it only looks at io mappings and script expressions. I can either just include all feel expressions (in the sense of any value starting with |
|
Gotcha. In that case, I think it's fine to move the complete support to a separate issue, but let's either fix or remove the script part. |
|
We don't want to check all of the values starting with |
Buckwich
force-pushed
the
5639_variable-input-requirements
branch
from
ccce04e to
3120e8a
Compare
lib/zeebe/util/feelUtility.js
Outdated
| const unresolved = findUnresolvedVariables(result); | ||
|
|
||
| return { expression, unresolved }; | ||
| // Analyze each non-output-mapping expression for input requirements |
There was a problem hiding this comment.
Output expression may also contain references to process variables - these are then required.
Example: Access process variable myList, appending items to it
= append(myList, [ 1 ])
There was a problem hiding this comment.
Can we safely determine that a variable mentioned in the output mapping is required, and not just part of the element output?
Example:
- My service task calls an API, and produces a
responseobject of structure:
{
"body": {
"items": [1,2]
}
}- In my output mapping, I take the
response.body.items, and append another item to it:
=append(response.body.items, 3)
While by an analysis of the expression I can tell that response.body.items needs to be provided, it is done by the job worker and not via process variables.
So I think we can only prefill the variables mentioned in in the inputs, and not in the output mapping.
There was a problem hiding this comment.
as this is only a maybe reference it should not be prefilled.
we could consider providing the variables here in the resolver. i'm not sure which use case we would have for knowing output mappings without further intelligence like io spec/connectors, then we could use it for validation.
so i could include them but prefill then needs to filter them out again. if we can't come up with a use case i would skip it for now
There was a problem hiding this comment.
Agreed! We can further improve in the future.
lib/zeebe/util/feelUtility.js
Outdated
| }); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`Failed to analyze expression for variable ${variable.name}:`, error); |
There was a problem hiding this comment.
Warning is a new pattern in this library - why do we introduce it? How is the user supposed to know about it?
There was a problem hiding this comment.
right, I think this is a remainder of earlier versions of the analyzer. I don't think it will throw anymore in the current version (and if it those we should let the error bubble). will remove
lib/zeebe/VariableResolver.js
Outdated
| for (const key in rawVariables) { | ||
| const variables = rawVariables[key]; | ||
| const newVariables = parseVariables(variables); | ||
| const { resolvedVariables, inputRequirements } = parseVariables(variables); |
There was a problem hiding this comment.
Are we sold on inputRequirements as a name? I read resolvedVariables, inputRequirements and this does not work together - I'd expect readVariables, consumedVariables, etc.
There was a problem hiding this comment.
consumedVariables could work. just to be clear it contains variables that are read by the task and not provided by itself, thats why i don't want to use read
| import { is } from 'bpmn-js/lib/util/ModelUtil'; | ||
| import { camundaBuiltins, camundaReservedNameBuiltins } from '@camunda/feel-builtins'; | ||
|
|
||
| const feelAnalyzer = new FeelAnalyzer({ |
There was a problem hiding this comment.
We had performance issues in the past - now we pull the list of all built-ins into the core of our intelligence. Are we aware of the consequences?
Do we have a test case that validates the performance for a reasonable diagram, let's say 500 expressions?
There was a problem hiding this comment.
The builtins are not used for parsing only to remove variables, so i thought it wouldn't affect performance too much. I'll measure it
There was a problem hiding this comment.
did some performance tests:
diagram with 4000 feel expressions (400 tasks with 10 input mappings)
average over 20 iterations:
- main (no feel-analyzer): 507ms
- feel-analyzer with builtins: 607ms
- feel-analyzer without builtins: 613ms
interpretation:
- feel analyzer adds 20% performance impact (I think thats acceptable)
- unexpecated: providing builtins is slightly faster:
- either builtins have no impact (measuring error)
- or filtering them out improves merging speed
There was a problem hiding this comment.
for reference you could cherrypick ea6f100 on main or this branch. for testing without builtins just comment them out in feel-utils
There was a problem hiding this comment.
i wouldn't add specific performance timings as expectation as they can vastly differ depending on the system. do we have a pattern to still add them to the repo (eg. skip by default)
There was a problem hiding this comment.
i wouldn't add specific performance timings as expectation as they can vastly differ depending on the system.
I'd add upper guardrails - just to prevent complexity explosion - the alternative is that we continuously record the numbers.
There was a problem hiding this comment.
Let's discuss the things mentioned:
- Performance - are we now "fast enough" with all built-ins used?
- Parsing - we parse the AST twice (this library, and
feel-analyzer) - does it matter, if so, how? inputRequrements, is that the name we want to use?- output mappings may establish dependencies
| variables.forEach(variable => { | ||
| const existingVariable = mergedVariables.find(v => | ||
| v.name === variable.name && v.scope === variable.scope | ||
| && !v.usedBy && !variable.usedBy |
There was a problem hiding this comment.
i didn't want to pollute the variable outline yet. probably not the best way, i'm currently testing integration with the panel, so probably can be removed soon
Buckwich
force-pushed
the
5639_variable-input-requirements
branch
from
3120e8a to
9e90ca4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
lib/zeebe/util/feelUtility.js:69
analysisResults.push({ ... })includes ascopeproperty, butbuildConsumedVariables(and other code paths in this file) never read it. This looks like leftover data and increases cognitive load—consider removingscopefrom the collected analysis results (or actually using it if intended).
analysisResults.push({
origin,
scope: origin,
targetName: variable.name,
inputs,
lib/base/VariableResolver.js:310
- The PR description mentions a new
getInputRequirementsForElementAPI, but the code introducesgetConsumedVariablesForElement. Please align the public API naming (either rename this method or update the PR description / consumer docs) to avoid confusion for downstream users.
/**
* Returns consumed variables for an element — variables
* the element needs as input for its expressions and mappings.
*
* Uses `getVariables()` instead of `getVariablesForElement()` to
* bypass the name-based deduplication that would drop requirement
* entries for variables that also exist in ancestor scopes.
*
* @param {Object} element
* @returns {Promise<Array<ProcessVariable>>}
*/
async getConsumedVariablesForElement(element) {
lib/base/VariableResolver.js:309
- JSDoc for
getConsumedVariablesForElementuses@param {Object} element, which is looser than the rest of this API (e.g.,getVariablesForElementtakes aModdleElementand normalizes viagetBusinessObject). Consider updating the type and (if desired) normalizing the input the same way for consistency.
*
* @param {Object} element
* @returns {Promise<Array<ProcessVariable>>}
*/
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Related to camunda/camunda-modeler#5639
Proposed Changes
runs feel-analyzer on every input "feel" expression (input mapping starting with
=and scripts) and adds them to raw variables without scopenew function
getInputRequirementsForElementcan be used by consumers to get those variablesvariables without scope are filtered out of the normal
getVariablesProbably best to try out in task testing demo:
Checklist
Ensure you provide everything we need to review your contribution:
Closes {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}@bpmn-io/srtool