Sfoglia il codice sorgente

fix: fail if-condition nodes on unresolved field paths instead of reading them as false

An unresolved field path (e.g. after upstream node output shape changes)
previously fell through to undefined and silently evaluated as false for
most operators. is_true on a missing field is the same answer as is_true
on an actually-false field, and a workflow can run green while quietly
skipping a step - this cost a production run when Should Generate in
[SWF] anime: process one image tested data.result.generate against a
set-fields node whose output was top-level, not wrapped in result.

Now a condition whose path does not resolve throws, naming the path, the
segment that broke, and what was actually available there - same model
as the loop node's existing path error. Presence operators (exists,
is_null, is_not_null, is_empty, is_not_empty) are exempt since a missing
field is their intended, meaningful answer. not_contains is deliberately
NOT exempt: like is_true, a missing field would otherwise read as a
silent affirmative match. Added a per-condition "optional" opt-out for
fields that are genuinely allowed to be absent.

Surveyed every if-condition in the running instance: all 16 real
production field paths resolve correctly against their upstream node's
actual output shape, so nothing in production starts failing. Only a
leftover debugging workflow ("tmp condition probe") that intentionally
reproduces the original incident would now fail, which is correct.
fszontagh 1 mese fa
parent
commit
90f0a232aa

+ 76 - 6
nodes/core/if-condition.js

@@ -2,7 +2,7 @@
  * @node if-condition
  * @name IF Condition
  * @category flow-control
- * @version 2.2.0
+ * @version 2.3.0
  * @description Conditional branching with TRUE/FALSE output paths
  * @icon git-branch
  */
@@ -36,6 +36,12 @@ const configSchema = {
             type: 'string',
             title: 'Value',
             description: 'Value to compare against'
+          },
+          optional: {
+            type: 'boolean',
+            title: 'Field Is Optional',
+            description: 'If the field path does not resolve, treat it as missing (matching is_null/exists/is_empty as usual) instead of failing the node. Only turn this on when the field genuinely may not be there - it silences the same check this node exists to make.',
+            default: false
           }
         }
       }
@@ -66,18 +72,46 @@ const outputSchema = {
   }
 };
 
-function getFieldValue(data, path) {
+// Describes what IS at a spot in the data, for use in an error message when a
+// path expected something else there.
+function describeAvailable(value) {
+  if (value === null || value === undefined) return 'nothing';
+  if (Array.isArray(value)) return 'an array (length ' + value.length + ')';
+  if (typeof value === 'object') {
+    const keys = Object.keys(value);
+    return keys.length ? keys.join(', ') : 'an empty object';
+  }
+  return typeof value;
+}
+
+// `diagnostics`, when passed, is filled in when the path does not resolve:
+// which segment broke, the path consumed up to that point, and what was
+// actually there instead - the detail needed for a message that names the
+// mistake instead of just reporting "false".
+function getFieldValue(data, path, diagnostics) {
   if (!path) return data;
 
   const keys = path.split('.');
   let value = data;
+  const consumed = [];
+
+  const fail = (missingKey, availableIn) => {
+    if (diagnostics) {
+      diagnostics.failed = true;
+      diagnostics.missingKey = missingKey;
+      diagnostics.failedAt = consumed.length ? consumed.join('.') : null;
+      diagnostics.availableDesc = describeAvailable(availableIn);
+    }
+    return undefined;
+  };
 
   for (const key of keys) {
-    if (value === null || value === undefined) return undefined;
+    if (value === null || value === undefined) return fail(key, value);
 
     // Handle array .length property
     if (key === 'length' && Array.isArray(value)) {
       value = value.length;
+      consumed.push(key);
       continue;
     }
 
@@ -90,30 +124,46 @@ function getFieldValue(data, path) {
       }
       if (Array.isArray(value)) {
         value = value[parseInt(index, 10)];
+        consumed.push(key);
         continue;
       }
-      return undefined;
+      return fail(key, value);
     }
 
     // Handle pure numeric index for arrays
     if (/^\d+$/.test(key) && Array.isArray(value)) {
       value = value[parseInt(key, 10)];
+      consumed.push(key);
       continue;
     }
 
     if (typeof value === 'object' && key in value) {
       value = value[key];
+      consumed.push(key);
     } else {
-      return undefined;
+      return fail(key, value);
     }
   }
 
   return value;
 }
 
