mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-10 00:05:27 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.5
parent
5c20b37a53
commit
22cce51db5
@@ -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<Postgres>) -> 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(())
|
||||
}
|
||||
|
||||
@@ -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::<Option<Box<RawValue>>>(&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)
|
||||
|
||||
@@ -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
|
||||
// =========================================================================
|
||||
}
|
||||
|
||||
@@ -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::<serde_json::Value>(&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
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user