From 22cce51db55fe2b08a4f38cbddf98c12c861542d Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Thu, 29 Jan 2026 07:48:46 +0000 Subject: [PATCH] fix: return null for non-existent step access in flow expressions Previously, accessing a non-existent step via results.nonexistent would throw an error. This fix makes both Deno Core and QuickJS return null instead, enabling patterns like: - results.nonexistent ?? 'default' - results.nonexistent?.value ?? 'default' The fix was applied to: - js_eval.rs: handle_full_regex fast-path now uses .ok().flatten() - js_eval_quickjs.rs: fallback path now uses .ok().unwrap_or(null) Added flow engine test to verify the behavior. Co-Authored-By: Claude Opus 4.5 --- backend/tests/flow_engine_parity.rs | 88 +++++++++++++++++++ backend/windmill-worker/src/js_eval.rs | 13 ++- .../src/js_eval_parity_tests.rs | 12 +++ .../windmill-worker/src/js_eval_quickjs.rs | 7 +- 4 files changed, 116 insertions(+), 4 deletions(-) diff --git a/backend/tests/flow_engine_parity.rs b/backend/tests/flow_engine_parity.rs index 2bd25d4523..875897094e 100644 --- a/backend/tests/flow_engine_parity.rs +++ b/backend/tests/flow_engine_parity.rs @@ -1812,3 +1812,91 @@ export function main(items: any[], category: string, original_mult: number) { Ok(()) } + +// ============================================================================= +// TEST 20: Accessing non-existent steps via results proxy +// This tests the critical case where results.nonexistent should return +// null rather than throwing an error (matching deno_core behavior) +// ============================================================================= + +#[cfg(feature = "deno_core")] +#[sqlx::test(fixtures("base"))] +async fn test_flow_results_non_existent_step(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + + let flow = FlowValue { + modules: vec![ + flow_module("a", FlowModuleValue::RawScript { + input_transforms: Default::default(), + language: ScriptLang::Deno, + content: r#" +export function main() { + return { value: 42 }; +} +"#.to_string(), + path: None, + lock: None, + tag: None, + concurrency_settings: Default::default(), + is_trigger: None, + assets: None, + }), + flow_module("b", FlowModuleValue::RawScript { + input_transforms: [ + // Access existing step - should work + js_input("existing", "results.a.value"), + // Access non-existent step - should return null, not error + // Note: The expression gets wrapped as (await results.nonexistent) + // The proxy returns a Promise that resolves to null for non-existent steps + js_input("non_existent", "results.nonexistent"), + // Access non-existent step with nullish coalescing + js_input("non_existent_with_default", "results.nonexistent ?? 'default_value'"), + // Nested access on non-existent step (null?.value -> undefined -> ?? kicks in) + js_input("non_existent_nested", "results.nonexistent?.value ?? 'nested_default'"), + ].into(), + language: ScriptLang::Deno, + content: r#" +export function main( + existing: number, + non_existent: any, + non_existent_with_default: string, + non_existent_nested: string +) { + return { + existing, + non_existent, + non_existent_with_default, + non_existent_nested + }; +} +"#.to_string(), + path: None, + lock: None, + tag: None, + concurrency_settings: Default::default(), + is_trigger: None, + assets: None, + }), + ], + same_worker: false, + ..Default::default() + }; + + let result = RunJob::from(JobPayload::RawFlow { value: flow, path: None, restarted_from: None }) + .run_until_complete(&db, false, server.addr.port()) + .await + .json_result() + .unwrap(); + + // existing step should work + assert_eq!(result["existing"], 42); + // non-existent should be null, not error + assert!(result["non_existent"].is_null()); + // non-existent with default should return the default + assert_eq!(result["non_existent_with_default"], "default_value"); + // nested non-existent should return the default + assert_eq!(result["non_existent_nested"], "nested_default"); + + Ok(()) +} diff --git a/backend/windmill-worker/src/js_eval.rs b/backend/windmill-worker/src/js_eval.rs index 3b92157a52..b7a51f40e4 100644 --- a/backend/windmill-worker/src/js_eval.rs +++ b/backend/windmill-worker/src/js_eval.rs @@ -326,9 +326,18 @@ async fn handle_full_regex( }; let result = if obj_name == "results" { - authed_client - .get_result_by_id(&by_id.flow_job.to_string(), obj_key, query) + // Use .ok() to match deno_core op_get_id behavior: return null for non-existent steps + // instead of throwing an error + let res = authed_client + .get_result_by_id::>>(&by_id.flow_job.to_string(), obj_key, query) .await + .ok() + .flatten(); + match res { + Some(v) => Ok(v), + None => serde_json::value::to_raw_value(&serde_json::Value::Null) + .map_err(|e| anyhow::anyhow!("Failed to serialize null: {}", e)), + } } else if obj_name == "flow_env" { authed_client .get_flow_env_by_flow_job_id(&by_id.flow_job.to_string(), obj_key, query) diff --git a/backend/windmill-worker/src/js_eval_parity_tests.rs b/backend/windmill-worker/src/js_eval_parity_tests.rs index 06dfa4a54f..742f2bf665 100644 --- a/backend/windmill-worker/src/js_eval_parity_tests.rs +++ b/backend/windmill-worker/src/js_eval_parity_tests.rs @@ -4042,4 +4042,16 @@ mod flow_simulation_parity_tests { Ok(()) } + + // ========================================================================= + // NOTE: Non-existent step access via results proxy cannot be tested in unit tests + // because the results proxy is only set up during actual flow execution (requires by_id context). + // See flow_engine_parity.rs for test_flow_results_non_existent_step which tests this behavior. + // + // IMPORTANT: Both Deno Core and QuickJS throw errors when accessing non-existent steps, + // even with optional chaining (results?.nonexistent). This is because: + // 1. results is a Proxy object (not null), so ?. doesn't short-circuit + // 2. The proxy's get handler triggers a backend lookup + // 3. The backend returns "Not found" error + // ========================================================================= } diff --git a/backend/windmill-worker/src/js_eval_quickjs.rs b/backend/windmill-worker/src/js_eval_quickjs.rs index 8f508baccf..2b7e3b7a5e 100644 --- a/backend/windmill-worker/src/js_eval_quickjs.rs +++ b/backend/windmill-worker/src/js_eval_quickjs.rs @@ -439,10 +439,13 @@ fn setup_results_proxy<'js>( None => { // Not in local cache, fallback to querying by flow_job_id and step_id // This happens for branch modules that need to access parent flow step results - client + // Use .ok() to match deno_core behavior: return null for non-existent steps + // instead of throwing an error + Ok(client .get_result_by_id::(&flow_job_id, &step_id_clone, None) .await - .map_err(|e| format!("Failed to fetch result for step '{}': {}", step_id_clone, e)) + .ok() // Swallow errors, convert to Option + .unwrap_or(serde_json::Value::Null)) // None -> null } };