+// Operators that exist specifically to ask "is this field here at all".
+// A missing field is a legitimate, meaningful answer for these - failing
+// the node would defeat the point of having them. is_empty/is_not_empty are
+// included because they are the standard way to check an optional field,
+// and a missing field reads the same as an explicitly empty one for that
+// purpose. Everything else - including is_true/is_false and not_contains,
+// which can each silently read "missing" as a valid match - requires the
+// path to actually resolve.
+const PRESENCE_OPERATORS = new Set([
+  'exists', 'is_null', 'is_not_null', 'is_empty', 'is_not_empty'
+]);
+
 function evaluateCondition(data, condition) {
   let fieldValue;
   const field = condition.field;
+  let pathDiagnostics = null;
 
   // If field is already a non-string value (boolean, number, etc. from expression evaluation),
   // use it directly instead of trying to resolve it as a path
@@ -126,7 +176,8 @@ function evaluateCondition(data, condition) {
     if (/^\d+$/.test(field) || /^\d+\.\d+$/.test(field)) {
       fieldValue = parseFloat(field);
     } else {
-      fieldValue = getFieldValue(data, field);
+      pathDiagnostics = {};
+      fieldValue = getFieldValue(data, field, pathDiagnostics);
     }
   } else if (field === null || field === undefined) {
     fieldValue = field;
@@ -135,6 +186,25 @@ function evaluateCondition(data, condition) {
     fieldValue = field;
   }
 
+  // A field path that never resolved is a configuration mistake, not a
+  // false answer - "this field is false" and "this field does not exist"
+  // are different questions, and blurring them is what let a workflow run
+  // green while silently skipping a step. Presence operators are exempt
+  // (see PRESENCE_OPERATORS above); an author who genuinely means "treat
+  // absent as false" can also set `optional: true` on the condition.
+  if (pathDiagnostics && pathDiagnostics.failed &&
+      !PRESENCE_OPERATORS.has(condition.operator) && !condition.optional) {
+    const at = pathDiagnostics.failedAt
+      ? 'under "' + pathDiagnostics.failedAt + '"'
+      : 'at the top level';
+    throw new Error(
+      'IF condition: nothing found at "' + field + '". Field "' + pathDiagnostics.missingKey +
+      '" does not exist ' + at + '. Available there: ' + pathDiagnostics.availableDesc + '. ' +
+      'If this field is genuinely optional, set "Field Is Optional" on the condition, ' +
+      'or use exists/is_null/is_empty to test for it directly.'
+    );
+  }
+
   const compareValue = condition.value;
 
   switch (condition.operator) {

+ 25 - 0
tests/nodes/if-condition-optional-field-opts-out.json

@@ -0,0 +1,25 @@
+{
+  "name": "verify-if-condition-optional-field-opts-out",
+  "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 { status: 'paid' };"}},
+    {"id": "n3", "name": "PromoApplied", "type": "if-condition", "position": {"x": 0, "y": 200},
+     "config": {"conditions": [{"field": "data.result.promo.applied", "operator": "is_true", "value": "", "optional": true}], "combineWith": "and"}},
+    {"id": "n4", "name": "OnTrue", "type": "code", "position": {"x": -100, "y": 300},
+     "config": {"code": "return { marker: 'should not run' };"}},
+    {"id": "n5", "name": "OnFalse", "type": "code", "position": {"x": 100, "y": 300},
+     "config": {"code": "return { marker: 'no promo, treated as false' };"}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n2", "targetInput": "data"},
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"},
+    {"sourceNodeId": "n3", "sourceOutput": "true", "targetNodeId": "n4", "targetInput": "data"},
+    {"sourceNodeId": "n3", "sourceOutput": "false", "targetNodeId": "n5", "targetInput": "data"}
+  ],
+  "expect": {
+    "n3": {"status": "completed", "output": {"result": false, "_activeBranch": "false"}},
+    "n5": {"status": "completed", "output": {"result": {"marker": "no promo, treated as false"}}}
+  },
+  "expectMissing": ["n4"]
+}

+ 25 - 0
tests/nodes/if-condition-presence-operator-on-missing-field.json

@@ -0,0 +1,25 @@
+{
+  "name": "verify-if-condition-presence-operator-on-missing-field",
+  "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 { status: 'paid' };"}},
+    {"id": "n3", "name": "HasDiscountCode", "type": "if-condition", "position": {"x": 0, "y": 200},
+     "config": {"conditions": [{"field": "data.result.discountCode", "operator": "exists", "value": ""}], "combineWith": "and"}},
+    {"id": "n4", "name": "OnTrue", "type": "code", "position": {"x": -100, "y": 300},
+     "config": {"code": "return { marker: 'should not run' };"}},
+    {"id": "n5", "name": "OnFalse", "type": "code", "position": {"x": 100, "y": 300},
+     "config": {"code": "return { marker: 'no discount code, as expected' };"}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n2", "targetInput": "data"},
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"},
+    {"sourceNodeId": "n3", "sourceOutput": "true", "targetNodeId": "n4", "targetInput": "data"},
+    {"sourceNodeId": "n3", "sourceOutput": "false", "targetNodeId": "n5", "targetInput": "data"}
+  ],
+  "expect": {
+    "n3": {"status": "completed", "output": {"result": false, "_activeBranch": "false"}},
+    "n5": {"status": "completed", "output": {"result": {"marker": "no discount code, as expected"}}}
+  },
+  "expectMissing": ["n4"]
+}

