Conversation
|
Failing due to insufficient escaping of |
bpmn-io-tasks
bot
added
needs review
There was a problem hiding this comment.
Pull request overview
This pull request addresses issue #81 by adding support for static input values in IO mappings. Previously, the code assumed all mapping source expressions started with = and stripped this prefix. This caused problems when users entered static values (without =) containing quotation marks, as the variable info property would be empty instead of showing the static value.
Changes:
- Modified
getResultContextto detect static values (no=prefix) and handle them as string literals using.literal()method - Removed
.substring(1)calls fromgetIoExpressionandgetScriptExpressionto preserve the full expression including the=prefix - Updated
getResultContextin connectors.js to handle expressions that may or may not have the=prefix
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/zeebe/util/feelUtility.js | Core logic changes to distinguish static values from FEEL expressions and handle each appropriately |
| lib/zeebe/extractors/connectors.js | Updated to work with the new signature of getResultContext |
| test/fixtures/zeebe/ioMappings.static.bpmn | New test fixture with static input values, including edge case with quotes |
| test/spec/zeebe/ZeebeVariableResolver.spec.js | New test suite to verify static values are correctly resolved with proper type and info |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // expression is a static value | ||
| if (!expression.startsWith('=')) { | ||
|
|
||
| // TODO(nikku): completely unsafe stuff, don't do, remove ASAP |
There was a problem hiding this comment.
The TODO comment indicates this implementation is "completely unsafe" and should be "removed ASAP". This suggests the current approach using contextTracker.start.literal() may be a temporary workaround. Consider documenting why this approach is unsafe and what the proper solution should be, or addressing the underlying issue before merging.
| // TODO(nikku): completely unsafe stuff, don't do, remove ASAP | |
| // NOTE: For expressions without a leading '=', we intentionally treat the | |
| // value as a FEEL string literal and construct it directly via | |
| // EntriesContext.literal(). This mirrors how static literals are handled | |
| // by the FEEL engine without going through the parser. Be aware that | |
| // this will not perform any additional validation beyond what | |
| // EntriesContext.literal() itself enforces. |
nikku
force-pushed
the
static-values
branch
2 times, most recently
from
a06f79e to
daedae0
Compare
* Ensure we don't eagerly cut of `=`, because we need to determine whether the expression is a string during evaluation * For string values, treat the result as a literal string, do not attempt any parsing / evaluation (!) Closes #81
barinali
left a comment
barinali
left a comment
There was a problem hiding this comment.
Plain text content is rendered as expected.
Proposed Changes
Rework the original implementation in this PR to handle static values in a robust manner - do not escape
", nor do any parsing when we know the result is a string literal value.Closes #81
Checklist
Ensure you provide everything we need to review your contribution:
Closes {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}@bpmn-io/srtool