Filter by uniqueness after filtering by scope#56
Conversation
* also, don't filter out external variables Closes #54
philippfromme
force-pushed
the
54-input-parameter-bug
branch
from
1060cb8 to
02e9c98
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where input variables were incorrectly filtered out when two elements with the same variable name existed but had different scopes. The change ensures variables are filtered by scope first, then by uniqueness, preserving variables with the correct scope.
Key Changes
- Modified the filtering logic in
BaseVariableResolverto filter by scope before applying uniqueness constraints - Added a new test case to verify variables with the same name but different scopes are not merged
- Updated the BPMN test fixture to include an additional ServiceTask for testing scope differentiation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/base/VariableResolver.js | Refactored variable filtering logic to preserve scope-specific variables and handle external variables correctly |
| test/spec/zeebe/ZeebeVariableResolver.spec.js | Added test case for different scopes with same variable name and updated existing test description |
| test/fixtures/zeebe/simple.bpmn | Added ServiceTask_2 to support testing of multiple scopes with same variable names |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
lib/zeebe/VariableResolver.js
Outdated
| if ( | ||
| is(moddleElement, 'zeebe:Output') && | ||
| v.provider.find(extractor => extractor !== this._baseExtractor) | ||
| (variable.provider || []).find(extractor => extractor !== this._baseExtractor) |
There was a problem hiding this comment.
Not really anything. It's save to assume that provider is always going to exist. But Copilot complained.
There was a problem hiding this comment.
Why does co-pilot complain? Let's fix the types if that is the issue, or otherwise build on existing contracts and not introduce these arbitrary null checks.
There was a problem hiding this comment.
I'll check if we can fix the types.
nikku
left a comment
nikku
left a comment
There was a problem hiding this comment.
Consider to unbuild the unneeded null check around providers.
philippfromme
force-pushed
the
54-input-parameter-bug
branch
from
1f49513 to
22f4dec
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
philippfromme
force-pushed
the
54-input-parameter-bug
branch
from
c05dc2a to
d6af0a6
Compare
| // if not external, keep only if first of its name | ||
| return reversedVariables.find(v => v.name === variable.name) === variable; | ||
| // if not external, keep only the first occurrence of each name | ||
| if (!seenNames.has(variable.name)) { |
Proposed Changes
When two input parameters with the same target exist on two elements and I resolve the variables for each I don't want an input variable to be missing due to an early uniqueBy which removes one of the two variables even though it might be the one with the correct scope.
Closes #54
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}