Selaa lähdekoodia

fix: apply config defaults inside loop bodies too, avoid extra source copies

Fix round 1 findings from review.

FINDING 1 (blocking): executeLoopBody is a separate ~440-line
re-implementation of the main node walk, and it called
evaluateExpressions(body_node->config, ...) directly, with no defaults
applied. A node relying on a schema default inside a loop body ran with
the raw stored config on every iteration, while the identical node
outside a loop now got its defaults. Applied the same defaulting step
used in the main walk, immediately before executeLoopBody's own
evaluateExpressions call, using the same applyConfigDefaults helper and
the same getNode-failure handling (falls through, executeNode does its
own lookup and reports "Node type not found" as before).

Added tests/nodes/config-defaults-loop-body.json: a loop whose body is a
code node with an empty config (no "code" key). It now completes with
the schema's default code executed, proving the read-time guarantee also
holds inside a loop.

FINDING 2 (non-blocking): registry_.getNode() returns NodeDefinition,
which carries the full JavaScript source, by value. The defaults lookup
added at each call site meant executeNode's own internal lookup ran a
second time immediately after, copying that source twice per node
execution, and would have been three times per loop iteration once
Finding 1 was fixed. Hoisted the lookup: executeNode now takes an
optional prefetched NodeDefinition (src/runner/workflow_engine.hpp), and
both call sites (the main walk and executeLoopBody) pass the definition
they already looked up for defaulting, instead of letting executeNode
look it up again.

FINDING 3 (non-blocking, documentation):
- lib/common/config_defaults.hpp now records that write-time
  materialisation is permanent - once a default is baked into a stored
  config, a later schema default change will never reach that workflow,
  because the read path correctly leaves a present key alone.
- docs/nodes.md now warns that a default containing {{ }} will be
  evaluated as an expression, since defaults flow through
  evaluateExpressions like any other stored config value.

Rebuilt, restarted webserver then runner (a stale runner process
survived a plain kill and needed kill -9), and reran the full fixture
suite: 40/40 passing (the 39 from the previous round plus this loop
fixture). Re-confirmed "Loaded 2 scheduled workflows" at webserver
startup, including wf_520f6f05-3256-4419-a8a5-42d7e7c3830f, unmodified.
fszontagh 1 kuukausi sitten
vanhempi
sitoutus
de2b70b77d

+ 9 - 0
docs/nodes.md

@@ -88,6 +88,15 @@ The comment block at the top of the file defines node metadata:
 
 The `configSchema` defines what options users can configure in the node editor.
 
