mattcasters commented on code in PR #8823:
URL: https://github.com/apache/hop/pull/8823#discussion_r4246745110
##########
plugins/transforms/javascript/src/test/java/org/apache/hop/pipeline/transforms/javascript/ScriptValuesTest.java:
##########
@@ -236,6 +236,78 @@ void undefinedFieldBecomesNull() throws Exception {
assertEquals(0, transform.getErrors());
}
+ /**
+ * A replaced field the script does not assign keeps its input value. It
used to depend on whether
+ * the field name appeared in the script text, so adding or removing a
comment changed the output.
+ */
+ @Test
+ void replacedFieldNotSetByScriptKeepsInputValue() throws Exception {
+ for (String script : new String[] {"var other = 0;", "//var test_int =
0;\nvar other = 0;"}) {
Review Comment:
**[suggestion]** `replacedFieldNotSetByScriptKeepsInputValue` claims the
comment and no-comment scripts are equivalent, but both use the string `"1"`,
which `Context.toObject` round-trips, so the comment iteration passes on the
pre-fix code and never executes the new branch (`test_int` is bound because the
comment contains the name). `replacedFieldNotSetByScriptIsConvertedToItsType`
only converts the string `"1"` with the name absent, so it cannot catch the
Number-to-Integer rounding split above. The integration pipeline
`0087-javascript-replaced-field-8724.hpl` has the same hole for `test_int`: the
script is `//var test_int = 0;` plus `var doubled = id * 2;`, so `test_int`
stays on the old bound path and the golden rows (10, 20, 30) would match before
this change. The pipeline does fail before the fix for `new_value` /
`other_int` (that name is absent, so `NOT_FOUND` used to null the field).
Neither test locks "a comment must not change the value" for a type that does
not survive
`Context.toObject`.
**Suggestion:** Keep the string and rename cases, and add one unit test that
runs the same replaced field twice (name absent, and name present only inside a
comment) for a BigNumber or Boolean and for a type change whose `convertData`
result differs from `convertFromJs`. In the integration pipeline, add a
replaced field whose name does not occur anywhere in the script text, not only
a rename onto a different input field.
##########
plugins/transforms/javascript/src/main/java/org/apache/hop/pipeline/transforms/javascript/ScriptValues.java:
##########
@@ -303,6 +303,19 @@ private void addValuesToContext(IRowMeta rowMeta, Object[]
row) throws HopExcept
for (int i = 0; i < meta.getScriptFields().size(); i++) {
ScriptValuesMeta.ScriptField field = meta.getScriptFields().get(i);
Object result = data.scope.get(field.getName(), data.scope);
+ if (result == Scriptable.NOT_FOUND && data.replaceIndex[i] >= 0) {
Review Comment:
**[bug]** `result == Scriptable.NOT_FOUND` is not "the script did not assign
this field." `determineUsedFields` treats any substring of the transform script
as a used field, comments included, and `addFieldsToScope` then does
`scope.put(name, Context.toObject(value))` before the script runs.
`Context.toObject` wraps the value: every `Number` (Long, Double, BigDecimal)
becomes a `NativeNumber` of `doubleValue()`, and a Boolean becomes a
`NativeBoolean`. After the script, `scope.get` returns that wrapper, the new
branch is skipped, and `getValueFromJScript` converts the wrapper. A real
assignment still works, because it replaces the scope property, but a
comment-only (or read-but-not-assigned) name does not keep the input value.
Concrete differences versus the `NOT_FOUND` path: a BigNumber or a Long outside
the 53-bit mantissa is rounded to double; a Boolean is left as a
`NativeBoolean`, and `ValueMetaBase.getBoolean` then does `(Boolean) object`
and throws ClassCastException; a Num
ber such as 10.5 replaced as Integer is truncated by `longValue()` (10) on the
comment path and `Math.round` (11) on the `convertData` path. Removing the
comment in issue #8724 therefore still changes the output for those values. The
guard also ignores an explicit `undefined` only when the name was not injected;
that part is fine. Rename-to is unaffected when the script field name is not an
input field (`new_value` -> `other_int`), because that name is never bound.
**Suggestion:** Before `script.exec`, remember the scope object
`addFieldsToScope` stored for each replaced field (or `NOT_FOUND` if it stored
nothing). After exec, if `scope.get` returns that same reference or
`NOT_FOUND`, the script did not assign the field: keep `outputRow[index]` and,
when the input and output types differ,
`outputValueMeta.convertData(inputValueMeta, outputRow[index])`. Do not pass
the injected wrapper through `convertFromJs`. A script assignment allocates a
new value, so reference equality still lets an assigned field win, including
`test_int = null`. Add a regression with and without a comment-only mention for
a BigDecimal that does not fit in a double, a Boolean, and a Number-to-Integer
conversion of `x.5`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
