From 40fe0a319c8b168fde15b52b859eeee58644d5f6 Mon Sep 17 00:00:00 2001 From: Pino de Candia Date: Tue, 4 Aug 2026 23:38:03 +0000 Subject: [PATCH] Fail loudly on corrupt Durofut envelopes (follow-up to #331) PR #331 fixed deep graph composition (#327) structurally but left two silent-corruption arms flagged in review: - Durofut::ensure() swallowed any unparseable JSON envelope into a raw SQL node, including the new deserialize_raw_object rejection of non-object children. Fail loudly instead, keying off the parse error plus a generic-JSON node_type probe (robust to serialized field order) rather than a prefix sniff. - collect_nodes() in df.explain() panicked via .expect() on an undeserializable child; align it to pgrx::error! like insert_nodes() so it raises a clean PostgreSQL error. Add regression tests: loud failure on a corrupt envelope, envelope detection independent of field order, and a deep JOIN chain locking in the shared opaque-child path. --- CHANGELOG.md | 1 + src/explain.rs | 4 ++-- src/lib.rs | 31 +++++++++++++++++++++++++++++ src/types.rs | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96a8975..9bf4fbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Pre-1.0 note: while `pg_durable` is in major version `0`, minor releases may inc ### Fixed - **Deep workflow composition (#327):** workflow graphs deeper than serde_json's 127-level recursion limit no longer silently collapse into SQL text. Nested children are deserialized one graph level at a time, and `df.explain()` now enforces the configured graph-depth limit before traversal. +- **Silent Durofut envelope corruption (follow-up to #327):** `Durofut::ensure()` now fails loudly when a JSON object carrying a `node_type` cannot be deserialized (e.g. a non-object child) instead of silently wrapping the raw envelope as a SQL node that only fails at execution time. `df.explain()` no longer panics on an undeserializable child, raising a clean PostgreSQL error consistent with `df.start()`. ## [0.2.5] - 2026-07-30 diff --git a/src/explain.rs b/src/explain.rs index 951718d..6812d1f 100644 --- a/src/explain.rs +++ b/src/explain.rs @@ -301,12 +301,12 @@ fn collect_nodes( // Recursively collect children first to get their IDs let left_id = node.left_node.as_ref().map(|raw| { let child = crate::types::Durofut::child_from_raw(raw) - .expect("validated Durofut child must deserialize"); + .unwrap_or_else(|e| pgrx::error!("Invalid left child in graph: {}", e)); collect_nodes(&child, nodes, id_counter) }); let right_id = node.right_node.as_ref().map(|raw| { let child = crate::types::Durofut::child_from_raw(raw) - .expect("validated Durofut child must deserialize"); + .unwrap_or_else(|e| pgrx::error!("Invalid right child in graph: {}", e)); collect_nodes(&child, nodes, id_counter) }); diff --git a/src/lib.rs b/src/lib.rs index 103d607..c4963bd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2237,6 +2237,37 @@ mod tests { } } + #[pg_test] + fn test_ensure_rejects_corrupt_durofut_envelope() { + // A JSON object carrying a node_type but failing to deserialize (here a + // non-object child) is a corrupt Durofut envelope. Durofut::ensure must + // fail loudly rather than silently wrap the raw JSON as a SQL node, + // which would only blow up later at execution time. + Spi::run( + "CREATE OR REPLACE FUNCTION pg_temp.capture_error(sql_text text) RETURNS text + LANGUAGE plpgsql AS $$ + BEGIN + EXECUTE sql_text; + RETURN NULL; + EXCEPTION WHEN OTHERS THEN + RETURN SQLERRM; + END; + $$;", + ) + .unwrap(); + let msg = Spi::get_one::( + r#"SELECT pg_temp.capture_error($$ + SELECT df.seq('{"node_type":"THEN","left_node":123}', df.sql('SELECT 1')) + $$)"#, + ) + .unwrap() + .unwrap(); + assert!( + msg.contains("Invalid Durofut JSON"), + "expected df.seq to reject the corrupt Durofut envelope, got: {msg}" + ); + } + #[pg_test] fn test_autowrap_start_plain_sql() { // Start with plain SQL - simplest possible durable function diff --git a/src/types.rs b/src/types.rs index 7f1ab8f..7b2b7cb 100644 --- a/src/types.rs +++ b/src/types.rs @@ -1279,6 +1279,16 @@ impl Durofut { .unwrap_or(false) } + /// True when `s` is a JSON object carrying a `node_type` field, i.e. a + /// Durofut envelope (possibly corrupt) rather than plain SQL text. Uses a + /// generic JSON parse so it is robust to serialized field ordering. + fn is_durofut_envelope(s: &str) -> bool { + matches!( + serde_json::from_str::(s), + Ok(v) if v.get("node_type").and_then(|nt| nt.as_str()).is_some() + ) + } + /// Ensure a string is a Durofut - if it's already one, parse it; if not, treat as SQL and create a node. /// Uses a single deserialization attempt to avoid redundant parsing. pub fn ensure(s: &str) -> Self { @@ -1287,6 +1297,16 @@ impl Durofut { } match serde_json::from_str::(s) { Ok(d) if VALID_NODE_TYPES.contains(&d.node_type.as_str()) => d, + Err(serde_err) if Self::is_durofut_envelope(s) => { + // A JSON object carrying a node_type is a Durofut envelope that + // failed to deserialize (e.g. a corrupt or non-object child). + // Fail loudly instead of silently wrapping the raw envelope as + // a SQL node, which would later blow up at execution time. + pgrx::error!( + "Invalid Durofut JSON: failed to deserialize workflow step: {}", + serde_err + ); + } _ => Durofut { node_type: "SQL".to_string(), query: Some(s.to_string()), @@ -1540,6 +1560,39 @@ mod tests { assert_eq!(durofut.to_json(), json); } + #[test] + fn deep_join_chain_deserializes_and_validates() { + // JOIN/RACE nest through left/right RawValue children just like THEN, + // so a deep JOIN fold must share the corrected opaque-child path. + let mut json = r#"{"node_type":"SQL","query":"SELECT 1"}"#.to_string(); + for _ in 0..129 { + json = format!( + r#"{{"node_type":"JOIN","left_node":{},"right_node":{{"node_type":"SQL","query":"SELECT 1"}}}}"#, + json + ); + } + + let durofut = Durofut::try_from_json(&json).unwrap(); + + assert!(durofut.validate_recursive().is_ok()); + assert_eq!(durofut.to_json(), json); + } + + #[test] + fn is_durofut_envelope_detects_node_type_regardless_of_order() { + // Robust to serialized field ordering: node_type not first. + assert!(Durofut::is_durofut_envelope( + r#"{"query":"SELECT 1","node_type":"SQL"}"# + )); + // A corrupt envelope (non-object child) still reads as an envelope. + assert!(Durofut::is_durofut_envelope( + r#"{"node_type":"THEN","left_node":123}"# + )); + // Plain SQL and non-envelope JSON are not envelopes. + assert!(!Durofut::is_durofut_envelope("SELECT 1")); + assert!(!Durofut::is_durofut_envelope(r#"{"foo":1}"#)); + } + #[test] fn whole_value_reference_accepts_single_references() { for value in [