+A top-level property's `default` is applied automatically whenever the
+stored config omits that key - both when the workflow is saved and again
+before the node executes - so `execute()` can rely on the key being
+present even for a workflow saved before the field existed. A default is
+resolved through the same expression evaluation as any other stored
+config value, so a `default:` containing `{{ }}` will be evaluated as an
+expression rather than kept as a literal string. No current node relies on
+this; keep it in mind if you're the first to try it.
+
 ### Supported Field Types
 
 ```javascript

+ 9 - 0
lib/common/config_defaults.hpp

@@ -21,6 +21,15 @@ namespace smartbotic::common {
 // Tolerant of a missing/null/non-object schema, or one with no
 // "properties" - in all of those cases the config is returned unchanged
 // rather than throwing.
+//
+// Write-time materialisation is permanent: once a workflow is saved with a
+// default baked into its stored config, that key now counts as "present",
+// so a later change to the schema's default will never reach that already-
+// saved workflow - the read path correctly leaves a present key alone. This
+// is an accepted consequence of applying defaults at write time, not a bug;
+// it only affects workflows saved after write-time materialisation shipped,
+// since a workflow saved before that never had the key baked in to begin
+// with, and still picks up schema default changes at read time.
 nlohmann::json applyConfigDefaults(const nlohmann::json& config,
                                     const nlohmann::json& config_schema);
 

+ 25 - 7
src/runner/workflow_engine.cpp

@@ -666,7 +666,8 @@ Result<ExecutionResult> WorkflowEngine::execute(const Workflow& workflow,
                     node_result.started_at = TimeUtils::nowMs();
                     node_result.finished_at = node_result.started_at;
                 } else {
-                    node_result = executeNode(evaluated_node, input, result.execution_id, workflow);
+                    node_result = executeNode(evaluated_node, input, result.execution_id, workflow,
+                                               node_def_for_defaults);
                 }
                 result.node_results[node_id] = node_result;
 
@@ -1119,15 +1120,22 @@ std::vector<std::string> WorkflowEngine::topologicalSort(
 NodeExecutionResult WorkflowEngine::executeNode(const WorkflowNode& node,
                                                 const nlohmann::json& input,
                                                 const std::string& execution_id,
-                                                const Workflow& workflow) {
+                                                const Workflow& workflow,
+                                                const std::optional<NodeDefinition>& prefetched_node_def) {
     NodeExecutionResult result;
     result.node_id = node.id;
     result.input = input;
     result.started_at = TimeUtils::nowMs();
     result.status = NodeStatus::Running;
 
-    // Get node definition
-    auto node_def = registry_.getNode(node.type);
+    // Get node definition - reuse the caller's lookup if it already made one
+    // (it did, to apply config defaults before expression evaluation) rather
+    // than looking it up, and copying its JavaScript source, a second time.
+    std::optional<NodeDefinition> looked_up_node_def;
+    if (!prefetched_node_def) {
+        looked_up_node_def = registry_.getNode(node.type);
+    }
+    const std::optional<NodeDefinition>& node_def = prefetched_node_def ? prefetched_node_def : looked_up_node_def;
     if (!node_def) {
         result.status = NodeStatus::Failed;
         result.error = "Node type not found: " + node.type;
@@ -2039,10 +2047,19 @@ bool WorkflowEngine::executeLoopBody(
                 merged_results[key] = value;
             }
 
-            // Evaluate expressions in body node config before execution
+            // Evaluate expressions in body node config before execution. Apply
+            // config defaults first, same as the main walk does, so a loop
+            // body node also gets its schema defaults on an already-saved
+            // workflow rather than only on nodes outside a loop.
             WorkflowNode evaluated_body_node = *body_node;
             t_disabled_reference_error.clear();
-            evaluated_body_node.config = evaluateExpressions(body_node->config, node_input, merged_results, workflow);
+            nlohmann::json defaulted_body_config = body_node->config;
+            auto body_node_def_for_defaults = registry_.getNode(body_node->type);
+            if (body_node_def_for_defaults) {
+                defaulted_body_config = smartbotic::common::applyConfigDefaults(
+                    body_node->config, body_node_def_for_defaults->config_schema);
+            }
+            evaluated_body_node.config = evaluateExpressions(defaulted_body_config, node_input, merged_results, workflow);
 
             NodeExecutionResult body_result;
             if (!t_disabled_reference_error.empty()) {
@@ -2054,7 +2071,8 @@ bool WorkflowEngine::executeLoopBody(
                 body_result.started_at = TimeUtils::nowMs();
                 body_result.finished_at = body_result.started_at;
             } else {
-                body_result = executeNode(evaluated_body_node, node_input, result.execution_id, workflow);
+                body_result = executeNode(evaluated_body_node, node_input, result.execution_id, workflow,
+                                           body_node_def_for_defaults);
             }
 
             rejectPauseInLoopBody(body_result);

+ 8 - 1
src/runner/workflow_engine.hpp

@@ -8,6 +8,7 @@
 #include <condition_variable>
 #include <thread>
 #include <atomic>
+#include <optional>
 #include <nlohmann/json.hpp>
 #include "node_registry.hpp"
 #include "error_handler.hpp"
@@ -212,10 +213,16 @@ private:
     std::vector<std::string> topologicalSort(const Workflow& workflow,
                                              const std::unordered_map<std::string, std::vector<std::string>>& dependencies);
 
+    // prefetched_node_def: when the caller already looked up the node's
+    // definition (e.g. to apply config defaults before evaluating
+    // expressions), pass it here so executeNode reuses it instead of
+    // looking it up - and copying its JavaScript source - a second time.
+    // Left empty, executeNode looks it up itself.
     NodeExecutionResult executeNode(const WorkflowNode& node,
                                    const nlohmann::json& input,
                                    const std::string& execution_id,
-                                   const Workflow& workflow);
+                                   const Workflow& workflow,
+                                   const std::optional<NodeDefinition>& prefetched_node_def = std::nullopt);
 
     nlohmann::json collectNodeInput(const std::string& node_id,
                                     const Workflow& workflow,

+ 20 - 0
tests/nodes/config-defaults-loop-body.json

@@ -0,0 +1,20 @@
+{
+  "name": "verify-config-defaults-loop-body",
+  "nodes": [
+    {"id": "n1", "name": "Trigger", "type": "click-trigger", "position": {"x": 0, "y": 0}, "config": {}},
+    {"id": "items", "name": "Items", "type": "code", "position": {"x": 0, "y": 100},
+     "config": {"code": "return { items: [1, 2] };"}},
+    {"id": "loop", "name": "Loop", "type": "loop", "position": {"x": 0, "y": 200},
+     "config": {"inputField": "data.result.items"}},
+    {"id": "body", "name": "NoConfigInLoop", "type": "code", "position": {"x": 0, "y": 300},
+     "config": {}}
+  ],
+  "connections": [
+    {"sourceNodeId": "n1", "sourceOutput": "main", "targetNodeId": "items", "targetInput": "data"},
+    {"sourceNodeId": "items", "sourceOutput": "main", "targetNodeId": "loop", "targetInput": "data"},
+    {"sourceNodeId": "loop", "sourceOutput": "loop", "targetNodeId": "body", "targetInput": "data"}
+  ],
+  "expect": {
+    "body": {"status": "completed", "output": {"result": {"processed": true}}}
+  }
+}