+ 36 - 0
tests/nodes/if-condition-resolvable-path-both-branches.json

@@ -0,0 +1,36 @@
+{
+  "name": "verify-if-condition-resolvable-path-both-branches",
+  "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 { status: 'paid', amount: 42 };"}},
+    {"id": "ifTrue", "name": "IfTrue", "type": "if-condition", "position": {"x": -150, "y": 200},
+     "config": {"conditions": [{"field": "data.result.status", "operator": "equals", "value": "paid"}], "combineWith": "and"}},
+    {"id": "ifFalse", "name": "IfFalse", "type": "if-condition", "position": {"x": 150, "y": 200},
+     "config": {"conditions": [{"field": "data.result.status", "operator": "equals", "value": "refunded"}], "combineWith": "and"}},
+    {"id": "onTrue", "name": "OnTrue", "type": "code", "position": {"x": -200, "y": 300},
+     "config": {"code": "return { marker: 'true-branch-ran' };"}},
+    {"id": "onTrueUnexpected", "name": "OnTrueUnexpected", "type": "code", "position": {"x": -100, "y": 300},
+     "config": {"code": "return { marker: 'should not run' };"}},
+    {"id": "onFalseUnexpected", "name": "OnFalseUnexpected", "type": "code", "position": {"x": 100, "y": 300},
+     "config": {"code": "return { marker: 'should not run' };"}},
+    {"id": "onFalse", "name": "OnFalse", "type": "code", "position": {"x": 200, "y": 300},
+     "config": {"code": "return { marker: 'false-branch-ran' };"}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n2", "targetInput": "data"},
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "ifTrue", "targetInput": "data"},
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "ifFalse", "targetInput": "data"},
+    {"sourceNodeId": "ifTrue", "sourceOutput": "true", "targetNodeId": "onTrue", "targetInput": "data"},
+    {"sourceNodeId": "ifTrue", "sourceOutput": "false", "targetNodeId": "onTrueUnexpected", "targetInput": "data"},
+    {"sourceNodeId": "ifFalse", "sourceOutput": "true", "targetNodeId": "onFalseUnexpected", "targetInput": "data"},
+    {"sourceNodeId": "ifFalse", "sourceOutput": "false", "targetNodeId": "onFalse", "targetInput": "data"}
+  ],
+  "expect": {
+    "ifTrue": {"status": "completed", "output": {"result": true, "_activeBranch": "true"}},
+    "ifFalse": {"status": "completed", "output": {"result": false, "_activeBranch": "false"}},
+    "onTrue": {"status": "completed", "output": {"result": {"marker": "true-branch-ran"}}},
+    "onFalse": {"status": "completed", "output": {"result": {"marker": "false-branch-ran"}}}
+  },
+  "expectMissing": ["onTrueUnexpected", "onFalseUnexpected"]
+}

+ 27 - 0
tests/nodes/if-condition-unresolved-path-fails.json

@@ -0,0 +1,27 @@
+{
+  "name": "verify-if-condition-unresolved-path-fails",
+  "nodes": [
+    {"id": "n1", "name": "Trigger", "type": "click-trigger", "position": {"x": 0, "y": 0}, "config": {}},
+    {"id": "n2", "name": "Params", "type": "set-fields", "position": {"x": 0, "y": 100},
+     "config": {"mode": "keep", "fields": [{"name": "generate", "value": "{{true}}"}]}},
+    {"id": "n3", "name": "Should Generate", "type": "if-condition", "position": {"x": 0, "y": 200},
+     "config": {"conditions": [{"field": "data.result.generate", "operator": "is_true", "value": ""}], "combineWith": "and"}},
+    {"id": "n4", "name": "OnTrue", "type": "code", "position": {"x": -100, "y": 300},
+     "config": {"code": "return { marker: 'should not run' };"}},
+    {"id": "n5", "name": "OnFalse", "type": "code", "position": {"x": 100, "y": 300},
+     "config": {"code": "return { marker: 'should not run' };"}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "n2", "targetInput": "data"},
+    {"sourceNodeId": "n2", "sourceOutput": "main", "targetNodeId": "n3", "targetInput": "data"},
+    {"sourceNodeId": "n3", "sourceOutput": "true", "targetNodeId": "n4", "targetInput": "data"},
+    {"sourceNodeId": "n3", "sourceOutput": "false", "targetNodeId": "n5", "targetInput": "data"}
+  ],
+  "expect": {
+    "n3": {
+      "status": "failed",
+      "errorContains": "nothing found at \"data.result.generate\""
+    }
+  },
+  "expectMissing": ["n4", "n5"]
+}