Bläddra i källkod

fix: close aggregate key-collision hole, add real Template run-time mode, guard against unusable dates

Consolidated fix wave from the final tier-1-nodes review, covering seven items:

- aggregate: the collision guard now also rejects outputField "key" when
  groupBy is set, since grouped mode writes entry.key and that value was
  silently overwritten before
- template: adds a templateSource config (config/input). input mode reads
  template text from input data at run time, where the engine has not
  pre-resolved its {{...}} placeholders, and renders it with
  utils.interpolate; config mode keeps today's behaviour and docs now say
  so accurately
- datetime: toTimestamp throws for values outside the +/-8.64e15 range
  JavaScript dates can represent, instead of quietly producing NaN output
- filter, aggregate, sort-limit-dedupe: "not an array" errors now name the
  defaulted field path actually looked up, not the raw unset config value
- split-out: renamed its field config to inputField with a default of
  "data", matching the other array-input nodes
- verify-node.py: expect entries can add errorContains to pin down which
  error a failed fixture is asserting, and every existing error fixture
  now uses it
- aggregate.js: removed a stray blank line in its JSDoc header

Adds fixtures for each new failure mode and updates split-aggregate.json
and template-json.json for the renamed/new config. Full tests/nodes suite:
26/26 passing.
fszontagh 1 månad sedan
förälder
incheckning
8612d70c24

+ 11 - 3
docs/nodes.md

