fix: show FEEL expression for unresolvable function invocations#104
fix: show FEEL expression for unresolvable function invocations#104
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts Zeebe FEEL expression resolution to preserve the original FEEL expression when function invocations cannot be resolved at design time, enabling downstream UI to display the expression instead of an empty value.
Changes:
- Preserve FEEL expression as
infofor unresolved (typeAny) function invocation results. - Normalize certain unresolved leaf entries in computed context results for display purposes.
- Add regression tests + a BPMN fixture covering function invocation cases; update changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/spec/zeebe/ZeebeVariableResolver.spec.js | Adds tests ensuring unresolved function calls expose the FEEL expression and nested context results are retained. |
| test/fixtures/zeebe/function-invocation.bpmn | New BPMN fixture with output mappings using FEEL built-in function invocations and a context-defined function. |
| lib/zeebe/util/feelUtility.js | Updates FEEL resolution to preserve expressions for unresolved results; adjusts unified-format conversion for unresolved leaf entries. |
| CHANGELOG.md | Documents the fix in the Unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Entries with unknown type, no info, and no sub-entries are values | ||
| // that cannot be determined at design time (e.g. nested results of | ||
| // unresolvable function invocations). In FEEL, absent values are null, | ||
| // there is no "unknown" concept. So we normalize these to Null. | ||
| if (annotated.type === 'Any' && !annotated.info && | ||
| (!entries || !entries.length)) { |
There was a problem hiding this comment.
The new Any → Null normalization will rewrite any leaf entry with unknown type + no info into { type: 'Null', info: 'null' }. That can misrepresent legitimate “unknown at design time” values (e.g. context outputs that forward runtime-provided variables) as proven-null. Consider narrowing this condition (e.g. only normalize when atomicValue === null, or add a guard to avoid rewriting values that originate from scoped variables).
| // Entries with unknown type, no info, and no sub-entries are values | |
| // that cannot be determined at design time (e.g. nested results of | |
| // unresolvable function invocations). In FEEL, absent values are null, | |
| // there is no "unknown" concept. So we normalize these to Null. | |
| if (annotated.type === 'Any' && !annotated.info && | |
| (!entries || !entries.length)) { | |
| // Only normalize unknown leaf values to FEEL null when the source value | |
| // explicitly represents null. Other leaf `Any` values may simply be | |
| // unknown at design time (for example, forwarded scoped/runtime values) | |
| // and must not be rewritten as proven-null. | |
| if (annotated.type === 'Any' && !annotated.info && | |
| (!entries || !entries.length) && | |
| has(variable, 'atomicValue') && variable.atomicValue === null) { |
There was a problem hiding this comment.
I wonder if we should simply extract this to a helper function - completely unreadable code - much more readable like this:
if (isNullishValue(...)) {
...
}
3095f41 to
a867dcc
Compare
nikku
left a comment
There was a problem hiding this comment.
We have the potential to type functions - they all offer more or less a contract. I.e. abs will always return number|null.
| // Entries with unknown type, no info, and no sub-entries are values | ||
| // that cannot be determined at design time (e.g. nested results of | ||
| // unresolvable function invocations). In FEEL, absent values are null, | ||
| // there is no "unknown" concept. So we normalize these to Null. |
There was a problem hiding this comment.
I thought we'd normalize these to Any (unknown).
There was a problem hiding this comment.
With Any, we use undefined as the value, but undefined does not exist in both JSON and FEEL specifications. So, while stringifying JSON, we lose undefined properties.
Not to lose them, I re-value them as null as it'll resolve as null from engine perspective to my understanding. By doing so, we keep the property in place instead of losing it.
updated thoughts below;
While info itself is important here more than the type for the value we display in variable-outline, marking this Any does not quite feel right as well.
In abs(...) scenario you mentioned previously, it's number|null, not Any clearly. Marking it as null looks closer to the possible types while both null or Any alone is somewhat misleading.
We do not show the type on variable-outline still, but properties-panel do show the variable type while giving suggestions. There, would you say it's better to see Any or Null, @nikku?
There was a problem hiding this comment.
We do not show the type on variable-outline still, but properties-panel do show the variable type while giving suggestions. There, would you say it's better to see Any or Null, @nikku?
As I previously indicated I'd not show values where we don't know where they come from ("unknown") as null. While mapped as null by the engine, i.e. for empty input mappings, the semantics are typically "this is a local variable, someone else defines the value" - I'd love to see that clearly represented.
There was a problem hiding this comment.
As I previously indicated I'd not show values where we don't know where they come from ("unknown") as null.
Sure, we are on the same page. For the variables, having a value as a direct function call is covered in this way.
| # | Variable name | Variable assignment value | Type | Context |
|---|---|---|---|---|
| 1 | casting_number |
=number("123") |
Any | { } |
| 2 | deepFuncCall |
={ a: function() { aa: { bb: abs(y) } }, b: a()}.b |
Any | { } |
Screenshots for variable in the row 1.
Screenshots for variable in the row 2.
I think, until here, it's all good. But when we are to display a context value (as JSON in CodeMirror), undefined properties are naturally being omitted while building CodeMirror document (via JSON.stringify).
At this point, I have thought we need to normalize these to null so that properties are not lost as we cannot currently compute any value for them (neither actual computed value or the FEEL expressions to display for that matter).
If I do not normalize it as null (regardless of its type, I can leave that as Any as in the screenshot if that makes sense), this is what we display:
Compared to
I'd be happy to hear suggestions to improve the view. I hope I could bring clarity for the motivation behind the implementation details.
a867dcc to
1839f6d
Compare
|
@barinali turned to draft. Let's get this back into review once upstream is incorporated. |
Related to camunda/camunda-modeler#5744
Proposed Changes
This pull request aims to preserve the FEEL expression for the built-in functions for downstream to display them on the user-interface. This pull request is enabled by lezer-feel changes.
Before
After
Tests
The tests can be observed in #105 with the necessary changes from the lezer-feel pull request.
Steps to try out
This is the command I'd have suggested to experiment with variable over variable-outline, but I had no luck having it run so far. I think it's failing because linked dependencies are not direct dependencies of
variable-outlineand they're not declared asoverridesby@bpmn-io/sr.npx @bpmn-io/sr bpmn-io/variable-outline -l bpmn-io/variable-resolver#issue-5744-show-expression-for-unresolvable-values -l bpmn-io/lezer-feel#issue-5744-fix-function-invocation-valueChecklist
Ensure you provide everything we need to review your contribution:
Closes {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}@bpmn-io/srtool