@@ -239,7 +239,7 @@ returns fixed keys alongside a user-named field must reject a name that would
 collide, throwing early:
 
 ```javascript
-if (outputField === 'count' || outputField === 'groups') {
+if (outputField === 'count' || outputField === 'groups' || (groupBy && outputField === 'key')) {
     throw new Error('Aggregate: outputField cannot be "' + outputField +
         '", which is a reserved output name for this node. Pick another name.');
 }
@@ -247,14 +247,22 @@ if (outputField === 'count' || outputField === 'groups') {
 
 `aggregate`, `sort-limit-dedupe` and `datetime` all need this. `template` and
 `json` do not, because they return exactly one key and there is nothing to
-collide with - do not add the guard where it protects nothing.
+collide with - do not add the guard where it protects nothing. When a node
+builds its reserved-key list dynamically, as `aggregate` does for `key`
+(only reserved once `groupBy` is set, since that is the only time it is
+written), check every mode the node has, not just the always-on keys.
 
 **Config values arrive already evaluated.** The engine resolves `{{...}}` in
 node config before `execute()` runs, and a config string that is exactly one
 expression keeps its native type. So a field typed as a string in the schema can
 legitimately hold an array at run time, which is why several nodes begin with
 `if (Array.isArray(inputField))`. That branch is not dead code. Nodes must not
-re-interpolate config values.
+re-interpolate config values. `template` is the one deliberate exception: its
+`templateSource: 'input'` mode reads template text out of the input DATA at run
+time, which the engine never walks, so that text still has its `{{...}}`
+placeholders intact when `execute()` sees it, and calling
+`smartbotic.utils.interpolate` on it is genuine work, not a re-interpolation of
+something the engine already resolved.
 
 **Read paths with the shared helper.** Use
 `smartbotic.utils.getFieldValue(data, path)` rather than writing a private path

+ 2 - 3
nodes/core/aggregate.js

@@ -5,7 +5,6 @@
  * @version 1.0.0
  * @description Collect items back into one array, optionally grouped by a key
  * @icon layers
-
  */
 
 const configSchema = {
@@ -57,7 +56,7 @@ async function execute(config, input, context) {
     const groupBy = config.groupBy;
     const fieldToAggregate = config.fieldToAggregate;
 
-    if (outputField === 'count' || outputField === 'groups') {
+    if (outputField === 'count' || outputField === 'groups' || (groupBy && outputField === 'key')) {
         throw new Error('Aggregate: outputField cannot be "' + outputField +
             '", which is a reserved output name for this node. Pick another name.');
     }
@@ -70,7 +69,7 @@ async function execute(config, input, context) {
     }
 
     if (!Array.isArray(items)) {
-        throw new Error('Aggregate: the value at "' + inputField + '" is not an array');
+        throw new Error('Aggregate: the value at "' + (inputField || 'data') + '" is not an array');
     }
 
     const values = fieldToAggregate

+ 10 - 3
nodes/core/datetime.js

@@ -83,21 +83,28 @@ const UNIT_MS = {
     weeks: 604800000
 };
 
+function checkRange(parsed, value) {
+    if (!isFinite(parsed) || Math.abs(parsed) > 8.64e15) {
+        throw new Error('Date and Time: "' + value + '" is not a usable date - it is outside the range JavaScript dates can represent');
+    }
+    return parsed;
+}
+
 function toTimestamp(value, label) {
     if (value === undefined || value === null || value === '') {
         throw new Error('Date and Time: no date found at "' + label + '"');
     }
     if (typeof value === 'number') {
-        return value;
+        return checkRange(value, value);
     }
     if (/^\d+$/.test(String(value))) {
-        return parseInt(String(value), 10);
+        return checkRange(parseInt(String(value), 10), value);
     }
     const parsed = Date.parse(String(value));
     if (isNaN(parsed)) {
         throw new Error('Date and Time: could not read "' + value + '" as a date');
     }
-    return parsed;
+    return checkRange(parsed, value);
 }
 
 function offsetMinutes(offset) {

+ 1 - 1
nodes/core/filter.js

@@ -127,7 +127,7 @@ async function execute(config, input, context) {
     }
 
     if (!Array.isArray(items)) {
-        throw new Error('Filter: the value at "' + inputField + '" is not an array');
+        throw new Error('Filter: the value at "' + (inputField || 'data') + '" is not an array');
     }
 
     const kept = [];

+ 1 - 1
nodes/core/sort-limit-dedupe.js

@@ -110,7 +110,7 @@ async function execute(config, input, context) {
     }
 
     if (!Array.isArray(items)) {
-        throw new Error('Sort / Limit / Dedupe: the value at "' + inputField + '" is not an array');
+        throw new Error('Sort / Limit / Dedupe: the value at "' + (inputField || 'data') + '" is not an array');
     }
 
     let working = items.slice();

+ 9 - 8
nodes/core/split-out.js

@@ -10,10 +10,11 @@
 const configSchema = {
     type: 'object',
     properties: {
-        field: {
+        inputField: {
             type: 'string',
             title: 'Array Field',
-            description: 'Path to the array to split out, such as data.result.orders'
+            description: 'Path to the array to split out, such as data.result.orders',
+            default: 'data'
         },
         include: {
             type: 'string',
@@ -71,7 +72,7 @@ function setByPath(target, path, value) {
 }
 
 async function execute(config, input, context) {
-    const field = config.field;
+    const inputField = config.inputField;
     const include = config.include || 'none';
     const includeFields = Array.isArray(config.includeFields) ? config.includeFields : [];
     const itemField = config.itemField || 'value';
@@ -79,14 +80,14 @@ async function execute(config, input, context) {
     const source = input && input.data !== undefined ? input.data : input;
 
     let list;
-    if (Array.isArray(field)) {
-        list = field;
+    if (Array.isArray(inputField)) {
+        list = inputField;
     } else {
-        list = smartbotic.utils.getFieldValue(input, field);
+        list = smartbotic.utils.getFieldValue(input, inputField || 'data');
     }
 
     if (!Array.isArray(list)) {
-        throw new Error('Split Out: the value at "' + field + '" is not an array');
+        throw new Error('Split Out: the value at "' + (inputField || 'data') + '" is not an array');
     }
 
     const parent = source !== null && typeof source === 'object' && !Array.isArray(source) ? source : {};
@@ -117,7 +118,7 @@ async function execute(config, input, context) {
         return item;
     });
 
-    smartbotic.log.info('Split Out: ' + items.length + ' items from "' + field + '"');
+    smartbotic.log.info('Split Out: ' + items.length + ' items from "' + (inputField || 'data') + '"');
 
     return { items: items, count: items.length };
 }

+ 32 - 7
nodes/core/template.js

@@ -3,27 +3,42 @@
  * @name Template
  * @category data
  * @version 1.0.0
- * @description Render text from a template, for email bodies and chat messages
+ * @description Render text from a template. In "config" mode the {{...}} placeholders in the
+ * template are already resolved by the engine before this node runs, so rendering only catches
+ * whatever the engine could not fill in. In "input" mode the template text is read from the
+ * input data at run time, where its {{...}} placeholders are still intact, and this node
+ * renders them against the input.
  * @icon file-text
  */
 
 const configSchema = {
     type: 'object',
     properties: {
+        templateSource: {
+            type: 'string',
+            title: 'Template Source',
+            description: 'config uses the Template text below, whose {{path.to.field}} placeholders the engine has already resolved before this node runs. input reads the template text at run time from Template Field, where its {{path.to.field}} placeholders are still unresolved and get rendered against the input here',
+            enum: ['config', 'input'],
+            default: 'config'
+        },
         template: {
             type: 'string',
             title: 'Template',
-            description: 'Text with {{path.to.field}} placeholders, resolved against the input',
+            description: 'Text with {{path.to.field}} placeholders. Used when Template Source is config. These placeholders are resolved by the engine before this node runs, not by this node',
             format: 'textarea',
             default: ''
         },
+        templateField: {
+            type: 'string',
+            title: 'Template Field',
+            description: 'Path in the input data holding the template text. Used when Template Source is input. Its {{path.to.field}} placeholders are resolved against the input by this node'
+        },
         outputField: {
             type: 'string',
             title: 'Output Field',
             default: 'text'
         }
-    },
-    required: ['template']
+    }
 };
 
 const inputSchema = {
@@ -41,11 +56,21 @@ const outputSchema = {
 };
 
 async function execute(config, input, context) {
-    const template = config.template;
+    const templateSource = config.templateSource || 'config';
     const outputField = config.outputField || 'text';
 
-    if (typeof template !== 'string') {
-        throw new Error('Template: no template text was given');
+    let template;
+    if (templateSource === 'input') {
+        const templateField = config.templateField;
+        template = smartbotic.utils.getFieldValue(input, templateField);
+        if (typeof template !== 'string' || template === '') {
+            throw new Error('Template: no template text found at "' + templateField + '"');
+        }
+    } else {
+        template = config.template;
+        if (typeof template !== 'string') {
+            throw new Error('Template: no template text was given');
+        }
     }
 
     const source = input && input.data !== undefined ? input.data : input;

+ 7 - 0
scripts/verify-node.py

@@ -99,6 +99,13 @@ def main():
                     f"{node_id}: status {got.get('status')!r}, expected {want['status']!r}"
                     + (f" (error: {got.get('error')})" if got.get("error") else "")
                 )
+            if "errorContains" in want:
+                actual_error = got.get("error") or ""
+                if want["errorContains"] not in actual_error:
+                    failures.append(
+                        f"{node_id}: error does not contain {want['errorContains']!r}, "
+                        f"got {actual_error!r}"
+                    )
             if "output" in want:
                 failures += subset_matches(want["output"], got.get("output"), node_id)
 

+ 17 - 0
tests/nodes/aggregate-errors-key.json

@@ -0,0 +1,17 @@
+{
+  "name": "verify-aggregate-errors-key",
+  "nodes": [
+    {"id": "n1", "name": "Trigger", "type": "click-trigger", "position": {"x": 0, "y": 0}, "config": {}},
+    {"id": "n2", "name": "Fixture", "type": "code", "position": {"x": 0, "y": 100},
+     "config": {"code": "return { items: [{sku: 'a', tier: 'x'}, {sku: 'b', tier: 'y'}] };"}},
+    {"id": "n3", "name": "AggregateReservedKey", "type": "aggregate", "position": {"x": 0, "y": 200},
+     "config": {"inputField": "data.result.items", "outputField": "key", "groupBy": "tier"}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n2", "targetInput": "data"},
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
+  ],
+  "expect": {
+    "n3": {"status": "failed", "errorContains": "outputField cannot be \"key\""}
+  }
+}

+ 1 - 1
tests/nodes/aggregate-errors.json

@@ -12,6 +12,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "outputField cannot be \"count\""}
   }
 }

+ 1 - 1
tests/nodes/datetime-errors-amount.json

@@ -12,6 +12,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "amount must be a number"}
   }
 }

+ 1 - 1
tests/nodes/datetime-errors-offset-negative.json

@@ -12,6 +12,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "is out of range"}
   }
 }

+ 1 - 1
tests/nodes/datetime-errors-offset.json

@@ -12,6 +12,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "is out of range"}
   }
 }

+ 17 - 0
tests/nodes/datetime-errors-range.json

@@ -0,0 +1,17 @@
+{
+  "name": "verify-datetime-errors-range",
+  "nodes": [
+    {"id": "n1", "name": "Trigger", "type": "click-trigger", "position": {"x": 0, "y": 0}, "config": {}},
+    {"id": "n2", "name": "Fixture", "type": "code", "position": {"x": 0, "y": 100},
+     "config": {"code": "return { when: '1722787200000000000' };"}},
+    {"id": "n3", "name": "DatetimeOutOfRangeTimestamp", "type": "datetime", "position": {"x": 0, "y": 200},
+     "config": {"operation": "format", "inputField": "data.result.when", "outputField": "value"}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n2", "targetInput": "data"},
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
+  ],
+  "expect": {
+    "n3": {"status": "failed", "errorContains": "is not a usable date"}
+  }
+}

+ 1 - 1
tests/nodes/datetime-errors.json

@@ -12,6 +12,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "outputField cannot be \"timestamp\""}
   }
 }

+ 1 - 1
tests/nodes/json-errors-invalid.json

@@ -12,6 +12,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "JSON parse failed:"}
   }
 }

+ 1 - 1
tests/nodes/json-errors.json

@@ -12,6 +12,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "is not a string"}
   }
 }

+ 1 - 1
tests/nodes/merge-errors.json

@@ -16,6 +16,6 @@
     {"sourceNodeId": "b", "sourceOutput": "main", "targetNodeId": "m", "targetInput": "input2"}
   ],
   "expect": {
-    "m": {"status": "failed"}
+    "m": {"status": "failed", "errorContains": "an item in input1 has no \"id\" to combine on"}
   }
 }

+ 1 - 1
tests/nodes/set-fields-errors-scalar-path.json

@@ -17,6 +17,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "result", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "Cannot set \"user.city\""}
   }
 }

+ 1 - 1
tests/nodes/set-fields-errors-string.json

@@ -17,6 +17,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "result", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "keep-all needs an object input"}
   }
 }

+ 1 - 1
tests/nodes/set-fields-errors.json

@@ -17,6 +17,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "result", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "keep-all needs an object input"}
   }
 }

+ 1 - 1
tests/nodes/sort-limit-dedupe-errors.json

@@ -12,6 +12,6 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n3": {"status": "failed"}
+    "n3": {"status": "failed", "errorContains": "outputField cannot be \"count\""}
   }
 }

+ 1 - 1
tests/nodes/split-aggregate.json

@@ -5,7 +5,7 @@
     {"id": "n2", "name": "Fixture", "type": "code", "position": {"x": 0, "y": 100},
      "config": {"code": "return { region: 'EU', orders: [{sku: 'a', tier: 'x'}, {sku: 'b', tier: 'y'}, {sku: 'c', tier: 'x'}] };"}},
     {"id": "n3", "name": "Split", "type": "split-out", "position": {"x": 0, "y": 200},
-     "config": {"field": "data.result.orders", "include": "none"}},
+     "config": {"inputField": "data.result.orders", "include": "none"}},
     {"id": "n4", "name": "Flat", "type": "aggregate", "position": {"x": -100, "y": 300},
      "config": {"inputField": "data.items", "outputField": "skus", "fieldToAggregate": "sku"}},
     {"id": "n5", "name": "Grouped", "type": "aggregate", "position": {"x": 100, "y": 300},

+ 1 - 1
tests/nodes/stop-and-error.json

@@ -12,7 +12,7 @@
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
   ],
   "expect": {
-    "n2": {"status": "failed"}
+    "n2": {"status": "failed", "errorContains": "deliberate stop"}
   },
   "expectMissing": ["n3"]
 }

+ 17 - 0
tests/nodes/template-errors.json

@@ -0,0 +1,17 @@
+{
+  "name": "verify-template-errors",
+  "nodes": [
+    {"id": "n1", "name": "Trigger", "type": "click-trigger", "position": {"x": 0, "y": 0}, "config": {}},
+    {"id": "n2", "name": "Fixture", "type": "code", "position": {"x": 0, "y": 100},
+     "config": {"code": "return { name: 'Ada' };"}},
+    {"id": "n3", "name": "RenderMissingField", "type": "template", "position": {"x": 0, "y": 200},
+     "config": {"templateSource": "input", "templateField": "data.result.nope", "outputField": "text"}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n2", "targetInput": "data"},
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"}
+  ],
+  "expect": {
+    "n3": {"status": "failed", "errorContains": "no template text found at \"data.result.nope\""}
+  }
+}

+ 10 - 3
tests/nodes/template-json.json

@@ -11,19 +11,26 @@
     {"id": "n5", "name": "Bad", "type": "json", "position": {"x": 100, "y": 200},
      "config": {"operation": "parse", "inputField": "data.result.name", "outputField": "value", "onError": "null"}},
     {"id": "n6", "name": "Render", "type": "template", "position": {"x": 0, "y": 400},
-     "config": {"template": "Hello {{data.result.name}}", "outputField": "text"}}
+     "config": {"template": "Hello {{data.result.name}}", "outputField": "text"}},
+    {"id": "n7", "name": "TemplateText", "type": "code", "position": {"x": 200, "y": 100},
+     "config": {"code": "var o = String.fromCharCode(123, 123); var c = String.fromCharCode(125, 125); return { message: 'Hi ' + o + 'result.who' + c + ', you have ' + o + 'result.n' + c + ' new items', who: 'Zora', n: 5 };"}},
+    {"id": "n8", "name": "RenderFromInput", "type": "template", "position": {"x": 200, "y": 200},
+     "config": {"templateSource": "input", "templateField": "data.result.message", "outputField": "text"}}
   ],
   "connections": [
     {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n2", "targetInput": "data"},
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"},
     {"sourceNodeId": "n3", "sourceOutput": "main", "targetNodeId": "n4", "targetInput": "data"},
     {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n5", "targetInput": "data"},
-    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n6", "targetInput": "data"}
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n6", "targetInput": "data"},
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n7", "targetInput": "data"},
+    {"sourceNodeId": "n7", "sourceOutput": "main", "targetNodeId": "n8", "targetInput": "data"}
   ],
   "expect": {
     "n3": {"status": "completed", "output": {"value": {"nested": {"count": 7}}}},
     "n4": {"status": "completed", "output": {"count": 7}},
     "n5": {"status": "completed", "output": {"value": null}},
-    "n6": {"status": "completed", "output": {"text": "Hello Ada"}}
+    "n6": {"status": "completed", "output": {"text": "Hello Ada"}},
+    "n8": {"status": "completed", "output": {"text": "Hi Zora, you have 5 new items"}}
   }
 }